Re: I think git show is broken

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: I think git show is broken

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:35

Jeff King [off-list ref] writes:
Yes, that is what's going on. But it's still a regression. There was
some discussion of what --quiet should do here:

  http://thread.gmane.org/gmane.comp.version-control.git/171357

which resulted in a patch that took away --quiet. But then this thread:

  http://thread.gmane.org/gmane.comp.version-control.git/174665

resulted in restoring it as a synonym for "-s". Unfortunately there's a
bug in that fix, which you are seeing. Patch is below.
Thanks for digging this through to the bottom.
...
However, that commit did not fix it in all cases. It sets
the flag after setup_revisions is called. Naively, this
makes sense because you would expect the setup_revisions
parser to overwrite our output format flag if "-p" or
another output format flag is seen.

However, that is not how the NO_OUTPUT flag works. We
actually store it in the bit-field as just another format.
At the end of setup_revisions, we call diff_setup_done,
which post-processes the bitfield and clears any other
formats if we have set NO_OUTPUT. By setting the flag after
setup_revisions is done, diff_setup_done does not have a
chance to make this tweak, and we end up with other format
options still set.

As a result, the flag would have no effect in "git log -p
--quiet" or "git show --quiet".  Fix it by setting the
format flag before the call to setup_revisions.
This also means that

	git show --name-status --quiet

will start erroring out, if I am not recalling what diff_setup_done()
does.  Which pretty much means "--quiet" given to the "log" family
is truly a synonym to "-s", as the error condition that triggers is
exactly the same for this:

	git show --name-status -s

which is fine, I think.
quoted hunk
Signed-off-by: Jeff King <redacted>
---
 builtin/log.c   |  2 +-
 t/t7007-show.sh | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/builtin/log.c b/builtin/log.c
index ecc2793..c22469c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -109,9 +109,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);
 
-	argc = setup_revisions(argc, argv, rev, opt);
 	if (quiet)
 		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
+	argc = setup_revisions(argc, argv, rev, opt);
 
 	/* Any arguments at this point are not recognized */
 	if (argc > 1)
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index a40cd36..e41fa00 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -108,4 +108,16 @@ test_expect_success 'showing range' '
 	test_cmp expect actual.filtered
 '
 
+test_expect_success '-s suppresses diff' '
+	echo main3 >expect &&
+	git show -s --format=%s main3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--quiet suppresses diff' '
+	echo main3 >expect &&
+	git show --quiet --format=%s main3 >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: I think git show is broken

From: Jeff King <hidden>
Date: 2016-06-15 22:54:35

On Tue, Aug 28, 2012 at 03:36:26PM -0700, Junio C Hamano wrote:
quoted
As a result, the flag would have no effect in "git log -p
--quiet" or "git show --quiet".  Fix it by setting the
format flag before the call to setup_revisions.
This also means that

	git show --name-status --quiet

will start erroring out, if I am not recalling what diff_setup_done()
does.  Which pretty much means "--quiet" given to the "log" family
is truly a synonym to "-s", as the error condition that triggers is
exactly the same for this:

	git show --name-status -s

which is fine, I think.
Yes, I noticed that. I think it is fine for "--quiet" to be a true
synonym for "-s" here.

Though I am puzzled why we would error out on "--name-status -s" but not
"--patch -s". What is the difference between "--name-status" and
"--patch" here? Shouldn't "-s" override all formatting options?

And one final thing I noticed that is probably not worth the trouble to
fix: the position of "-s" is independent of its effect. Normally options
which override each other would be position dependent, so:

  git log --relative-date --date=local

and

  git log --date=local --relative-date

would both throw away the first option and let the latter take effect.
But doing:

  git log --patch -s

and

  git log -s --patch

will always have "-s" take over. I don't think it's a huge deal, and
fixing it would be a pain. We'd have to take NO_OUTPUT out of the
bit-field and make it a special option, and fix any callers who try to
be clever about recognizing NO_OUTPUT as a specifically-given option.
And then for "--quiet", we'd have to handle it at its proper spot on the
command-line, which would mean converting log's parse-options invocation
to be step-wise. Probably not worth it for a minor bit of consistency
that nobody has actually complained about.

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help