Re: [PATCH] use -q instead of redirect to /dev/null for git update-index
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 21, 2010, 13:17 UTC
- Message-ID
- <7vy6hlvmoy.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1269168827-18694-1-git-send-email-romain@peerfuse.org>
Romain Bignon <romain@peerfuse.org> writes:
Show 15 quoted lines
> Signed-off-by: Romain Bignon <romain@peerfuse.org> > --- > ... > diff --git a/git-rebase.sh b/git-rebase.sh > index fb4fef7..c814c30 100755 > --- a/git-rebase.sh > +++ b/git-rebase.sh > @@ -389,7 +389,7 @@ else > fi > > # The tree must be really really clean. > -if ! git update-index --ignore-submodules --refresh > /dev/null; then > +if ! git update-index --ignore-submodules --refresh -q; then > echo >&2 "cannot rebase: you have unstaged changes" > git diff-files --name-status -r --ignore-submodules -- >&2
I don't think this is quite "identical" conversion, if that is what you were aiming at. But it is not even clear why you wanted to do this patch; it is not justified with any proposed commit log message.
The original is written in such a way that you won't see any output, not just for the "needs-update" entries, but also not for the "needs-merge" entries; "-q" traditionally never squelched the output for the latter kind.
Maybe in some other codepath, it might be the correct thing not to squelch the "needs-merge" message, but because we run diff-files to show them in the error codepath, redirecting everything to /dev/null is the right thing to do in this particular case.
Because the patch wasn't accompanied by any explanation to justify why this change is necessary or desirable, it forced me to study and read through the history to come up with the two paragraph analysis above, costing 30 minutes of my time, only to reject it.
Even if this _were_ a correct conversion, we are not gaining much (what's the point of removing the perfectly-well-working discard-to-null?).
The above can be said to other "/dev/null to -q" patches we saw on the list recently. The saddest part of the story is that, the review cost (not necessarily spent by me, but time spent by other people reviewing patches is precious) is paid only to prevent a breakage like this patch tries to introduce from getting accepted, without much potential reward. Even if some of those patches turn out to be harmless, the only thing we would have bought with the cost of reviewing is that we made sure that the system would continue to work as before, but that is what we could have easily done by not even looking at the patches at all.
Sadly, the cost-to-reward tradeoff is simply not worth it, unless the patch is accompanied by necessary homework to reduce the review load.