From: phillip.wood123@gmail.com
Date: Wed, 11 Jun 2025 09:15:26 GMT
Subject: Re: [PATCH v2 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`
Message-ID: <407631ab-1184-41b1-8043-fd91d860e845@gmail.com>
In-Reply-To: <xmqq8qlzkukw.fsf@gitster.g>

On 11/06/2025 01:09, Junio C Hamano wrote:
> Li Chen <me@linux.beauty> writes:
> 
>> * `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?

Exactly - I was expecting to see a refactoring of interpret_trailers() 
in builtin/interpret-trailers.c that moved most of the function body 
into a new function in trailer.c that added the trailers to an strbuf. 
This seems to be a parallel implementation which doesn't sound like the 
best plan.

I'm going to be off the list for a couple of weeks, I'll take a more 
detailed look at this series when I'm back

Best Wishes

Phillip

> 
>> * `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 ...
> 
>> +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?