Re: I think git show is broken

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2012, 22:36 UTC
Message-ID: <7v4nnmzlsl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120828212934.GA396@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

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

Show 19 quoted lines

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

Show 42 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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