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

[Jeff King <peff@peff.net>]

On Tue, Apr 21, 2026 at 04:37:54PM -0400, Taylor Blau wrote:

>  static int repack_fill_midx_stdin_packs(struct child_process *cmd,
> -					struct string_list *include)
> +					struct string_list *include,
> +					struct string_list *out)
>  {
>  	struct string_list_item *item;
>  	FILE *in;
>  	int ret;
>  
>  	cmd->in = -1;
> +	if (out)
> +		cmd->out = -1;

We are already piping to the sub-command, and now we will read the
output. Looks like we do them sequentially:

> @@ -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.

> +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.

> +	}
> +
> +	ret = finish_command(&cmd);
> +
> +out:
> +	if (out)
> +		fclose(out);
> +	strbuf_release(&buf);

It would be nice if we had an idempotent version of finish_command(),
that would do nothing when the program was already done. But I don't
think we do. We don't even clear cmd->pid when we finish the command, so
you can't even do something like:

  out:
  if (cmd->pid)
	finish_command(&cmd);

So probably that "goto out" in the loop needs to call fclose() and
finish_command() itself.

-Peff