Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

From: Jeff King <peff@peff.net>
Date: May 26, 2016, 16:59 UTC
Message-ID: <20160526165946.GA18383@sigill.intra.peff.net>
In-Reply-To: <20160526130647.27001-2-mehul.jain2029@gmail.com>

On Thu, May 26, 2016 at 06:36:46PM +0530, Mehul Jain wrote:

Show 11 quoted lines

> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 03f9580..f39f800 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -196,6 +196,10 @@ log.showRoot::
>  	`git log -p` output would be shown without a diff attached.
>  	The default is `true`.
>  
> +log.showSignature::
> +	If `true`, `git log` and `git show` will act as if `--show-signature`
> +	option was passed to them.

This should be:

  ...if the `--show-signature` option was...

or:

  ...if `--show-signature` was...

Either is correct; you just need an article when not referring directly to the option by its name.

The documentation here mentions "log" and "show". But I think this will affect other programs, too, including "whatchanged" and "reflog". Those ones are probably good, but the documentation is a little misleading (I think other options just say "git-log and related commands" or something).

I thought at first it would affect format-patch, too, which would be weird. But in that command we _do_ parse the variable and end up setting default_show_signature, but we never call cmd_log_init_defaults(), which is what copies that value into the rev_info struct. That's kind of a weird way to split it, but it's certainly not something you introduced here.

Show 18 quoted lines

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 128ba93..36be9a1 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
>  	grep "^| | gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
> +	git checkout -b test_sign master &&
> +	echo foo >foo &&
> +	git add foo &&
> +	git commit -S -m signed_commit &&
> +	test_config log.showsignature true &&
> +	git log -1 signed >actual &&
> +	test_i18ngrep "gpg: Signature made" actual &&
> +	test_i18ngrep "gpg: Good signature" actual
> +'

You can see in the context that we do not use test_i18ngrep for finding gpg output in existing tests. I'm not sure if the new tests should be consistent, or if they should be changed to use test_i18ngrep. I don't think it's actually doing anything here, though. It's used with a git-specific GETTEXT_POISON flag that tweaks the output generated by git, but not by sub-programs like gpg.

> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
> +	test_when_finished "git reset --hard && git checkout master" &&
> +	git config log.showsignature false &&

Should this be test_config?

> +test_expect_success GPG 'log.showsignature behaves like --show-signature' '
> +	git config log.showsignature true &&

Ditto here.

-Peff