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

From

PWPhillip Wood <phillip.wood123@gmail.com>

Date

Oct 23, 2025, 13:21 UTC

Message-ID

<7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com>

In-Reply-To

<20251022053951.602605-2-me@linux.beauty>

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.

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,