From: Adrian Ratiu Date: Mon, 06 Oct 2025 13:01:30 GMT Subject: Re: [PATCH 01/10] run-command: add stdin callback for parallelization Message-ID: <87sefw421h.fsf@collabora.com> In-Reply-To: On Thu, 02 Oct 2025, Junio C Hamano wrote: > Patrick Steinhardt writes: > >>> + /* Buffer stdin for each pipe. */ + for (int i = 0; i >>> < opts->processes; i++) { >> >> `opts->processes` is of type `size_t`, so let's use the same >> type as iterator. > > Good eyes. > >>> + /** >> >> Nit: multi-line comments should start with "/*", not >> "/**". This is also present in multiple other > > True. > > Especially for a comment about a specific piece of code and not > about an interface---even in a future where we use some tool to > extract them, we would not place them in documentation. > >>> + * 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. > > Good eyes. Does -1 also have special meaning or we have no risk > mistaking this proc->in that was once used with a request to > open a pipe? Regardless, I agree 0 would be a bad choice here. > Ack, will fix all these in v2.