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

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

[John Passaro via GitGitGadget]

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>
---
    builtin/tag.c: add --trailer arg

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1723%2Fjpassaro%2Fjp%2Ftag-trailer-arg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1723/jpassaro/jp/tag-trailer-arg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1723

 Documentation/git-tag.txt |  18 ++++++-
 builtin/tag.c             |  59 +++++++++++++++++----
 t/t7004-tag.sh            | 104 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+), 12 deletions(-)

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 &&

base-commit: e326e520101dcf43a0499c3adc2df7eca30add2d
-- 
gitgitgadget

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 7952 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[John Passaro]

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

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

[John Passaro]

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

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

[Junio C Hamano]

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

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

[John Passaro]

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

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

[John Passaro via GitGitGadget]

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. Move the code that git-commit uses for
trailers to the trailer.h API, so it can be re-used for git-tag.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
    builtin/tag.c: add --trailer arg
    
    cc: Patrick Steinhardt ps@pks.im cc: John Passaro
    john.a.passaro@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1723%2Fjpassaro%2Fjp%2Ftag-trailer-arg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1723/jpassaro/jp/tag-trailer-arg-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1723

Range-diff vs v1:

 1:  02d7a0f035e ! 1:  d4beb7cd67e builtin/tag.c: add --trailer arg
     @@ Commit message
          builtin/tag.c: add --trailer arg
      
          Teach git-tag to accept --trailer option to add trailers to annotated
     -    tag messages, like git-commit.
     +    tag messages, like git-commit. Move the code that git-commit uses for
     +    trailers to the trailer.h API, so it can be re-used for git-tag.
      
          Signed-off-by: John Passaro <john.a.passaro@gmail.com>
      
     @@ Documentation/git-tag.txt: This option is only applicable when listing tags with
       --edit::
       	The message taken from file with `-F` and command line with
      
     + ## builtin/commit.c ##
     +@@
     + #include "commit-reach.h"
     + #include "commit-graph.h"
     + #include "pretty.h"
     ++#include "trailer.h"
     + 
     + static const char * const builtin_commit_usage[] = {
     + 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
     +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
     + 	fclose(s->fp);
     + 
     + 	if (trailer_args.nr) {
     +-		struct child_process run_trailer = CHILD_PROCESS_INIT;
     +-
     +-		strvec_pushl(&run_trailer.args, "interpret-trailers",
     +-			     "--in-place", "--no-divider",
     +-			     git_path_commit_editmsg(), NULL);
     +-		strvec_pushv(&run_trailer.args, trailer_args.v);
     +-		run_trailer.git_cmd = 1;
     +-		if (run_command(&run_trailer))
     ++		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
     + 			die(_("unable to pass trailers to --trailers"));
     + 		strvec_clear(&trailer_args);
     + 	}
     +
       ## builtin/tag.c ##
      @@
       #include "date.h"
       #include "write-or-die.h"
       #include "object-file-convert.h"
     -+#include "run-command.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"
     @@ builtin/tag.c: static const char message_advice_nested_tag[] =
       		       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)
     ++		       struct object_id *prev, struct object_id *result,
     ++		       struct strvec *trailer_args, char *path)
       {
       	enum object_type type;
       	struct strbuf header = STRBUF_INIT;
     @@ builtin/tag.c: static void create_tag(const struct object_id *object, const char
       
      -		if (opt->message_given) {
      +		if (opt->message_given && buf->len) {
     ++			strbuf_complete(buf, '\n');
       			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);
      @@ builtin/tag.c: static void create_tag(const struct object_id *object, const char *object_ref,
       		}
       		close(fd);
     @@ builtin/tag.c: static void create_tag(const struct object_id *object, const char
      -			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 (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"));
     ++					_("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"));
     ++					_("Please supply the message using either -m or -F option.\n"));
      +				exit(1);
      +			}
       		}
     @@ builtin/tag.c: 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_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),
     @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
       		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);
     ++	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);
      @@ builtin/tag.c: 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"))
     -@@ builtin/tag.c: 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);
     + 		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
     +-			   path);
     ++			   &trailer_args, path);
       	}
       
     + 	transaction = ref_transaction_begin(&err);
      @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
       	strbuf_release(&reflog_msg);
       	strbuf_release(&msg.buf);
     @@ t/t7004-tag.sh: test_expect_success \
       
      +# 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_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
      +'
     @@ t/t7004-tag.sh: test_expect_success \
      +	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 &&
     ++	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 'set up editor' '
     -+	write_script fakeeditor <<-\EOF
     ++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
     ++	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 &&
     ++	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
      +'
      +
     -+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 &&
     ++	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 'set up editor' '
     -+	write_script fakeeditor <<-\EOF
     ++test_expect_success 'create annotated tag and force editor when only --trailer is given' '
     ++	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
     ++	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 &&
     ++	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
      +'
     @@ t/t7004-tag.sh: test_expect_success 'git tag --format with ahead-behind' '
       	refs/tags/tag-zero-lines 0 1 !
       	EOF
       	git tag -l --format="%(refname) %(ahead-behind:HEAD) !" >actual 2>err &&
     +
     + ## trailer.c ##
     +@@
     + #include "commit.h"
     + #include "trailer.h"
     + #include "list.h"
     ++#include "run-command.h"
     + /*
     +  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
     +  */
     +@@ trailer.c: void trailer_iterator_release(struct trailer_iterator *iter)
     + 	strbuf_release(&iter->val);
     + 	strbuf_release(&iter->key);
     + }
     ++
     ++int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
     ++	struct child_process run_trailer = CHILD_PROCESS_INIT;
     ++
     ++	run_trailer.git_cmd = 1;
     ++	strvec_pushl(&run_trailer.args, "interpret-trailers",
     ++		     "--in-place", "--no-divider",
     ++		     path, NULL);
     ++	strvec_pushv(&run_trailer.args, trailer_args->v);
     ++	return run_command(&run_trailer);
     ++}
     +
     + ## trailer.h ##
     +@@
     + 
     + #include "list.h"
     + #include "strbuf.h"
     ++#include "strvec.h"
     + 
     + enum trailer_where {
     + 	WHERE_DEFAULT,
     +@@ trailer.h: int trailer_iterator_advance(struct trailer_iterator *iter);
     +  */
     + void trailer_iterator_release(struct trailer_iterator *iter);
     + 
     ++/*
     ++ * Augment a file to add trailers to it by running git-interpret-trailers.
     ++ * This calls run_command() and its return value is the same (i.e. 0 for
     ++ * success, various non-zero for other errors). See run-command.h.
     ++ */
     ++int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);
     ++
     + #endif /* TRAILER_H */


 Documentation/git-tag.txt |  18 +++++-
 builtin/commit.c          |  10 +---
 builtin/tag.c             |  49 +++++++++++++---
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 trailer.c                 |  12 ++++
 trailer.h                 |   8 +++
 6 files changed, 192 insertions(+), 19 deletions(-)

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/commit.c b/builtin/commit.c
index 6e1484446b0..a1cbc128429 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -38,6 +38,7 @@
 #include "commit-reach.h"
 #include "commit-graph.h"
 #include "pretty.h"
+#include "trailer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
@@ -1038,14 +1039,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	fclose(s->fp);
 
 	if (trailer_args.nr) {
-		struct child_process run_trailer = CHILD_PROCESS_INIT;
-
-		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", "--no-divider",
-			     git_path_commit_editmsg(), NULL);
-		strvec_pushv(&run_trailer.args, trailer_args.v);
-		run_trailer.git_cmd = 1;
-		if (run_command(&run_trailer))
+		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
 			die(_("unable to pass trailers to --trailers"));
 		strvec_clear(&trailer_args);
 	}
diff --git a/builtin/tag.c b/builtin/tag.c
index 9a33cb50b45..0a029fb8c30 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);
+			}
 		}
 	}
 
@@ -416,6 +434,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 +489,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 +506,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_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 +577,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 +684,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 +716,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 &&
diff --git a/trailer.c b/trailer.c
index c72ae687099..843c378199e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -7,6 +7,7 @@
 #include "commit.h"
 #include "trailer.h"
 #include "list.h"
+#include "run-command.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
  */
@@ -1170,3 +1171,14 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->val);
 	strbuf_release(&iter->key);
 }
+
+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
+	struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+	run_trailer.git_cmd = 1;
+	strvec_pushl(&run_trailer.args, "interpret-trailers",
+		     "--in-place", "--no-divider",
+		     path, NULL);
+	strvec_pushv(&run_trailer.args, trailer_args->v);
+	return run_command(&run_trailer);
+}
diff --git a/trailer.h b/trailer.h
index 9f42aa75994..55f85b008ee 100644
--- a/trailer.h
+++ b/trailer.h
@@ -3,6 +3,7 @@
 
 #include "list.h"
 #include "strbuf.h"
+#include "strvec.h"
 
 enum trailer_where {
 	WHERE_DEFAULT,
@@ -158,4 +159,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
  */
 void trailer_iterator_release(struct trailer_iterator *iter);
 
+/*
+ * Augment a file to add trailers to it by running git-interpret-trailers.
+ * This calls run_command() and its return value is the same (i.e. 0 for
+ * success, various non-zero for other errors). See run-command.h.
+ */
+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);
+
 #endif /* TRAILER_H */

base-commit: e326e520101dcf43a0499c3adc2df7eca30add2d
-- 
gitgitgadget

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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.

[PATCH v3 0/3] builtin/tag.c: add --trailer option

[John Passaro via GitGitGadget]

Follow-up patch taking welcome feedback from Patrick and JCH.

Since git-tag --list --format="%(trailers)" can interpret trailers from
annotated tag messages, it seems natural to support --trailer when writing a
new tag message.

git-commit accomplishes this by taking --trailer arguments and passing them
to git interpret-trailer. This patch series refactors that logic and uses it
to implement --trailer on git-tag.

Also Included are updates to the i18n files, since the git-tag patch changes
some strings that are subject to translation. If I am out of my lane here,
please feel free to separate or leave out the i18n patch.

John Passaro (3):
  builtin/commit.c: refactor --trailer logic
  builtin/tag.c: add --trailer arg
  po: update git-tag translations

 Documentation/git-tag.txt |  18 +++++-
 builtin/commit.c          |  20 +------
 builtin/tag.c             |  41 +++++++++++---
 po/bg.po                  |   2 +
 po/ca.po                  |   4 +-
 po/de.po                  |   2 +
 po/el.po                  |   9 ++-
 po/es.po                  |  14 +++--
 po/fr.po                  |   2 +
 po/id.po                  |   2 +
 po/it.po                  |   6 +-
 po/ko.po                  |  10 ++--
 po/pl.po                  |   6 +-
 po/ru.po                  |   1 +
 po/sv.po                  |   2 +
 po/tr.po                  |   2 +
 po/uk.po                  |   2 +
 po/vi.po                  |   2 +
 po/zh_CN.po               |   2 +
 po/zh_TW.po               |   2 +
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 trailer.c                 |  12 ++++
 trailer.h                 |   8 +++
 23 files changed, 237 insertions(+), 46 deletions(-)


