From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 02 Mar 2026 14:56:33 GMT
Subject: Re: [PATCH v7 4/5] commit, tag: parse --trailer with OPT_STRVEC
Message-ID: <dffc34a6-3a89-4535-9caf-95625a221797@gmail.com>
In-Reply-To: <20260224070552.148591-5-me@linux.beauty>

Hi Li

On 24/02/2026 07:05, Li Chen wrote:
> Now that amend_file_with_trailers() expects raw trailer lines, do not
> store argv-style "--trailer=<trailer>" strings in git commit and git
> tag.
> 
> Parse --trailer using OPT_STRVEC so trailer_args contains only the
> trailer value, and drop the temporary prefix stripping in
> amend_file_with_trailers().

This looks good. I'll stop here for today and look at the last patch 
tomorrow

Thanks

Phillip

> Signed-off-by: Li Chen <me@linux.beauty>
> ---
> v7:
> New patch.
> 
>   builtin/commit.c |  3 ++-
>   builtin/tag.c    |  4 ++--
>   trailer.c        | 21 +--------------------
>   trailer.h        |  4 ++--
>   4 files changed, 7 insertions(+), 25 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9e3a09d532..d9983230de 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1720,7 +1720,8 @@ int cmd_commit(int argc,
>   		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_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
> +		OPT_STRVEC(0, "trailer", &trailer_args, N_("trailer"),
> +			   N_("add custom trailer(s)")),
>   		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/builtin/tag.c b/builtin/tag.c
> index aeb04c487f..15aee1b03a 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -499,8 +499,8 @@ int cmd_tag(int argc,
>   		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_STRVEC(0, "trailer", &trailer_args, N_("trailer"),
> +			   N_("add custom trailer(s)")),
>   		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),
> diff --git a/trailer.c b/trailer.c
> index 8e87d185d9..e85c6c9fbe 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1342,40 +1342,21 @@ int amend_file_with_trailers(const char *path,
>   			     const struct strvec *trailer_args)
>   {
>   	struct strbuf buf = STRBUF_INIT;
> -	struct strvec stripped_trailer_args = STRVEC_INIT;
>   	int ret = 0;
> -	size_t i;
>   
>   	if (!trailer_args)
>   		BUG("amend_file_with_trailers called with NULL trailer_args");
>   	if (!trailer_args->nr)
>   		return 0;
>   
> -	for (i = 0; i < trailer_args->nr; i++) {
> -		const char *txt = trailer_args->v[i];
> -
> -		/*
> -		 * Historically amend_file_with_trailers() passed its arguments
> -		 * to "git interpret-trailers", which expected argv entries in
> -		 * "--trailer=<trailer>" form. Continue to accept those for
> -		 * existing callers, but pass only the value portion to the
> -		 * in-process implementation.
> -		 */
> -		skip_prefix(txt, "--trailer=", &txt);
> -		if (!*txt)
> -			die(_("empty --trailer argument"));
> -		strvec_push(&stripped_trailer_args, txt);
> -	}
> -
>   	if (strbuf_read_file(&buf, path, 0) < 0)
>   		ret = error_errno(_("could not read '%s'"), path);
>   	else
> -		amend_strbuf_with_trailers(&buf, &stripped_trailer_args);
> +		amend_strbuf_with_trailers(&buf, trailer_args);
>   
>   	if (!ret)
>   		ret = write_file_in_place(path, &buf);
>   
> -	strvec_clear(&stripped_trailer_args);
>   	strbuf_release(&buf);
>   	return ret;
>   }
> diff --git a/trailer.h b/trailer.h
> index d05dab050b..e5bd355aad 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -209,8 +209,8 @@ void amend_strbuf_with_trailers(struct strbuf *buf,
>   /*
>    * Augment a file by appending trailers specified in trailer_args.
>    *
> - * Each element of trailer_args should be an argv-style --trailer=<trailer>
> - * option (i.e., including the --trailer= prefix).
> + * Each element of trailer_args should be in the same format as the value
> + * accepted by --trailer=<trailer> (i.e., without the --trailer= prefix).
>    *
>    * Returns 0 on success or a non-zero error code on failure.
>    */