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

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

[Romain Bignon]

Signed-off-by: Romain Bignon <romain@peerfuse.org>
---
 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

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

[Junio C Hamano]

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.

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

[Benjamin Meyer]

> 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

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

[Junio C Hamano]

Benjamin Meyer <ben@meyerhome.net> 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.