builtin/tag.c: add --trailer option

[jpassaro]

5th follow-up patch taking welcome feedback from Patrick and JCH. Net new changes include suggested tweaks in documentation, error messaging, code formatting, and patch description.

Since git-tag --list --format="%(trailers)" can interpret trailers from annotated tag messages, it seems natural to support --trailer when writing a new tag message.

git-commit accomplishes this by taking --trailer arguments and passing them to git-interpret-trailer. This patch series refactors that logic and uses it to implement --trailer on git-tag.

cc: Junio C Hamano gitster@pobox.com cc: Patrick Steinhardt ps@pks.im cc: John Passaro john.a.passaro@gmail.com

[gitgitgadget[bot]]

Welcome to GitGitGadget

Hi @jpassaro, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that either:

• Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
• Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.

You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <revi.ewer@example.com>, Ill Takalook <ill.takalook@example.net>

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget[bot]]

There are issues in commit 3af37febe2fad9be216927f5e3679a12b6c8ccf1: builtin/tag.c: add --trailer arg Commit not signed off

[jpassaro]

@dscho could you kindly /allow this PR?

[dscho]

/allow

[gitgitgadget[bot]]

User jpassaro is now allowed to use GitGitGadget.

WARNING: jpassaro has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.

[jpassaro]

/submit

[gitgitgadget[bot]]

Submitted as pull.1723.git.1714365076246.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1723/jpassaro/jp/tag-trailer-arg-v1

To fetch this version to local tag pr-1723/jpassaro/jp/tag-trailer-arg-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1723/jpassaro/jp/tag-trailer-arg-v1

[gitgitgadget[bot]]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
> From: John Passaro <john.a.passaro@gmail.com>
> 
> Teach git-tag to accept --trailer option to add trailers to annotated
> tag messages, like git-commit.
> 
> Signed-off-by: John Passaro <john.a.passaro@gmail.com>

This feels like a sensible addition to me indeed, thanks!

[snip]
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9a33cb50b45..0334a5d15ec 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -28,9 +28,11 @@
>  #include "date.h"
>  #include "write-or-die.h"
>  #include "object-file-convert.h"
> +#include "run-command.h"
>  
>  static const char * const git_tag_usage[] = {
>   N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +    "        [(--trailer <token>[(=|:)<value>])...]\n"
>      "        <tagname> [<commit> | <object>]"),
>   N_("git tag -d <tagname>..."),
>   N_("git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]\n"
> @@ -290,10 +292,11 @@ static const char message_advice_nested_tag[] =
>  static void create_tag(const struct object_id *object, const char *object_ref,
>              const char *tag,
>              struct strbuf *buf, struct create_tag_options *opt,
> -            struct object_id *prev, struct object_id *result, char *path)
> +            struct object_id *prev, struct object_id *result, struct strvec *trailer_args, char *path)

This line is overly long now, let's break it.