base-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1723%2Fjpassaro%2Fjp%2Ftag-trailer-arg-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1723/jpassaro/jp/tag-trailer-arg-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1723

Range-diff vs v2:

 -:  ----------- > 1:  0c9517f434a builtin/commit.c: refactor --trailer logic
 1:  d4beb7cd67e ! 2:  5b6239167b8 builtin/tag.c: add --trailer arg
     @@ Metadata
       ## Commit message ##
          builtin/tag.c: add --trailer arg
      
     -    Teach git-tag to accept --trailer option to add trailers to annotated
     -    tag messages, like git-commit. Move the code that git-commit uses for
     -    trailers to the trailer.h API, so it can be re-used for git-tag.
     +    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>
      
       ## Documentation/git-tag.txt ##
      @@ Documentation/git-tag.txt: SYNOPSIS
     @@ Documentation/git-tag.txt: This option is only applicable when listing tags with
       --edit::
       	The message taken from file with `-F` and command line with
      
     - ## builtin/commit.c ##
     -@@
     - #include "commit-reach.h"
     - #include "commit-graph.h"
     - #include "pretty.h"
     -+#include "trailer.h"
     - 
     - static const char * const builtin_commit_usage[] = {
     - 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
     -@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
     - 	fclose(s->fp);
     - 
     - 	if (trailer_args.nr) {
     --		struct child_process run_trailer = CHILD_PROCESS_INIT;
     --
     --		strvec_pushl(&run_trailer.args, "interpret-trailers",
     --			     "--in-place", "--no-divider",
     --			     git_path_commit_editmsg(), NULL);
     --		strvec_pushv(&run_trailer.args, trailer_args.v);
     --		run_trailer.git_cmd = 1;
     --		if (run_command(&run_trailer))
     -+		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
     - 			die(_("unable to pass trailers to --trailers"));
     - 		strvec_clear(&trailer_args);
     - 	}
     -
       ## builtin/tag.c ##
      @@
       #include "date.h"
     @@ builtin/tag.c: static void create_tag(const struct object_id *object, const char
       		}
       	}
       
     -@@ builtin/tag.c: 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;
      @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
       	struct ref_sorting *sorting;
       	struct string_list sorting_options = STRING_LIST_INIT_DUP;
     @@ builtin/tag.c: 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_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),
     @@ t/t7004-tag.sh: test_expect_success 'git tag --format with ahead-behind' '
       	refs/tags/tag-zero-lines 0 1 !
       	EOF
       	git tag -l --format="%(refname) %(ahead-behind:HEAD) !" >actual 2>err &&
     -
     - ## trailer.c ##
     -@@
     - #include "commit.h"
     - #include "trailer.h"
     - #include "list.h"
     -+#include "run-command.h"
     - /*
     -  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
     -  */
     -@@ trailer.c: void trailer_iterator_release(struct trailer_iterator *iter)
     - 	strbuf_release(&iter->val);
     - 	strbuf_release(&iter->key);
     - }
     -+
     -+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
     -+	struct child_process run_trailer = CHILD_PROCESS_INIT;
     -+
     -+	run_trailer.git_cmd = 1;
     -+	strvec_pushl(&run_trailer.args, "interpret-trailers",
     -+		     "--in-place", "--no-divider",
     -+		     path, NULL);
     -+	strvec_pushv(&run_trailer.args, trailer_args->v);
     -+	return run_command(&run_trailer);
     -+}
     -
     - ## trailer.h ##
     -@@
     - 
     - #include "list.h"
     - #include "strbuf.h"
     -+#include "strvec.h"
     - 
     - enum trailer_where {
     - 	WHERE_DEFAULT,
     -@@ trailer.h: int trailer_iterator_advance(struct trailer_iterator *iter);
     -  */
     - void trailer_iterator_release(struct trailer_iterator *iter);
     - 
     -+/*
     -+ * Augment a file to add trailers to it by running git-interpret-trailers.
     -+ * This calls run_command() and its return value is the same (i.e. 0 for
     -+ * success, various non-zero for other errors). See run-command.h.
     -+ */
     -+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);
     -+
     - #endif /* TRAILER_H */
 -:  ----------- > 3:  d5335e30b0b po: update git-tag translations

-- 
gitgitgadget

[PATCH v3 1/3] builtin/commit.c: refactor --trailer logic

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

git-commit adds user trailers to the commit message by passing its
`--trailer` arguments to a child process running `git-interpret-trailers
--in-place`. This logic is broadly useful, not just for git-commit but
for other commands constructing message bodies (e.g. git-tag).

Let's move this logic from git-commit to a new function in the trailer
API, so that it can be re-used in other commands.

Additionally, replace git-commit's bespoke callback for --trailer with
the standard OPT_PASSTHRU_ARGV macro. This bespoke callback was only
adding its values to a strvec and sanity-checking that `unset` is always
false; both of these are already implemented in the parse-option API.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c | 20 +++-----------------
 trailer.c        | 12 ++++++++++++
 trailer.h        |  8 ++++++++
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6e1484446b0..63cd090b6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -38,6 +38,7 @@
 #include "commit-reach.h"
 #include "commit-graph.h"
 #include "pretty.h"
+#include "trailer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
@@ -142,14 +143,6 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
-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 opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
 	enum wt_status_format *value = (enum wt_status_format *)opt->value;
@@ -1038,14 +1031,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	fclose(s->fp);
 
 	if (trailer_args.nr) {
-		struct child_process run_trailer = CHILD_PROCESS_INIT;
-
-		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", "--no-divider",
-			     git_path_commit_editmsg(), NULL);
-		strvec_pushv(&run_trailer.args, trailer_args.v);
-		run_trailer.git_cmd = 1;
-		if (run_command(&run_trailer))
+		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
 			die(_("unable to pass trailers to --trailers"));
 		strvec_clear(&trailer_args);
 	}
@@ -1673,7 +1659,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
-		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
+		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
diff --git a/trailer.c b/trailer.c
index c72ae687099..843c378199e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -7,6 +7,7 @@
 #include "commit.h"
 #include "trailer.h"
 #include "list.h"
+#include "run-command.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
  */
@@ -1170,3 +1171,14 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->val);
 	strbuf_release(&iter->key);
 }
+
+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
+	struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+	run_trailer.git_cmd = 1;
+	strvec_pushl(&run_trailer.args, "interpret-trailers",
+		     "--in-place", "--no-divider",
+		     path, NULL);
+	strvec_pushv(&run_trailer.args, trailer_args->v);
+	return run_command(&run_trailer);
+}
diff --git a/trailer.h b/trailer.h
index 9f42aa75994..55f85b008ee 100644
--- a/trailer.h
+++ b/trailer.h
@@ -3,6 +3,7 @@
 
 #include "list.h"
 #include "strbuf.h"
+#include "strvec.h"
 
 enum trailer_where {
 	WHERE_DEFAULT,
@@ -158,4 +159,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
  */
 void trailer_iterator_release(struct trailer_iterator *iter);
 
+/*
+ * Augment a file to add trailers to it by running git-interpret-trailers.
+ * This calls run_command() and its return value is the same (i.e. 0 for
+ * success, various non-zero for other errors). See run-command.h.
+ */
+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);
+
 #endif /* TRAILER_H */
-- 
gitgitgadget

[PATCH v3 2/3] builtin/tag.c: add --trailer arg

[John Passaro via GitGitGadget]

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>
---
 Documentation/git-tag.txt |  18 +++++-
 builtin/tag.c             |  41 +++++++++++---
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+), 11 deletions(-)

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 &&
-- 
gitgitgadget

[PATCH v3 3/3] po: update git-tag translations

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

Update translation files to translate the updated git-tag help headline
with its new --trailer option.

Also perform some related cleanup. Most notably, update git-tag headline
in files where it references a stale version: i.e. where we are still
attempting to translate the former one-line description of git-tag that
described the argument as "[<head>]", instead of current "[<commit> |
<object>]".

In both cases, reuse translations found elsewhere in the file.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 po/bg.po    |  2 ++
 po/ca.po    |  4 +++-
 po/de.po    |  2 ++
 po/el.po    |  9 ++++++---
 po/es.po    | 14 ++++++++------
 po/fr.po    |  2 ++
 po/id.po    |  2 ++
 po/it.po    |  6 ++++--
 po/ko.po    | 10 ++++++----
 po/pl.po    |  6 ++++--
 po/ru.po    |  1 +
 po/sv.po    |  2 ++
 po/tr.po    |  2 ++
 po/uk.po    |  2 ++
 po/vi.po    |  2 ++
 po/zh_CN.po |  2 ++
 po/zh_TW.po |  2 ++
 17 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/po/bg.po b/po/bg.po
index a7cbc617a53..d2fe6c87186 100644
--- a/po/bg.po
+++ b/po/bg.po
@@ -13555,9 +13555,11 @@ msgstr "причина за обновяването"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a|-s|-u ИДЕНТИФИКАТОР_НА_КЛЮЧ] [-f] [-m СЪОБЩЕНИЕ|-F ФАЙЛ] [-e]\n"
+"        [(--trailer ЛЕКСЕМА[(=|:)СТОЙНОСТ])…]\n"
 "        ЕТИКЕТ [ПОДАВАНЕ|ОБЕКТ]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/ca.po b/po/ca.po
index bcb4da80fb9..8c8f52985b7 100644
--- a/po/ca.po
+++ b/po/ca.po
@@ -13114,10 +13114,12 @@ msgstr "raó de l'actualització"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <fitxer>] [-e]\n"
-"        <tagname> [<comissió> | <objecte>]"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <nom-d'etiqueta> [<comissió> | <objecte>]"
 
 msgid "git tag -d <tagname>..."
 msgstr "git tag -d <nom-d'etiqueta>..."
diff --git a/po/de.po b/po/de.po
index 29465665976..5380f294920 100644
--- a/po/de.po
+++ b/po/de.po
@@ -13297,9 +13297,11 @@ msgstr "Grund für die Aktualisierung"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <Key-ID>] [-f] [-m <Beschreibung> | -F <Datei>] [-e]\n"
+"        [(--trailer <Token>[(=|:)<Wert>])...]\n"
 "        <Tagname> [<Commit> | <Objekt>]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/el.po b/po/el.po
