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

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
Reviewed-by: Jeff King 
---
 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..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=" 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 ] [-f] [-m  | -F ]
+'git tag' [-a | -s | --no-sign | -u ] [-f] [-m  | -F ]
 [ | ]
 'git tag' -d ...
 'git tag' [-n[]] -l [--contains ] [--points-at ]
@@ -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 ::
 --local-user=::
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", );
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(, 0, sizeof(opt));
memset(, 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