Re: [PATCH v5] Add the option to force sign annotated tags

[Junio C Hamano <gitster@pobox.com>]

Laurent Arnoud <laurent@spkdev.net> writes:

> The `tag.forcesignannotated` config option allows to sign
> annotated tags automatically.

It looks like it does a lot more than that to me, though.

> @@ -327,7 +333,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	char *cleanup_arg = NULL;
>  	int create_reflog = 0;
>  	int annotate = 0, force = 0;
> -	int cmdmode = 0;
> +	int cmdmode = 0, create_tag_object = 0;
>  	const char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct ref_transaction *transaction;
> @@ -385,12 +391,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		opt.sign = 1;
>  		set_signing_key(keyid);
>  	}
> -	if (opt.sign)
> -		annotate = 1;
> +	if (opt.sign || annotate || force_sign_annotate)
> +		create_tag_object = 1;

This means that create_tag_object is always on if the configuration
is set and there is no way to override that from the command line,
doesn't it?  I cannot see how a user would create a lightweight tag
if this configuration variable is set with this change.

I think it makes sense to have this here instead of these two lines:

	create_tag_object = (opt.sign || annotate || msg.given || msgfile);

>  	if (argc == 0 && !cmdmode)
>  		cmdmode = 'l';
>  
> -	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
> +	if ((create_tag_object || msg.given || msgfile || force) && (cmdmode != 0))

and then simplify this to

	if ((create_tag_object || force) && (cmdmode != 0))

perhaps?  Then ...

>  		usage_with_options(git_tag_usage, options);
>  
>  	finalize_colopts(&colopts, -1);
> @@ -431,7 +438,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (msg.given || msgfile) {
>  		if (msg.given && msgfile)
>  			die(_("only one -F or -m option is allowed."));
> -		annotate = 1;
> +		create_tag_object = 1;

... this line can just go, as we are taking the presense of various
ways to say "I'll give this message to the resulting tag" as the
sign that the user wants to create a tag object much earlier.

>  		if (msg.given)
>  			strbuf_addbuf(&buf, &(msg.buf));
>  		else {
> @@ -474,8 +481,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	else
>  		die(_("Invalid cleanup mode %s"), cleanup_arg);
>  
> -	if (annotate)
> +	if (create_tag_object) {
> +		if (force_sign_annotate && !annotate)
> +			opt.sign = 1;
>  		create_tag(object, tag, &buf, &opt, prev, object);
> +    }

And this hunk is OK.

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index cf3469b..be95318 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -775,6 +775,39 @@ test_expect_success GPG '-s implies annotated tag' '
>  	test_cmp expect actual
>  '
>  
> +get_tag_header config-implied-annotate $commit commit $time >expect
> +./fakeeditor >>expect
> +echo '-----BEGIN PGP SIGNATURE-----' >>expect
> +test_expect_success GPG \
> +	'git tag -s implied if configured with tag.forcesignannotated' \
> +	'test_config tag.forcesignannotated true &&
> +	GIT_EDITOR=./fakeeditor git tag config-implied-annotate &&

This contradicts with what you said earlier in your
<20160321192904.GC20083@spk-laptop> aka
http://thread.gmane.org/gmane.comp.version-control.git/289322/focus=289441

    > If you are forcing users to always leave a message and then further
    > forcing users to always sign with the single new configuration, i.e.
    > 
    >     $ git tag v1.0
    >     ... opens the editor to ask for a message ...
    >     ... then makes the user sign with GPG ...

    I'm not forcing this type of user to enable global
    configuration, that will be annoying for them of course.

But this test expects that this invocation of "git tag $tagname"
forces the user to give a message with editor and sign it, instead
of creating a lightweight tag.