Re: [PATCH v2 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 11, 2025, 00:09 UTC
Message-ID: <xmqq8qlzkukw.fsf@gitster.g>
In-Reply-To: <20250610123459.278582-2-me@linux.beauty>

Li Chen <me@linux.beauty> writes:

Show 6 quoted lines

> * `amend_strbuf_with_trailers()`
>   – parses the existing message,
>   – merges trailers from the command line and config,
>   – formats the final trailer block, and
>   – rewrites the supplied `struct strbuf`
>   without ever leaving the current process.

If such a helper function exists, shouldn't "interpret-trailers" be able to lose quite a lot of lines, at least nearly as many as the new lines introduced to this new function, by making it call this function as well? And that would ensure that the internal call can safely replace the external call and produce exactly the same output? If so, that can be a pure refactoring patch that can become a commit on its own, I presume?

> * `amend_file_with_trailers()` becomes a thin wrapper that reads a file
>   into a `strbuf`, calls the new helper, and writes the result back.
> * `builtin/rebase.c` now calls `amend_file_with_trailers()` instead of
>   executing `interpret-trailers`.

And then these two changes can become a separate patch on top?

> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)

This lone "removed line" can be avoided if the patch did not touch this line ...

Show 35 quoted lines

> +static size_t first_comment_pos(const struct strbuf *buf)
>  {
> -	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);
> +	const char *p = buf->buf;
> +	const char *end = buf->buf + buf->len;
> +
> +	while (p < end) {
> +		const char *line = p;
> +		const char *nl = memchr(p, '\n', end - p);
> +		size_t len = nl ? (size_t)(nl - p) : (size_t)(end - p);
> +
> +		/* skip leading whitespace */
> +		size_t i = 0;
> +		while (i < len && isspace((unsigned char)line[i]))
> +			i++;
> +
> +		if (i < len && line[i] == '#')
> +			return (size_t)(line - buf->buf); /* comment starts here */
> +
> +		if (!nl)              /* last line without newline */
> +			break;
> +		p = nl + 1;
> +	}
> +	return buf->len;          /* no comment line found */
> +}
> +
> +int amend_strbuf_with_trailers(struct strbuf *buf,
> +							   const struct strvec *trailer_args)

... like this?