Re: [PATCH v3 2/3] builtin/tag.c: add --trailer arg
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 30, 2024, 16:53 UTC
- Message-ID
- <xmqqv83yrduw.fsf@gitster.g>
- In-Reply-To
- <5b6239167b8d7c26f96e5c23d0d82b7a3a9b01fe.1714416865.git.gitgitgadget@gmail.com>
"John Passaro via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 16 quoted lines
> @@ -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?
Show 50 quoted lines
> @@ -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.
Show 5 quoted lines
> + 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?