index 703f46d0c7c..210304b75da 100644
--- a/po/el.po
+++ b/po/el.po
@@ -18093,11 +18093,14 @@ msgstr ""
 
 #: builtin/tag.c:25
 msgid ""
-"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> "
-"[<head>]"
+"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <αναγνωριστικό κλειδί>] [-f] [-m <μήνυμα> | -F "
-"<αρχείο>] <όνομα ετικέτας> [<head>]"
+"<αρχείο>]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <όνομα ετικέτας> [<υποβολή> | <αντικείμενο>]"
 
 #: builtin/tag.c:26
 msgid "git tag -d <tagname>..."
diff --git a/po/es.po b/po/es.po
index 1ff5ff3911d..b7b15ad6e4b 100644
--- a/po/es.po
+++ b/po/es.po
@@ -12682,10 +12682,12 @@ msgstr "razón de la actualización"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
-"        <tagname> [<head>]"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <tagname> [<commit> | <object>]"
 msgstr ""
-"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
-"        <tagname> [<head>]"
+"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <archivo>]\n"
+"        [(--trailer <token>[(=|:)<valor>])...]\n"
+"        <nombre-de-tag> [<commit> | <objeto>]"
 
 msgid "git tag -d <tagname>..."
 msgstr "git tag -d <nombre-de-tag>..."
@@ -12697,8 +12699,8 @@ msgid ""
 "[<pattern>...]"
 msgstr ""
 "git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--"
-"points-at <object>]\n"
-"        [--format=<format>] [--merged <commit>] [--no-merged <commit>] "
+"points-at <objeto>]\n"
+"        [--format=<formato>] [--merged <commit>] [--no-merged <commit>] "
 "[<pattern>...]"
 
 msgid "git tag -v [--format=<format>] <tagname>..."
@@ -18461,7 +18463,7 @@ msgstr "%%(body) no toma ningún argumento"
 
 #, c-format
 msgid "expected %%(trailers:key=<value>)"
-msgstr "se esperaba %%(trailers:key=<value>)"
+msgstr "se esperaba %%(trailers:key=<valor>)"
 
 #, c-format
 msgid "unknown %%(trailers) argument: %s"
diff --git a/po/fr.po b/po/fr.po
index 837a695485a..2f7a345c74a 100644
--- a/po/fr.po
+++ b/po/fr.po
@@ -13266,9 +13266,11 @@ msgstr "raison de la mise à jour"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <id-clé>] [-f] [-m <msg> | -F <fichier>] [-e]\n"
+"        [(--trailer <symbole>[(=|:)<valeur>])...]\n"
 "        <nom-d-étiquette> [<commit> | <objet>]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/id.po b/po/id.po
index 2dc4b79e8a5..85b5631b3c7 100644
--- a/po/id.po
+++ b/po/id.po
@@ -16230,9 +16230,11 @@ msgstr "alasan pembaruan"
 #: builtin/tag.c
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <id kunci>] [-f] [-m <pesan> | -F <berkas>] [-e]\n"
+"        [(--trailer <token>[(=|:)<nilai>])...]\n"
 "        <nama tag> [<komit> | <objek>]"
 
 #: builtin/tag.c
diff --git a/po/it.po b/po/it.po
index c31560834d8..c4ceb8b2444 100644
--- a/po/it.po
+++ b/po/it.po
@@ -22486,10 +22486,12 @@ msgstr "motivo dell'aggiornamento"
 #: builtin/tag.c:25
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
-"\t\t<tagname> [<head>]"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <ID chiave>] [-f] [-m <messaggio> | -F <file>]\n"
-"\t\t<nome tag> [<head>]"
+"        [(--trailer <token>[(=|:)<valore>])...]\n"
+"        <nome tag> [<commit> | <oggetto>]"
 
 #: builtin/tag.c:27
 msgid "git tag -d <tagname>..."
diff --git a/po/ko.po b/po/ko.po
index 5d190e52380..7484304eddb 100644
--- a/po/ko.po
+++ b/po/ko.po
@@ -14397,11 +14397,13 @@ msgstr "업데이트의 이유"
 
 #: builtin/tag.c:24
 msgid ""
-"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> "
-"[<head>]"
+"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <tagname> [<commit> | <object>]"
 msgstr ""
-"git tag [-a | -s | -u <키-ID>] [-f] [-m <메시지> | -F <파일>] <태그이름>\n"
-"\t\t[<헤드>]"
+"git tag [-a | -s | -u <키-ID>] [-f] [-m <메시지> | -F <파일>]\n"
+"        [(--trailer <토큰>[(=|:)<값>])...]\n"
+"        <태그이름> [<커밋> | <오브젝트>]"
 
 #: builtin/tag.c:25
 msgid "git tag -d <tagname>..."
diff --git a/po/pl.po b/po/pl.po
index 0ec127e14cc..287354b17b4 100644
--- a/po/pl.po
+++ b/po/pl.po
@@ -23549,10 +23549,12 @@ msgstr "powód aktualizacji"
 #: builtin/tag.c:25
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
-"        <tagname> [<head>]"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
+"        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <id-klucza>] [-f] [-m <komunikat> | -F <plik>]\n"
-"        <tag> [<czoło>]"
+"        [(--trailer <klucz>[(=|:)<wartość>])...]\n"
+"        <nazwa-tagu> [<zapis> | <obiekt>]"
 
 #: builtin/tag.c:27
 msgid "git tag -d <tagname>..."
diff --git a/po/ru.po b/po/ru.po
index 3e56eb546ea..d357bc30c2c 100644
--- a/po/ru.po
+++ b/po/ru.po
@@ -12490,6 +12490,7 @@ msgstr "причина обновления"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 
diff --git a/po/sv.po b/po/sv.po
index ad1aad94fff..1f878ee28bc 100644
--- a/po/sv.po
+++ b/po/sv.po
@@ -12835,9 +12835,11 @@ msgstr "skäl till uppdateringen"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <nyckel-id>] [-f] [-m <medd> | -F <fil>] [-e]\n"
+"        [(--trailer <symbol>[(=|:)<värde>])...]\n"
 "        <taggnamn> [<incheckning> | <objekt>]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/tr.po b/po/tr.po
index 0e220e1c1cd..8c506c34a9e 100644
--- a/po/tr.po
+++ b/po/tr.po
@@ -12975,10 +12975,12 @@ msgstr "güncelleme nedeni"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <anahtar-kimliği>] [-f] [-m <ileti> | -F <dosya>] [-"
 "e]\n"
+"        [(--trailer <jeton>[(=|:)<değer>])...]\n"
 "        <etiket-adı> [<işleme> | <nesne>]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/uk.po b/po/uk.po
index 528a3dc6f76..e3b6441e3c1 100644
--- a/po/uk.po
+++ b/po/uk.po
@@ -13086,10 +13086,12 @@ msgstr "причина оновлення"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <ідентифікатор-ключа>] [-f] [-m <допис> | -F <файл>] [-"
 "e]\n"
+"        [(--trailer <токен>[(=|:)<значення>])...]\n"
 "        <назва-тегу> [<коміт> | <об’єкт>]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/vi.po b/po/vi.po
index 965e79e9658..0b206309f9b 100644
--- a/po/vi.po
+++ b/po/vi.po
@@ -12915,9 +12915,11 @@ msgstr "lý do cập nhật"
 
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <tập-tin>] [-e]\n"
+"        [(--trailer <thẻ>[(=|:)<giá-trị>])...]\n"
 "        <tên-thẻ> [<lần chuyển giao> | <đối tượng> ]"
 
 msgid "git tag -d <tagname>..."
diff --git a/po/zh_CN.po b/po/zh_CN.po
index 4838c19b0be..aaedddcd864 100644
--- a/po/zh_CN.po
+++ b/po/zh_CN.po
@@ -16033,9 +16033,11 @@ msgstr "更新的原因"
 #: builtin/tag.c
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <私钥 ID>] [-f] [-m <消息> | -F <文件>] [-e]\n"
+"        [(--trailer <键>[(=|:)<值>])...]\n"
 "        <标签名> [<提交> | <对象>]"
 
 #: builtin/tag.c
diff --git a/po/zh_TW.po b/po/zh_TW.po
index f554381a7af..168591c141e 100644
--- a/po/zh_TW.po
+++ b/po/zh_TW.po
@@ -15911,9 +15911,11 @@ msgstr "更新的原因"
 #: builtin/tag.c
 msgid ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 msgstr ""
 "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
+"        [(--trailer <token>[(=|:)<value>])...]\n"
 "        <tagname> [<commit> | <object>]"
 
 #: builtin/tag.c
-- 
gitgitgadget

Re: [PATCH v3 3/3] po: update git-tag translations

[Junio C Hamano]

"John Passaro via GitGitGadget" <gitgitgadget@gmail.com> writes:

> In both cases, reuse translations found elsewhere in the file.

When each l10n team sees an updated code and translations together,
will they have a chance to easily (1) notice that there are
translations for their language that came from outside the normal
process (namely, this patch), and (2) double-check such a
translated text that came from sidelines are reasonable?

I am wondering if we should be doing some trick that involve use of
comments like "# fuzzy" in the file here.  While I applaud your good
intention to help reduce the workload for translators, unless you
belong to all of the l10n teams for these languages (note: I did not
say "unless you are fluent in these languages" here---I am playing
safe by assuming that l10n teams would want to have a chance to
verify even a translation that comes from a native that comes
outside the usual workflow of the l10n groups), I would really want
to avoid a change like this go unnoticed by l10n teams (iow, as long
as we are sure they will notice and have chance to verify, I am
fine with a change like this).

Thanks.

