| Message ID | pull.1723.git.1714365076246.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/tag.c: add --trailer arg | expand |
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
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
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
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
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
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.
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.
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index 5fe519c31ec..79b0a7e9644 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -10,6 +10,7 @@ SYNOPSIS -------- [verse] 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e] + [(--trailer <token>[(=|:)<value>])...] <tagname> [<commit> | <object>] 'git tag' -d <tagname>... 'git tag' [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>] @@ -31,8 +32,8 @@ creates a 'tag' object, and requires a tag message. Unless `-m <msg>` or `-F <file>` is given, an editor is started for the user to type in the tag message. -If `-m <msg>` or `-F <file>` is given and `-a`, `-s`, and `-u <key-id>` -are absent, `-a` is implied. +If `-m <msg>` or `-F <file>` or `--trailer <token>[=<value>]` is given +and `-a`, `-s`, and `-u <key-id>` are absent, `-a` is implied. Otherwise, a tag reference that points directly at the given object (i.e., a lightweight tag) is created. @@ -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. + -e:: --edit:: The message taken from file with `-F` and command line with 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) { 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); + } 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")); + } + + 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) { + 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; 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); } @@ -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 +test_expect_success 'create tag with -m and --trailer' ' + git tag -m "create tag with trailers" --trailer my-trailer=here --trailer alt-trailer=there tag-with-inline-message-and-trailers && + get_tag_msg tag-with-inline-message-and-trailers >actual && + test_cmp expect actual +' + +test_expect_success 'list tag extracting trailers' ' + cat >expect <<-\EOF && + my-trailer: here + alt-trailer: there + + EOF + git tag --list --format="%(trailers)" tag-with-inline-message-and-trailers >actual && + test_cmp expect actual +' + +echo 'create tag from message file using --trailer' >messagefilewithnotrailers +get_tag_header tag-with-file-message-and-trailers $commit commit $time >expect +cat >>expect <<EOF +create tag from message file using --trailer + +my-trailer: here +alt-trailer: there +EOF +test_expect_success 'create tag with -F and --trailer' ' + git tag -F messagefilewithnotrailers --trailer my-trailer=here --trailer alt-trailer=there tag-with-file-message-and-trailers && + get_tag_msg tag-with-file-message-and-trailers >actual && + test_cmp expect actual +' + +test_expect_success 'set up editor' ' + write_script fakeeditor <<-\EOF + sed -e "1s/^/EDITED: /g" <"$1" >"$1-" + mv "$1-" "$1" + EOF +' + +get_tag_header tag-with-edited-inline-message-and-trailers $commit commit $time >expect +cat >>expect <<EOF +EDITED: create tag with trailers + +my-trailer: here +alt-trailer: there +EOF +test_expect_success 'create tag with -m and --trailer and --edit' ' + GIT_EDITOR=./fakeeditor git tag --edit -m "create tag with trailers" --trailer my-trailer=here --trailer alt-trailer=there tag-with-edited-inline-message-and-trailers && + get_tag_msg tag-with-edited-inline-message-and-trailers >actual && + test_cmp expect actual +' + +echo 'create tag from message file using --trailer' >messagefilewithnotrailers +get_tag_header tag-with-edited-file-message-and-trailers $commit commit $time >expect +cat >>expect <<EOF +EDITED: create tag from message file using --trailer + +my-trailer: here +alt-trailer: there +EOF +test_expect_success 'create tag with -F and --trailer and --edit' ' + GIT_EDITOR=./fakeeditor git tag --edit -F messagefilewithnotrailers --trailer my-trailer=here --trailer alt-trailer=there tag-with-edited-file-message-and-trailers && + get_tag_msg tag-with-edited-file-message-and-trailers >actual && + test_cmp expect actual +' + +test_expect_success 'set up editor' ' + write_script fakeeditor <<-\EOF + echo "add a line" >"$1-" + echo >>"$1-" + cat <"$1" >>"$1-" + mv "$1-" "$1" + EOF +' + +get_tag_header tag-with-trailers-and-no-message $commit commit $time >expect +cat >>expect <<EOF +add a line + +my-trailer: here +alt-trailer: there +EOF +test_expect_success 'create annotated tag and force editor when only --trailer is given' ' + GIT_EDITOR=./fakeeditor git tag --trailer my-trailer=here --trailer alt-trailer=there tag-with-trailers-and-no-message && + get_tag_msg tag-with-trailers-and-no-message >actual && + test_cmp expect actual +' + +test_expect_success 'bad editor causes panic when only --trailer is given' ' + test_must_fail env GIT_EDITOR=false git tag --trailer my-trailer=here tag-will-not-exist +' + # listing messages for annotated non-signed tags: test_expect_success \ @@ -810,6 +909,11 @@ test_expect_success 'git tag --format with ahead-behind' ' refs/tags/tag-lines 0 1 ! refs/tags/tag-one-line 0 1 ! refs/tags/tag-right 0 0 ! + refs/tags/tag-with-edited-file-message-and-trailers 0 1 ! + refs/tags/tag-with-edited-inline-message-and-trailers 0 1 ! + refs/tags/tag-with-file-message-and-trailers 0 1 ! + refs/tags/tag-with-inline-message-and-trailers 0 1 ! + refs/tags/tag-with-trailers-and-no-message 0 1 ! refs/tags/tag-zero-lines 0 1 ! EOF git tag -l --format="%(refname) %(ahead-behind:HEAD) !" >actual 2>err &&