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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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.