> Signed-off-by: John Passaro <john.a.passaro@gmail.com>
> ---
>  po/bg.po    |  2 ++
>  po/ca.po    |  4 +++-
>  po/de.po    |  2 ++
>  po/el.po    |  9 ++++++---
>  po/es.po    | 14 ++++++++------
>  po/fr.po    |  2 ++
>  po/id.po    |  2 ++
>  po/it.po    |  6 ++++--
>  po/ko.po    | 10 ++++++----
>  po/pl.po    |  6 ++++--
>  po/ru.po    |  1 +
>  po/sv.po    |  2 ++
>  po/tr.po    |  2 ++
>  po/uk.po    |  2 ++
>  po/vi.po    |  2 ++
>  po/zh_CN.po |  2 ++
>  po/zh_TW.po |  2 ++
>  17 files changed, 52 insertions(+), 18 deletions(-)
>
> diff --git a/po/bg.po b/po/bg.po
> index a7cbc617a53..d2fe6c87186 100644
> --- a/po/bg.po
> +++ b/po/bg.po
> @@ -13555,9 +13555,11 @@ msgstr "причина за обновяването"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a|-s|-u ИДЕНТИФИКАТОР_НА_КЛЮЧ] [-f] [-m СЪОБЩЕНИЕ|-F ФАЙЛ] [-e]\n"
> +"        [(--trailer ЛЕКСЕМА[(=|:)СТОЙНОСТ])…]\n"
>  "        ЕТИКЕТ [ПОДАВАНЕ|ОБЕКТ]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/ca.po b/po/ca.po
> index bcb4da80fb9..8c8f52985b7 100644
> --- a/po/ca.po
> +++ b/po/ca.po
> @@ -13114,10 +13114,12 @@ msgstr "raó de l'actualització"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <fitxer>] [-e]\n"
> -"        <tagname> [<comissió> | <objecte>]"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <nom-d'etiqueta> [<comissió> | <objecte>]"
>  
>  msgid "git tag -d <tagname>..."
>  msgstr "git tag -d <nom-d'etiqueta>..."
> diff --git a/po/de.po b/po/de.po
> index 29465665976..5380f294920 100644
> --- a/po/de.po
> +++ b/po/de.po
> @@ -13297,9 +13297,11 @@ msgstr "Grund für die Aktualisierung"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <Key-ID>] [-f] [-m <Beschreibung> | -F <Datei>] [-e]\n"
> +"        [(--trailer <Token>[(=|:)<Wert>])...]\n"
>  "        <Tagname> [<Commit> | <Objekt>]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/el.po b/po/el.po
> index 703f46d0c7c..210304b75da 100644
> --- a/po/el.po
> +++ b/po/el.po
> @@ -18093,11 +18093,14 @@ msgstr ""
>  
>  #: builtin/tag.c:25
>  msgid ""
> -"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> "
> -"[<head>]"
> +"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <αναγνωριστικό κλειδί>] [-f] [-m <μήνυμα> | -F "
> -"<αρχείο>] <όνομα ετικέτας> [<head>]"
> +"<αρχείο>]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <όνομα ετικέτας> [<υποβολή> | <αντικείμενο>]"
>  
>  #: builtin/tag.c:26
>  msgid "git tag -d <tagname>..."
> diff --git a/po/es.po b/po/es.po
> index 1ff5ff3911d..b7b15ad6e4b 100644
> --- a/po/es.po
> +++ b/po/es.po
> @@ -12682,10 +12682,12 @@ msgstr "razón de la actualización"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> -"        <tagname> [<head>]"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <tagname> [<commit> | <object>]"
>  msgstr ""
> -"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> -"        <tagname> [<head>]"
> +"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <archivo>]\n"
> +"        [(--trailer <token>[(=|:)<valor>])...]\n"
> +"        <nombre-de-tag> [<commit> | <objeto>]"
>  
>  msgid "git tag -d <tagname>..."
>  msgstr "git tag -d <nombre-de-tag>..."
> @@ -12697,8 +12699,8 @@ msgid ""
>  "[<pattern>...]"
>  msgstr ""
>  "git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--"
> -"points-at <object>]\n"
> -"        [--format=<format>] [--merged <commit>] [--no-merged <commit>] "
> +"points-at <objeto>]\n"
> +"        [--format=<formato>] [--merged <commit>] [--no-merged <commit>] "
>  "[<pattern>...]"
>  
>  msgid "git tag -v [--format=<format>] <tagname>..."
> @@ -18461,7 +18463,7 @@ msgstr "%%(body) no toma ningún argumento"
>  
>  #, c-format
>  msgid "expected %%(trailers:key=<value>)"
> -msgstr "se esperaba %%(trailers:key=<value>)"
> +msgstr "se esperaba %%(trailers:key=<valor>)"
>  
>  #, c-format
>  msgid "unknown %%(trailers) argument: %s"
> diff --git a/po/fr.po b/po/fr.po
> index 837a695485a..2f7a345c74a 100644
> --- a/po/fr.po
> +++ b/po/fr.po
> @@ -13266,9 +13266,11 @@ msgstr "raison de la mise à jour"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <id-clé>] [-f] [-m <msg> | -F <fichier>] [-e]\n"
> +"        [(--trailer <symbole>[(=|:)<valeur>])...]\n"
>  "        <nom-d-étiquette> [<commit> | <objet>]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/id.po b/po/id.po
> index 2dc4b79e8a5..85b5631b3c7 100644
> --- a/po/id.po
> +++ b/po/id.po
> @@ -16230,9 +16230,11 @@ msgstr "alasan pembaruan"
>  #: builtin/tag.c
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <id kunci>] [-f] [-m <pesan> | -F <berkas>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<nilai>])...]\n"
>  "        <nama tag> [<komit> | <objek>]"
>  
>  #: builtin/tag.c
> diff --git a/po/it.po b/po/it.po
> index c31560834d8..c4ceb8b2444 100644
> --- a/po/it.po
> +++ b/po/it.po
> @@ -22486,10 +22486,12 @@ msgstr "motivo dell'aggiornamento"
>  #: builtin/tag.c:25
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> -"\t\t<tagname> [<head>]"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <ID chiave>] [-f] [-m <messaggio> | -F <file>]\n"
> -"\t\t<nome tag> [<head>]"
> +"        [(--trailer <token>[(=|:)<valore>])...]\n"
> +"        <nome tag> [<commit> | <oggetto>]"
>  
>  #: builtin/tag.c:27
>  msgid "git tag -d <tagname>..."
> diff --git a/po/ko.po b/po/ko.po
> index 5d190e52380..7484304eddb 100644
> --- a/po/ko.po
> +++ b/po/ko.po
> @@ -14397,11 +14397,13 @@ msgstr "업데이트의 이유"
>  
>  #: builtin/tag.c:24
>  msgid ""
> -"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> "
> -"[<head>]"
> +"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <tagname> [<commit> | <object>]"
>  msgstr ""
> -"git tag [-a | -s | -u <키-ID>] [-f] [-m <메시지> | -F <파일>] <태그이름>\n"
> -"\t\t[<헤드>]"
> +"git tag [-a | -s | -u <키-ID>] [-f] [-m <메시지> | -F <파일>]\n"
> +"        [(--trailer <토큰>[(=|:)<값>])...]\n"
> +"        <태그이름> [<커밋> | <오브젝트>]"
>  
>  #: builtin/tag.c:25
>  msgid "git tag -d <tagname>..."
> diff --git a/po/pl.po b/po/pl.po
> index 0ec127e14cc..287354b17b4 100644
> --- a/po/pl.po
> +++ b/po/pl.po
> @@ -23549,10 +23549,12 @@ msgstr "powód aktualizacji"
>  #: builtin/tag.c:25
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> -"        <tagname> [<head>]"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
> +"        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <id-klucza>] [-f] [-m <komunikat> | -F <plik>]\n"
> -"        <tag> [<czoło>]"
> +"        [(--trailer <klucz>[(=|:)<wartość>])...]\n"
> +"        <nazwa-tagu> [<zapis> | <obiekt>]"
>  
>  #: builtin/tag.c:27
>  msgid "git tag -d <tagname>..."
> diff --git a/po/ru.po b/po/ru.po
> index 3e56eb546ea..d357bc30c2c 100644
> --- a/po/ru.po
> +++ b/po/ru.po
> @@ -12490,6 +12490,7 @@ msgstr "причина обновления"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  
> diff --git a/po/sv.po b/po/sv.po
> index ad1aad94fff..1f878ee28bc 100644
> --- a/po/sv.po
> +++ b/po/sv.po
> @@ -12835,9 +12835,11 @@ msgstr "skäl till uppdateringen"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <nyckel-id>] [-f] [-m <medd> | -F <fil>] [-e]\n"
> +"        [(--trailer <symbol>[(=|:)<värde>])...]\n"
>  "        <taggnamn> [<incheckning> | <objekt>]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/tr.po b/po/tr.po
> index 0e220e1c1cd..8c506c34a9e 100644
> --- a/po/tr.po
> +++ b/po/tr.po
> @@ -12975,10 +12975,12 @@ msgstr "güncelleme nedeni"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <anahtar-kimliği>] [-f] [-m <ileti> | -F <dosya>] [-"
>  "e]\n"
> +"        [(--trailer <jeton>[(=|:)<değer>])...]\n"
>  "        <etiket-adı> [<işleme> | <nesne>]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/uk.po b/po/uk.po
> index 528a3dc6f76..e3b6441e3c1 100644
> --- a/po/uk.po
> +++ b/po/uk.po
> @@ -13086,10 +13086,12 @@ msgstr "причина оновлення"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <ідентифікатор-ключа>] [-f] [-m <допис> | -F <файл>] [-"
>  "e]\n"
> +"        [(--trailer <токен>[(=|:)<значення>])...]\n"
>  "        <назва-тегу> [<коміт> | <об’єкт>]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/vi.po b/po/vi.po
> index 965e79e9658..0b206309f9b 100644
> --- a/po/vi.po
> +++ b/po/vi.po
> @@ -12915,9 +12915,11 @@ msgstr "lý do cập nhật"
>  
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <tập-tin>] [-e]\n"
> +"        [(--trailer <thẻ>[(=|:)<giá-trị>])...]\n"
>  "        <tên-thẻ> [<lần chuyển giao> | <đối tượng> ]"
>  
>  msgid "git tag -d <tagname>..."
> diff --git a/po/zh_CN.po b/po/zh_CN.po
> index 4838c19b0be..aaedddcd864 100644
> --- a/po/zh_CN.po
> +++ b/po/zh_CN.po
> @@ -16033,9 +16033,11 @@ msgstr "更新的原因"
>  #: builtin/tag.c
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <私钥 ID>] [-f] [-m <消息> | -F <文件>] [-e]\n"
> +"        [(--trailer <键>[(=|:)<值>])...]\n"
>  "        <标签名> [<提交> | <对象>]"
>  
>  #: builtin/tag.c
> diff --git a/po/zh_TW.po b/po/zh_TW.po
> index f554381a7af..168591c141e 100644
> --- a/po/zh_TW.po
> +++ b/po/zh_TW.po
> @@ -15911,9 +15911,11 @@ msgstr "更新的原因"
>  #: builtin/tag.c
>  msgid ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  msgstr ""
>  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +"        [(--trailer <token>[(=|:)<value>])...]\n"
>  "        <tagname> [<commit> | <object>]"
>  
>  #: builtin/tag.c

Re: [PATCH v3 3/3] po: update git-tag translations

