[PATCH] Add the tag.gpgsign option to sign all created tags

[PATCH] Add the tag.gpgsign option to sign all created tags

[Laurent Arnoud]

The tag.gpgsign config option allows to sign all
commits automatically.

Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
---
 Documentation/config.txt |  3 +++
 builtin/tag.c            | 19 ++++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..076c68a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2729,6 +2729,9 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.gpgSign::
+	A boolean to specify whether all tags created should be GPG signed.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..53cad28 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static const char *sign_tag;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
+	if (!strcmp(var, "tag.gpgsign")) {
+		sign_tag = git_config_bool(var, value) ? "" : NULL;
+		return 0;
+	}
+
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
 	return git_default_config(var, value, cb);
@@ -381,14 +387,21 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+	if (argc == 0 && !cmdmode)
+		cmdmode = 'l';
+
+	/* Remove config option when calling command other than create tag */
+	if (cmdmode != 0 && sign_tag)
+		sign_tag = NULL;
+
 	if (keyid) {
 		opt.sign = 1;
 		set_signing_key(keyid);
 	}
-	if (opt.sign)
+	if (opt.sign || sign_tag) {
+		opt.sign = 1;
 		annotate = 1;
-	if (argc == 0 && !cmdmode)
-		cmdmode = 'l';
+	}
 
 	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
-- 
2.7.0


-- 
Laurent

Re: [PATCH] Add the tag.gpgsign option to sign all created tags

[Jeff King]

On Sat, Mar 19, 2016 at 07:23:10PM +0100, Laurent Arnoud wrote:

> The tag.gpgsign config option allows to sign all
> commits automatically.

