Re: [PATCH] use -q instead of redirect to /dev/null for git update-index

[Junio C Hamano <gitster@pobox.com>]

Romain Bignon <romain@peerfuse.org> writes:

> 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.