[John Passaro]

Fair enough. I thought there might be some process making this
unnecessary and it seems that is so. Sounds like the best move is to
reject this patch, or at least to consider it separately.

I assume that if the other two patches LGTY that you can easily queue
them up without this one. If that's not so, let me know and I'll
resubmit the series without l10n changes.

On Mon, Apr 29, 2024 at 3:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "John Passaro via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > In both cases, reuse translations found elsewhere in the file.
>
> When each l10n team sees an updated code and translations together,
> will they have a chance to easily (1) notice that there are
> translations for their language that came from outside the normal
> process (namely, this patch), and (2) double-check such a
> translated text that came from sidelines are reasonable?
>
> I am wondering if we should be doing some trick that involve use of
> comments like "# fuzzy" in the file here.  While I applaud your good
> intention to help reduce the workload for translators, unless you
> belong to all of the l10n teams for these languages (note: I did not
> say "unless you are fluent in these languages" here---I am playing
> safe by assuming that l10n teams would want to have a chance to
> verify even a translation that comes from a native that comes
> outside the usual workflow of the l10n groups), I would really want
> to avoid a change like this go unnoticed by l10n teams (iow, as long
> as we are sure they will notice and have chance to verify, I am
> fine with a change like this).
>
> Thanks.
>
> > Signed-off-by: John Passaro <john.a.passaro@gmail.com>
> > ---
> >  po/bg.po    |  2 ++
> >  po/ca.po    |  4 +++-
> >  po/de.po    |  2 ++
> >  po/el.po    |  9 ++++++---
> >  po/es.po    | 14 ++++++++------
> >  po/fr.po    |  2 ++
> >  po/id.po    |  2 ++
> >  po/it.po    |  6 ++++--
> >  po/ko.po    | 10 ++++++----
> >  po/pl.po    |  6 ++++--
> >  po/ru.po    |  1 +
> >  po/sv.po    |  2 ++
> >  po/tr.po    |  2 ++
> >  po/uk.po    |  2 ++
> >  po/vi.po    |  2 ++
> >  po/zh_CN.po |  2 ++
> >  po/zh_TW.po |  2 ++
> >  17 files changed, 52 insertions(+), 18 deletions(-)
> >
> > diff --git a/po/bg.po b/po/bg.po
> > index a7cbc617a53..d2fe6c87186 100644
> > --- a/po/bg.po
> > +++ b/po/bg.po
> > @@ -13555,9 +13555,11 @@ msgstr "причина за обновяването"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a|-s|-u ИДЕНТИФИКАТОР_НА_КЛЮЧ] [-f] [-m СЪОБЩЕНИЕ|-F ФАЙЛ] [-e]\n"
> > +"        [(--trailer ЛЕКСЕМА[(=|:)СТОЙНОСТ])…]\n"
> >  "        ЕТИКЕТ [ПОДАВАНЕ|ОБЕКТ]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/ca.po b/po/ca.po
> > index bcb4da80fb9..8c8f52985b7 100644
> > --- a/po/ca.po
> > +++ b/po/ca.po
> > @@ -13114,10 +13114,12 @@ msgstr "raó de l'actualització"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <fitxer>] [-e]\n"
> > -"        <tagname> [<comissió> | <objecte>]"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <nom-d'etiqueta> [<comissió> | <objecte>]"
> >
> >  msgid "git tag -d <tagname>..."
> >  msgstr "git tag -d <nom-d'etiqueta>..."
> > diff --git a/po/de.po b/po/de.po
> > index 29465665976..5380f294920 100644
> > --- a/po/de.po
> > +++ b/po/de.po
> > @@ -13297,9 +13297,11 @@ msgstr "Grund für die Aktualisierung"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <Key-ID>] [-f] [-m <Beschreibung> | -F <Datei>] [-e]\n"
> > +"        [(--trailer <Token>[(=|:)<Wert>])...]\n"
> >  "        <Tagname> [<Commit> | <Objekt>]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/el.po b/po/el.po
> > index 703f46d0c7c..210304b75da 100644
> > --- a/po/el.po
> > +++ b/po/el.po
> > @@ -18093,11 +18093,14 @@ msgstr ""
> >
> >  #: builtin/tag.c:25
> >  msgid ""
> > -"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> "
> > -"[<head>]"
> > +"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <αναγνωριστικό κλειδί>] [-f] [-m <μήνυμα> | -F "
> > -"<αρχείο>] <όνομα ετικέτας> [<head>]"
> > +"<αρχείο>]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <όνομα ετικέτας> [<υποβολή> | <αντικείμενο>]"
> >
> >  #: builtin/tag.c:26
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/es.po b/po/es.po
> > index 1ff5ff3911d..b7b15ad6e4b 100644
> > --- a/po/es.po
> > +++ b/po/es.po
> > @@ -12682,10 +12682,12 @@ msgstr "razón de la actualización"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> > -"        <tagname> [<head>]"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <tagname> [<commit> | <object>]"
> >  msgstr ""
> > -"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> > -"        <tagname> [<head>]"
> > +"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <archivo>]\n"
> > +"        [(--trailer <token>[(=|:)<valor>])...]\n"
> > +"        <nombre-de-tag> [<commit> | <objeto>]"
> >
> >  msgid "git tag -d <tagname>..."
> >  msgstr "git tag -d <nombre-de-tag>..."
> > @@ -12697,8 +12699,8 @@ msgid ""
> >  "[<pattern>...]"
> >  msgstr ""
> >  "git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--"
> > -"points-at <object>]\n"
> > -"        [--format=<format>] [--merged <commit>] [--no-merged <commit>] "
> > +"points-at <objeto>]\n"
> > +"        [--format=<formato>] [--merged <commit>] [--no-merged <commit>] "
> >  "[<pattern>...]"
> >
> >  msgid "git tag -v [--format=<format>] <tagname>..."
> > @@ -18461,7 +18463,7 @@ msgstr "%%(body) no toma ningún argumento"
> >
> >  #, c-format
> >  msgid "expected %%(trailers:key=<value>)"
> > -msgstr "se esperaba %%(trailers:key=<value>)"
> > +msgstr "se esperaba %%(trailers:key=<valor>)"
> >
> >  #, c-format
> >  msgid "unknown %%(trailers) argument: %s"
> > diff --git a/po/fr.po b/po/fr.po
> > index 837a695485a..2f7a345c74a 100644
> > --- a/po/fr.po
> > +++ b/po/fr.po
> > @@ -13266,9 +13266,11 @@ msgstr "raison de la mise à jour"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <id-clé>] [-f] [-m <msg> | -F <fichier>] [-e]\n"
> > +"        [(--trailer <symbole>[(=|:)<valeur>])...]\n"
> >  "        <nom-d-étiquette> [<commit> | <objet>]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/id.po b/po/id.po
> > index 2dc4b79e8a5..85b5631b3c7 100644
> > --- a/po/id.po
> > +++ b/po/id.po
> > @@ -16230,9 +16230,11 @@ msgstr "alasan pembaruan"
> >  #: builtin/tag.c
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <id kunci>] [-f] [-m <pesan> | -F <berkas>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<nilai>])...]\n"
> >  "        <nama tag> [<komit> | <objek>]"
> >
> >  #: builtin/tag.c
> > diff --git a/po/it.po b/po/it.po
> > index c31560834d8..c4ceb8b2444 100644
> > --- a/po/it.po
> > +++ b/po/it.po
> > @@ -22486,10 +22486,12 @@ msgstr "motivo dell'aggiornamento"
> >  #: builtin/tag.c:25
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> > -"\t\t<tagname> [<head>]"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <ID chiave>] [-f] [-m <messaggio> | -F <file>]\n"
> > -"\t\t<nome tag> [<head>]"
> > +"        [(--trailer <token>[(=|:)<valore>])...]\n"
> > +"        <nome tag> [<commit> | <oggetto>]"
> >
> >  #: builtin/tag.c:27
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/ko.po b/po/ko.po
> > index 5d190e52380..7484304eddb 100644
> > --- a/po/ko.po
> > +++ b/po/ko.po
> > @@ -14397,11 +14397,13 @@ msgstr "업데이트의 이유"
> >
> >  #: builtin/tag.c:24
> >  msgid ""
> > -"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> "
> > -"[<head>]"
> > +"git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <tagname> [<commit> | <object>]"
> >  msgstr ""
> > -"git tag [-a | -s | -u <키-ID>] [-f] [-m <메시지> | -F <파일>] <태그이름>\n"
> > -"\t\t[<헤드>]"
> > +"git tag [-a | -s | -u <키-ID>] [-f] [-m <메시지> | -F <파일>]\n"
> > +"        [(--trailer <토큰>[(=|:)<값>])...]\n"
> > +"        <태그이름> [<커밋> | <오브젝트>]"
> >
> >  #: builtin/tag.c:25
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/pl.po b/po/pl.po
> > index 0ec127e14cc..287354b17b4 100644
> > --- a/po/pl.po
> > +++ b/po/pl.po
> > @@ -23549,10 +23549,12 @@ msgstr "powód aktualizacji"
> >  #: builtin/tag.c:25
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> > -"        <tagname> [<head>]"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> > +"        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <id-klucza>] [-f] [-m <komunikat> | -F <plik>]\n"
> > -"        <tag> [<czoło>]"
> > +"        [(--trailer <klucz>[(=|:)<wartość>])...]\n"
> > +"        <nazwa-tagu> [<zapis> | <obiekt>]"
> >
> >  #: builtin/tag.c:27
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/ru.po b/po/ru.po
> > index 3e56eb546ea..d357bc30c2c 100644
> > --- a/po/ru.po
> > +++ b/po/ru.po
> > @@ -12490,6 +12490,7 @@ msgstr "причина обновления"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >
> > diff --git a/po/sv.po b/po/sv.po
> > index ad1aad94fff..1f878ee28bc 100644
> > --- a/po/sv.po
> > +++ b/po/sv.po
> > @@ -12835,9 +12835,11 @@ msgstr "skäl till uppdateringen"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <nyckel-id>] [-f] [-m <medd> | -F <fil>] [-e]\n"
> > +"        [(--trailer <symbol>[(=|:)<värde>])...]\n"
> >  "        <taggnamn> [<incheckning> | <objekt>]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/tr.po b/po/tr.po
> > index 0e220e1c1cd..8c506c34a9e 100644
> > --- a/po/tr.po
> > +++ b/po/tr.po
> > @@ -12975,10 +12975,12 @@ msgstr "güncelleme nedeni"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <anahtar-kimliği>] [-f] [-m <ileti> | -F <dosya>] [-"
> >  "e]\n"
> > +"        [(--trailer <jeton>[(=|:)<değer>])...]\n"
> >  "        <etiket-adı> [<işleme> | <nesne>]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/uk.po b/po/uk.po
> > index 528a3dc6f76..e3b6441e3c1 100644
> > --- a/po/uk.po
> > +++ b/po/uk.po
> > @@ -13086,10 +13086,12 @@ msgstr "причина оновлення"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <ідентифікатор-ключа>] [-f] [-m <допис> | -F <файл>] [-"
> >  "e]\n"
> > +"        [(--trailer <токен>[(=|:)<значення>])...]\n"
> >  "        <назва-тегу> [<коміт> | <об’єкт>]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/vi.po b/po/vi.po
> > index 965e79e9658..0b206309f9b 100644
> > --- a/po/vi.po
> > +++ b/po/vi.po
> > @@ -12915,9 +12915,11 @@ msgstr "lý do cập nhật"
> >
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <tập-tin>] [-e]\n"
> > +"        [(--trailer <thẻ>[(=|:)<giá-trị>])...]\n"
> >  "        <tên-thẻ> [<lần chuyển giao> | <đối tượng> ]"
> >
> >  msgid "git tag -d <tagname>..."
> > diff --git a/po/zh_CN.po b/po/zh_CN.po
> > index 4838c19b0be..aaedddcd864 100644
> > --- a/po/zh_CN.po
> > +++ b/po/zh_CN.po
> > @@ -16033,9 +16033,11 @@ msgstr "更新的原因"
> >  #: builtin/tag.c
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <私钥 ID>] [-f] [-m <消息> | -F <文件>] [-e]\n"
> > +"        [(--trailer <键>[(=|:)<值>])...]\n"
> >  "        <标签名> [<提交> | <对象>]"
> >
> >  #: builtin/tag.c
> > diff --git a/po/zh_TW.po b/po/zh_TW.po
> > index f554381a7af..168591c141e 100644
> > --- a/po/zh_TW.po
> > +++ b/po/zh_TW.po
> > @@ -15911,9 +15911,11 @@ msgstr "更新的原因"
> >  #: builtin/tag.c
> >  msgid ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >  msgstr ""
> >  "git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> > +"        [(--trailer <token>[(=|:)<value>])...]\n"
> >  "        <tagname> [<commit> | <object>]"
> >
> >  #: builtin/tag.c

