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

[Laurent Arnoud <laurent@spkdev.net>]

On Tue, Mar 22, 2016 at 12:48:50PM -0700, Junio C Hamano wrote:
> 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.

Yes you're right, I will fix that...

-- 
Laurent