Re: [PATCH v2 14/16] repack: implement incremental MIDX repacking

[Taylor Blau <me@ttaylorr.com>]

On Wed, Apr 29, 2026 at 03:51:50AM -0400, Jeff King wrote:
> > @@ -312,6 +319,17 @@ static int repack_fill_midx_stdin_packs(struct child_process *cmd,
> >  		fprintf(in, "%s\n", item->string);
> >  	fclose(in);
> >
> > +	if (out) {
> > +		struct strbuf buf = STRBUF_INIT;
> > +		FILE *outf = xfdopen(cmd->out, "r");
> > +
> > +		while (strbuf_getline(&buf, outf) != EOF)
> > +			string_list_append(out, buf.buf);
> > +		strbuf_release(&buf);
> > +
> > +		fclose(outf);
> > +	}
>
> Is it possible to deadlock here where we block writing to the child, but
> the child is blocked trying to write back to us. It's probably quite
> unlikely as it implies both pipe buffers are filled up (and we are
> counting packs and midx hashes here, neither of which we'd expect to be
> too numerous).
>
> Using pipe_command() would solve this, but it also might be impossible
> to trigger if the child reads all input before generating any output. I
> _think_ that's the case looking at cmd_multi_pack_index_write(). So
> we're OK, but you might want to double check.

I think in theory this could deadlock, though in practice it is highly
unlikely. Regardless, using pipe_command() here is straightforward, and
guarantees that we'll avoid a nasty deadlock, so let's do that.

The resulting code is a lot easier to read, too, which is nice:

--- 8< ---
diff --git a/repack-midx.c b/repack-midx.c
index 8f3720772b8..9db59b18334 100644
--- a/repack-midx.c
+++ b/repack-midx.c
@@ -300,37 +300,27 @@ static int repack_fill_midx_stdin_packs(struct child_process *cmd,
 					struct string_list *include,
 					struct string_list *out)
 {
+	struct strbuf in_buf = STRBUF_INIT;
+	struct strbuf out_buf = STRBUF_INIT;
 	struct string_list_item *item;
-	FILE *in;
 	int ret;

-	cmd->in = -1;
-	if (out)
-		cmd->out = -1;
-
 	strvec_push(&cmd->args, "--stdin-packs");

-	ret = start_command(cmd);
-	if (ret)
-		return ret;
-
-	in = xfdopen(cmd->in, "w");
 	for_each_string_list_item(item, include)
-		fprintf(in, "%s\n", item->string);
-	fclose(in);
+		strbuf_addf(&in_buf, "%s\n", item->string);

-	if (out) {
-		struct strbuf buf = STRBUF_INIT;
-		FILE *outf = xfdopen(cmd->out, "r");
+	ret = pipe_command(cmd, in_buf.buf, in_buf.len,
+			   out ? &out_buf : NULL, 0, NULL, 0);

-		while (strbuf_getline(&buf, outf) != EOF)
-			string_list_append(out, buf.buf);
-		strbuf_release(&buf);
+	if (out)
+		string_list_split_f(out, out_buf.buf, "\n", -1,
+				    STRING_LIST_SPLIT_NONEMPTY);

-		fclose(outf);
-	}
+	strbuf_release(&in_buf);
+	strbuf_release(&out_buf);

-	return finish_command(cmd);
+	return ret;
 }

 static int write_midx_included_packs(struct repack_write_midx_opts *opts)
--- >8 ---

> > +static int midx_compaction_step_exec_compact(struct midx_compaction_step *step,
> > +					     struct repack_write_midx_opts *opts)
> > +{
> > [...]
> > +	ret = start_command(&cmd);
> > +	if (ret)
> > +		goto out;
>
> OK, if we get an error here we'll jump to "out", but run-command.c will
> have cleaned up everything about "cmd" that we need.
>
> But later...
>
> > +	out = xfdopen(cmd.out, "r");
> > +	while (strbuf_getline_lf(&buf, out) != EOF) {
> > +		if (step->csum) {
> > +			ret = error(_("unexpected MIDX output: '%s'"), buf.buf);
> > +			goto out;
> > +		}
>
> ...here we jump to "out" but the command is left running. I guess it
> will eventually get SIGPIPE when we fclose(out), but of course we won't
> wait() for it and we're probably leaking some memory, too.

Good catch, I fixed this up in the way you suggested to call
`fclose(out)` and `finish_command()` within this loop before jumping
out.

Thanks,
Taylor