Re: [PATCH v3 1/3] builtin/commit.c: refactor --trailer logic

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6184 bytes --]

On Mon, Apr 29, 2024 at 06:54:21PM +0000, John Passaro via GitGitGadget wrote:
> From: John Passaro <john.a.passaro@gmail.com>
> 
> git-commit adds user trailers to the commit message by passing its
> `--trailer` arguments to a child process running `git-interpret-trailers
> --in-place`. This logic is broadly useful, not just for git-commit but
> for other commands constructing message bodies (e.g. git-tag).
> 
> Let's move this logic from git-commit to a new function in the trailer
> API, so that it can be re-used in other commands.
> 
> Additionally, replace git-commit's bespoke callback for --trailer with
> the standard OPT_PASSTHRU_ARGV macro. This bespoke callback was only
> adding its values to a strvec and sanity-checking that `unset` is always
> false; both of these are already implemented in the parse-option API.
> 
> Signed-off-by: John Passaro <john.a.passaro@gmail.com>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Helped-by: Junio C Hamano <gitster@pobox.com>

Your signed-off-by should always go last.

> ---
>  builtin/commit.c | 20 +++-----------------
>  trailer.c        | 12 ++++++++++++
>  trailer.h        |  8 ++++++++
>  3 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6e1484446b0..63cd090b6f2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -38,6 +38,7 @@
>  #include "commit-reach.h"
>  #include "commit-graph.h"
>  #include "pretty.h"
> +#include "trailer.h"
>  
>  static const char * const builtin_commit_usage[] = {
>  	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
> @@ -142,14 +143,6 @@ static struct strbuf message = STRBUF_INIT;
>  
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
>  
> -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;
> -}
> -

Nice to see this gone. I would have moved this refactoring into a
separate commit because it is completely unrelated to the new trailer
function that you're introducing.

>  static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
>  {
>  	enum wt_status_format *value = (enum wt_status_format *)opt->value;
> @@ -1038,14 +1031,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	fclose(s->fp);
>  
>  	if (trailer_args.nr) {
> -		struct child_process run_trailer = CHILD_PROCESS_INIT;
> -
> -		strvec_pushl(&run_trailer.args, "interpret-trailers",
> -			     "--in-place", "--no-divider",
> -			     git_path_commit_editmsg(), NULL);
> -		strvec_pushv(&run_trailer.args, trailer_args.v);
> -		run_trailer.git_cmd = 1;
> -		if (run_command(&run_trailer))
> +		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
>  			die(_("unable to pass trailers to --trailers"));
>  		strvec_clear(&trailer_args);
>  	}
> @@ -1673,7 +1659,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
>  		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>  		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
> -		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> +		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
>  		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
>  		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
>  		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
> diff --git a/trailer.c b/trailer.c
> index c72ae687099..843c378199e 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -7,6 +7,7 @@
>  #include "commit.h"
>  #include "trailer.h"
>  #include "list.h"
> +#include "run-command.h"
>  /*
>   * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
>   */
> @@ -1170,3 +1171,14 @@ void trailer_iterator_release(struct trailer_iterator *iter)
>  	strbuf_release(&iter->val);
>  	strbuf_release(&iter->key);
>  }
> +
> +int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {

I would have called this `amend_trailers_to_file()`, which feels a bit
easier to understand. But I don't mind this much, your version should be
okay, too.

In any case, the second argument should be `const struct strvec *`. For
one, the `const` should come first. Second, the `*` always sticks to the
variable name in our codebase.

> +	struct child_process run_trailer = CHILD_PROCESS_INIT;
> +
> +	run_trailer.git_cmd = 1;
> +	strvec_pushl(&run_trailer.args, "interpret-trailers",
> +		     "--in-place", "--no-divider",
> +		     path, NULL);
> +	strvec_pushv(&run_trailer.args, trailer_args->v);
> +	return run_command(&run_trailer);
> +}
> diff --git a/trailer.h b/trailer.h
> index 9f42aa75994..55f85b008ee 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -3,6 +3,7 @@
>  
>  #include "list.h"
>  #include "strbuf.h"
> +#include "strvec.h"

Arguably you don't have to include "strvec.h" here, but can instead add
a simple forward declaration `struct strvec`.

>  enum trailer_where {
>  	WHERE_DEFAULT,
> @@ -158,4 +159,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
>   */
>  void trailer_iterator_release(struct trailer_iterator *iter);
>  
> +/*
> + * Augment a file to add trailers to it by running git-interpret-trailers.
> + * This calls run_command() and its return value is the same (i.e. 0 for
> + * success, various non-zero for other errors). See run-command.h.
> + */
> +int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);

Same comments here regarding the second argument.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 0/3] builtin/tag.c: add --trailer option

[John Passaro via GitGitGadget]

4th follow-up patch taking welcome feedback from Patrick and JCH. Net new
changes include separating from a 2-patch series to 3.

Since git-tag --list --format="%(trailers)" can interpret trailers from
annotated tag messages, it seems natural to support --trailer when writing a
new tag message.

git-commit accomplishes this by taking --trailer arguments and passing them
to git-interpret-trailer. This patch series refactors that logic and uses it
to implement --trailer on git-tag.

John Passaro (3):
  builtin/commit.c: remove bespoke option callback
  builtin/commit.c: refactor --trailer logic
  builtin/tag.c: add --trailer option

 Documentation/git-tag.txt |  18 +++++-
 builtin/commit.c          |  20 +------
 builtin/tag.c             |  41 +++++++++++---
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 trailer.c                 |  11 ++++
 trailer.h                 |   9 +++
 6 files changed, 185 insertions(+), 28 deletions(-)


base-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1723%2Fjpassaro%2Fjp%2Ftag-trailer-arg-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1723/jpassaro/jp/tag-trailer-arg-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1723

