# [PATCH] use -q instead of redirect to /dev/null for git update-index 4 messages from 2010-03-21 to 2010-03-21. Participants: Romain Bignon, Junio C Hamano, Benjamin Meyer. Thread: https://gitlist.dev/t/23118 ## Romain Bignon, 2010-03-21 10:53 Subject: [PATCH] use -q instead of redirect to /dev/null for git update-index Message-ID: <1269168827-18694-1-git-send-email-romain@peerfuse.org> URL: https://gitlist.dev/e/1269168827-18694-1-git-send-email-romain%40peerfuse.org ``` Signed-off-by: Romain Bignon --- fixup-builtins | 2 +- git-rebase.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fixup-builtins b/fixup-builtins index 63dfa4c..172dbf6 100755 --- a/fixup-builtins +++ b/fixup-builtins @@ -12,5 +12,5 @@ done sed -i 's/git merge-one-file/git-merge-one-file/g s/git rebase-todo/git-rebase-todo/g' $(git ls-files '*.sh') -git update-index --refresh >& /dev/null +git update-index --refresh -q exit 0 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 exit 1 -- 1.7.0 ``` ## Junio C Hamano, 2010-03-21 13:17 Subject: Re: [PATCH] use -q instead of redirect to /dev/null for git update-index Message-ID: <7vy6hlvmoy.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6hlvmoy.fsf%40alter.siamese.dyndns.org In-Reply-To: <1269168827-18694-1-git-send-email-romain@peerfuse.org> ``` Romain Bignon writes: > Signed-off-by: Romain Bignon > --- > ... > 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. ``` ## Benjamin Meyer, 2010-03-21 17:02 Subject: Re: [PATCH] use -q instead of redirect to /dev/null for git update-index Message-ID: <70E4FE83-0DC9-4AF1-BA20-BE6467FC8D07@meyerhome.net> URL: https://gitlist.dev/e/70E4FE83-0DC9-4AF1-BA20-BE6467FC8D07%40meyerhome.net In-Reply-To: <7vy6hlvmoy.fsf@alter.siamese.dyndns.org> ``` > 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. For what it is worth I got the task from the Git Janitor wiki page. https://git.wiki.kernel.org/index.php/Janitor Sorry I didn't include that in my changelog. -Benjamin Meyer ``` ## Junio C Hamano, 2010-03-21 19:50 Subject: Re: [PATCH] use -q instead of redirect to /dev/null for git update-index Message-ID: <7vr5ndpi8g.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr5ndpi8g.fsf%40alter.siamese.dyndns.org In-Reply-To: <70E4FE83-0DC9-4AF1-BA20-BE6467FC8D07@meyerhome.net> ``` Benjamin Meyer writes: > For what it is worth I got the task from the Git Janitor wiki page. > https://git.wiki.kernel.org/index.php/Janitor > > Sorry I didn't include that in my changelog. Thanks for a pointer; I tried fixing the Janitor page. The description did sound as if we invite anybody to send in sed -e 's|>/dev/null|-q|' output as a patch without thinking, which is very wrong. ```