Tigran MkrtchyanAs there was some interest on my original pull request on github[1], I decided to rebase, update the commit message and re-post this patch.
[1] https://github.com/git/git/pull/419
threads / discuss / 51237
config option to force all tags be GPG-signed - comeback
Subject: config option to force all tags be GPG-signed - comeback
Tigran MkrtchyanTigran Mkrtchyan[PATCH v2] tag: add tag.gpgSign config option to force all tags be GPG-signed
As may CI/CD tools don't allow to control command line options when executing `git tag` command, a default value in the configuration file will allow to enforce tag signing if required.
The new config-file option tag.gpgSign enforces signed tags. Additional command line option --no-gpg-sign is added to disable such behavior if needed. E.g.:
$ git tag -m "commit message"
will generate a GPG signed tag if tag.gpgSign option is true, while
$ git tag --no-gpg-sign -m "commit message"
will skip the signing step.
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> --- Documentation/git-tag.txt | 7 +++++++ builtin/tag.c | 18 +++++++++++++++--- t/t7004-tag.sh | 21 +++++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index a74e7b926d..d9dbfb4e37 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -64,6 +64,9 @@ OPTIONS -s:: --sign:: Make a GPG-signed tag, using the default e-mail address's key. + The default behabior of tag GPG-signing controlled by `tag.gpgSign` + configuration variable if it exists, or disabled oder otherwise. + See linkgit:git-config[1]. -u <keyid>:: --local-user=<keyid>:: @@ -193,6 +196,10 @@ This option is only applicable when listing tags without annotation lines. that of linkgit:git-for-each-ref[1]. When unspecified, defaults to `%(refname:strip=2)`. +--no-gpg-sign:: + Countermand `tag.gpgSign` configuration variable that is + set to force each and every tag to be signed. + <tagname>:: The name of the tag to create, delete, or describe. The new tag name must pass all checks defined by diff --git a/builtin/tag.c b/builtin/tag.c index ef37dccf86..7f9aef4840 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -33,6 +33,7 @@ static const char * const git_tag_usage[] = { static unsigned int colopts; static int force_sign_annotate; +static int sign_tag; static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format) @@ -144,6 +145,11 @@ static int git_tag_config(const char *var, const char *value, void *cb) int status; struct ref_sorting **sorting_tail = (struct ref_sorting **)cb; + if (!strcmp(var, "tag.gpgsign")) { + sign_tag = git_config_bool(var, value); + return 0; + } + if (!strcmp(var, "tag.sort")) { if (!value) return config_error_nonbool(var); @@ -392,6 +398,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) struct ref_format format = REF_FORMAT_INIT; int icase = 0; int edit_flag = 0; + int no_gpg_sign = 0; struct option options[] = { OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'), { OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"), @@ -413,6 +420,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) N_("use another key to sign the tag")), OPT__FORCE(&force, N_("replace the tag if exists"), 0), OPT_BOOL(0, "create-reflog", &create_reflog, N_("create a reflog")), + OPT_BOOL(0, "no-gpg-sign", &no_gpg_sign, N_("do not GPG-sign tag")), OPT_GROUP(N_("Tag listing options")), OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")), @@ -445,6 +453,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0); + if (no_gpg_sign) { + sign_tag = 0; + } + if (keyid) { opt.sign = 1; set_signing_key(keyid); @@ -463,7 +475,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (cmdmode == 'l') setup_auto_pager("tag", 1); - if ((create_tag_object || force) && (cmdmode != 0)) + if ((create_tag_object || force || no_gpg_sign) && (cmdmode != 0)) usage_with_options(git_tag_usage, options); finalize_colopts(&colopts, -1); @@ -556,8 +568,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) create_reflog_msg(&object, &reflog_msg); - if (create_tag_object) { - if (force_sign_annotate && !annotate) + if (create_tag_object || sign_tag) { + if (sign_tag || (force_sign_annotate && !annotate)) opt.sign = 1; create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object); } diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 6aeeb279a0..98a07a29d2 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -932,6 +932,27 @@ test_expect_success GPG \ test_cmp expect actual ' +get_tag_header gpgsign-enabled $commit commit $time >expect +echo "A message" >>expect +echo '-----BEGIN PGP SIGNATURE-----' >>expect +test_expect_success GPG \ + 'git tag configured tag.gpgsign enables GPG sign' \ + 'test_config tag.gpgsign true && + git tag -m "A message" gpgsign-enabled && + get_tag_msg gpgsign-enabled>actual && + test_cmp expect actual +' + +get_tag_header no-gpg-sign $commit commit $time >expect +echo "A message" >>expect +test_expect_success GPG \ + 'git tag --no-gpg-sign configured tag.gpgsign skip GPG sign' \ + 'test_config tag.gpgsign true && + git tag -a --no-gpg-sign -m "A message" no-gpg-sign && + get_tag_msg no-gpg-sign>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.21.0
Johannes SchindelinRe: [PATCH v2] tag: add tag.gpgSign config option to force all tags be GPG-signed
Hi Tigran,
On Tue, 4 Jun 2019, Tigran Mkrtchyan wrote:
> As may CI/CD tools don't allow to control command line options when
s/may/many/, maybe?
Show 19 quoted lines
> executing `git tag` command, a default value in the configuration file > will allow to enforce tag signing if required. > > The new config-file option tag.gpgSign enforces signed tags. Additional > command line option --no-gpg-sign is added to disable such behavior if > needed. E.g.: > > $ git tag -m "commit message" > > will generate a GPG signed tag if tag.gpgSign option is true, while > > $ git tag --no-gpg-sign -m "commit message" > > will skip the signing step. > > Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> > --- > Documentation/git-tag.txt | 7 +++++++ > builtin/tag.c | 18 +++++++++++++++---
How about adding a section to Documentation/config/tag.txt as well?
Even better: it could be modeled after the description of commit.gpgsign to which Stefan Beller linked in his review of your earlier patch (see https://public-inbox.org/git/20131105112840.GZ4589@mars-attacks.org/):
tag.gpgsign:: A boolean to specify whether all tags should be GPG signed. Use of this option when running in an automated script can result in a large number of tags being signed. It is therefore convenient to use an agent to avoid typing your gpg passphrase several times.
Show 12 quoted lines
> t/t7004-tag.sh | 21 +++++++++++++++++++++ > 3 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt > index a74e7b926d..d9dbfb4e37 100644 > --- a/Documentation/git-tag.txt > +++ b/Documentation/git-tag.txt > @@ -64,6 +64,9 @@ OPTIONS > -s:: > --sign:: > Make a GPG-signed tag, using the default e-mail address's key. > + The default behabior of tag GPG-signing controlled by `tag.gpgSign`
s/behabior/behavior/ maybe?
And I would also insert an "is" before "controlled".
Show 10 quoted lines
> + configuration variable if it exists, or disabled oder otherwise. > + See linkgit:git-config[1]. > > -u <keyid>:: > --local-user=<keyid>:: > @@ -193,6 +196,10 @@ This option is only applicable when listing tags without annotation lines. > that of linkgit:git-for-each-ref[1]. When unspecified, > defaults to `%(refname:strip=2)`. > > +--no-gpg-sign::
Should this not be `--no-sign`? There is already a `--sign` option, and I would wager a guess that it automagically handles `--no-sign`...
> + Countermand `tag.gpgSign` configuration variable that is > + set to force each and every tag to be signed.
If you replace "Countermand" by "Override", you could simply merge this into the section talking about `--sign`...
Show 9 quoted lines
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ef37dccf86..7f9aef4840 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -33,6 +33,7 @@ static const char * const git_tag_usage[] = {
>
> static unsigned int colopts;
> static int force_sign_annotate;
> +static int sign_tag;Since this holds the value parsed from the config, I would like to see some indication of that in the name. Maybe something like `config_sign_tag`?
Also, I would recommend to initialize it with `-1` to be able to discern between the three states `true`, `false` and `unspecified`.
Show 38 quoted lines
> static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
> struct ref_format *format)
> @@ -144,6 +145,11 @@ static int git_tag_config(const char *var, const char *value, void *cb)
> int status;
> struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
>
> + if (!strcmp(var, "tag.gpgsign")) {
> + sign_tag = git_config_bool(var, value);
> + return 0;
> + }
> +
> if (!strcmp(var, "tag.sort")) {
> if (!value)
> return config_error_nonbool(var);
> @@ -392,6 +398,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> struct ref_format format = REF_FORMAT_INIT;
> int icase = 0;
> int edit_flag = 0;
> + int no_gpg_sign = 0;
> struct option options[] = {
> OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
> { OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
> @@ -413,6 +420,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> N_("use another key to sign the tag")),
> OPT__FORCE(&force, N_("replace the tag if exists"), 0),
> OPT_BOOL(0, "create-reflog", &create_reflog, N_("create a reflog")),
> + OPT_BOOL(0, "no-gpg-sign", &no_gpg_sign, N_("do not GPG-sign tag")),
>
> OPT_GROUP(N_("Tag listing options")),
> OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
> @@ -445,6 +453,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>
> argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
>
> + if (no_gpg_sign) {
> + sign_tag = 0;
> + }
> +Hmm. I'd rather like to see this folded into the `--no-sign` option implied by the `OPT_BOOL('s', "sign", ...)` part. The way this would work would be to initialize the struct like this:
struct create_tag_options opt = { .sign = -1 };then leave
OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),as-is, and later do this:
if (opt.sign < 0) opt.sign = config_sign_tag > 0;
Show 7 quoted lines
> @@ -463,7 +475,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> if (cmdmode == 'l')
> setup_auto_pager("tag", 1);
>
> - if ((create_tag_object || force) && (cmdmode != 0))
> + if ((create_tag_object || force || no_gpg_sign) && (cmdmode != 0))
> usage_with_options(git_tag_usage, options);Should we really try to be that strict? If so, we would have to test for `opt.sign > 0` here and make sure that above-mentioned `if (opt.sign < 0)` block comes *after* this block.
Show 9 quoted lines
> @@ -556,8 +568,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>
> create_reflog_msg(&object, &reflog_msg);
>
> - if (create_tag_object) {
> - if (force_sign_annotate && !annotate)
> + if (create_tag_object || sign_tag) {
> + if (sign_tag || (force_sign_annotate && !annotate))
> opt.sign = 1;This would probably be better handled via
- create_tag_object = (opt.sign || annotate || msg.given || msgfile); + create_tag_object = (opt.sign > 0 || config_sign_tag > 0 || + annotate || msg.given || msgfile);
earlier. After all, this assignment of `create_tag_object` suggests that it is a Boolean that catches *all* cases where a tag should be created.
But then, we would have to be very careful about this, as we don't want to error out when a user calls `git tag -l` while `tag.gpgSign = true`. So maybe instead:
- create_tag_object = (opt.sign || annotate || msg.given || msgfile); + create_tag_object = (opt.sign > 0 || annotate || msg.given || msgfile);
and later, after that test whether `(create_tag_object || force) && cmd_mode != 0` do:
if (opt.sign < 0) {
opt.sign = config_sign_tag > 0;
create_tag_object ||= opt.sign;Show 25 quoted lines
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 6aeeb279a0..98a07a29d2 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -932,6 +932,27 @@ test_expect_success GPG \ > test_cmp expect actual > ' > > +get_tag_header gpgsign-enabled $commit commit $time >expect > +echo "A message" >>expect > +echo '-----BEGIN PGP SIGNATURE-----' >>expect > +test_expect_success GPG \ > + 'git tag configured tag.gpgsign enables GPG sign' \ > + 'test_config tag.gpgsign true && > + git tag -m "A message" gpgsign-enabled && > + get_tag_msg gpgsign-enabled>actual && > + test_cmp expect actual > +' > + > +get_tag_header no-gpg-sign $commit commit $time >expect > +echo "A message" >>expect > +test_expect_success GPG \ > + 'git tag --no-gpg-sign configured tag.gpgsign skip GPG sign' \ > + 'test_config tag.gpgsign true && > + git tag -a --no-gpg-sign -m "A message" no-gpg-sign &&
With my idea above, this would of course become `--no-sign`.
Show 7 quoted lines
> + get_tag_msg no-gpg-sign>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 &&
Apart from the things I suggested in the hope to help you improve the patch, this looks good to me so far.
Thanks, Johannes
Mkrtchyan, TigranRe: [PATCH v2] tag: add tag.gpgSign config option to force all tags be GPG-signed
Hi Johannes,
Thanks for the comments. I update the patch ant re-post.
Tigran.
----- Original Message -----
Show 5 quoted lines
> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de> > To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de> > Cc: "git" <git@vger.kernel.org>, "Jonathan Nieder" <jrnieder@gmail.com> > Sent: Tuesday, June 4, 2019 4:33:08 PM > Subject: Re: [PATCH v2] tag: add tag.gpgSign config option to force all tags be GPG-signed
Show 239 quoted lines
> Hi Tigran,
>
> On Tue, 4 Jun 2019, Tigran Mkrtchyan wrote:
>
>> As may CI/CD tools don't allow to control command line options when
>
> s/may/many/, maybe?
>
>> executing `git tag` command, a default value in the configuration file
>> will allow to enforce tag signing if required.
>>
>> The new config-file option tag.gpgSign enforces signed tags. Additional
>> command line option --no-gpg-sign is added to disable such behavior if
>> needed. E.g.:
>>
>> $ git tag -m "commit message"
>>
>> will generate a GPG signed tag if tag.gpgSign option is true, while
>>
>> $ git tag --no-gpg-sign -m "commit message"
>>
>> will skip the signing step.
>>
>> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
>> ---
>> Documentation/git-tag.txt | 7 +++++++
>> builtin/tag.c | 18 +++++++++++++++---
>
> How about adding a section to Documentation/config/tag.txt as well?
>
> Even better: it could be modeled after the description of commit.gpgsign
> to which Stefan Beller linked in his review of your earlier patch (see
> https://public-inbox.org/git/20131105112840.GZ4589@mars-attacks.org/):
>
> tag.gpgsign::
> A boolean to specify whether all tags should be GPG signed.
> Use of this option when running in an automated script can
> result in a large number of tags being signed. It is therefore
> convenient to use an agent to avoid typing your gpg passphrase
> several times.
>
>> t/t7004-tag.sh | 21 +++++++++++++++++++++
>> 3 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index a74e7b926d..d9dbfb4e37 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -64,6 +64,9 @@ OPTIONS
>> -s::
>> --sign::
>> Make a GPG-signed tag, using the default e-mail address's key.
>> + The default behabior of tag GPG-signing controlled by `tag.gpgSign`
>
> s/behabior/behavior/ maybe?
>
> And I would also insert an "is" before "controlled".
>
>> + configuration variable if it exists, or disabled oder otherwise.
>> + See linkgit:git-config[1].
>>
>> -u <keyid>::
>> --local-user=<keyid>::
>> @@ -193,6 +196,10 @@ This option is only applicable when listing tags without
>> annotation lines.
>> that of linkgit:git-for-each-ref[1]. When unspecified,
>> defaults to `%(refname:strip=2)`.
>>
>> +--no-gpg-sign::
>
> Should this not be `--no-sign`? There is already a `--sign` option, and I
> would wager a guess that it automagically handles `--no-sign`...
>
>> + Countermand `tag.gpgSign` configuration variable that is
>> + set to force each and every tag to be signed.
>
> If you replace "Countermand" by "Override", you could simply merge this
> into the section talking about `--sign`...
>
>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index ef37dccf86..7f9aef4840 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -33,6 +33,7 @@ static const char * const git_tag_usage[] = {
>>
>> static unsigned int colopts;
>> static int force_sign_annotate;
>> +static int sign_tag;
>
> Since this holds the value parsed from the config, I would like to see
> some indication of that in the name. Maybe something like
> `config_sign_tag`?
>
> Also, I would recommend to initialize it with `-1` to be able to discern
> between the three states `true`, `false` and `unspecified`.
>
>> static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
>> struct ref_format *format)
>> @@ -144,6 +145,11 @@ static int git_tag_config(const char *var, const char
>> *value, void *cb)
>> int status;
>> struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
>>
>> + if (!strcmp(var, "tag.gpgsign")) {
>> + sign_tag = git_config_bool(var, value);
>> + return 0;
>> + }
>> +
>> if (!strcmp(var, "tag.sort")) {
>> if (!value)
>> return config_error_nonbool(var);
>> @@ -392,6 +398,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> struct ref_format format = REF_FORMAT_INIT;
>> int icase = 0;
>> int edit_flag = 0;
>> + int no_gpg_sign = 0;
>> struct option options[] = {
>> OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>> { OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
>> @@ -413,6 +420,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> N_("use another key to sign the tag")),
>> OPT__FORCE(&force, N_("replace the tag if exists"), 0),
>> OPT_BOOL(0, "create-reflog", &create_reflog, N_("create a reflog")),
>> + OPT_BOOL(0, "no-gpg-sign", &no_gpg_sign, N_("do not GPG-sign tag")),
>>
>> OPT_GROUP(N_("Tag listing options")),
>> OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
>> @@ -445,6 +453,10 @@ int cmd_tag(int argc, const char **argv, const char
>> *prefix)
>>
>> argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
>>
>> + if (no_gpg_sign) {
>> + sign_tag = 0;
>> + }
>> +
>
> Hmm. I'd rather like to see this folded into the `--no-sign` option
> implied by the `OPT_BOOL('s', "sign", ...)` part. The way this would work
> would be to initialize the struct like this:
>
> struct create_tag_options opt = { .sign = -1 };
>
> then leave
>
> OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
>
> as-is, and later do this:
>
> if (opt.sign < 0)
> opt.sign = config_sign_tag > 0;
>
>> @@ -463,7 +475,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> if (cmdmode == 'l')
>> setup_auto_pager("tag", 1);
>>
>> - if ((create_tag_object || force) && (cmdmode != 0))
>> + if ((create_tag_object || force || no_gpg_sign) && (cmdmode != 0))
>> usage_with_options(git_tag_usage, options);
>
> Should we really try to be that strict? If so, we would have to test for
> `opt.sign > 0` here and make sure that above-mentioned `if (opt.sign < 0)`
> block comes *after* this block.
>
>> @@ -556,8 +568,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>
>> create_reflog_msg(&object, &reflog_msg);
>>
>> - if (create_tag_object) {
>> - if (force_sign_annotate && !annotate)
>> + if (create_tag_object || sign_tag) {
>> + if (sign_tag || (force_sign_annotate && !annotate))
>> opt.sign = 1;
>
> This would probably be better handled via
>
> - create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> + create_tag_object = (opt.sign > 0 || config_sign_tag > 0 ||
> + annotate || msg.given || msgfile);
>
> earlier. After all, this assignment of `create_tag_object` suggests that
> it is a Boolean that catches *all* cases where a tag should be created.
>
> But then, we would have to be very careful about this, as we don't want to
> error out when a user calls `git tag -l` while `tag.gpgSign = true`. So
> maybe instead:
>
> - create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> + create_tag_object = (opt.sign > 0 || annotate || msg.given || msgfile);
>
> and later, after that test whether `(create_tag_object || force) &&
> cmd_mode != 0` do:
>
> if (opt.sign < 0) {
> opt.sign = config_sign_tag > 0;
> create_tag_object ||= opt.sign;
>
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> index 6aeeb279a0..98a07a29d2 100755
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>> @@ -932,6 +932,27 @@ test_expect_success GPG \
>> test_cmp expect actual
>> '
>>
>> +get_tag_header gpgsign-enabled $commit commit $time >expect
>> +echo "A message" >>expect
>> +echo '-----BEGIN PGP SIGNATURE-----' >>expect
>> +test_expect_success GPG \
>> + 'git tag configured tag.gpgsign enables GPG sign' \
>> + 'test_config tag.gpgsign true &&
>> + git tag -m "A message" gpgsign-enabled &&
>> + get_tag_msg gpgsign-enabled>actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +get_tag_header no-gpg-sign $commit commit $time >expect
>> +echo "A message" >>expect
>> +test_expect_success GPG \
>> + 'git tag --no-gpg-sign configured tag.gpgsign skip GPG sign' \
>> + 'test_config tag.gpgsign true &&
>> + git tag -a --no-gpg-sign -m "A message" no-gpg-sign &&
>
> With my idea above, this would of course become `--no-sign`.
>
>> + get_tag_msg no-gpg-sign>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 &&
>
> Apart from the things I suggested in the hope to help you improve the
> patch, this looks good to me so far.
>
> Thanks,
> JohannesTigran Mkrtchyan[PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
As many CI/CD tools don't allow to control command line options when executing `git tag` command, a default value in the configuration file will allow to enforce tag signing if required.
The new config-file option tag.gpgSign enforces signed tags. Additional command line option --no-sign is added to disable such behavior if needed. E.g.:
$ git tag -m "commit message"
will generate a GPG signed tag if tag.gpgSign option is true, while
$ git tag --no-sign -m "commit message"
will skip the signing step.
The combination of -u <key-id> and --no-sign not allowed.
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> --- Documentation/config/tag.txt | 7 +++++++ Documentation/git-tag.txt | 7 +++++++ builtin/tag.c | 23 ++++++++++++++++++----- t/t7004-tag.sh | 21 +++++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/tag.txt b/Documentation/config/tag.txt index 663663bdec..675483c3c3 100644 --- a/Documentation/config/tag.txt +++ b/Documentation/config/tag.txt @@ -8,6 +8,13 @@ tag.sort:: linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the value of this variable will be used as the default. +tag.gpgsign:: + A boolean to specify whether all tags should be GPG signed. + Use of this option when running in an automated script can + result in a large number of tags being signed. It is therefore + convenient to use an agent to avoid typing your gpg passphrase + several times. + tar.umask:: This variable can be used to restrict the permission bits of tar archive entries. The default is 0002, which turns off the diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index a74e7b926d..2e5599a67f 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -64,6 +64,13 @@ OPTIONS -s:: --sign:: Make a GPG-signed tag, using the default e-mail address's key. + The default behavior of tag GPG-signing is controlled by `tag.gpgSign` + configuration variable if it exists, or disabled oder otherwise. + See linkgit:git-config[1]. + +--no-sign:: + Override `tag.gpgSign` configuration variable that is + set to force each and every tag to be signed. -u <keyid>:: --local-user=<keyid>:: diff --git a/builtin/tag.c b/builtin/tag.c index ef37dccf86..ec5fd1dcc0 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -33,6 +33,7 @@ static const char * const git_tag_usage[] = { static unsigned int colopts; static int force_sign_annotate; +static int config_sign_tag = -1; /* unspecified */ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format) @@ -144,6 +145,11 @@ static int git_tag_config(const char *var, const char *value, void *cb) int status; struct ref_sorting **sorting_tail = (struct ref_sorting **)cb; + if (!strcmp(var, "tag.gpgsign")) { + config_sign_tag = git_config_bool(var, value); + return 0; + } + if (!strcmp(var, "tag.sort")) { if (!value) return config_error_nonbool(var); @@ -442,14 +448,12 @@ 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 (keyid) { - opt.sign = 1; - set_signing_key(keyid); - } - create_tag_object = (opt.sign || annotate || msg.given || msgfile); + if (keyid && !opt.sign) + die(_("user key can't be specified with --no-sign option")); if (!cmdmode) { if (argc == 0) @@ -463,6 +467,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (cmdmode == 'l') setup_auto_pager("tag", 1); + if (opt.sign == -1) + opt.sign = cmdmode ? 0 : config_sign_tag > 0; + + if (keyid) { + opt.sign = 1; + set_signing_key(keyid); + } + create_tag_object = (opt.sign || annotate || msg.given || msgfile); + if ((create_tag_object || force) && (cmdmode != 0)) usage_with_options(git_tag_usage, options); diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 6aeeb279a0..80eb13d94e 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -932,6 +932,27 @@ test_expect_success GPG \ test_cmp expect actual ' +get_tag_header gpgsign-enabled $commit commit $time >expect +echo "A message" >>expect +echo '-----BEGIN PGP SIGNATURE-----' >>expect +test_expect_success GPG \ + 'git tag configured tag.gpgsign enables GPG sign' \ + 'test_config tag.gpgsign true && + git tag -m "A message" gpgsign-enabled && + get_tag_msg gpgsign-enabled>actual && + test_cmp expect actual +' + +get_tag_header no-sign $commit commit $time >expect +echo "A message" >>expect +test_expect_success GPG \ + 'git tag --no-sign configured tag.gpgsign skip GPG sign' \ + 'test_config tag.gpgsign true && + git tag -a --no-sign -m "A message" no-sign && + get_tag_msg no-sign>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.22.0.rc3.1.gdcc2db3530.dirty
Todd ZullingerRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Hi,
Tigran Mkrtchyan wrote:
Show 14 quoted lines
> diff --git a/Documentation/config/tag.txt b/Documentation/config/tag.txt > index 663663bdec..675483c3c3 100644 > --- a/Documentation/config/tag.txt > +++ b/Documentation/config/tag.txt > @@ -8,6 +8,13 @@ tag.sort:: > linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the > value of this variable will be used as the default. > > +tag.gpgsign:: > + A boolean to specify whether all tags should be GPG signed. > + Use of this option when running in an automated script can > + result in a large number of tags being signed. It is therefore > + convenient to use an agent to avoid typing your gpg passphrase > + several times.
I think the variable should be camelCased, i.e. tag.gpgSign, for consistency with other documentation.
-- Todd
Junio C HamanoRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> writes:
> As many CI/CD tools don't allow to control command line options when > executing `git tag` command, a default value in the configuration file > will allow to enforce tag signing if required.
Hmm. Would these "many" tools still allow arbigrary configuration set to affect their operation? It sounds like a bigger issue but it is a separate one.
> The new config-file option tag.gpgSign enforces signed tags. Additional > ... > will skip the signing step.
This paragraph is well written.
> The combination of -u <key-id> and --no-sign not allowed.
This sentence lacks a verb. Perhaps s/not allowed/is &/.
But more importantly, I think we should justify why this "not allowed" makes sense as the design of the feature. A plausible alternative design would simply follow the "last one wins" paradigm, where
git tag -u key # "-u key" implies "-s"
git tag -u key --no-sign # "--no-sign' trumps the implied "-s"
git tag --no-sign -u key # "-u key"'s implication of "-s" trumps the # earlier "--no-sign"
and having "[tag] gpgsign" simply adds to the implication early in the chain to be overridden by later command line options.
Let's explain why "you cannot give -u <key> and --no-sign at the same time" is better than "the last one wins".
Show 18 quoted lines
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt > index a74e7b926d..2e5599a67f 100644 > --- a/Documentation/git-tag.txt > +++ b/Documentation/git-tag.txt > @@ -64,6 +64,13 @@ OPTIONS > -s:: > --sign:: > Make a GPG-signed tag, using the default e-mail address's key. > + The default behavior of tag GPG-signing is controlled by `tag.gpgSign` > + configuration variable if it exists, or disabled oder otherwise. > + See linkgit:git-config[1]. > + > +--no-sign:: > + Override `tag.gpgSign` configuration variable that is > + set to force each and every tag to be signed. > > -u <keyid>:: > --local-user=<keyid>::
If we justify "-u and --no-sign do not mix", that design needs to be explained to the end users in the documentation, not just in the proposed log messsage.
Mkrtchyan, TigranRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
----- Original Message -----
Show 6 quoted lines
> From: "Junio C Hamano" <gitster@pobox.com> > To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de> > Cc: "git" <git@vger.kernel.org>, "Jonathan Nieder" <jrnieder@gmail.com>, "Johannes Schindelin" > <Johannes.Schindelin@gmx.de> > Sent: Wednesday, June 5, 2019 6:25:46 PM > Subject: Re: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Show 5 quoted lines
> Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> writes: > >> As many CI/CD tools don't allow to control command line options when >> executing `git tag` command, a default value in the configuration file >> will allow to enforce tag signing if required.
Must of them blindly execute git commands with some hard-coded combination of options. It's clear to me, that they are the source of the problem, but git can be the solution.
Show 15 quoted lines
> > Hmm. Would these "many" tools still allow arbigrary configuration > set to affect their operation? It sounds like a bigger issue but it > is a separate one. > >> The new config-file option tag.gpgSign enforces signed tags. Additional >> ... >> will skip the signing step. > > This paragraph is well written. > >> The combination of -u <key-id> and --no-sign not allowed. > > This sentence lacks a verb. Perhaps s/not allowed/is &/. >
Jup. Sorry.
Show 17 quoted lines
> But more importantly, I think we should justify why this "not > allowed" makes sense as the design of the feature. A plausible > alternative design would simply follow the "last one wins" paradigm, > where > > git tag -u key # "-u key" implies "-s" > > git tag -u key --no-sign # "--no-sign' trumps the implied "-s" > > git tag --no-sign -u key # "-u key"'s implication of "-s" trumps the > # earlier "--no-sign" > > and having "[tag] gpgsign" simply adds to the implication early in > the chain to be overridden by later command line options. > > Let's explain why "you cannot give -u <key> and --no-sign at the > same time" is better than "the last one wins".
This is matter of convention. I never pay attention to the order of options on the git command line, but I don't put conflicting options either, I hope. Does git already have position depended options? If yes, then fine with me. Otherwise, I don't want to introduce ambiguity. Yes, less ambiguity is my answer to why it's better than "the last one wins".
Show 23 quoted lines
> >> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt >> index a74e7b926d..2e5599a67f 100644 >> --- a/Documentation/git-tag.txt >> +++ b/Documentation/git-tag.txt >> @@ -64,6 +64,13 @@ OPTIONS >> -s:: >> --sign:: >> Make a GPG-signed tag, using the default e-mail address's key. >> + The default behavior of tag GPG-signing is controlled by `tag.gpgSign` >> + configuration variable if it exists, or disabled oder otherwise. >> + See linkgit:git-config[1]. >> + >> +--no-sign:: >> + Override `tag.gpgSign` configuration variable that is >> + set to force each and every tag to be signed. >> >> -u <keyid>:: >> --local-user=<keyid>:: > > If we justify "-u and --no-sign do not mix", that design needs to be > explained to the end users in the documentation, not just in the > proposed log messsage.
Make sense.
Thanks, Tigran.
Junio C HamanoRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
"Mkrtchyan, Tigran" <tigran.mkrtchyan@desy.de> writes:
Show 5 quoted lines
>> But more importantly, I think we should justify why this "not >> allowed" makes sense as the design of the feature. A plausible >> alternative design would simply follow the "last one wins" paradigm, >> ... > This is matter of convention.
Oh, if you put it that way, it is quite clear that we should redo this part, as the rest of Git command line processing all pretty much follow "last one wins". I was mostly curious if there was any strong reason why this combination has to be different.
Mkrtchyan, TigranRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Thanks for clarification. I will update the patch and re-send it.
Tigran.
----- Original Message -----
Show 6 quoted lines
> From: "Junio C Hamano" <gitster@pobox.com> > To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de> > Cc: "git" <git@vger.kernel.org>, "Jonathan Nieder" <jrnieder@gmail.com>, "Johannes Schindelin" > <Johannes.Schindelin@gmx.de> > Sent: Wednesday, June 5, 2019 10:46:05 PM > Subject: Re: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Show 12 quoted lines
> "Mkrtchyan, Tigran" <tigran.mkrtchyan@desy.de> writes: > >>> But more importantly, I think we should justify why this "not >>> allowed" makes sense as the design of the feature. A plausible >>> alternative design would simply follow the "last one wins" paradigm, >>> ... >> This is matter of convention. > > Oh, if you put it that way, it is quite clear that we should redo > this part, as the rest of Git command line processing all pretty > much follow "last one wins". I was mostly curious if there was any > strong reason why this combination has to be different.
Junio C HamanoRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> "Mkrtchyan, Tigran" <tigran.mkrtchyan@desy.de> writes: > >>> But more importantly, I think we should justify why this "not >>> allowed" makes sense as the design of the feature. A plausible >>> alternative design would simply follow the "last one wins" paradigm, >>> ... >> This is matter of convention. > > Oh, if you put it that way, it is quite clear that we should redo > this part, as the rest of Git command line processing all pretty > much follow "last one wins". I was mostly curious if there was any > strong reason why this combination has to be different.
One reason why "last one wins" aka "you have a chance to correct yourself" is so useful is because people can then use alias to define leading options, e.g. with
[alias] myfoo = foo --option1 --option2 --option3
a user can say "git myfoo" to save typing, but in a rare occasion where only options 2 and 3 (but not option 1) are desired, can use "git myfoo --no-option1" to countermand it.
Unfortunately, I do not think we can use the same technique for options that is given to the "git" potty, e.g. with
[alias] paged-status = --paginate status
you can say "git paged-status" to have your "git status" output sent to the pager, but you cannot say "git paged-status --no-pager" to defeat that leading option. We may want to find a way to correct it, but it is rather low on the priority scale ;-)
Mkrtchyan, TigranRe: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
----- Original Message -----
Show 6 quoted lines
> From: "Junio C Hamano" <gitster@pobox.com> > To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de> > Cc: "git" <git@vger.kernel.org>, "Jonathan Nieder" <jrnieder@gmail.com>, "Johannes Schindelin" > <Johannes.Schindelin@gmx.de> > Sent: Wednesday, June 5, 2019 10:57:34 PM > Subject: Re: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Show 34 quoted lines
> Junio C Hamano <gitster@pobox.com> writes: > >> "Mkrtchyan, Tigran" <tigran.mkrtchyan@desy.de> writes: >> >>>> But more importantly, I think we should justify why this "not >>>> allowed" makes sense as the design of the feature. A plausible >>>> alternative design would simply follow the "last one wins" paradigm, >>>> ... >>> This is matter of convention. >> >> Oh, if you put it that way, it is quite clear that we should redo >> this part, as the rest of Git command line processing all pretty >> much follow "last one wins". I was mostly curious if there was any >> strong reason why this combination has to be different. > > One reason why "last one wins" aka "you have a chance to correct > yourself" is so useful is because people can then use alias to > define leading options, e.g. with > > [alias] myfoo = foo --option1 --option2 --option3 > > a user can say "git myfoo" to save typing, but in a rare occasion > where only options 2 and 3 (but not option 1) are desired, can use > "git myfoo --no-option1" to countermand it. > > Unfortunately, I do not think we can use the same technique for > options that is given to the "git" potty, e.g. with > > [alias] paged-status = --paginate status > > you can say "git paged-status" to have your "git status" output sent > to the pager, but you cannot say "git paged-status --no-pager" to > defeat that leading option. We may want to find a way to correct > it, but it is rather low on the priority scale ;-)
Makes sense. Thanks for that use case, which I actually use myself :).
BTW, `--no-sign` currently doesn't affects `-u <key>` in any order:
```
if (keyid) {
opt.sign = 1;
set_signing_key(keyid);
}
```IOW, `-u <key>` is another way to enable signing, what is, actually, consistent with man page:
```
-u <keyid>, --local-user=<keyid>
Make a GPG-signed tag, using the given key.
```I will leave it as-is, I will just describe that tag.gpgSign options doesn't affect '-u' behavior.
Tigran.
Tigran Mkrtchyan[PATCH v4] tag: add tag.gpgSign config option to force all tags be GPG-signed
As many CI/CD tools don't allow to control command line options when executing `git tag` command, a default value in the configuration file will allow to enforce tag signing if required.
The new config-file option tag.gpgSign is added to define default behavior of tag signings. To override default behavior the command line option -s, --sign and --no-sign can be used:
$ git tag -m "commit message"
will generate a GPG signed tag if tag.gpgSign option is true, while
$ git tag --no-sign -m "commit message"
will skip the signing step.
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> --- Documentation/config/tag.txt | 8 ++++++++ Documentation/git-tag.txt | 7 +++++++ builtin/tag.c | 22 ++++++++++++++++------ t/t7004-tag.sh | 21 +++++++++++++++++++++ 4 files changed, 52 insertions(+), 6 deletions(-)
diff --git a/Documentation/config/tag.txt b/Documentation/config/tag.txt index 663663bdec..ef5adb3f42 100644 --- a/Documentation/config/tag.txt +++ b/Documentation/config/tag.txt @@ -8,6 +8,14 @@ tag.sort:: linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the value of this variable will be used as the default. +tag.gpgSign:: + A boolean to specify whether all tags should be GPG signed. + Use of this option when running in an automated script can + result in a large number of tags being signed. It is therefore + convenient to use an agent to avoid typing your gpg passphrase + several times. Note that this option doesn't affects tag signing + behavior enabled by "-u <keyid>" or "--local-user=<keyid>" options. + tar.umask:: This variable can be used to restrict the permission bits of tar archive entries. The default is 0002, which turns off the diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index a74e7b926d..2e5599a67f 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -64,6 +64,13 @@ OPTIONS -s:: --sign:: Make a GPG-signed tag, using the default e-mail address's key. + The default behavior of tag GPG-signing is controlled by `tag.gpgSign` + configuration variable if it exists, or disabled oder otherwise. + See linkgit:git-config[1]. + +--no-sign:: + Override `tag.gpgSign` configuration variable that is + set to force each and every tag to be signed. -u <keyid>:: --local-user=<keyid>:: diff --git a/builtin/tag.c b/builtin/tag.c index ef37dccf86..e0a4c25382 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -33,6 +33,7 @@ static const char * const git_tag_usage[] = { static unsigned int colopts; static int force_sign_annotate; +static int config_sign_tag = -1; /* unspecified */ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format) @@ -144,6 +145,11 @@ static int git_tag_config(const char *var, const char *value, void *cb) int status; struct ref_sorting **sorting_tail = (struct ref_sorting **)cb; + if (!strcmp(var, "tag.gpgsign")) { + config_sign_tag = git_config_bool(var, value); + return 0; + } + if (!strcmp(var, "tag.sort")) { if (!value) return config_error_nonbool(var); @@ -442,15 +448,10 @@ 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 (keyid) { - opt.sign = 1; - set_signing_key(keyid); - } - create_tag_object = (opt.sign || annotate || msg.given || msgfile); - if (!cmdmode) { if (argc == 0) cmdmode = 'l'; @@ -463,6 +464,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (cmdmode == 'l') setup_auto_pager("tag", 1); + if (opt.sign == -1) + opt.sign = cmdmode ? 0 : config_sign_tag > 0; + + if (keyid) { + opt.sign = 1; + set_signing_key(keyid); + } + create_tag_object = (opt.sign || annotate || msg.given || msgfile); + if ((create_tag_object || force) && (cmdmode != 0)) usage_with_options(git_tag_usage, options); diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 6aeeb279a0..80eb13d94e 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -932,6 +932,27 @@ test_expect_success GPG \ test_cmp expect actual ' +get_tag_header gpgsign-enabled $commit commit $time >expect +echo "A message" >>expect +echo '-----BEGIN PGP SIGNATURE-----' >>expect +test_expect_success GPG \ + 'git tag configured tag.gpgsign enables GPG sign' \ + 'test_config tag.gpgsign true && + git tag -m "A message" gpgsign-enabled && + get_tag_msg gpgsign-enabled>actual && + test_cmp expect actual +' + +get_tag_header no-sign $commit commit $time >expect +echo "A message" >>expect +test_expect_success GPG \ + 'git tag --no-sign configured tag.gpgsign skip GPG sign' \ + 'test_config tag.gpgsign true && + git tag -a --no-sign -m "A message" no-sign && + get_tag_msg no-sign>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.22.0.rc3.1.gdbadaa0575