Range-diff vs v3:

 -:  ----------- > 1:  ce047c58aa8 builtin/commit.c: remove bespoke option callback
 1:  0c9517f434a ! 2:  8f53a54bbfe builtin/commit.c: refactor --trailer logic
     @@ Commit message
          Let's move this logic from git-commit to a new function in the trailer
          API, so that it can be re-used in other commands.
      
     -    Additionally, replace git-commit's bespoke callback for --trailer with
     -    the standard OPT_PASSTHRU_ARGV macro. This bespoke callback was only
     -    adding its values to a strvec and sanity-checking that `unset` is always
     -    false; both of these are already implemented in the parse-option API.
     -
     -    Signed-off-by: John Passaro <john.a.passaro@gmail.com>
          Helped-by: Patrick Steinhardt <ps@pks.im>
          Helped-by: Junio C Hamano <gitster@pobox.com>
     +    Signed-off-by: John Passaro <john.a.passaro@gmail.com>
      
       ## builtin/commit.c ##
      @@
     @@ builtin/commit.c
       
       static const char * const builtin_commit_usage[] = {
       	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
     -@@ builtin/commit.c: static struct strbuf message = STRBUF_INIT;
     - 
     - static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
     - 
     --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 opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
     - {
     - 	enum wt_status_format *value = (enum wt_status_format *)opt->value;
      @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
       	fclose(s->fp);
       
     @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
       			die(_("unable to pass trailers to --trailers"));
       		strvec_clear(&trailer_args);
       	}
     -@@ builtin/commit.c: int cmd_commit(int argc, const char **argv, const char *prefix)
     - 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
     - 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
     - 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
     --		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
     -+		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
     - 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
     - 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
     - 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
      
       ## trailer.c ##
     -@@
     - #include "commit.h"
     - #include "trailer.h"
     - #include "list.h"
     -+#include "run-command.h"
     - /*
     -  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
     -  */
      @@ trailer.c: void trailer_iterator_release(struct trailer_iterator *iter)
       	strbuf_release(&iter->val);
       	strbuf_release(&iter->key);
       }
      +
     -+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
     ++int amend_file_with_trailers(const char *path, const struct strvec *trailer_args) {
      +	struct child_process run_trailer = CHILD_PROCESS_INIT;
      +
      +	run_trailer.git_cmd = 1;
     @@ trailer.c: void trailer_iterator_release(struct trailer_iterator *iter)
      
       ## trailer.h ##
      @@
     - 
       #include "list.h"
       #include "strbuf.h"
     -+#include "strvec.h"
       
     ++struct strvec;
     ++
       enum trailer_where {
       	WHERE_DEFAULT,
     + 	WHERE_END,
      @@ trailer.h: int trailer_iterator_advance(struct trailer_iterator *iter);
        */
       void trailer_iterator_release(struct trailer_iterator *iter);
     @@ trailer.h: int trailer_iterator_advance(struct trailer_iterator *iter);
      + * This calls run_command() and its return value is the same (i.e. 0 for
      + * success, various non-zero for other errors). See run-command.h.
      + */
     -+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);
     ++int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
      +
       #endif /* TRAILER_H */
 2:  5b6239167b8 ! 3:  f1d68337eda builtin/tag.c: add --trailer arg
     @@ Metadata
      Author: John Passaro <john.a.passaro@gmail.com>
      
       ## Commit message ##
     -    builtin/tag.c: add --trailer arg
     +    builtin/tag.c: add --trailer option
      
          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.
     +    message, using --list --format="%(trailers)". However, to add a trailer
     +    to a tag message, you must do so using `-F` or by editing the 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.
     +    In a previous patch, we moved git-commit's implementation of its
     +    --trailer option to the trailer.h API. Let's use that new function to
     +    teach git-tag the same --trailer option, 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>
     +    Signed-off-by: John Passaro <john.a.passaro@gmail.com>
      
       ## Documentation/git-tag.txt ##
      @@ Documentation/git-tag.txt: SYNOPSIS
 3:  d5335e30b0b < -:  ----------- po: update git-tag translations

-- 
gitgitgadget

[PATCH v4 1/3] builtin/commit.c: remove bespoke option callback

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

Replace git-commit's bespoke callback for --trailer with the standard
OPT_PASSTHRU_ARGV macro. The bespoke callback was only adding its values
to a strvec and sanity-checking that `unset` is always false; both of
these are already implemented in the parse-option API.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 builtin/commit.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6e1484446b0..5a3248370db 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,6 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
-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 opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
 	enum wt_status_format *value = (enum wt_status_format *)opt->value;
@@ -1673,7 +1665,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
-		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
+		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
-- 
gitgitgadget

[PATCH v4 2/3] builtin/commit.c: refactor --trailer logic

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

git-commit adds user trailers to the commit message by passing its
`--trailer` arguments to a child process running `git-interpret-trailers
--in-place`. This logic is broadly useful, not just for git-commit but
for other commands constructing message bodies (e.g. git-tag).

Let's move this logic from git-commit to a new function in the trailer
API, so that it can be re-used in other commands.

Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 builtin/commit.c | 10 ++--------
 trailer.c        | 11 +++++++++++
 trailer.h        |  9 +++++++++
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5a3248370db..63cd090b6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -38,6 +38,7 @@
 #include "commit-reach.h"
 #include "commit-graph.h"
 #include "pretty.h"
+#include "trailer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
@@ -1030,14 +1031,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	fclose(s->fp);
 
 	if (trailer_args.nr) {
-		struct child_process run_trailer = CHILD_PROCESS_INIT;
-
-		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", "--no-divider",
-			     git_path_commit_editmsg(), NULL);
-		strvec_pushv(&run_trailer.args, trailer_args.v);
-		run_trailer.git_cmd = 1;
-		if (run_command(&run_trailer))
+		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
 			die(_("unable to pass trailers to --trailers"));
 		strvec_clear(&trailer_args);
 	}
diff --git a/trailer.c b/trailer.c
index c72ae687099..ae0597d919e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1170,3 +1170,14 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->val);
 	strbuf_release(&iter->key);
 }
+
+int amend_file_with_trailers(const char *path, const struct strvec *trailer_args) {
+	struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+	run_trailer.git_cmd = 1;
+	strvec_pushl(&run_trailer.args, "interpret-trailers",
+		     "--in-place", "--no-divider",
+		     path, NULL);
+	strvec_pushv(&run_trailer.args, trailer_args->v);
+	return run_command(&run_trailer);
+}
diff --git a/trailer.h b/trailer.h
index 9f42aa75994..c364405267a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,6 +4,8 @@
 #include "list.h"
 #include "strbuf.h"
 
+struct strvec;
+
 enum trailer_where {
 	WHERE_DEFAULT,
 	WHERE_END,
@@ -158,4 +160,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
  */
 void trailer_iterator_release(struct trailer_iterator *iter);
 
+/*
+ * Augment a file to add trailers to it by running git-interpret-trailers.
+ * This calls run_command() and its return value is the same (i.e. 0 for
+ * success, various non-zero for other errors). See run-command.h.
+ */
+int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
+
 #endif /* TRAILER_H */
-- 
gitgitgadget

[PATCH v4 3/3] builtin/tag.c: add --trailer option

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

git-tag currently supports interpreting trailers from an annotated tag
message, using --list --format="%(trailers)". However, to add a trailer
to a tag message, you must do so using `-F` or by editing the message.

In a previous patch, we moved git-commit's implementation of its
--trailer option to the trailer.h API. Let's use that new function to
teach git-tag the same --trailer option, emulating as much of
git-commit's behavior as much as possible.

Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 Documentation/git-tag.txt |  18 +++++-
 builtin/tag.c             |  41 +++++++++++---
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+), 11 deletions(-)

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 &&
-- 
gitgitgadget

Re: [PATCH v3 1/3] builtin/commit.c: refactor --trailer logic

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
>
> I would have called this `amend_trailers_to_file()`, which feels a bit
> easier to understand. But I don't mind this much, your version should be
> okay, too.

I had the same reaction, but we are editing trailers in the file
(without touching other things in the same file), so I would have
suggested "in" instead of "to" here.

I agree with everything else you said you your review, and I do not
think I have anything to add to this step.

> Arguably you don't have to include "strvec.h" here, but can instead add
> a simple forward declaration `struct strvec`.

Sensible.

Thanks.

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

[Junio C Hamano]

"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?

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

[John Passaro]

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).

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

[Junio C Hamano]

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?

Re: [PATCH v4 1/3] builtin/commit.c: remove bespoke option callback

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On Tue, Apr 30, 2024 at 02:41:49PM +0000, John Passaro via GitGitGadget wrote:
> From: John Passaro <john.a.passaro@gmail.com>
> 
> Replace git-commit's bespoke callback for --trailer with the standard
> OPT_PASSTHRU_ARGV macro. The bespoke callback was only adding its values
> to a strvec and sanity-checking that `unset` is always false; both of
> these are already implemented in the parse-option API.

Nit: I feel like saying "bespoke" is a bit vague. I would have said
something like the following:

    builtin/commit: use ARGV option to collect custom trailers

In any case, I don't think this is a huge deal and not worth a reroll on
its own.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/3] builtin/commit.c: refactor --trailer logic

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On Tue, Apr 30, 2024 at 02:41:50PM +0000, John Passaro via GitGitGadget wrote:
> From: John Passaro <john.a.passaro@gmail.com>
[snip]
> diff --git a/trailer.c b/trailer.c
> index c72ae687099..ae0597d919e 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1170,3 +1170,14 @@ void trailer_iterator_release(struct trailer_iterator *iter)
>  	strbuf_release(&iter->val);
>  	strbuf_release(&iter->key);
>  }
> +
> +int amend_file_with_trailers(const char *path, const struct strvec *trailer_args) {

Nit: the opening brace should go on the next line.

Other than that this patch looks good to me.

Patrick

> +	struct child_process run_trailer = CHILD_PROCESS_INIT;
> +
> +	run_trailer.git_cmd = 1;
> +	strvec_pushl(&run_trailer.args, "interpret-trailers",
> +		     "--in-place", "--no-divider",
> +		     path, NULL);
> +	strvec_pushv(&run_trailer.args, trailer_args->v);
> +	return run_command(&run_trailer);
> +}
> diff --git a/trailer.h b/trailer.h
> index 9f42aa75994..c364405267a 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -4,6 +4,8 @@
>  #include "list.h"
>  #include "strbuf.h"
>  
> +struct strvec;
> +
>  enum trailer_where {
>  	WHERE_DEFAULT,
>  	WHERE_END,
> @@ -158,4 +160,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
>   */
>  void trailer_iterator_release(struct trailer_iterator *iter);
>  
> +/*
> + * Augment a file to add trailers to it by running git-interpret-trailers.
> + * This calls run_command() and its return value is the same (i.e. 0 for
> + * success, various non-zero for other errors). See run-command.h.
> + */
> +int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
> +
>  #endif /* TRAILER_H */
> -- 
> gitgitgadget
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/3] builtin/tag.c: add --trailer option

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On Tue, Apr 30, 2024 at 02:41:48PM +0000, John Passaro via GitGitGadget wrote:
> 4th follow-up patch taking welcome feedback from Patrick and JCH. Net new
> changes include separating from a 2-patch series to 3.
> 
> Since git-tag --list --format="%(trailers)" can interpret trailers from
> annotated tag messages, it seems natural to support --trailer when writing a
> new tag message.
> 
> git-commit accomplishes this by taking --trailer arguments and passing them
> to git-interpret-trailer. This patch series refactors that logic and uses it
> to implement --trailer on git-tag.

This version looks mostly good. There are two tiny nits, but other than
that I don't have anything else to add.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 0/3] builtin/tag.c: add --trailer option

[John Passaro via GitGitGadget]

5th follow-up patch taking welcome feedback from Patrick and JCH. Net new
changes include suggested tweaks in documentation, error messaging, code
formatting, and patch description.

Since git-tag --list --format="%(trailers)" can interpret trailers from
annotated tag messages, it seems natural to support --trailer when writing a
new tag message.

git-commit accomplishes this by taking --trailer arguments and passing them
to git-interpret-trailer. This patch series refactors that logic and uses it
to implement --trailer on git-tag.

John Passaro (3):
  builtin/commit: use ARGV macro to collect trailers
  builtin/commit: refactor --trailer logic
  builtin/tag: add --trailer option

 Documentation/git-tag.txt |  16 +++++-
 builtin/commit.c          |  20 +------
 builtin/tag.c             |  38 ++++++++++---
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 trailer.c                 |  12 ++++
 trailer.h                 |   9 +++
 6 files changed, 181 insertions(+), 28 deletions(-)


base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1723%2Fjpassaro%2Fjp%2Ftag-trailer-arg-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1723/jpassaro/jp/tag-trailer-arg-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1723

