Re: [PATCH] builtin/tag.c: add --trailer arg

From: John Passaro <john.a.passaro@gmail.com>
Date: Apr 29, 2024, 15:05 UTC
Message-ID: <CAJdN7KhCesU-j=yyM5Sk0sMj8W2Gk=LJc-Dzf8-2+gjhkgA4HQ@mail.gmail.com>
In-Reply-To: <Zi9DGYwlT7VnW7oj@tanuki>

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

On Mon, Apr 29, 2024 at 2:50 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 10 quoted lines

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

Show 31 quoted lines

> > @@ -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?

Show 25 quoted lines

> >               } 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?

Show 144 quoted lines

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