On Mon, Apr 29, 2024 at 2:50 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote: > > From: John Passaro <john.a.passaro@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > 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
