git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Apr 29, 2024, 06:50 UTC
Message-ID
<Zi9DGYwlT7VnW7oj@tanuki>
In-Reply-To
<pull.1723.git.1714365076246.gitgitgadget@gmail.com>
On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
Show 6 quoted lines
> 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]
Show 22 quoted lines
> 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.
Show 25 quoted lines
>  {
>  	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?
Show 22 quoted lines
>  			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?

Show 6 quoted lines
> +		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.

Show 57 quoted lines
> +		} 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.

Show 8 quoted lines
>  	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.
Show 27 quoted lines
>  	}
>  
> @@ -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
Previous: John Passaro via GitGitGadgetNext: John Passaro
Message 2 of 37 in “builtin/tag.c: add --trailer arg”
  1. builtin/tag.c: add --trailer argJohn Passaro via GitGitGadget, Apr 29, 2024
  2. Patrick SteinhardtApr 29, 2024
  3. John PassaroApr 29, 2024
  4. John PassaroApr 29, 2024
  5. Junio C HamanoApr 29, 2024
  6. Junio C HamanoApr 29, 2024
  7. John PassaroApr 29, 2024
  8. Junio C HamanoApr 29, 2024
  9. builtin/tag.c: add --trailer argJohn Passaro via GitGitGadget, Apr 29, 2024
  10. 0/3 builtin/tag.c: add --trailer optionJohn Passaro via GitGitGadget, Apr 29, 2024
  11. 1/3 builtin/commit.c: refactor --trailer logicJohn Passaro via GitGitGadget, Apr 29, 2024
  12. Patrick SteinhardtApr 30, 2024
  13. Junio C HamanoApr 30, 2024
  14. 2/3 builtin/tag.c: add --trailer argJohn Passaro via GitGitGadget, Apr 29, 2024
  15. Patrick SteinhardtApr 30, 2024
  16. Junio C HamanoApr 30, 2024
  17. John PassaroApr 30, 2024
  18. Junio C HamanoApr 30, 2024
  19. John PassaroMay 5, 2024
  20. 3/3 po: update git-tag translationsJohn Passaro via GitGitGadget, Apr 29, 2024
  21. Junio C HamanoApr 29, 2024
  22. John PassaroApr 29, 2024
  23. 0/3 builtin/tag.c: add --trailer optionJohn Passaro via GitGitGadget, Apr 30, 2024
  24. 1/3 builtin/commit.c: remove bespoke option callbackJohn Passaro via GitGitGadget, Apr 30, 2024
  25. Patrick SteinhardtMay 2, 2024
  26. 2/3 builtin/commit.c: refactor --trailer logicJohn Passaro via GitGitGadget, Apr 30, 2024
  27. Patrick SteinhardtMay 2, 2024
  28. 3/3 builtin/tag.c: add --trailer optionJohn Passaro via GitGitGadget, Apr 30, 2024
  29. Patrick SteinhardtMay 2, 2024
  30. 0/3 builtin/tag.c: add --trailer optionJohn Passaro via GitGitGadget, May 5, 2024
  31. 1/3 builtin/commit: use ARGV macro to collect trailersJohn Passaro via GitGitGadget, May 5, 2024
  32. John PassaroMay 7, 2024
  33. Junio C HamanoMay 7, 2024
  34. 2/3 builtin/commit: refactor --trailer logicJohn Passaro via GitGitGadget, May 5, 2024
  35. 3/3 builtin/tag: add --trailer optionJohn Passaro via GitGitGadget, May 5, 2024
  36. Patrick SteinhardtMay 6, 2024
  37. Junio C HamanoMay 6, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.