We have commit.gpgsign, so this makes some sense. Would you want to sign
_all_ tags created with "git tag", including lightweight tags, or only
those that would already create a tag object (i.e., annotated tags)?

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..53cad28 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
>  };
>  
>  static unsigned int colopts;
> +static const char *sign_tag;
>  
>  static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
>  {
> @@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
>  	status = git_gpg_config(var, value, cb);
>  	if (status)
>  		return status;
> +	if (!strcmp(var, "tag.gpgsign")) {
> +		sign_tag = git_config_bool(var, value) ? "" : NULL;
> +		return 0;
> +	}

Why is "sign_tag" a pointer, and not simply an "int"?

If it is just representing the config, should it perhaps be given a more
specific name, like "sign_tag_config"?

> @@ -381,14 +387,21 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
>  
> +	if (argc == 0 && !cmdmode)
> +		cmdmode = 'l';
> +
> +	/* Remove config option when calling command other than create tag */
> +	if (cmdmode != 0 && sign_tag)
> +		sign_tag = NULL;
> +

Perhaps rather than rearranging the setup of cmdmode in this function,
you can just use have a conditional that makes explicit when this config
option kicks in, like:

  if (!cmdmode && sign_tag_config)
	opt.sign = 1;

That seems easier to follow to me (and then we don't have to check
sign_tag_config later; it acts as if the user gave "-s"). Although...

>  	if (keyid) {
>  		opt.sign = 1;
>  		set_signing_key(keyid);
>  	}
> -	if (opt.sign)
> +	if (opt.sign || sign_tag) {
> +		opt.sign = 1;
>  		annotate = 1;
> -	if (argc == 0 && !cmdmode)
> -		cmdmode = 'l';
> +	}
>  
>  	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
>  		usage_with_options(git_tag_usage, options);

How do I disable tag.gpgsign if I want an unsigned tag? I should be able
to do:

  git config tag.gpgsign true
  git tag --no-sign foo

but I don't think that works with your patch. You'd need to tweak the
handling of opt.sign to be able to tell the difference between
"--no-sign was given" and "there was no signing-related option given",
and only kick in the config in the latter case.

-Peff

Re: [PATCH] Add the tag.gpgsign option to sign all created tags

[Laurent Arnoud]

Hi Jeff,

On Sun, Mar 20, 2016 at 12:29:12AM -0400, Jeff King wrote:
> We have commit.gpgsign, so this makes some sense. Would you want to sign
> _all_ tags created with "git tag", including lightweight tags, or only
> those that would already create a tag object (i.e., annotated tags)?

Yes those that create a tag object (annotated tags).
All you suggestions make sense and I will prepare a patch v2.

Cheers,

-- 
Laurent

[PATCH v2] Add the tag.gpgsign option to sign all created tags

[Laurent Arnoud]

The `tag.gpgsign` config option allows to sign all
commits 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>
---
 Documentation/config.txt  |  3 +++
 Documentation/git-tag.txt |  4 ++++
 builtin/tag.c             | 21 ++++++++++++++++-----
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..076c68a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2729,6 +2729,9 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.gpgSign::
+	A boolean to specify whether all tags created should be GPG signed.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index abab481..757baa1 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -64,6 +64,10 @@ OPTIONS
 --sign::
 	Make a GPG-signed tag, using the default e-mail address's key.
 
+--no-sign::
+	Countermand `tag.gpgSign` configuration variable that is
+	set to force each and every tag to be signed.
+
 -u <keyid>::
 --local-user=<keyid>::
 	Make a GPG-signed tag, using the given key.
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..2a7b2f2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static unsigned int sign_tag_config;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
+	if (!strcmp(var, "tag.gpgsign")) {
+		sign_tag_config = git_config_bool(var, value) ? 1 : 0;
+		return 0;
+	}
+
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
 	return git_default_config(var, value, cb);
@@ -195,7 +201,7 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 
 static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 {
-	if (sign && do_sign(buf) < 0)
+	if (sign > 0 && do_sign(buf) < 0)
 		return error(_("unable to sign the tag"));
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error(_("unable to write tag file"));
@@ -204,7 +210,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 
 struct create_tag_options {
 	unsigned int message_given:1;
-	unsigned int sign;
+	int sign;
 	enum {
 		CLEANUP_NONE,
 		CLEANUP_SPACE,
@@ -378,17 +384,22 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
 	filter.lines = -1;
+	opt.sign = -1;
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+	if (argc == 0 && !cmdmode)
+		cmdmode = 'l';
+
+	if (!cmdmode && sign_tag_config && opt.sign != 0)
+		opt.sign = 1;
+
 	if (keyid) {
 		opt.sign = 1;
 		set_signing_key(keyid);
 	}
-	if (opt.sign)
+	if (opt.sign > 0)
 		annotate = 1;
-	if (argc == 0 && !cmdmode)
-		cmdmode = 'l';
 
 	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
-- 
2.7.0

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Ramsay Jones]

On 20/03/16 15:07, Laurent Arnoud wrote:
> The `tag.gpgsign` config option allows to sign all
> commits automatically.
  ^^^^^^^
presumably you meant tags.

> 
> Support `--no-sign` option to countermand configuration `tag.gpgsign`.
> 
> Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
> Reviewed-by: Jeff King <peff@peff.net>
> ---
>  Documentation/config.txt  |  3 +++
>  Documentation/git-tag.txt |  4 ++++
>  builtin/tag.c             | 21 ++++++++++++++++-----
>  3 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2cd6bdd..076c68a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2729,6 +2729,9 @@ submodule.<name>.ignore::
>  	"--ignore-submodules" option. The 'git submodule' commands are not
>  	affected by this setting.
>  
> +tag.gpgSign::
> +	A boolean to specify whether all tags created should be GPG signed.
> +
>  tag.sort::
>  	This variable controls the sort ordering of tags when displayed by
>  	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index abab481..757baa1 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -64,6 +64,10 @@ OPTIONS
>  --sign::
>  	Make a GPG-signed tag, using the default e-mail address's key.
>  
> +--no-sign::
> +	Countermand `tag.gpgSign` configuration variable that is
> +	set to force each and every tag to be signed.
> +

I assume that, after setting tag.gpgsign in the config, this is the
only way to get a lightweight tag. Maybe here, or above in config.txt,
this could be stated more obviously? dunno.

[Also, don't we normally describe --[no]-sign options together?]

ATB,
Ramsay Jones

Re: [PATCH] Add the tag.gpgsign option to sign all created tags

[Jeff King]

On Sun, Mar 20, 2016 at 01:20:23PM +0100, Laurent Arnoud wrote:

> Hi Jeff,
> 
> On Sun, Mar 20, 2016 at 12:29:12AM -0400, Jeff King wrote:
> > We have commit.gpgsign, so this makes some sense. Would you want to sign
> > _all_ tags created with "git tag", including lightweight tags, or only
> > those that would already create a tag object (i.e., annotated tags)?
> 
> Yes those that create a tag object (annotated tags).
> All you suggestions make sense and I will prepare a patch v2.

That behavior makes more sense to me, but I don't think it's what your
patch does (v1 or v2). Perhaps it would make sense to add some tests,
both to verify that it is behaving as expected, and to protect that
behavior from future changes?

-Peff

Re: [PATCH] Add the tag.gpgsign option to sign all created tags

[Laurent Arnoud]

On Sun, Mar 20, 2016 at 12:52:17PM -0400, Jeff King wrote:
> That behavior makes more sense to me, but I don't think it's what your
> patch does (v1 or v2). Perhaps it would make sense to add some tests,
> both to verify that it is behaving as expected, and to protect that
> behavior from future changes?

Yes, I will try to do so during the week.

Cheers,

-- 
Laurent

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Junio C Hamano]

Laurent Arnoud <laurent@spkdev.net> writes:

> The `tag.gpgsign` config option allows to sign all
> commits automatically.

I presume that you meant "all annotated tags" here.  But I am not
sure it this is sensible.

> Support `--no-sign` option to countermand configuration `tag.gpgsign`.

That sound quite counter-intuitive.

    $ git tag -s -m "my message" v1.0

is an explicit request to create a signed tag, as opposed to


    $ git tag -a -m "my message" v1.0

is an explicit request to create an unsigned annotated tag.  So 

I think a short-hand

    $ git tag -m "my message" v1.0

falls back to annotated and not signed tag, and I can understand
if the patch is about allowing the user to tweak this fallback to
create signed tag instead.

So I do not see why you need a new --no-sign option at all.  If
you have the configuration and you do want to create an unsigned
annotated tag one-shot, all you need is to explicitly ask for "-a"
i.e.

    $ git tag -a -m "my message" v1.0

isn't it?

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

then I would first have to say that is a bad idea.

I can sort-of understand (but do not necessarily agree that it is a
good idea) adding new two configurations, i.e.

 - "even without -a/-s, force the user to annotate the tag" is one
   configuration, and

 - "even when the user did not say -s, force the user to sign an
   annotated tag" is the other.

And with such a system, I can see why you would need an option
"--lightweight" to force creation of a light-weight tag (i.e. to
countermand the first one).  You can view this new option as
something that sits next to existing -a/-s.  The current system lets
user choose among the three variants (lightweight, annotated and
signed) by not giving any, giving -a, and giving -s option
respectively, but with the "--lightweight" option, the user can ask
for one of the three explicitly, as opposed to using "lack of either
-a/-s" as a signal to create lightweight one.

And in the context of such a system, "--no-sign" may make sense to
override the second configuration (i.e. "force the user to sign an
annotated tag").

But otherwise, adding only "--no-sign" does not make much sense to
me, as it implies "not signing always means annotated", which is not
true.  It is unclear between lightweight and annotated which one the
user who says "--no-sign" really wants.

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Laurent Arnoud]

On Sun, Mar 20, 2016 at 10:50:48PM -0700, Junio C Hamano wrote:
> > The `tag.gpgsign` config option allows to sign all
> > commits automatically.
> 
> I presume that you meant "all annotated tags" here.  But I am not
> sure it this is sensible.

Yes its a mistake.

> > Support `--no-sign` option to countermand configuration `tag.gpgsign`.
> So I do not see why you need a new --no-sign option at all.  If
> you have the configuration and you do want to create an unsigned
> annotated tag one-shot, all you need is to explicitly ask for "-a"
> i.e.
> 
>     $ git tag -a -m "my message" v1.0
> 
> isn't it?

You know that when you have sign configuration enabled globally annotate is
implicite, so its difficult to join both world. I use same idea as in your
patch `55ca3f99ae4895605a348322dd2fc50f2065f508`.

> 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.
I tried to fix a need I have currently and this is a good compromise for me.

> then I would first have to say that is a bad idea.
> 
> I can sort-of understand (but do not necessarily agree that it is a
> good idea) adding new two configurations, i.e.
> 
>  - "even without -a/-s, force the user to annotate the tag" is one
>    configuration, and
> 
>  - "even when the user did not say -s, force the user to sign an
>    annotated tag" is the other.
> 
> And with such a system, I can see why you would need an option
> "--lightweight" to force creation of a light-weight tag (i.e. to
> countermand the first one).  You can view this new option as
> something that sits next to existing -a/-s.  The current system lets
> user choose among the three variants (lightweight, annotated and
> signed) by not giving any, giving -a, and giving -s option
> respectively, but with the "--lightweight" option, the user can ask
> for one of the three explicitly, as opposed to using "lack of either
> -a/-s" as a signal to create lightweight one.
> 
> And in the context of such a system, "--no-sign" may make sense to
> override the second configuration (i.e. "force the user to sign an
> annotated tag").
> 
> But otherwise, adding only "--no-sign" does not make much sense to
> me, as it implies "not signing always means annotated", which is not
> true.  It is unclear between lightweight and annotated which one the
> user who says "--no-sign" really wants.

As I said it's difficult to easily join both world, as you know with
configuration and command line options. This is an override and if its really a
no go for this patch without refactoring this I will stop my work on it.

Just let me know I will send a patch v3 updated with tests after this.

Cheers,

-- 
Laurent

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

[Laurent Arnoud]

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>
---
 Documentation/config.txt  |  5 +++++
 Documentation/git-tag.txt |  6 +++++-
 builtin/tag.c             | 21 ++++++++++++++++-----
 t/t7004-tag.sh            | 45 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..ba9d4da 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2729,6 +2729,11 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+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.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index abab481..180edd2 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -9,7 +9,7 @@ git-tag - Create, list, delete or verify a tag object signed with GPG
 SYNOPSIS
 --------
 [verse]
-'git tag' [-a | -s | -u <keyid>] [-f] [-m <msg> | -F <file>]
+'git tag' [-a | -s | --no-sign | -u <keyid>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
@@ -64,6 +64,10 @@ OPTIONS
 --sign::
 	Make a GPG-signed tag, using the default e-mail address's key.
 
+--no-sign::
+	Countermand `tag.gpgSign` configuration variable that is
+	set to force annoted tags to be signed.
+
 -u <keyid>::
 --local-user=<keyid>::
 	Make a GPG-signed tag, using the given key.
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..2a7b2f2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static unsigned int sign_tag_config;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
+	if (!strcmp(var, "tag.gpgsign")) {
+		sign_tag_config = git_config_bool(var, value) ? 1 : 0;
+		return 0;
+	}
+
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
 	return git_default_config(var, value, cb);
@@ -195,7 +201,7 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 
 static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 {
-	if (sign && do_sign(buf) < 0)
+	if (sign > 0 && do_sign(buf) < 0)
 		return error(_("unable to sign the tag"));
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error(_("unable to write tag file"));
@@ -204,7 +210,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 
 struct create_tag_options {
 	unsigned int message_given:1;
-	unsigned int sign;
+	int sign;
 	enum {
 		CLEANUP_NONE,
 		CLEANUP_SPACE,
@@ -378,17 +384,22 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
 	filter.lines = -1;
+	opt.sign = -1;
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+	if (argc == 0 && !cmdmode)
+		cmdmode = 'l';
+
+	if (!cmdmode && sign_tag_config && opt.sign != 0)
+		opt.sign = 1;
+
 	if (keyid) {
 		opt.sign = 1;
 		set_signing_key(keyid);
 	}
-	if (opt.sign)
+	if (opt.sign > 0)
 		annotate = 1;
-	if (argc == 0 && !cmdmode)
-		cmdmode = 'l';
 
 	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index cf3469b..4e45361 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -775,6 +775,51 @@ 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
+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
+
+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
+
+get_tag_header config-disabled-gpgsign $commit commit $time >expect
+echo "A message" >>expect
+echo '-----BEGIN PGP SIGNATURE-----' >>expect
+git config tag.gpgsign false
+test_expect_success GPG \
+	'git tag --sign enable GPG sign' \
+	'git tag --sign -m "A message" config-disabled-gpgsign &&
+	get_tag_msg config-disabled-gpgsign >actual &&
+	test_cmp expect actual
+'
+git config --unset tag.gpgsign
+
+git config tag.gpgsign true
+test_expect_success GPG \
+	'git tag --no-sign disable configured tag.gpgsign and annotate' \
+	'git tag --no-sign config-non-annotated-tag &&
+	tag_exists config-non-annotated-tag &&
+	test_must_fail git tag -v config-non-annotated-tag
+'
+git config --unset tag.gpgsign
+
 test_expect_success GPG \
 	'trying to create a signed tag with non-existing -F file should fail' '
 	! test -f nonexistingfile &&
-- 
2.7.0

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

[Jeff King]

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

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

> +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);

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

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Junio C Hamano]

Laurent Arnoud <laurent@spkdev.net> writes:

>> > Support `--no-sign` option to countermand configuration `tag.gpgsign`.
>> So I do not see why you need a new --no-sign option at all.  If
>> you have the configuration and you do want to create an unsigned
>> annotated tag one-shot, all you need is to explicitly ask for "-a"
>> i.e.
>> 
>>     $ git tag -a -m "my message" v1.0
>> 
>> isn't it?
>
> You know that when you have sign configuration enabled globally annotate is
> implicite, so its difficult to join both world.

Sorry, I am not sure what you mean by that.  It is unclear what two
worlds you are referring to.

> I use same idea as in your patch
> `55ca3f99ae4895605a348322dd2fc50f2065f508`.

That is not a good comparison.  55ca3f99 (commit-tree: add and
document --no-gpg-sign, 2013-12-13) is about signed commit, and over
there there are only two choices, i.e. a commit that corresponds to
an annotated tag, and a signed commit that corresponds to a signed
tag.  There is no "lightweight-tag" equivalent.

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

Good.

If so, then the configuration is "when the user gives us a message
to create a tag without explicitly saying -a/-s, we create an
annotated tag by default, but create a signed tag instead in such a
case", I would think.  That is:

    $ git tag -m 'foo' $tagname

would create signed tag under such a configuration option, and I
think such an option may make sense.  And the way to override it
would be

    $ git tag -a -m 'foo' $tagname

So there is no need for --no-sign option.  When the user explicitly
asks to create an annotated tag with

    $ git tag -a -m 'foo' $tagname

it is unreasonable to override that explicit wish with a
configuration setting.

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Jeff King]

On Sun, Mar 20, 2016 at 10:50:48PM -0700, Junio C Hamano wrote:

> > Support `--no-sign` option to countermand configuration `tag.gpgsign`.
> 
> That sound quite counter-intuitive.
> [...]

I was the one who suggested --no-sign, as we usually like to have a way
to countermand the config. But having read your message, I agree that is
probably not the right mental model.

In particular, this:

> I can sort-of understand (but do not necessarily agree that it is a
> good idea) adding new two configurations, i.e.
> 
>  - "even without -a/-s, force the user to annotate the tag" is one
>    configuration, and
> 
>  - "even when the user did not say -s, force the user to sign an
>    annotated tag" is the other.
> 
> And with such a system, I can see why you would need an option
> "--lightweight" to force creation of a light-weight tag (i.e. to
> countermand the first one).  You can view this new option as
> something that sits next to existing -a/-s.  The current system lets
> user choose among the three variants (lightweight, annotated and
> signed) by not giving any, giving -a, and giving -s option
> respectively, but with the "--lightweight" option, the user can ask
> for one of the three explicitly, as opposed to using "lack of either
> -a/-s" as a signal to create lightweight one.

makes sense to me (though like you, I do not necessarily think it is a
good idea and would not use it myself).

Another similar approach would be to collapse this down to a single
variable that selects from the options. IOW:

  1. Add --lightweight for explicitly adding a lightweight tag.

  2. When we are creating a tag and none of "-a", "-s", or
     "--lightweight" is given, use the default given in
     tag.defaultTagType (or whatever we call it), which can
     be "lightweight", "annotated", or "signed".

  3. tag.defaultTagType defaults to "lightweight".

That is conceptually simpler to me, with the main differences being:

  - in yours, the second config would mean that an explicit "-a" implies
    "-s" (unless the user says --no-sign).

  - in mine, there is no way to kick in the signing _only_ when we are
    annotating. If you configure "signed", then you have to explicitly
    say "--lightweight" for lightweight tags.

I dunno. It sounds like Laurent would set it to "signed", and that would
do what he wants. But like I said, I would not plan to use the feature
myself, and I could see it ending up a little bit annoying when you _do_
want a lightweight tag.

-Peff

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Laurent Arnoud]

On Mon, Mar 21, 2016 at 12:43:45PM -0700, Junio C Hamano wrote:
> > You know that when you have sign configuration enabled globally annotate is
> > implicite, so its difficult to join both world.
> 
> Sorry, I am not sure what you mean by that.  It is unclear what two
> worlds you are referring to.

Command line options and configuration, but forget about it.

> 
> > I use same idea as in your patch
> > `55ca3f99ae4895605a348322dd2fc50f2065f508`.
> 
> That is not a good comparison.  55ca3f99 (commit-tree: add and
> document --no-gpg-sign, 2013-12-13) is about signed commit, and over
> there there are only two choices, i.e. a commit that corresponds to
> an annotated tag, and a signed commit that corresponds to a signed
> tag.  There is no "lightweight-tag" equivalent.
> 
> >> 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.
> 
> Good.
> 
> If so, then the configuration is "when the user gives us a message
> to create a tag without explicitly saying -a/-s, we create an
> annotated tag by default, but create a signed tag instead in such a
> case", I would think.  That is:
> 
>     $ git tag -m 'foo' $tagname
> 
> would create signed tag under such a configuration option, and I
> think such an option may make sense.  And the way to override it
> would be
> 
>     $ git tag -a -m 'foo' $tagname
> 
> So there is no need for --no-sign option.  When the user explicitly
> asks to create an annotated tag with
> 
>     $ git tag -a -m 'foo' $tagname
> 
> it is unreasonable to override that explicit wish with a
> configuration setting.
> 

Ah, I think I understand now, I think this will not take to much effort to fix.

-- 
Laurent

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Jeff King]

On Mon, Mar 21, 2016 at 12:43:45PM -0700, Junio C Hamano wrote:

> If so, then the configuration is "when the user gives us a message
> to create a tag without explicitly saying -a/-s, we create an
> annotated tag by default, but create a signed tag instead in such a
> case", I would think.  That is:
> 
>     $ git tag -m 'foo' $tagname
> 
> would create signed tag under such a configuration option, and I
> think such an option may make sense.  And the way to override it
> would be
> 
>     $ git tag -a -m 'foo' $tagname
> 
> So there is no need for --no-sign option.  When the user explicitly
> asks to create an annotated tag with
> 
>     $ git tag -a -m 'foo' $tagname
> 
> it is unreasonable to override that explicit wish with a
> configuration setting.

FWIW, of the schemes discussed in this thread, this one makes the most
sense to me (and is one I might actually use).

I think you could shoe-horn it into my "tag-type" config scheme as a new
mode ("sign-if-message" or something), but I think it is much clearer as
a standalone config variable.

Thanks for a dose of sanity.

-Peff

[PATCH v4] Add the tag.gpgsign option to sign annotated tags

[Laurent Arnoud]

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

`--annotate` command line option disable configuration `tag.gpgsign`.

Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
---
 Documentation/config.txt |  5 +++++
 builtin/tag.c            | 20 +++++++++++++++-----
 t/t7004-tag.sh           | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..62f7d2a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2729,6 +2729,11 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.gpgSign::
+	A boolean to specify whether annotated tags created should be GPG signed.
+	If `--annotate` is specified on the command line, it takes
+	precedence over this option.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..748aeac 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static unsigned int sign_tag_config;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
+	if (!strcmp(var, "tag.gpgsign")) {
+		sign_tag_config = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
 	return git_default_config(var, value, cb);
@@ -326,7 +332,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int create_reflog = 0;
-	int annotate = 0, force = 0;
+	int annotate = -1, force = 0;
 	int cmdmode = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
@@ -381,16 +387,20 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+	if (argc == 0 && !cmdmode)
+		cmdmode = 'l';
+
+	if (!cmdmode && sign_tag_config && annotate < 1)
+		opt.sign = 1;
+
 	if (keyid) {
 		opt.sign = 1;
 		set_signing_key(keyid);
 	}
 	if (opt.sign)
 		annotate = 1;
-	if (argc == 0 && !cmdmode)
-		cmdmode = 'l';
 
-	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
+	if ((annotate > 0 || msg.given || msgfile || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
 
 	finalize_colopts(&colopts, -1);
@@ -474,7 +484,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
-	if (annotate)
+	if (annotate > 0)
 		create_tag(object, tag, &buf, &opt, prev, object);
 
 	transaction = ref_transaction_begin(&err);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index cf3469b..7791d00 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.gpgsign' \
+	'test_config tag.gpgsign true &&
+	GIT_EDITOR=./fakeeditor git tag config-implied-annotate &&
+	get_tag_msg config-implied-annotate >actual &&
+	test_cmp expect actual
+'
+
+get_tag_header config-implied-annotate-disabled $commit commit $time >expect
+echo "A message" >>expect
+test_expect_success GPG \
+	'git tag -a disable configured tag.gpgsign' \
+	'test_config tag.gpgsign true &&
+	git tag -a -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
+'
+
+get_tag_header config-disabled-gpgsign $commit commit $time >expect
+echo "A message" >>expect
+echo '-----BEGIN PGP SIGNATURE-----' >>expect
+test_expect_success GPG \
+	'git tag --sign enable GPG sign' \
+	'test_config tag.gpgsign false &&
+	git tag --sign -m "A message" config-disabled-gpgsign &&
+	get_tag_msg config-disabled-gpgsign >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success GPG \
 	'trying to create a signed tag with non-existing -F file should fail' '
 	! test -f nonexistingfile &&
-- 
2.7.0

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

[Junio C Hamano]

Laurent Arnoud <laurent@spkdev.net> writes:

> @@ -326,7 +332,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	struct create_tag_options opt;
>  	char *cleanup_arg = NULL;
>  	int create_reflog = 0;
> -	int annotate = 0, force = 0;
> +	int annotate = -1, force = 0;

I really do not like this part of the change.  The original code is
already bad in the sense that it uses "annotate" to mean "we need to
create a tag object, not just a lightweight tag" in some places
which already needs careful reading, but with this it no longer is
clear what this argument even means.  It sometimes means "we are
creating a tag object" and some other times means "we did not see
'-a' on the command line" but only until the variable gets assigned
by existing "if opt.sign is there, set it to true" tweaking.

I wonder if an approach like the following would result in a cleaner
code that is easier to maintain:

 - Leave this to be initialized to 0, allow it to be toggled upon
   seeing "-a" via the parse_options() mechanism, but redefine its
   meaning to be "-a option was passed".  Make it never be touched
   by anything else (e.g. "if (opt.sign) annotate = 1;" is wrong).

 - Introduce a new "create_tag_object" variable, initialized to 0.
   Existing code that sets "annotate = 1" (e.g. "-s" is passed,
   "-m/-F" is passed, etc.) will instead set this variable.  The
   condition of if() statement to call create_tag() would also be
   looking at this variable, not "annotate".

 - Before calling create_tag(), look at the value of your new
   configuration and "annotate", and set opt.sign as needed.

A patch to do so might look something like this (totally untested).


 builtin/tag.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..d262f92 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static int force_sign_annotate;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -168,6 +169,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 		return status;
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
+	if (!strcmp(var, "tag.forcesignannotated")) {
+		force_sign_annotate = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -327,7 +332,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 +390,12 @@ 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)
+		create_tag_object = 1;
 	if (argc == 0 && !cmdmode)
 		cmdmode = 'l';
 
-	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
+	if ((create_tag_object || msg.given || msgfile || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
 
 	finalize_colopts(&colopts, -1);
@@ -431,7 +436,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;
 		if (msg.given)
 			strbuf_addbuf(&buf, &(msg.buf));
 		else {
@@ -474,8 +479,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);
+	}
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||

Re: [PATCH v2] Add the tag.gpgsign option to sign all created tags

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Eh, it seems that your test actually expects this kind of
interaction, though.

This piece from your PATCH v4:

    diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
    index cf3469b..7791d00 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.gpgsign' \
    +	'test_config tag.gpgsign true &&
    +	GIT_EDITOR=./fakeeditor git tag config-implied-annotate &&
    +	get_tag_msg config-implied-annotate >actual &&
    +	test_cmp expect actual
    +'
    +

wants to see that "git tag config-implied-annotate" (which looks
exactly like "git tag v1.0" we discussed above) runs the fakeeditor
and makes a signed tag.

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

[Laurent Arnoud]

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

`--annotate` command line option disable configuration
`tag.forcesignannotated`.

Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
---
 Documentation/config.txt |  5 +++++
 builtin/tag.c            | 22 ++++++++++++++++------
 t/t7004-tag.sh           | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..95d5c9d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2729,6 +2729,11 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.forceSignAnnotated::
+	A boolean to specify whether annotated tags created should be GPG signed.
+	If `--annotate` is specified on the command line, it takes
+	precedence over this option.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..c447738 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static int force_sign_annotate;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
+	if (!strcmp(var, "tag.forcesignannotated")) {
+		force_sign_annotate = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
 	return git_default_config(var, value, cb);
@@ -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;
+
 	if (argc == 0 && !cmdmode)
 		cmdmode = 'l';
 
-	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
+	if ((create_tag_object || msg.given || msgfile || force) && (cmdmode != 0))
 		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;
 		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);
+    }
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
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 &&
+	get_tag_msg config-implied-annotate >actual &&
+	test_cmp expect actual
+'
+
+get_tag_header config-implied-annotate-disabled $commit commit $time >expect
+echo "A message" >>expect
+test_expect_success GPG \
+	'git tag -a disable configured tag.forcesignannotated' \
+	'test_config tag.forcesignannotated true &&
+	git tag -a -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
+'
+
+get_tag_header config-disabled-forcesignannotated $commit commit $time >expect
+echo "A message" >>expect
+echo '-----BEGIN PGP SIGNATURE-----' >>expect
+test_expect_success GPG \
+	'git tag --sign enable GPG sign' \
+	'test_config tag.forcesignannotated false &&
+	git tag --sign -m "A message" config-disabled-forcesignannotated &&
+	get_tag_msg config-disabled-forcesignannotated >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success GPG \
 	'trying to create a signed tag with non-existing -F file should fail' '
 	! test -f nonexistingfile &&
-- 
2.7.0

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

[Junio C Hamano]

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.

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

[Laurent Arnoud]

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

[PATCH v6] Add the option to force sign annotated tags

[Laurent Arnoud]

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

`--annotate` command line option disable configuration
`tag.forcesignannotated`.

Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
---
 Documentation/config.txt |  5 +++++
 builtin/tag.c            | 20 ++++++++++++++------
 t/t7004-tag.sh           | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..95d5c9d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2729,6 +2729,11 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.forceSignAnnotated::
+	A boolean to specify whether annotated tags created should be GPG signed.
+	If `--annotate` is specified on the command line, it takes
+	precedence over this option.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..528a1ba 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static int force_sign_annotate;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -166,6 +167,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
+	if (!strcmp(var, "tag.forcesignannotated")) {
+		force_sign_annotate = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
 	return git_default_config(var, value, cb);
@@ -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,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		opt.sign = 1;
 		set_signing_key(keyid);
 	}
-	if (opt.sign)
-		annotate = 1;
+	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 || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
 
 	finalize_colopts(&colopts, -1);
@@ -431,7 +437,6 @@ 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;
 		if (msg.given)
 			strbuf_addbuf(&buf, &(msg.buf));
 		else {
@@ -474,8 +479,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);
+	}
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index cf3469b..f9b7d79 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -775,6 +775,47 @@ test_expect_success GPG '-s implies annotated tag' '
 	test_cmp expect actual
 '
 
+get_tag_header forcesignannotated-implied-sign $commit commit $time >expect
+echo "A message" >>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 tag -m "A message" forcesignannotated-implied-sign &&
+	get_tag_msg forcesignannotated-implied-sign >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG \
+	'lightweight with no message when configured with tag.forcesignannotated' \
+	'test_config tag.forcesignannotated true &&
+	git tag forcesignannotated-lightweight &&
+	tag_exists forcesignannotated-lightweight &&
+	test_must_fail git tag -v forcesignannotated-no-message
+'
+
+get_tag_header forcesignannotated-annotate $commit commit $time >expect
+echo "A message" >>expect
+test_expect_success GPG \
+	'git tag -a disable configured tag.forcesignannotated' \
+	'test_config tag.forcesignannotated true &&
+	git tag -a -m "A message" forcesignannotated-annotate &&
+	get_tag_msg forcesignannotated-annotate >actual &&
+	test_cmp expect actual &&
+	test_must_fail git tag -v forcesignannotated-annotate
+'
+
+get_tag_header forcesignannotated-disabled $commit commit $time >expect
+echo "A message" >>expect
+echo '-----BEGIN PGP SIGNATURE-----' >>expect
+test_expect_success GPG \
+	'git tag --sign enable GPG sign' \
+	'test_config tag.forcesignannotated false &&
+	git tag --sign -m "A message" forcesignannotated-disabled &&
+	get_tag_msg forcesignannotated-disabled >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success GPG \
 	'trying to create a signed tag with non-existing -F file should fail' '
 	! test -f nonexistingfile &&
-- 
2.7.0