| Message ID | 5b6239167b8d7c26f96e5c23d0d82b7a3a9b01fe.1714416865.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/tag.c: add --trailer option | expand |
On Mon, Apr 29, 2024 at 06:54:22PM +0000, John Passaro via GitGitGadget wrote: > 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. > > 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. > > Signed-off-by: John Passaro <john.a.passaro@gmail.com> > Helped-by: Patrick Steinhardt <ps@pks.im> Same comment here regarding the order of trailers. Other than that this commit looks good to me, thanks! Patrick
"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?
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).
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?
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.
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..1ae7ea50b3f 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 "trailer.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,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) { + 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); + } } } @@ -463,6 +481,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 +498,8 @@ 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_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), + N_("add custom trailer(s)"), PARSE_OPT_NONEG), 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 +569,8 @@ 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); @@ -654,7 +676,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) opt.sign = 1; path = git_pathdup("TAG_EDITMSG"); create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, - path); + &trailer_args, path); } transaction = ref_transaction_begin(&err); @@ -686,6 +708,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..fa6336edf98 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -668,6 +668,115 @@ test_expect_success \ test_cmp expect actual ' +# trailers + +test_expect_success 'create tag with -m and --trailer' ' + 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 + 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 +' + +test_expect_success 'create tag with -F and --trailer' ' + 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 + 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 'create tag with -m and --trailer and --edit' ' + 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 + 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 +' + +test_expect_success 'create tag with -F and --trailer and --edit' ' + 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 + 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 'create annotated tag and force editor when only --trailer is given' ' + write_script fakeeditor <<-\EOF && + echo "add a line" >"$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 + 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 +919,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 &&