>  {
>   enum object_type type;
>   struct strbuf header = STRBUF_INIT;
> + int should_edit;
>  
>   type = oid_object_info(the_repository, object, NULL);
>   if (type <= OBJ_NONE)
> @@ -313,14 +316,18 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>           tag,
>           git_committer_info(IDENT_STRICT));
>  
> - if (!opt->message_given || opt->use_editor) {
> + should_edit = opt->use_editor || !opt->message_given;
> + if (should_edit || trailer_args->nr) {
>       int fd;
>  
>       /* write the template message before editing: */
>       fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
>  
> -     if (opt->message_given) {
> +     if (opt->message_given && buf->len) {
>           write_or_die(fd, buf->buf, buf->len);
> +         if (trailer_args->nr && buf->buf[buf->len-1] != '\n') {
> +             write_or_die(fd, "\n", 1);
> +         }

We avoid braces around single-line statements.

I was also wondering whether we can simplify this to:

    if (opt->message_given && buf->len) {
        strbuf_complete(buf, '\n');
        write_or_die(fd, buf->buf, buf->len);
    }

Or does changing `buf` cause problems for us?

>           strbuf_reset(buf);
>       } else if (!is_null_oid(prev)) {
>           write_tag_body(fd, prev);
> @@ -338,10 +345,31 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>       }
>       close(fd);
>  
> -     if (launch_editor(path, buf, NULL)) {
> -         fprintf(stderr,
> -         _("Please supply the message using either -m or -F option.\n"));
> -         exit(1);
> +     if (trailer_args->nr) {
> +         struct child_process run_trailer = CHILD_PROCESS_INIT;
> +
> +         strvec_pushl(&run_trailer.args, "interpret-trailers",
> +                  "--in-place", "--no-divider",
> +                  path, NULL);
> +         strvec_pushv(&run_trailer.args, trailer_args->v);
> +         run_trailer.git_cmd = 1;
> +         if (run_command(&run_trailer))
> +             die(_("unable to pass trailers to --trailers"));
> +     }

This part is copied from `builtin/commit.c`. Would it make sense to move
it into a function `amend_trailers_to_file()` or similar that we add to
our trailer API so that we can avoid the code duplication?

> +     if (should_edit) {
> +         if (launch_editor(path, buf, NULL)) {
> +             fprintf(stderr,
> +             _("Please supply the message using either -m or -F option.\n"));
> +             exit(1);
> +         }

I know you simply re-indented the block here, but let's also fix the
indentation of the second argument to fprintf(3P) while at it.

> +     } else if (trailer_args->nr) {
> +         strbuf_reset(buf);
> +         if (strbuf_read_file(buf, path, 0) < 0) {
> +             fprintf(stderr,
> +                     _("Please supply the message using either -m or -F option.\n"));
> +             exit(1);
> +         }
>       }
>   }
>  
> @@ -416,6 +444,14 @@ struct msg_arg {
>   struct strbuf buf;
>  };
>  
> +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
> +{
> + BUG_ON_OPT_NEG(unset);
> +
> + strvec_pushl(opt->value, "--trailer", arg, NULL);
> + return 0;
> +}
> +
>  static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  {
>   struct msg_arg *msg = opt->value;
> @@ -463,6 +499,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>   struct ref_sorting *sorting;
>   struct string_list sorting_options = STRING_LIST_INIT_DUP;
>   struct ref_format format = REF_FORMAT_INIT;
> + struct strvec trailer_args = STRVEC_INIT;
>   int icase = 0;
>   int edit_flag = 0;
>   struct option options[] = {
> @@ -479,6 +516,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>       OPT_CALLBACK_F('m', "message", &msg, N_("message"),
>                  N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
>       OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
> +     OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
>       OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
>       OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
>       OPT_CLEANUP(&cleanup_arg),
> @@ -548,7 +586,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>       opt.sign = 1;
>       set_signing_key(keyid);
>   }
> - create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> + create_tag_object = (opt.sign || annotate || msg.given || msgfile || edit_flag || trailer_args.nr);
>  
>   if ((create_tag_object || force) && (cmdmode != 0))
>       usage_with_options(git_tag_usage, options);
> @@ -635,7 +673,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>   else if (!force)
>       die(_("tag '%s' already exists"), tag);
>  
> - opt.message_given = msg.given || msgfile;
> + opt.message_given = msg.given || (msgfile != NULL);
>   opt.use_editor = edit_flag;

Besides being not required, this change also violates our coding style
where we don't explicitly check for NULL pointers.

>   if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
> @@ -653,7 +691,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>       if (force_sign_annotate && !annotate)
>           opt.sign = 1;
>       path = git_pathdup("TAG_EDITMSG");
> -     create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
> +     create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, &trailer_args,
>              path);

Nit: let's move `&trailer_args` to the next line to not make it overly
long.

>   }
>  
> @@ -686,6 +724,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>   strbuf_release(&reflog_msg);
>   strbuf_release(&msg.buf);
>   strbuf_release(&err);
> + strvec_clear(&trailer_args);
>   free(msgfile);
>   return ret;
>  }
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 696866d7794..364db2b4685 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -668,6 +668,105 @@ test_expect_success \
>   test_cmp expect actual
>  '
>  
> +# trailers
> +
> +get_tag_header tag-with-inline-message-and-trailers $commit commit $time >expect
> +cat >>expect <<EOF
> +create tag with trailers
> +
> +my-trailer: here
> +alt-trailer: there
> +EOF

You probably just follow precedent in this file, but our modern coding
style sets up the `expect` file inside of the test body itself. You also
do it like that in other tests, so let's be consistent.

The same comment applies to other tests, as well.

Patrick

[gitgitgadget[bot]]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[gitgitgadget[bot]]

There are issues in commit f94a1a6056266a6d4d1e3541faf6b2ddd11a16d8: po: update git-tag translations Commit not signed off

[gitgitgadget[bot]]

There are issues in commit 8b96c45a9c0eb88639ee1616e4d10eca66d841b2: feedback: move setups inside test Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

There are issues in commit f94a1a6056266a6d4d1e3541faf6b2ddd11a16d8: po: update git-tag translations Commit not signed off

[gitgitgadget[bot]]

There are issues in commit 8b96c45a9c0eb88639ee1616e4d10eca66d841b2: feedback: move setups inside test Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

There are issues in commit c72486824d0c5a68f8fd9ddc1aa2b46adc7b8f69: feedback: line length Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

There are issues in commit bbd6464d7ecdbaf3479ce682dcf26ecb04865df3: feedback: remove "!= NULL" check Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

On the Git mailing list, John Passaro wrote (reply to this):

On Mon, Apr 29, 2024 at 2:50 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
> > From: John Passaro <john.a.passaro@gmail.com>
> >
> > Teach git-tag to accept --trailer option to add trailers to annotated
> > tag messages, like git-commit.
> >
> > Signed-off-by: John Passaro <john.a.passaro@gmail.com>
>
> This feels like a sensible addition to me indeed, thanks!
>

Thanks, and thank you for the thoughtful feedback.
I have incorporated most of it on the github PR branch
(https://github.com/gitgitgadget/git/pull/1723).
Before submitting a new patch I had a couple of questions.

[snip]

> > @@ -313,14 +316,18 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >                   tag,
> >                   git_committer_info(IDENT_STRICT));
> >
> > -     if (!opt->message_given || opt->use_editor) {
> > +     should_edit = opt->use_editor || !opt->message_given;
> > +     if (should_edit || trailer_args->nr) {
> >               int fd;
> >
> >               /* write the template message before editing: */
> >               fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> >
> > -             if (opt->message_given) {
> > +             if (opt->message_given && buf->len) {
> >                       write_or_die(fd, buf->buf, buf->len);
> > +                     if (trailer_args->nr && buf->buf[buf->len-1] != '\n') {
> > +                             write_or_die(fd, "\n", 1);
> > +                     }
>
> We avoid braces around single-line statements.
>
> I was also wondering whether we can simplify this to:
>
>     if (opt->message_given && buf->len) {
>         strbuf_complete(buf, '\n');
>         write_or_die(fd, buf->buf, buf->len);
>     }
>
> Or does changing `buf` cause problems for us?

Changing `buf

>
> >                       strbuf_reset(buf);
> >               } else if (!is_null_oid(prev)) {
> >                       write_tag_body(fd, prev);
> > @@ -338,10 +345,31 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >               }
> >               close(fd);
> >
> > -             if (launch_editor(path, buf, NULL)) {
> > -                     fprintf(stderr,
> > -                     _("Please supply the message using either -m or -F option.\n"));
> > -                     exit(1);
> > +             if (trailer_args->nr) {
> > +                     struct child_process run_trailer = CHILD_PROCESS_INIT;
> > +
> > +                     strvec_pushl(&run_trailer.args, "interpret-trailers",
> > +                                  "--in-place", "--no-divider",
> > +                                  path, NULL);
> > +                     strvec_pushv(&run_trailer.args, trailer_args->v);
> > +                     run_trailer.git_cmd = 1;
> > +                     if (run_command(&run_trailer))
> > +                             die(_("unable to pass trailers to --trailers"));
> > +             }
>
> This part is copied from `builtin/commit.c`. Would it make sense to move
> it into a function `amend_trailers_to_file()` or similar that we add to
> our trailer API so that we can avoid the code duplication?
>
> > +             if (should_edit) {
> > +                     if (launch_editor(path, buf, NULL)) {
> > +                             fprintf(stderr,
> > +                             _("Please supply the message using either -m or -F option.\n"));
> > +                             exit(1);
> > +                     }
>
> I know you simply re-indented the block here, but let's also fix the
> indentation of the second argument to fprintf(3P) while at it.
>
> > +             } else if (trailer_args->nr) {
> > +                     strbuf_reset(buf);
> > +                     if (strbuf_read_file(buf, path, 0) < 0) {
> > +                             fprintf(stderr,
> > +                                             _("Please supply the message using either -m or -F option.\n"));
> > +                             exit(1);
> > +                     }
> >               }
> >       }
> >
> > @@ -416,6 +444,14 @@ struct msg_arg {
> >       struct strbuf buf;
> >  };
> >
> > +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
> > +{
> > +     BUG_ON_OPT_NEG(unset);
> > +
> > +     strvec_pushl(opt->value, "--trailer", arg, NULL);
> > +     return 0;
> > +}
> > +
> >  static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> >  {
> >       struct msg_arg *msg = opt->value;
> > @@ -463,6 +499,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >       struct ref_sorting *sorting;
> >       struct string_list sorting_options = STRING_LIST_INIT_DUP;
> >       struct ref_format format = REF_FORMAT_INIT;
> > +     struct strvec trailer_args = STRVEC_INIT;
> >       int icase = 0;
> >       int edit_flag = 0;
> >       struct option options[] = {
> > @@ -479,6 +516,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >               OPT_CALLBACK_F('m', "message", &msg, N_("message"),
> >                              N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
> >               OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
> > +             OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> >               OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
> >               OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
> >               OPT_CLEANUP(&cleanup_arg),
> > @@ -548,7 +586,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >               opt.sign = 1;
> >               set_signing_key(keyid);
> >       }
> > -     create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> > +     create_tag_object = (opt.sign || annotate || msg.given || msgfile || edit_flag || trailer_args.nr);
> >
> >       if ((create_tag_object || force) && (cmdmode != 0))
> >               usage_with_options(git_tag_usage, options);
> > @@ -635,7 +673,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >       else if (!force)
> >               die(_("tag '%s' already exists"), tag);
> >
> > -     opt.message_given = msg.given || msgfile;
> > +     opt.message_given = msg.given || (msgfile != NULL);
> >       opt.use_editor = edit_flag;
>
> Besides being not required, this change also violates our coding style
> where we don't explicitly check for NULL pointers.
>
> >       if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
> > @@ -653,7 +691,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >               if (force_sign_annotate && !annotate)
> >                       opt.sign = 1;
> >               path = git_pathdup("TAG_EDITMSG");
> > -             create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
> > +             create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, &trailer_args,
> >                          path);
>
> Nit: let's move `&trailer_args` to the next line to not make it overly
> long.
>
> >       }
> >
> > @@ -686,6 +724,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >       strbuf_release(&reflog_msg);
> >       strbuf_release(&msg.buf);
> >       strbuf_release(&err);
> > +     strvec_clear(&trailer_args);
> >       free(msgfile);
> >       return ret;
> >  }
> > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> > index 696866d7794..364db2b4685 100755
> > --- a/t/t7004-tag.sh
> > +++ b/t/t7004-tag.sh
> > @@ -668,6 +668,105 @@ test_expect_success \
> >       test_cmp expect actual
> >  '
> >
> > +# trailers
> > +
> > +get_tag_header tag-with-inline-message-and-trailers $commit commit $time >expect
> > +cat >>expect <<EOF
> > +create tag with trailers
> > +
> > +my-trailer: here
> > +alt-trailer: there
> > +EOF
>
> You probably just follow precedent in this file, but our modern coding
> style sets up the `expect` file inside of the test body itself. You also
> do it like that in other tests, so let's be consistent.
>
> The same comment applies to other tests, as well.
>
> Patrick

[gitgitgadget[bot]]

User John Passaro <john.a.passaro@gmail.com> has been added to the cc: list.

[gitgitgadget[bot]]

On the Git mailing list, John Passaro wrote (reply to this):

apologies for the half-cocked message before, please disregard!

On Mon, Apr 29, 2024 at 2:50 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
> > From: John Passaro <john.a.passaro@gmail.com>
> >
> > Teach git-tag to accept --trailer option to add trailers to annotated
> > tag messages, like git-commit.
> >
> > Signed-off-by: John Passaro <john.a.passaro@gmail.com>
>
> This feels like a sensible addition to me indeed, thanks!

Thanks, and thank you for the thoughtful feedback.
I have incorporated most of it on the github PR branch
(https://github.com/gitgitgadget/git/pull/1723).
Before submitting a new patch I had a couple of questions.

[snip]

> > @@ -313,14 +316,18 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >                   tag,
> >                   git_committer_info(IDENT_STRICT));
> >
> > -     if (!opt->message_given || opt->use_editor) {
> > +     should_edit = opt->use_editor || !opt->message_given;
> > +     if (should_edit || trailer_args->nr) {
> >               int fd;
> >
> >               /* write the template message before editing: */
> >               fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> >
> > -             if (opt->message_given) {
> > +             if (opt->message_given && buf->len) {
> >                       write_or_die(fd, buf->buf, buf->len);
> > +                     if (trailer_args->nr && buf->buf[buf->len-1] != '\n') {
> > +                             write_or_die(fd, "\n", 1);
> > +                     }
>
> We avoid braces around single-line statements.
>
> I was also wondering whether we can simplify this to:
>
>     if (opt->message_given && buf->len) {
>         strbuf_complete(buf, '\n');
>         write_or_die(fd, buf->buf, buf->len);
>     }
>
> Or does changing `buf` cause problems for us?
>
> >                       strbuf_reset(buf);

I think altering `buf` does not cause problems precisely
because we do it on the next line here with strbuf_reset().
strbuf_complete() seems like a clearly better choice.
Two questions:
* should it be done conditionally on `trailer_args->nr` as I did
  here, or should it be unconditional?
* You left strbuf_reset() out of your snippet - i don't know for sure
  why it was there in the first place, should it stay?

> >               } else if (!is_null_oid(prev)) {
> >                       write_tag_body(fd, prev);
> > @@ -338,10 +345,31 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >               }
> >               close(fd);
> >
> > -             if (launch_editor(path, buf, NULL)) {
> > -                     fprintf(stderr,
> > -                     _("Please supply the message using either -m or -F option.\n"));
> > -                     exit(1);
> > +             if (trailer_args->nr) {
> > +                     struct child_process run_trailer = CHILD_PROCESS_INIT;
> > +
> > +                     strvec_pushl(&run_trailer.args, "interpret-trailers",
> > +                                  "--in-place", "--no-divider",
> > +                                  path, NULL);
> > +                     strvec_pushv(&run_trailer.args, trailer_args->v);
> > +                     run_trailer.git_cmd = 1;
> > +                     if (run_command(&run_trailer))
> > +                             die(_("unable to pass trailers to --trailers"));
> > +             }
>
> This part is copied from `builtin/commit.c`. Would it make sense to move
> it into a function `amend_trailers_to_file()` or similar that we add to
> our trailer API so that we can avoid the code duplication?

It seems to me the duplication goes further than that.
Ideally I would love to have a unified approach to the problem of
combining `-m`, `-F`, `-e`, and `--trailer` generally. That would change
one existing nit I have in git-tag, which is that with `-m`, `-F`,
or `-fae` to replace an existing tag, it doesn't add commentary guidance
the way git-commit does.

That's a bigger change than I'm comfortable taking on and it's
arguably beyond the scope of this ticket. Question here is - first,
is such a consolidation possible in the future, and second, if it were,
would this refactor (dedicated function for augmenting a strbuf/file
with trailers) still be valuable?

> >               } else if (!is_null_oid(prev)) {
> >                       write_tag_body(fd, prev);
> > @@ -338,10 +345,31 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >               }
> >               close(fd);
> >
> > -             if (launch_editor(path, buf, NULL)) {
> > -                     fprintf(stderr,
> > -                     _("Please supply the message using either -m or -F option.\n"));
> > -                     exit(1);
> > +             if (trailer_args->nr) {
> > +                     struct child_process run_trailer = CHILD_PROCESS_INIT;
> > +
> > +                     strvec_pushl(&run_trailer.args, "interpret-trailers",
> > +                                  "--in-place", "--no-divider",
> > +                                  path, NULL);
> > +                     strvec_pushv(&run_trailer.args, trailer_args->v);
> > +                     run_trailer.git_cmd = 1;
> > +                     if (run_command(&run_trailer))
> > +                             die(_("unable to pass trailers to --trailers"));
> > +             }
>
> This part is copied from `builtin/commit.c`. Would it make sense to move
> it into a function `amend_trailers_to_file()` or similar that we add to
> our trailer API so that we can avoid the code duplication?
>
> > +             if (should_edit) {
> > +                     if (launch_editor(path, buf, NULL)) {
> > +                             fprintf(stderr,
> > +                             _("Please supply the message using either -m or -F option.\n"));
> > +                             exit(1);
> > +                     }
>
> I know you simply re-indented the block here, but let's also fix the
> indentation of the second argument to fprintf(3P) while at it.
>
> > +             } else if (trailer_args->nr) {
> > +                     strbuf_reset(buf);
> > +                     if (strbuf_read_file(buf, path, 0) < 0) {
> > +                             fprintf(stderr,
> > +                                             _("Please supply the message using either -m or -F option.\n"));
> > +                             exit(1);
> > +                     }
> >               }
> >       }
> >
> > @@ -416,6 +444,14 @@ struct msg_arg {
> >       struct strbuf buf;
> >  };
> >
> > +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
> > +{
> > +     BUG_ON_OPT_NEG(unset);
> > +
> > +     strvec_pushl(opt->value, "--trailer", arg, NULL);
> > +     return 0;
> > +}
> > +
> >  static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> >  {
> >       struct msg_arg *msg = opt->value;
> > @@ -463,6 +499,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >       struct ref_sorting *sorting;
> >       struct string_list sorting_options = STRING_LIST_INIT_DUP;
> >       struct ref_format format = REF_FORMAT_INIT;
> > +     struct strvec trailer_args = STRVEC_INIT;
> >       int icase = 0;
> >       int edit_flag = 0;
> >       struct option options[] = {
> > @@ -479,6 +516,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >               OPT_CALLBACK_F('m', "message", &msg, N_("message"),
> >                              N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
> >               OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
> > +             OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> >               OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
> >               OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
> >               OPT_CLEANUP(&cleanup_arg),
> > @@ -548,7 +586,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >               opt.sign = 1;
> >               set_signing_key(keyid);
> >       }
> > -     create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> > +     create_tag_object = (opt.sign || annotate || msg.given || msgfile || edit_flag || trailer_args.nr);
> >
> >       if ((create_tag_object || force) && (cmdmode != 0))
> >               usage_with_options(git_tag_usage, options);
> > @@ -635,7 +673,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >       else if (!force)
> >               die(_("tag '%s' already exists"), tag);
> >
> > -     opt.message_given = msg.given || msgfile;
> > +     opt.message_given = msg.given || (msgfile != NULL);
> >       opt.use_editor = edit_flag;
>
> Besides being not required, this change also violates our coding style
> where we don't explicitly check for NULL pointers.
>
> >       if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
> > @@ -653,7 +691,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >               if (force_sign_annotate && !annotate)
> >                       opt.sign = 1;
> >               path = git_pathdup("TAG_EDITMSG");
> > -             create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
> > +             create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, &trailer_args,
> >                          path);
>
> Nit: let's move `&trailer_args` to the next line to not make it overly
> long.
>
> >       }
> >
> > @@ -686,6 +724,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >       strbuf_release(&reflog_msg);
> >       strbuf_release(&msg.buf);
> >       strbuf_release(&err);
> > +     strvec_clear(&trailer_args);
> >       free(msgfile);
> >       return ret;
> >  }
> > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> > index 696866d7794..364db2b4685 100755
> > --- a/t/t7004-tag.sh
> > +++ b/t/t7004-tag.sh
> > @@ -668,6 +668,105 @@ test_expect_success \
> >       test_cmp expect actual
> >  '
> >
> > +# trailers
> > +
> > +get_tag_header tag-with-inline-message-and-trailers $commit commit $time >expect
> > +cat >>expect <<EOF
> > +create tag with trailers
> > +
> > +my-trailer: here
> > +alt-trailer: there
> > +EOF
>
> You probably just follow precedent in this file, but our modern coding
> style sets up the `expect` file inside of the test body itself. You also
> do it like that in other tests, so let's be consistent.
>
> The same comment applies to other tests, as well.
>
> Patrick

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
>> From: John Passaro <john.a.passaro@gmail.com>
>> 
>> Teach git-tag to accept --trailer option to add trailers to annotated
>> tag messages, like git-commit.
>> 
>> Signed-off-by: John Passaro <john.a.passaro@gmail.com>
>
> This feels like a sensible addition to me indeed, thanks!

At the surface level, I tend to agree, but I am of two minds,
especially around the "-s" option, though.  "commit -s" is to work
with the "Signed-off-by" trailer, but "tag -s" is not.

More importantly, I doubt that many trailers we commonly see in the
comit objects, like "Acked-by", "Reviewed-by", or even "CC", are
applicable in the context of tags.  So I am ambivalent.

If we were to adop this new feature, your review already has done a
very good job and I see room for adding nothing more on the
implementation.

Thanks, both.

> [snip]
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 9a33cb50b45..0334a5d15ec 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -28,9 +28,11 @@
>>  #include "date.h"
>>  #include "write-or-die.h"
>>  #include "object-file-convert.h"
>> +#include "run-command.h"
>>  
>>  static const char * const git_tag_usage[] = {
>>      N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
>> +       "        [(--trailer <token>[(=|:)<value>])...]\n"
>>         "        <tagname> [<commit> | <object>]"),
>>      N_("git tag -d <tagname>..."),
>>      N_("git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]\n"
>> @@ -290,10 +292,11 @@ static const char message_advice_nested_tag[] =
>>  static void create_tag(const struct object_id *object, const char *object_ref,
>>                 const char *tag,
>>                 struct strbuf *buf, struct create_tag_options *opt,
>> -               struct object_id *prev, struct object_id *result, char *path)
>> +               struct object_id *prev, struct object_id *result, struct strvec *trailer_args, char *path)
>
> This line is overly long now, let's break it.
>
>>  {
>>      enum object_type type;
>>      struct strbuf header = STRBUF_INIT;
>> +    int should_edit;
>>  
>>      type = oid_object_info(the_repository, object, NULL);
>>      if (type <= OBJ_NONE)
>> @@ -313,14 +316,18 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>>              tag,
>>              git_committer_info(IDENT_STRICT));
>>  
>> -    if (!opt->message_given || opt->use_editor) {
>> +    should_edit = opt->use_editor || !opt->message_given;
>> +    if (should_edit || trailer_args->nr) {
>>          int fd;
>>  
>>          /* write the template message before editing: */
>>          fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
>>  
>> -        if (opt->message_given) {
>> +        if (opt->message_given && buf->len) {
>>              write_or_die(fd, buf->buf, buf->len);
>> +            if (trailer_args->nr && buf->buf[buf->len-1] != '\n') {
>> +                write_or_die(fd, "\n", 1);
>> +            }
>
> We avoid braces around single-line statements.
>
> I was also wondering whether we can simplify this to:
>
>     if (opt->message_given && buf->len) {
>         strbuf_complete(buf, '\n');
>         write_or_die(fd, buf->buf, buf->len);
>     }
>
> Or does changing `buf` cause problems for us?
>
>>              strbuf_reset(buf);
>>          } else if (!is_null_oid(prev)) {
>>              write_tag_body(fd, prev);
>> @@ -338,10 +345,31 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>>          }
>>          close(fd);
>>  
>> -        if (launch_editor(path, buf, NULL)) {
>> -            fprintf(stderr,
>> -            _("Please supply the message using either -m or -F option.\n"));
>> -            exit(1);
>> +        if (trailer_args->nr) {
>> +            struct child_process run_trailer = CHILD_PROCESS_INIT;
>> +
>> +            strvec_pushl(&run_trailer.args, "interpret-trailers",
>> +                     "--in-place", "--no-divider",
>> +                     path, NULL);
>> +            strvec_pushv(&run_trailer.args, trailer_args->v);
>> +            run_trailer.git_cmd = 1;
>> +            if (run_command(&run_trailer))
>> +                die(_("unable to pass trailers to --trailers"));
>> +        }
>
> This part is copied from `builtin/commit.c`. Would it make sense to move
> it into a function `amend_trailers_to_file()` or similar that we add to
> our trailer API so that we can avoid the code duplication?
>
>> +        if (should_edit) {
>> +            if (launch_editor(path, buf, NULL)) {
>> +                fprintf(stderr,
>> +                _("Please supply the message using either -m or -F option.\n"));
>> +                exit(1);
>> +            }
>
> I know you simply re-indented the block here, but let's also fix the
> indentation of the second argument to fprintf(3P) while at it.
>
>> +        } else if (trailer_args->nr) {
>> +            strbuf_reset(buf);
>> +            if (strbuf_read_file(buf, path, 0) < 0) {
>> +                fprintf(stderr,
>> +                        _("Please supply the message using either -m or -F option.\n"));
>> +                exit(1);
>> +            }
>>          }
>>      }
>>  
>> @@ -416,6 +444,14 @@ struct msg_arg {
>>      struct strbuf buf;
>>  };
>>  
>> +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
>> +{
>> +    BUG_ON_OPT_NEG(unset);
>> +
>> +    strvec_pushl(opt->value, "--trailer", arg, NULL);
>> +    return 0;
>> +}
>> +
>>  static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>      struct msg_arg *msg = opt->value;
>> @@ -463,6 +499,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>      struct ref_sorting *sorting;
>>      struct string_list sorting_options = STRING_LIST_INIT_DUP;
>>      struct ref_format format = REF_FORMAT_INIT;
>> +    struct strvec trailer_args = STRVEC_INIT;
>>      int icase = 0;
>>      int edit_flag = 0;
>>      struct option options[] = {
>> @@ -479,6 +516,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>          OPT_CALLBACK_F('m', "message", &msg, N_("message"),
>>                     N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
>>          OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
>> +        OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
>>          OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
>>          OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
>>          OPT_CLEANUP(&cleanup_arg),
>> @@ -548,7 +586,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>          opt.sign = 1;
>>          set_signing_key(keyid);
>>      }
>> -    create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>> +    create_tag_object = (opt.sign || annotate || msg.given || msgfile || edit_flag || trailer_args.nr);
>>  
>>      if ((create_tag_object || force) && (cmdmode != 0))
>>          usage_with_options(git_tag_usage, options);
>> @@ -635,7 +673,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>      else if (!force)
>>          die(_("tag '%s' already exists"), tag);
>>  
>> -    opt.message_given = msg.given || msgfile;
>> +    opt.message_given = msg.given || (msgfile != NULL);
>>      opt.use_editor = edit_flag;
>
> Besides being not required, this change also violates our coding style
> where we don't explicitly check for NULL pointers.
>
>>      if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
>> @@ -653,7 +691,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>          if (force_sign_annotate && !annotate)
>>              opt.sign = 1;
>>          path = git_pathdup("TAG_EDITMSG");
>> -        create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
>> +        create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, &trailer_args,
>>                 path);
>
> Nit: let's move `&trailer_args` to the next line to not make it overly
> long.
>
>>      }
>>  
>> @@ -686,6 +724,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>      strbuf_release(&reflog_msg);
>>      strbuf_release(&msg.buf);
>>      strbuf_release(&err);
>> +    strvec_clear(&trailer_args);
>>      free(msgfile);
>>      return ret;
>>  }
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> index 696866d7794..364db2b4685 100755
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>> @@ -668,6 +668,105 @@ test_expect_success \
>>      test_cmp expect actual
>>  '
>>  
>> +# trailers
>> +
>> +get_tag_header tag-with-inline-message-and-trailers $commit commit $time >expect
>> +cat >>expect <<EOF
>> +create tag with trailers
>> +
>> +my-trailer: here
>> +alt-trailer: there
>> +EOF
>
> You probably just follow precedent in this file, but our modern coding
> style sets up the `expect` file inside of the test body itself. You also
> do it like that in other tests, so let's be consistent.
>
> The same comment applies to other tests, as well.
>
> Patrick

[gitgitgadget[bot]]

There are issues in commit f94a1a6056266a6d4d1e3541faf6b2ddd11a16d8: po: update git-tag translations Commit not signed off

[gitgitgadget[bot]]

There are issues in commit 8b96c45a9c0eb88639ee1616e4d10eca66d841b2: feedback: move setups inside test Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

There are issues in commit c72486824d0c5a68f8fd9ddc1aa2b46adc7b8f69: feedback: line length Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

There are issues in commit bbd6464d7ecdbaf3479ce682dcf26ecb04865df3: feedback: remove "!= NULL" check Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

There are issues in commit 020aa999ce6c44e124f5af93f86365ada1a43c11: feedback: refactor shared trailer code to trailer.c Commit checks stopped - the message is too short Commit not signed off

[gitgitgadget[bot]]

On the Git mailing list, John Passaro wrote (reply to this):

On Mon, Apr 29, 2024 at 11:29 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
> >> From: John Passaro <john.a.passaro@gmail.com>
> >>
> >> Teach git-tag to accept --trailer option to add trailers to annotated
> >> tag messages, like git-commit.
> >>
> >> Signed-off-by: John Passaro <john.a.passaro@gmail.com>
> >
> > This feels like a sensible addition to me indeed, thanks!
>
> At the surface level, I tend to agree, but I am of two minds,
> especially around the "-s" option, though.  "commit -s" is to work
> with the "Signed-off-by" trailer, but "tag -s" is not.
>
> More importantly, I doubt that many trailers we commonly see in the
> comit objects, like "Acked-by", "Reviewed-by", or even "CC", are
> applicable in the context of tags.  So I am ambivalent.

A couple of words on the motivation here. First, by way of --list
--format="%(trailer)",
git-tag arguably has read-side support for trailers already; adding write
support seems pretty reasonable. Second, even though not all the trailers
broadly used for commits are an obvious fit for tags, some still are -
"Signed-off-by" for one would seem plausibly useful. In my team's usage (which
inspired this change), tag trailers have emerged as a convenient way to pass
machine-readable metadata to CICD.

>
> If we were to adop this new feature, your review already has done a
> very good job and I see room for adding nothing more on the
> implementation.
>
> Thanks, both.
>
> > [snip]
> >> diff --git a/builtin/tag.c b/builtin/tag.c
> >> index 9a33cb50b45..0334a5d15ec 100644
> >> --- a/builtin/tag.c
> >> +++ b/builtin/tag.c
> >> @@ -28,9 +28,11 @@
> >>  #include "date.h"
> >>  #include "write-or-die.h"
> >>  #include "object-file-convert.h"
> >> +#include "run-command.h"
> >>
> >>  static const char * const git_tag_usage[] = {
> >>      N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> >> +       "        [(--trailer <token>[(=|:)<value>])...]\n"
> >>         "        <tagname> [<commit> | <object>]"),
> >>      N_("git tag -d <tagname>..."),
> >>      N_("git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]\n"
> >> @@ -290,10 +292,11 @@ static const char message_advice_nested_tag[] =
> >>  static void create_tag(const struct object_id *object, const char *object_ref,
> >>                     const char *tag,
> >>                     struct strbuf *buf, struct create_tag_options *opt,
> >> -                   struct object_id *prev, struct object_id *result, char *path)
> >> +                   struct object_id *prev, struct object_id *result, struct strvec *trailer_args, char *path)
> >
> > This line is overly long now, let's break it.
> >
> >>  {
> >>      enum object_type type;
> >>      struct strbuf header = STRBUF_INIT;
> >> +    int should_edit;
> >>
> >>      type = oid_object_info(the_repository, object, NULL);
> >>      if (type <= OBJ_NONE)
> >> @@ -313,14 +316,18 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >>                  tag,
> >>                  git_committer_info(IDENT_STRICT));
> >>
> >> -    if (!opt->message_given || opt->use_editor) {
> >> +    should_edit = opt->use_editor || !opt->message_given;
> >> +    if (should_edit || trailer_args->nr) {
> >>              int fd;
> >>
> >>              /* write the template message before editing: */
> >>              fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> >>
> >> -            if (opt->message_given) {
> >> +            if (opt->message_given && buf->len) {
> >>                      write_or_die(fd, buf->buf, buf->len);
> >> +                    if (trailer_args->nr && buf->buf[buf->len-1] != '\n') {
> >> +                            write_or_die(fd, "\n", 1);
> >> +                    }
> >
> > We avoid braces around single-line statements.
> >
> > I was also wondering whether we can simplify this to:
> >
> >     if (opt->message_given && buf->len) {
> >         strbuf_complete(buf, '\n');
> >         write_or_die(fd, buf->buf, buf->len);
> >     }
> >
> > Or does changing `buf` cause problems for us?
> >
> >>                      strbuf_reset(buf);
> >>              } else if (!is_null_oid(prev)) {
> >>                      write_tag_body(fd, prev);
> >> @@ -338,10 +345,31 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >>              }
> >>              close(fd);
> >>
> >> -            if (launch_editor(path, buf, NULL)) {
> >> -                    fprintf(stderr,
> >> -                    _("Please supply the message using either -m or -F option.\n"));
> >> -                    exit(1);
> >> +            if (trailer_args->nr) {
> >> +                    struct child_process run_trailer = CHILD_PROCESS_INIT;
> >> +
> >> +                    strvec_pushl(&run_trailer.args, "interpret-trailers",
> >> +                                 "--in-place", "--no-divider",
> >> +                                 path, NULL);
> >> +                    strvec_pushv(&run_trailer.args, trailer_args->v);
> >> +                    run_trailer.git_cmd = 1;
> >> +                    if (run_command(&run_trailer))
> >> +                            die(_("unable to pass trailers to --trailers"));
> >> +            }
> >
> > This part is copied from `builtin/commit.c`. Would it make sense to move
> > it into a function `amend_trailers_to_file()` or similar that we add to
> > our trailer API so that we can avoid the code duplication?
> >
> >> +            if (should_edit) {
> >> +                    if (launch_editor(path, buf, NULL)) {
> >> +                            fprintf(stderr,
> >> +                            _("Please supply the message using either -m or -F option.\n"));
> >> +                            exit(1);
> >> +                    }
> >
> > I know you simply re-indented the block here, but let's also fix the
> > indentation of the second argument to fprintf(3P) while at it.
> >
> >> +            } else if (trailer_args->nr) {
> >> +                    strbuf_reset(buf);
> >> +                    if (strbuf_read_file(buf, path, 0) < 0) {
> >> +                            fprintf(stderr,
> >> +                                            _("Please supply the message using either -m or -F option.\n"));
> >> +                            exit(1);
> >> +                    }
> >>              }
> >>      }
> >>
> >> @@ -416,6 +444,14 @@ struct msg_arg {
> >>      struct strbuf buf;
> >>  };
> >>
> >> +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
> >> +{
> >> +    BUG_ON_OPT_NEG(unset);
> >> +
> >> +    strvec_pushl(opt->value, "--trailer", arg, NULL);
> >> +    return 0;
> >> +}
> >> +
> >>  static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> >>  {
> >>      struct msg_arg *msg = opt->value;
> >> @@ -463,6 +499,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >>      struct ref_sorting *sorting;
> >>      struct string_list sorting_options = STRING_LIST_INIT_DUP;
> >>      struct ref_format format = REF_FORMAT_INIT;
> >> +    struct strvec trailer_args = STRVEC_INIT;
> >>      int icase = 0;
> >>      int edit_flag = 0;
> >>      struct option options[] = {
> >> @@ -479,6 +516,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >>              OPT_CALLBACK_F('m', "message", &msg, N_("message"),
> >>                             N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
> >>              OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
> >> +            OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> >>              OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
> >>              OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
> >>              OPT_CLEANUP(&cleanup_arg),
> >> @@ -548,7 +586,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >>              opt.sign = 1;
> >>              set_signing_key(keyid);
> >>      }
> >> -    create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> >> +    create_tag_object = (opt.sign || annotate || msg.given || msgfile || edit_flag || trailer_args.nr);
> >>
> >>      if ((create_tag_object || force) && (cmdmode != 0))
> >>              usage_with_options(git_tag_usage, options);
> >> @@ -635,7 +673,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >>      else if (!force)
> >>              die(_("tag '%s' already exists"), tag);
> >>
> >> -    opt.message_given = msg.given || msgfile;
> >> +    opt.message_given = msg.given || (msgfile != NULL);
> >>      opt.use_editor = edit_flag;
> >
> > Besides being not required, this change also violates our coding style
> > where we don't explicitly check for NULL pointers.
> >
> >>      if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
> >> @@ -653,7 +691,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >>              if (force_sign_annotate && !annotate)
> >>                      opt.sign = 1;
> >>              path = git_pathdup("TAG_EDITMSG");
> >> -            create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
> >> +            create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, &trailer_args,
> >>                         path);
> >
> > Nit: let's move `&trailer_args` to the next line to not make it overly
> > long.
> >
> >>      }
> >>
> >> @@ -686,6 +724,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >>      strbuf_release(&reflog_msg);
> >>      strbuf_release(&msg.buf);
> >>      strbuf_release(&err);
> >> +    strvec_clear(&trailer_args);
> >>      free(msgfile);
> >>      return ret;
> >>  }
> >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> >> index 696866d7794..364db2b4685 100755
> >> --- a/t/t7004-tag.sh
> >> +++ b/t/t7004-tag.sh
> >> @@ -668,6 +668,105 @@ test_expect_success \
> >>      test_cmp expect actual
> >>  '
> >>
> >> +# trailers
> >> +
> >> +get_tag_header tag-with-inline-message-and-trailers $commit commit $time >expect
> >> +cat >>expect <<EOF
> >> +create tag with trailers
> >> +
> >> +my-trailer: here
> >> +alt-trailer: there
> >> +EOF
> >
> > You probably just follow precedent in this file, but our modern coding
> > style sets up the `expect` file inside of the test body itself. You also
> > do it like that in other tests, so let's be consistent.
> >
> > The same comment applies to other tests, as well.
> >
> > Patrick

[jpassaro]

/submit

[gitgitgadget[bot]]

Submitted as pull.1723.v2.git.1714409638089.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1723/jpassaro/jp/tag-trailer-arg-v2

To fetch this version to local tag pr-1723/jpassaro/jp/tag-trailer-arg-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1723/jpassaro/jp/tag-trailer-arg-v2

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

John Passaro <john.a.passaro@gmail.com> writes:

>> More importantly, I doubt that many trailers we commonly see in the
>> comit objects, like "Acked-by", "Reviewed-by", or even "CC", are
>> applicable in the context of tags.  So I am ambivalent.
>
> A couple of words on the motivation here. First, by way of --list
> --format="%(trailer)",
> git-tag arguably has read-side support for trailers already; adding write
> support seems pretty reasonable. Second, even though not all the trailers
> broadly used for commits are an obvious fit for tags, some still are -
> "Signed-off-by" for one would seem plausibly useful. In my team's usage (which
> inspired this change), tag trailers have emerged as a convenient way to pass
> machine-readable metadata to CICD.

That is a good thing to describe in the proposed log message.

If a reviewer feels puzzled by a commit and did not get the
motivation from the proposed log message, there is a good chance
that the motivation is not well described to help future developers
who find this commit in "git log" output.

Thanks.

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

John Passaro <john.a.passaro@gmail.com> writes:

> It seems to me the duplication goes further than that.
> Ideally I would love to have a unified approach to the problem of
> combining `-m`, `-F`, `-e`, and `--trailer` generally. That would change
> one existing nit I have in git-tag, which is that with `-m`, `-F`,
> or `-fae` to replace an existing tag, it doesn't add commentary guidance
> the way git-commit does.
>
> That's a bigger change than I'm comfortable taking on and it's
> arguably beyond the scope of this ticket. Question here is - first,
> is such a consolidation possible in the future, and second, if it were,
> would this refactor (dedicated function for augmenting a strbuf/file
> with trailers) still be valuable?

There is no doubt it would be, if it were ;-)

At least, it should be straight-forward to turn the copying and
pasting in this patch to a proper refactoring into a common helper
function as Patrick suggested and add it to the trailer API.  It
should become a separate, preparatory clean-up patch on a two-patch
series, upon which the main "now we have a reusable piece split out
of 'git commit --trailer' implementation, let's use it to teach the
same '--trailer' to 'git tag'" patch can come.

Thanks.

[jpassaro]

/submit

[gitgitgadget[bot]]

Submitted as pull.1723.v3.git.1714416863.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1723/jpassaro/jp/tag-trailer-arg-v3

To fetch this version to local tag pr-1723/jpassaro/jp/tag-trailer-arg-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1723/jpassaro/jp/tag-trailer-arg-v3

[jpassaro]

/submit

[gitgitgadget[bot]]

Submitted as pull.1723.v4.git.1714488111.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1723/jpassaro/jp/tag-trailer-arg-v4

To fetch this version to local tag pr-1723/jpassaro/jp/tag-trailer-arg-v4:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1723/jpassaro/jp/tag-trailer-arg-v4

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this), regarding 5b6239167b8d7c26f96e5c23d0d82b7a3a9b01fe (outdated):

"John Passaro via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Passaro <john.a.passaro@gmail.com>
>
> git-tag currently supports interpreting trailers from an annotated tag
> message, using --list --format="%(trailers)". There is no ergonomic way
> to add trailers to an annotated tag message.

Well said.  Drop "currently", though.  The usual way to compose a
log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.

> In a previous patch, we refactored git-commit's implementation of its
> --trailer arg to the trailer.h API. Let's use that new function to teach
> git-tag the same --trailer argument, emulating as much of git-commit's
> behavior as much as possible.

Nicely described.

> @@ -178,6 +179,19 @@ This option is only applicable when listing tags without annotation lines.
>   Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
>   is given.
>  
> +--trailer <token>[(=|:)<value>]::
> + Specify a (<token>, <value>) pair that should be applied as a
> + trailer. (e.g. `git tag --trailer "Signed-off-by:T A Ger \
> + <tagger@example.com>" --trailer "Helped-by:C O Mitter \
> + <committer@example.com>"` will add the "Signed-off-by" trailer
> + and the "Helped-by" trailer to the tag message.)
> + The `trailer.*` configuration variables
> + (linkgit:git-interpret-trailers[1]) can be used to define if
> + a duplicated trailer is omitted, where in the run of trailers
> + each trailer would appear, and other details.
> + The trailers can be seen in `git tag --list` using
> + `--format="%(trailers)"` placeholder.

I can see this was copied-and-pasted from git-commit, but I am not
sure if the ones used in the example are good fit for tag objects.
What does Helped-by even mean in the context of an annotated tag?

> @@ -290,10 +292,12 @@ static const char message_advice_nested_tag[] =
>  static void create_tag(const struct object_id *object, const char *object_ref,
>              const char *tag,
>              struct strbuf *buf, struct create_tag_options *opt,
> -            struct object_id *prev, struct object_id *result, char *path)
> +            struct object_id *prev, struct object_id *result,
> +            struct strvec *trailer_args, char *path)
>  {
>   enum object_type type;
>   struct strbuf header = STRBUF_INIT;
> + int should_edit;
>  
>   type = oid_object_info(the_repository, object, NULL);
>   if (type <= OBJ_NONE)
> @@ -313,13 +317,15 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>           tag,
>           git_committer_info(IDENT_STRICT));
>  
> - if (!opt->message_given || opt->use_editor) {
> + should_edit = opt->use_editor || !opt->message_given;
> + if (should_edit || trailer_args->nr) {
>       int fd;
>  
>       /* write the template message before editing: */
>       fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
>  
> -     if (opt->message_given) {
> +     if (opt->message_given && buf->len) {
> +         strbuf_complete(buf, '\n');
>           write_or_die(fd, buf->buf, buf->len);
>           strbuf_reset(buf);
>       } else if (!is_null_oid(prev)) {
> @@ -338,10 +344,22 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>       }
>       close(fd);
>  
> -     if (launch_editor(path, buf, NULL)) {
> -         fprintf(stderr,
> -         _("Please supply the message using either -m or -F option.\n"));
> -         exit(1);
> +     if (trailer_args->nr && amend_file_with_trailers(path, trailer_args))
> +         die(_("unable to pass trailers to --trailers"));
> +
> +     if (should_edit) {
> +         if (launch_editor(path, buf, NULL)) {
> +             fprintf(stderr,
> +                 _("Please supply the message using either -m or -F option.\n"));
> +             exit(1);
> +         }
> +     } else if (trailer_args->nr) {

When both should_edit and trailer_args->nr are true, this block will
not be entered.  We first do the "amend_file" thing, and then run an
editor on it, and that is the end of the story in that case.

When we do not have should_edit (e.g., -m "tag message" is given),
we would have run "amend_file" thing on it to tweak the message,
and we come in here.

> +         strbuf_reset(buf);
> +         if (strbuf_read_file(buf, path, 0) < 0) {
> +             fprintf(stderr,
> +                 _("Please supply the message using either -m or -F option.\n"));
> +             exit(1);

Does this error message make sense here in this context?  The
earlier one was introduced by 7198203a (editor.c: Libify
launch_editor(), 2008-07-25)---after we fail to run the editor, as
we somehow seem to be unable to run an editor, we suggest the user
to give us a message in other ways.  But this one is different.  The
user gave us in one of these other ways already instead of using an
editor, but mucking with that with the "amend_file" thing somehow
made it unreadable.  Shouldn't it be more like

    die_errno(_("failed to read '%s'"), path);

or something along that line?

[gitgitgadget[bot]]

On the Git mailing list, John Passaro wrote (reply to this), regarding 5b6239167b8d7c26f96e5c23d0d82b7a3a9b01fe (outdated):

On Tue, Apr 30, 2024 at 12:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>

Thanks for the feedback. Hoping for a couple points of clarification
then I'll put in one more version of this patch series.

> "John Passaro via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Passaro <john.a.passaro@gmail.com>
> >
> > git-tag currently supports interpreting trailers from an annotated tag
> > message, using --list --format="%(trailers)". There is no ergonomic way
> > to add trailers to an annotated tag message.
>
> Well said.  Drop "currently", though.  The usual way to compose a
> log message of this project is to
>
>  - Give an observation on how the current system work in the present
>    tense (so no need to say "Currently X is Y", just "X is Y"), and
>    discuss what you perceive as a problem in it.
>
>  - Propose a solution (optional---often, problem description
>    trivially leads to an obvious solution in reader's minds).
>
>  - Give commands to the codebase to "become like so".
>
> in this order.

Understood. In the most recent version of this patch, I updated the
message. However on second thought I think I'm gonna keep this on
the next submission of this patch (without "currently" of course).

>
> > In a previous patch, we refactored git-commit's implementation of its
> > --trailer arg to the trailer.h API. Let's use that new function to teach
> > git-tag the same --trailer argument, emulating as much of git-commit's
> > behavior as much as possible.
>
> Nicely described.
>
> > @@ -178,6 +179,19 @@ This option is only applicable when listing tags without annotation lines.
> >       Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
> >       is given.
> >
> > +--trailer <token>[(=|:)<value>]::
> > +     Specify a (<token>, <value>) pair that should be applied as a
> > +     trailer. (e.g. `git tag --trailer "Signed-off-by:T A Ger \
> > +     <tagger@example.com>" --trailer "Helped-by:C O Mitter \
> > +     <committer@example.com>"` will add the "Signed-off-by" trailer
> > +     and the "Helped-by" trailer to the tag message.)
> > +     The `trailer.*` configuration variables
> > +     (linkgit:git-interpret-trailers[1]) can be used to define if
> > +     a duplicated trailer is omitted, where in the run of trailers
> > +     each trailer would appear, and other details.
> > +     The trailers can be seen in `git tag --list` using
> > +     `--format="%(trailers)"` placeholder.
>
> I can see this was copied-and-pasted from git-commit, but I am not
> sure if the ones used in the example are good fit for tag objects.
> What does Helped-by even mean in the context of an annotated tag?

I can see that the git project itself doesn't typically add trailers to tags.
If y'all were in that habit I imagine this feature would already be
implemented :-)
Nonetheless Signed-off-by or Approved-by is easy to imagine, for example
in an environment where multiple sign-offs are required (i.e. not just
the implicit
sign-off of the tagger). So we could just leave that in and be done with it.

I have a couple more hypothetical trailers that are both plausible and somewhat
generic; do any of them seem expressive enough to include in the docs?

* Tested-by: T E Ster <tester@example.com>
* Testing-assigned-to: T E Ster <tester@example.com>
* Scheduled-Deployment-Date: 2024-05-15 (or 1714500385 -05:00)
* Deployment-assigned-to: Oscar P Staff <ops@example.com>
* (for RC/alpha tags) Full-release-scheduled-for: 2024-06-05

There's also project-specific trailers. For example, on my team,
we use "Deploy-Strategy: ..." to tell CICD what deployment routines to run. This
is pretty specific to us but worth calling out. Maybe could translate to a
documentation example with something like "<Project-specific-trailer>: foo"

> > @@ -338,10 +344,22 @@ static void create_tag(const struct object_id *object, const char *object_ref,
> >               }
> >               close(fd);
> >
> > -             if (launch_editor(path, buf, NULL)) {
> > -                     fprintf(stderr,
> > -                     _("Please supply the message using either -m or -F option.\n"));
> > -                     exit(1);
> > +             if (trailer_args->nr && amend_file_with_trailers(path, trailer_args))
> > +                     die(_("unable to pass trailers to --trailers"));
> > +
> > +             if (should_edit) {
> > +                     if (launch_editor(path, buf, NULL)) {
> > +                             fprintf(stderr,
> > +                                     _("Please supply the message using either -m or -F option.\n"));
> > +                             exit(1);
> > +                     }
> > +             } else if (trailer_args->nr) {
>
> When both should_edit and trailer_args->nr are true, this block will
> not be entered.  We first do the "amend_file" thing, and then run an
> editor on it, and that is the end of the story in that case.
>
> When we do not have should_edit (e.g., -m "tag message" is given),
> we would have run "amend_file" thing on it to tweak the message,
> and we come in here.
>
> > +                     strbuf_reset(buf);
> > +                     if (strbuf_read_file(buf, path, 0) < 0) {
> > +                             fprintf(stderr,
> > +                                     _("Please supply the message using either -m or -F option.\n"));
> > +                             exit(1);
>
> Does this error message make sense here in this context?  The
> earlier one was introduced by 7198203a (editor.c: Libify
> launch_editor(), 2008-07-25)---after we fail to run the editor, as
> we somehow seem to be unable to run an editor, we suggest the user
> to give us a message in other ways.  But this one is different.  The
> user gave us in one of these other ways already instead of using an
> editor, but mucking with that with the "amend_file" thing somehow
> made it unreadable.  Shouldn't it be more like
>
>         die_errno(_("failed to read '%s'"), path);
>
> or something along that line?

I didn't realize that the first message is intended to augment more
expressive failure messages previously printed in launch_editor().
Knowing that, your suggested message will point users in the right
direction much more effectively. Also i guess die() probably preferable
since unlike launch_editor(), which may signal non-exceptional failure,
this error is more likely to be a bug.

However, in service of helping users find workarounds, shouldn't we tell them
--trailer may be the culprit?

> Failed to read '%s'. Try again without --trailer (use -e or -F to add trailers manually).

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this), regarding 5b6239167b8d7c26f96e5c23d0d82b7a3a9b01fe (outdated):

John Passaro <john.a.passaro@gmail.com> writes:

> There's also project-specific trailers. For example, on my team,
> we use "Deploy-Strategy: ..." to tell CICD what deployment routines to run. This
> is pretty specific to us but worth calling out. Maybe could translate to a
> documentation example with something like "<Project-specific-trailer>: foo"

The last one that uses placeholders for both trailer tag and value
may be generic enough.

> However, in service of helping users find workarounds, shouldn't we tell them
> --trailer may be the culprit?
>
>> Failed to read '%s'. Try again without --trailer (use -e or -F to add trailers manually).

I dunno.  

If -m/-F that wrote the original using the open/write_or_die/close
sequence succeeded, the "amend_file" thing successfully spawned
"interpret-trailers --in-place" and got control back, yet we fail
to read that message back, it does not smell like a failure with
that "--trailer" option to me.  A failure with "--trailer" that
could be worked around would have been caught in "amend_file" thing,
before the control reaches this point, no?

[gitgitgadget[bot]]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Tue, Apr 30, 2024 at 02:41:48PM +0000, John Passaro via GitGitGadget wrote:
> 4th follow-up patch taking welcome feedback from Patrick and JCH. Net new
> changes include separating from a 2-patch series to 3.
> 
> Since git-tag --list --format="%(trailers)" can interpret trailers from
> annotated tag messages, it seems natural to support --trailer when writing a
> new tag message.
> 
> git-commit accomplishes this by taking --trailer arguments and passing them
> to git-interpret-trailer. This patch series refactors that logic and uses it
> to implement --trailer on git-tag.

This version looks mostly good. There are two tiny nits, but other than
that I don't have anything else to add.

Patrick

[jpassaro]

/submit

[gitgitgadget[bot]]

Submitted as pull.1723.v5.git.1714934950.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1723/jpassaro/jp/tag-trailer-arg-v5

To fetch this version to local tag pr-1723/jpassaro/jp/tag-trailer-arg-v5:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1723/jpassaro/jp/tag-trailer-arg-v5

[gitgitgadget[bot]]

On the Git mailing list, John Passaro wrote (reply to this), regarding 5b6239167b8d7c26f96e5c23d0d82b7a3a9b01fe (outdated):

On Tue, Apr 30, 2024 at 6:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> John Passaro <john.a.passaro@gmail.com> writes:
>
> > There's also project-specific trailers. For example, on my team,
> > we use "Deploy-Strategy: ..." to tell CICD what deployment routines to run. This
> > is pretty specific to us but worth calling out. Maybe could translate to a
> > documentation example with something like "<Project-specific-trailer>: foo"
>
> The last one that uses placeholders for both trailer tag and value
> may be generic enough.
>

done

> > However, in service of helping users find workarounds, shouldn't we tell them
> > --trailer may be the culprit?
> >
> >> Failed to read '%s'. Try again without --trailer (use -e or -F to add trailers manually).
>
> I dunno.
>
> If -m/-F that wrote the original using the open/write_or_die/close
> sequence succeeded, the "amend_file" thing successfully spawned
> "interpret-trailers --in-place" and got control back, yet we fail
> to read that message back, it does not smell like a failure with
> that "--trailer" option to me.  A failure with "--trailer" that
> could be worked around would have been caught in "amend_file" thing,
> before the control reaches this point, no?
>
I considered "Failed to read '%s' after adding trailers to it". But on
reflection it's really pretty difficult to imagine why this would happen at
all - like it could only really happen if the call to git-interpret-trailers
somehow corrupted the file. Probably best, as you say, not to speculate
about the cause when telling the user what happened and potentially
mislead them.

Plus this wording reduces load on the l10n teams in case this patch is
accepted, and allows future changes to enable this code path for other
reasons with minimal change.

[gitgitgadget[bot]]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Sun, May 05, 2024 at 06:49:07PM +0000, John Passaro via GitGitGadget wrote:
> 5th follow-up patch taking welcome feedback from Patrick and JCH. Net new
> changes include suggested tweaks in documentation, error messaging, code
> formatting, and patch description.
> 
> Since git-tag --list --format="%(trailers)" can interpret trailers from
> annotated tag messages, it seems natural to support --trailer when writing a
> new tag message.
> 
> git-commit accomplishes this by taking --trailer arguments and passing them
> to git-interpret-trailer. This patch series refactors that logic and uses it
> to implement --trailer on git-tag.

This version looks good to me, thanks!

Patrick

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <ps@pks.im> writes:

> On Sun, May 05, 2024 at 06:49:07PM +0000, John Passaro via GitGitGadget wrote:
>> 5th follow-up patch taking welcome feedback from Patrick and JCH. Net new
>> changes include suggested tweaks in documentation, error messaging, code
>> formatting, and patch description.
>> 
>> Since git-tag --list --format="%(trailers)" can interpret trailers from
>> annotated tag messages, it seems natural to support --trailer when writing a
>> new tag message.
>> 
>> git-commit accomplishes this by taking --trailer arguments and passing them
>> to git-interpret-trailer. This patch series refactors that logic and uses it
>> to implement --trailer on git-tag.
>
> This version looks good to me, thanks!

Thanks.  Will queue.

[gitgitgadget[bot]]

This branch is now known as jp/tag-trailer.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/2e6fe8d39bc267665aaf744750033da96db76d64.

[gitgitgadget[bot]]

There was a status update in the "New Topics" section about the branch jp/tag-trailer on the Git mailing list:

"git tag" learned the "--trailer" option to futz with the trailers
in the same way as "git commit" does.

Will merge to 'next'.
source: <pull.1723.v5.git.1714934950.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/819bcc64a0af1a8b0e35eaa72d6d5e3b0ae7747d.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/6211d1f3950c3ae0e904c8cc1c8a5232b89522b4.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/646013793d6874929b436d7e7993419bf5c571e1.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/feb7cd3a19b0515692bb0209f862a413d3b4028d.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch jp/tag-trailer on the Git mailing list:

"git tag" learned the "--trailer" option to futz with the trailers
in the same way as "git commit" does.

Will merge to 'master'.
source: <pull.1723.v5.git.1714934950.gitgitgadget@gmail.com>