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

Laurent Arnoud [off-list ref] 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.

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

quoted

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.

quoted

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

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 <redacted>
Reviewed-by: Jeff King <redacted>
---
 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

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 <redacted>
Reviewed-by: Jeff King <redacted>

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

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

quoted

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