From: Adrian Ratiu Date: Tue, 14 Oct 2025 17:35:14 GMT Subject: Re: [PATCH 01/10] run-command: add stdin callback for parallelization 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 wrote: > Hi Patrick and thanks for review! I'll fix in v2 all the issues > you pointed out. > > On Thu, 02 Oct 2025, Patrick Steinhardt 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.