Range-diff vs v4:

 1:  ce047c58aa8 ! 1:  85f45a57f35 builtin/commit.c: remove bespoke option callback
     @@ Metadata
      Author: John Passaro <john.a.passaro@gmail.com>
      
       ## Commit message ##
     -    builtin/commit.c: remove bespoke option callback
     +    builtin/commit: use ARGV macro to collect trailers
      
     -    Replace git-commit's bespoke callback for --trailer with the standard
     -    OPT_PASSTHRU_ARGV macro. The bespoke callback was only adding its values
     -    to a strvec and sanity-checking that `unset` is always false; both of
     -    these are already implemented in the parse-option API.
     +    Replace git-commit's callback for --trailer with the standard
     +    OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
     +    and sanity-checking that `unset` is always false; both of these are
     +    already implemented in the parse-option API.
      
          Signed-off-by: John Passaro <john.a.passaro@gmail.com>
      
 2:  8f53a54bbfe ! 2:  75bb5cc0e8a builtin/commit.c: refactor --trailer logic
     @@ Metadata
      Author: John Passaro <john.a.passaro@gmail.com>
      
       ## Commit message ##
     -    builtin/commit.c: refactor --trailer logic
     +    builtin/commit: refactor --trailer logic
      
          git-commit adds user trailers to the commit message by passing its
          `--trailer` arguments to a child process running `git-interpret-trailers
     @@ trailer.c: void trailer_iterator_release(struct trailer_iterator *iter)
       	strbuf_release(&iter->key);
       }
      +
     -+int amend_file_with_trailers(const char *path, const struct strvec *trailer_args) {
     ++int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
     ++{
      +	struct child_process run_trailer = CHILD_PROCESS_INIT;
      +
      +	run_trailer.git_cmd = 1;
 3:  f1d68337eda ! 3:  d8b3f6229bc builtin/tag.c: add --trailer option
     @@ Metadata
      Author: John Passaro <john.a.passaro@gmail.com>
      
       ## Commit message ##
     -    builtin/tag.c: add --trailer option
     +    builtin/tag: add --trailer option
      
     -    git-tag currently supports interpreting trailers from an annotated tag
     -    message, using --list --format="%(trailers)". However, to add a trailer
     -    to a tag message, you must do so using `-F` or by editing the message.
     +    git-tag supports interpreting trailers from an annotated tag message,
     +    using --list --format="%(trailers)". However, the available methods to
     +    add a trailer to a tag message (namely -F or --editor) are not as
     +    ergonomic.
      
          In a previous patch, we moved git-commit's implementation of its
          --trailer option to the trailer.h API. Let's use that new function to
     @@ Documentation/git-tag.txt: This option is only applicable when listing tags with
       
      +--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.)
     ++	trailer. (e.g. `git tag --trailer "Custom-Key: value"`
     ++	will add a "Custom-Key" 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
     ++	The trailers can be extracted in `git tag --list`, using
      +	`--format="%(trailers)"` placeholder.
      +
       -e::
     @@ builtin/tag.c: static void create_tag(const struct object_id *object, const char
      +			}
      +		} 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);
     -+			}
     ++			if (strbuf_read_file(buf, path, 0) < 0)
     ++				die_errno(_("failed to read '%s'"), path);
       		}
       	}
       

-- 
gitgitgadget

[PATCH v5 1/3] builtin/commit: use ARGV macro to collect trailers

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

Replace git-commit's callback for --trailer with the standard
OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
and sanity-checking that `unset` is always false; both of these are
already implemented in the parse-option API.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 builtin/commit.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6e1484446b0..5a3248370db 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,6 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
-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 opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
 	enum wt_status_format *value = (enum wt_status_format *)opt->value;
@@ -1673,7 +1665,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
-		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
+		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
-- 
gitgitgadget

[PATCH v5 2/3] builtin/commit: refactor --trailer logic

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

git-commit adds user trailers to the commit message by passing its
`--trailer` arguments to a child process running `git-interpret-trailers
--in-place`. This logic is broadly useful, not just for git-commit but
for other commands constructing message bodies (e.g. git-tag).

Let's move this logic from git-commit to a new function in the trailer
API, so that it can be re-used in other commands.

Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 builtin/commit.c | 10 ++--------
 trailer.c        | 12 ++++++++++++
 trailer.h        |  9 +++++++++
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5a3248370db..63cd090b6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -38,6 +38,7 @@
 #include "commit-reach.h"
 #include "commit-graph.h"
 #include "pretty.h"
+#include "trailer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
@@ -1030,14 +1031,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	fclose(s->fp);
 
 	if (trailer_args.nr) {
-		struct child_process run_trailer = CHILD_PROCESS_INIT;
-
-		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", "--no-divider",
-			     git_path_commit_editmsg(), NULL);
-		strvec_pushv(&run_trailer.args, trailer_args.v);
-		run_trailer.git_cmd = 1;
-		if (run_command(&run_trailer))
+		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
 			die(_("unable to pass trailers to --trailers"));
 		strvec_clear(&trailer_args);
 	}
diff --git a/trailer.c b/trailer.c
index c72ae687099..724cb78be5f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1170,3 +1170,15 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->val);
 	strbuf_release(&iter->key);
 }
+
+int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
+{
+	struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+	run_trailer.git_cmd = 1;
+	strvec_pushl(&run_trailer.args, "interpret-trailers",
+		     "--in-place", "--no-divider",
+		     path, NULL);
+	strvec_pushv(&run_trailer.args, trailer_args->v);
+	return run_command(&run_trailer);
+}
diff --git a/trailer.h b/trailer.h
index 9f42aa75994..c364405267a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,6 +4,8 @@
 #include "list.h"
 #include "strbuf.h"
 
+struct strvec;
+
 enum trailer_where {
 	WHERE_DEFAULT,
 	WHERE_END,
@@ -158,4 +160,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
  */
 void trailer_iterator_release(struct trailer_iterator *iter);
 
+/*
+ * Augment a file to add trailers to it by running git-interpret-trailers.
+ * This calls run_command() and its return value is the same (i.e. 0 for
+ * success, various non-zero for other errors). See run-command.h.
+ */
+int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
+
 #endif /* TRAILER_H */
-- 
gitgitgadget

[PATCH v5 3/3] builtin/tag: add --trailer option

[John Passaro via GitGitGadget]

From: John Passaro <john.a.passaro@gmail.com>

git-tag supports interpreting trailers from an annotated tag message,
using --list --format="%(trailers)". However, the available methods to
add a trailer to a tag message (namely -F or --editor) are not as
ergonomic.

In a previous patch, we moved git-commit's implementation of its
--trailer option to the trailer.h API. Let's use that new function to
teach git-tag the same --trailer option, emulating as much of
git-commit's behavior as much as possible.

Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 Documentation/git-tag.txt |  16 +++++-
 builtin/tag.c             |  38 ++++++++++---
 t/t7004-tag.sh            | 114 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5fe519c31ec..4494729f5e1 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,17 @@ 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 "Custom-Key: value"`
+	will add a "Custom-Key" 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 extracted 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..01b19a5b6c9 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,19 @@ 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)
+				die_errno(_("failed to read '%s'"), path);
 		}
 	}
 
@@ -463,6 +478,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 +495,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 +566,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 +673,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 +705,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 &&
-- 
gitgitgadget

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

[John Passaro]

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.

Re: [PATCH v5 0/3] builtin/tag.c: add --trailer option

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

On Sun, May 05, 2024 at 06:49:07PM +0000, John Passaro via GitGitGadget wrote:
> 5th follow-up patch taking welcome feedback from Patrick and JCH. Net new
> changes include suggested tweaks in documentation, error messaging, code
> formatting, and patch description.
> 
> Since git-tag --list --format="%(trailers)" can interpret trailers from
> annotated tag messages, it seems natural to support --trailer when writing a
> new tag message.
> 
> git-commit accomplishes this by taking --trailer arguments and passing them
> to git-interpret-trailer. This patch series refactors that logic and uses it
> to implement --trailer on git-tag.

This version looks good to me, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/3] builtin/tag.c: add --trailer option

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Sun, May 05, 2024 at 06:49:07PM +0000, John Passaro via GitGitGadget wrote:
>> 5th follow-up patch taking welcome feedback from Patrick and JCH. Net new
>> changes include suggested tweaks in documentation, error messaging, code
>> formatting, and patch description.
>> 
>> Since git-tag --list --format="%(trailers)" can interpret trailers from
>> annotated tag messages, it seems natural to support --trailer when writing a
>> new tag message.
>> 
>> git-commit accomplishes this by taking --trailer arguments and passing them
>> to git-interpret-trailer. This patch series refactors that logic and uses it
>> to implement --trailer on git-tag.
>
> This version looks good to me, thanks!

Thanks.  Will queue.

Re: [PATCH v5 1/3] builtin/commit: use ARGV macro to collect trailers

[John Passaro]

On Sun, May 5, 2024 at 2:49 PM John Passaro via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>

Thank you for queuing the patch series! Hopeful and excited that this
may go in a future release.

> From: John Passaro <john.a.passaro@gmail.com>
>
> Replace git-commit's callback for --trailer with the standard
> OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
> and sanity-checking that `unset` is always false; both of these are
> already implemented in the parse-option API.
>
> Signed-off-by: John Passaro <john.a.passaro@gmail.com>

Looking over the patch series I notice that I left a grammar error in
this description,
if the patch makes it to the "next" branch I hope you can amend the
message first,
as follows:

Replace git-commit's callback for --trailer with the standard
OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
and sanity-checks that `unset` is always false; both of these are
already implemented in the parse-option API.

> ---
>  builtin/commit.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6e1484446b0..5a3248370db 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -142,14 +142,6 @@ static struct strbuf message = STRBUF_INIT;
>
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
>
> -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 opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
>  {
>         enum wt_status_format *value = (enum wt_status_format *)opt->value;
> @@ -1673,7 +1665,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>                 OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
>                 OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>                 OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
> -               OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> +               OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
>                 OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
>                 OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
>                 OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
> --
> gitgitgadget
>

Re: [PATCH v5 1/3] builtin/commit: use ARGV macro to collect trailers

[Junio C Hamano]

John Passaro <john.a.passaro@gmail.com> writes:

>> From: John Passaro <john.a.passaro@gmail.com>
>>
>> Replace git-commit's callback for --trailer with the standard
>> OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
>> and sanity-checking that `unset` is always false; both of these are
>> already implemented in the parse-option API.
>>
>> Signed-off-by: John Passaro <john.a.passaro@gmail.com>
>
> Looking over the patch series I notice that I left a grammar error in
> this description,
> if the patch makes it to the "next" branch I hope you can amend the
> message first,
> as follows:
>
> Replace git-commit's callback for --trailer with the standard
> OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
> and sanity-checks that `unset` is always false; both of these are
> already implemented in the parse-option API.

Thanks for stopping me before the topic got merged to 'next'.
Amended.