{"thread":{"id":"23118","subject":"[PATCH] use -q instead of redirect to /dev/null for git update-index","startedAt":"2010-03-21T10:53:47Z","lastAt":"2010-03-21T19:50:07Z","messageCount":4,"participants":["Romain Bignon","Junio C Hamano","Benjamin Meyer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"137435","messageId":"1269168827-18694-1-git-send-email-romain@peerfuse.org","threadId":"23118","inReplyTo":null,"subject":"[PATCH] use -q instead of redirect to /dev/null for git update-index","fromName":"Romain Bignon","fromEmail":"romain@peerfuse.org","sentAt":"2010-03-21T10:53:47Z","receivedAt":"2010-03-21T10:53:47Z","isPatch":true,"sender":{"key":"romain@peerfuse.org","avatar":null},"body":"Signed-off-by: Romain Bignon <romain@peerfuse.org>\n---\n fixup-builtins |    2 +-\n git-rebase.sh  |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fixup-builtins b/fixup-builtins\nindex 63dfa4c..172dbf6 100755\n--- a/fixup-builtins\n+++ b/fixup-builtins\n@@ -12,5 +12,5 @@ done\n \n sed -i 's/git merge-one-file/git-merge-one-file/g\n s/git rebase-todo/git-rebase-todo/g' $(git ls-files '*.sh')\n-git update-index --refresh >& /dev/null\n+git update-index --refresh -q\n exit 0\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex fb4fef7..c814c30 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -389,7 +389,7 @@ else\n fi\n \n # The tree must be really really clean.\n-if ! git update-index --ignore-submodules --refresh > /dev/null; then\n+if ! git update-index --ignore-submodules --refresh -q; then\n \techo >&2 \"cannot rebase: you have unstaged changes\"\n \tgit diff-files --name-status -r --ignore-submodules -- >&2\n \texit 1\n-- \n1.7.0\n"},{"id":"137443","messageId":"7vy6hlvmoy.fsf@alter.siamese.dyndns.org","threadId":"23118","inReplyTo":"1269168827-18694-1-git-send-email-romain@peerfuse.org","subject":"Re: [PATCH] use -q instead of redirect to /dev/null for git update-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-21T13:17:17Z","receivedAt":"2010-03-21T13:17:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Romain Bignon <romain@peerfuse.org> writes:\n\n> Signed-off-by: Romain Bignon <romain@peerfuse.org>\n> ---\n> ...\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index fb4fef7..c814c30 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -389,7 +389,7 @@ else\n>  fi\n>  \n>  # The tree must be really really clean.\n> -if ! git update-index --ignore-submodules --refresh > /dev/null; then\n> +if ! git update-index --ignore-submodules --refresh -q; then\n>  \techo >&2 \"cannot rebase: you have unstaged changes\"\n>  \tgit diff-files --name-status -r --ignore-submodules -- >&2\n\nI don't think this is quite \"identical\" conversion, if that is what you\nwere aiming at.  But it is not even clear why you wanted to do this patch;\nit is not justified with any proposed commit log message.\n\nThe original is written in such a way that you won't see any output, not\njust for the \"needs-update\" entries, but also not for the \"needs-merge\"\nentries; \"-q\" traditionally never squelched the output for the latter\nkind.\n\nMaybe in some other codepath, it might be the correct thing not to squelch\nthe \"needs-merge\" message, but because we run diff-files to show them in\nthe error codepath, redirecting everything to /dev/null is the right thing\nto do in this particular case.\n\nBecause the patch wasn't accompanied by any explanation to justify why\nthis change is necessary or desirable, it forced me to study and read\nthrough the history to come up with the two paragraph analysis above,\ncosting 30 minutes of my time, only to reject it.\n\nEven if this _were_ a correct conversion, we are not gaining much (what's\nthe point of removing the perfectly-well-working discard-to-null?).\n\nThe above can be said to other \"/dev/null to -q\" patches we saw on the\nlist recently.  The saddest part of the story is that, the review cost\n(not necessarily spent by me, but time spent by other people reviewing\npatches is precious) is paid only to prevent a breakage like this patch\ntries to introduce from getting accepted, without much potential reward.\nEven if some of those patches turn out to be harmless, the only thing we\nwould have bought with the cost of reviewing is that we made sure that the\nsystem would continue to work as before, but that is what we could have\neasily done by not even looking at the patches at all.\n\nSadly, the cost-to-reward tradeoff is simply not worth it, unless the\npatch is accompanied by necessary homework to reduce the review load.\n"},{"id":"137454","messageId":"70E4FE83-0DC9-4AF1-BA20-BE6467FC8D07@meyerhome.net","threadId":"23118","inReplyTo":"7vy6hlvmoy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] use -q instead of redirect to /dev/null for git update-index","fromName":"Benjamin Meyer","fromEmail":"ben@meyerhome.net","sentAt":"2010-03-21T17:02:29Z","receivedAt":"2010-03-21T17:02:29Z","isPatch":true,"sender":{"key":"ben@meyerhome.net","avatar":null},"body":"> The above can be said to other \"/dev/null to -q\" patches we saw on the\n> list recently.  The saddest part of the story is that, the review cost\n> (not necessarily spent by me, but time spent by other people reviewing\n> patches is precious) is paid only to prevent a breakage like this patch\n> tries to introduce from getting accepted, without much potential reward.\n> Even if some of those patches turn out to be harmless, the only thing we\n> would have bought with the cost of reviewing is that we made sure that the\n> system would continue to work as before, but that is what we could have\n> easily done by not even looking at the patches at all.\n\nFor what it is worth I got the task from the Git Janitor wiki page.  https://git.wiki.kernel.org/index.php/Janitor\n\nSorry I didn't include that in my changelog.\n\n-Benjamin Meyer\n"},{"id":"137472","messageId":"7vr5ndpi8g.fsf@alter.siamese.dyndns.org","threadId":"23118","inReplyTo":"70E4FE83-0DC9-4AF1-BA20-BE6467FC8D07@meyerhome.net","subject":"Re: [PATCH] use -q instead of redirect to /dev/null for git update-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-21T19:50:07Z","receivedAt":"2010-03-21T19:50:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benjamin Meyer <ben@meyerhome.net> writes:\n\n> For what it is worth I got the task from the Git Janitor wiki page.\n> https://git.wiki.kernel.org/index.php/Janitor\n>\n> Sorry I didn't include that in my changelog.\n\nThanks for a pointer; I tried fixing the Janitor page.  The description\ndid sound as if we invite anybody to send in sed -e 's|>/dev/null|-q|'\noutput as a patch without thinking, which is very wrong.\n"}]}