Stefan Tauner posted comments on this change.
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".
Instead of stable or "random" we might better call it subjectively_perfect or something ;) Staging was meant to iron out the most obvious problems and I would then work on the minor details before merging them to stable. The current process (including the lag of stable) is not as it was envisaged... we will see how the next weeks will work out, then I'll write something down/propose a more strictly ruled process.
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?
Adding the comment was meant to exactly reduce the chance of somebody adding non-POSIX code without himself or a reviewer noticing it.
The point is that we want the script to work on all possible build systems of course... and there we get away with little violations. We just need to keep it in mind.
I like your suggestion of damping down the authority of the comment very much (see next PS).
To view, visit change 21828. To unsubscribe, visit settings.