Re: [PATCH v3] Add the tag.gpgsign option to sign annotated tags

From: Jeff King <peff@peff.net>
Date: Mar 21, 2016, 19:42 UTC
Message-ID: <20160321194237.GA28301@sigill.intra.peff.net>
In-Reply-To: <20160321193207.GD20083@spk-laptop>

On Mon, Mar 21, 2016 at 08:32:07PM +0100, Laurent Arnoud wrote:

Show 7 quoted lines

> The `tag.gpgsign` config option allows to sign all
> annotated tags automatically.
> 
> Support `--no-sign` option to countermand configuration `tag.gpgsign`.
> 
> Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
> Reviewed-by: Jeff King <peff@peff.net>

The meaning of "Reviewed-by" in this project is generally that the mentioned person has read and approved of the change. But in this case, I have not seen v3 at all yet, and I am also not sure that the ones I _did_ review are ready for merging.

So you should probably drop that.

> +tag.gpgSign::
> +	A boolean to specify whether annotated tags created should be GPG signed.
> +	If `--no-sign` is specified on the command line, it takes
> +	precedence over this option.

I take this to mean that we _only_ kick in signing if the created tag would otherwise be annotated (and I thought that's what you meant in your other mail, too). But that's not what happens with this patch, and your tests check for the opposite:

Show 11 quoted lines

> +get_tag_header config-implied-annotate $commit commit $time >expect
> +./fakeeditor >>expect
> +echo '-----BEGIN PGP SIGNATURE-----' >>expect
> +git config tag.gpgsign true
> +test_expect_success GPG \
> +	'git tag -s implied if configured with tag.gpgsign' \
> +	'GIT_EDITOR=./fakeeditor git tag config-implied-annotate &&
> +	get_tag_msg config-implied-annotate >actual &&
> +	test_cmp expect actual
> +'
> +git config --unset tag.gpgsign

That's a lightweight tag that becomes an annotated one due to the config variable.

So I think there may be some design-level issues to work out here, but I'll comment on a few more code-specific things, in case that code does get carried through:

> +	if (!strcmp(var, "tag.gpgsign")) {
> +		sign_tag_config = git_config_bool(var, value) ? 1 : 0;
> +		return 0;
> +	}

git_config_bool() already converts to 0/1, you can just say:

  sign_tag_config = git_config_bool(var, value);

Show 11 quoted lines

> +get_tag_header config-implied-annotate-disabled $commit commit $time >expect
> +echo "A message" >>expect
> +git config tag.gpgsign true
> +test_expect_success GPG \
> +	'git tag --no-sign disable configured tag.gpgsign' \
> +	'git tag --no-sign -m "A message" config-implied-annotate-disabled &&
> +	get_tag_msg config-implied-annotate-disabled >actual &&
> +	test_cmp expect actual &&
> +	test_must_fail git tag -v config-implied-annotate-disabled
> +'
> +git config --unset tag.gpgsign

Here (and in the other tests), you can use:

  test_config tag.gpgsign true &&
  ...

inside the test_expect_success block. That has two advantages:

  1. If setting the config fails for some reason, we'll notice and the
     test will fail.

  2. At the end of the test block, it will automatically clean up the
     variable for us.

-Peff