Re: [PATCH v5 01/29] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Li

On 22/10/2025 06:39, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> Route all trailer insertion through trailer_process() and make
> builtin/interpret-trailers just do file I/O before calling into it.
> amend_file_with_trailers() now shares the same code path.
> 
> This removes the fork/exec and tempfile juggling, cutting overhead and
> simplifying error handling. No functional change. It also
> centralizes logic to prepare for follow-up rebase --trailer patch.
> 
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>

When I review v3 of this series I said

>> As I said above reusing the existing code as you have done here is a
>> much better approach. However it would be much easier to review if
>> the code movement was separated from the refactoring. I'm also
>> struggling to see the benefit of a lot of the refactoring - I was
>> expecting the conversion to use an strubf would essentially look like
>> fwrite() being replaced with strbuf_add() and fprintf() being
>> replaced with strbuf_addf() etc. rather than reworking the logic.

Unfortunately this version has the same code changes as v3 that make it
are virtually impossible to verify if the behavior is changed or not.
The diff below shows what I was hoping to see as the first step. It
refactors interpret_trailers() in place to factor out the code that
processes the trailers into a separate function. That makes it easy to
see that fwrite() is replaced with strbuf_add() etc. and so verify that
the behavior is unchanged. If you view the diff with "--color-moved"
you'll see that virtually all of the code in the new
interpret_trailers() function is moved from the old one which in turn
makes it easy to verify that there is no change in behavior. I would
expect the next patch in the series to move the new function for
processing trailers into trailer.c and the patch after that to refactor
amend_file_with_trailers() to add and use amend_strbuf_with_trailers()
and stop forking "git interpret-trailers". Then builtin/rebase.c can be
modified to add support for trailers using amend_strbuf_with_trailers().

Thanks

Phillip

---- 8< ----
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 41b0750e5af..4c90580ffff 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)
  	strbuf_complete_line(sb);
  }
  
-static void interpret_trailers(const struct process_trailer_options *opts,
-			       struct list_head *new_trailer_head,
-			       const char *file)
+static void process_trailers(const struct process_trailer_options *opts,
+			     struct list_head *new_trailer_head,
+			     struct strbuf *sb, struct strbuf *out)
  {
  	LIST_HEAD(head);
-	struct strbuf sb = STRBUF_INIT;
-	struct strbuf trailer_block_sb = STRBUF_INIT;
  	struct trailer_block *trailer_block;
-	FILE *outfile = stdout;
-
-	trailer_config_init();
-
-	read_input_file(&sb, file);
-
-	if (opts->in_place)
-		outfile = create_in_place_tempfile(file);
-
-	trailer_block = parse_trailers(opts, sb.buf, &head);
+
+	trailer_block = parse_trailers(opts, sb->buf, &head);
  
  	/* Print the lines before the trailer block */
  	if (!opts->only_trailers)
-		fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
+		strbuf_add(out, sb->buf, trailer_block_start(trailer_block));
  
  	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
-		fprintf(outfile, "\n");
-
+		strbuf_addch(out, '\n');
  
  	if (!opts->only_input) {
  		LIST_HEAD(config_head);
@@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,
  	}
  
  	/* Print trailer block. */
-	format_trailers(opts, &head, &trailer_block_sb);
+	format_trailers(opts, &head, out);
  	free_trailers(&head);
-	fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
-	strbuf_release(&trailer_block_sb);
  
  	/* Print the lines after the trailer block as is. */
  	if (!opts->only_trailers)
-		fwrite(sb.buf + trailer_block_end(trailer_block), 1,
-		       sb.len - trailer_block_end(trailer_block), outfile);
+		strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
+			   sb->len - trailer_block_end(trailer_block));
  	trailer_block_release(trailer_block);
-
+}
+
+static void interpret_trailers(const struct process_trailer_options *opts,
+			       struct list_head *new_trailer_head,
+			       const char *file)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
+	FILE *outfile = stdout;
+
+	trailer_config_init();
+
+	read_input_file(&sb, file);
+
+	if (opts->in_place)
+		outfile = create_in_place_tempfile(file);
+
+	process_trailers(opts, new_trailer_head, &sb, &out);
+
+	fwrite(out.buf, out.len, 1, outfile);
  	if (opts->in_place)
  		if (rename_tempfile(&trailers_tempfile, file))
  			die_errno(_("could not rename temporary file to %s"), file);
  
  	strbuf_release(&sb);
+	strbuf_release(&out);
  }
  
  int cmd_interpret_trailers(int argc,