Re: [PATCH 01/10] run-command: add stdin callback for parallelization
- From
Adrian Ratiu <adrian.ratiu@collabora.com>
- Date
- Oct 14, 2025, 17:35 UTC
- Message-ID
- <87jz0xfktp.fsf@collabora.com>
- In-Reply-To
- <87v7ks424z.fsf@collabora.com>
Hi again Patrick and Junio,
On Mon, 06 Oct 2025, Adrian Ratiu <adrian.ratiu@collabora.com> wrote:
Show 22 quoted lines
> Hi Patrick and thanks for review! I'll fix in v2 all the issues
> you pointed out.
>
> On Thu, 02 Oct 2025, Patrick Steinhardt <ps@pks.im> wrote:
>>> + * child input is provided via path_to_stdin when
>>> the feed_pipe cb is + * missing, so we just
>>> signal an EOF. + */ + if
>>> (!opts->feed_pipe) { + close(proc->in); +
>>> proc->in = 0;
>> Hm. It's curious that we use a valid file descriptor
>> here. Shouldn't we rather use `-1`? Otherwise I could see that
>> we might try to close this seemingly valid file descriptor at a
>> later point in time.
>
> I actually asked myself this while preparing the patches, since
> -1 is a better fit.
>
> I only left = 0 for historical reasons, to not modify these
> patches too much. :) However I do 100% agree with both you and
> Junio that -1 should be used here.
>
> Will do in v2. This is much harder and riskier than I originally anticipated, so I gave up trying to implement it after a few failed attempts.
In a nutshell, we have to change the < 0, 0 and > 0 semantics defined in run-command.h for .in, .out, and .err fds across the entire source tree.
It's a massive, error-prone and out-of-scope amount of work.
Can we please just keep the current run-command API which uses 0 for "no fd passed"? I'd very much like to avoid changing this run-command API.