Nico Huber has posted comments on this change. ( https://review.coreboot.org/21828 )
Change subject: fixup! Convert flashrom to git ......................................................................
Patch Set 1:
(1 comment)
... Hence I want to have such rather benign things in it if possible.
Yeah, that's ok, I guess. We are already making exceptions for the Git conversion commit anyway. Generally, I'd prefer only changes that fix semantical bugs in the code to be fixups, i.e. what you really need to make it "stable".
The comment in question documents intent to make this clear for reviewers as contributors (while the sh shebang should be a clear hint I think the comment is still warranted due to the local restriction and its outspokenness). There was no strict review for this I guess but a) I always intended it to be that way (but of course there might be bugs... e.g., I wasn't aware local is not POSIX before you brought it up), b) you had a look for that obviously in the first review.
Not sure if that was me. At least I don't recall really checking the whole file. Maybe just rephrase (see inline comment), I'd like to avoid compatibility claims if we are not 100% sure. (It's my code-comment allergy. I've seen too many misleading comments over the years. So I prefer to write code that explains itself.) Also, who'd maintain the comment if anybody puts another non- POSIX construct in this file?
https://review.coreboot.org/#/c/21828/1/util/getrevision.sh File util/getrevision.sh:
https://review.coreboot.org/#/c/21828/1/util/getrevision.sh@2 PS1, Line 2: # NB: POSIX-compatible apart from the usage of 'local' to define variables within the scope of functions. How about "Supposed to be POSIX compatible but at least the usage of 'local' is off."?