From: Junio C Hamano Date: Thu, 02 Oct 2025 15:46:08 GMT Subject: Re: [PATCH 01/10] run-command: add stdin callback for parallelization Message-ID: In-Reply-To: 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. >> + /** >> + * Feed the pipe: >> + * ret < 0 means error >> + * ret == 0 means there is more data to be fed >> + * ret > 0 means feeding finished >> + */ >> + ret = opts->feed_pipe(proc->in, opts->data, pp->children[i].data); >> + if (ret < 0) >> + die_errno("feed_pipe"); >> + >> + if (ret == 1) { > > This condition mismatches the comment: you explicitly check for 1, but > the comment above says `ret > 0` indicates that feeding has finished. True. We already handled negative, so we can just do "if (ret)" here, but "if (0 < ret)" is also fine. Thanks.