Re: [PATCH 01/10] run-command: add stdin callback for parallelization

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2025, 15:46 UTC
Message-ID: <xmqqbjmpxq67.fsf@gitster.g>
In-Reply-To: <aN4c6l7gRi4auss1@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

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

Show 10 quoted lines

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

Show 14 quoted lines

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