Nico Huber posted comments on this change.
Patch set 1:
... 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?
(1 comment)
Patch Set #1, 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."?
To view, visit change 21828. To unsubscribe, visit settings.