Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Nov 29 2022, Glen Choo wrote:

> Calvin Wan <calvinwan@google.com> writes:
>
>> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,
>>  	for (size_t i = 0; i < opts->processes; i++) {
>>  		if (pp->children[i].state == GIT_CP_WORKING &&
>>  		    pp->pfd[i].revents & (POLLIN | POLLHUP)) {
>> -			int n = strbuf_read_once(&pp->children[i].err,
>> -						 pp->children[i].process.err, 0);
>> +			struct strbuf buf = STRBUF_INIT;
>> +			int n = strbuf_read_once(&buf, pp->children[i].process.err, 0);
>> +			strbuf_addbuf(&pp->children[i].err, &buf);
>> +			if (opts->duplicate_output)
>> +				opts->duplicate_output(&buf, &pp->children[i].err,
>> +					  opts->data,
>> +					  pp->children[i].data);
>> +			strbuf_release(&buf);
>>  			if (n == 0) {
>>  				close(pp->children[i].process.err);
>>  				pp->children[i].state = GIT_CP_WAIT_CLEANUP;
>
> A common pattern is that optional behavior does not impose additional
> costs on the non-optional part. Here, we unconditionally do a
> strbuf_addbuf() even though we don't use the result in the "else" case.
>
> So this might be more idiomatically written as:
>
>         int n = strbuf_read_once(&pp->children[i].err,
>         			 pp->children[i].process.err, 0);
>  +      if (opts->duplicate_output) {
>  +          struct strbuf buf = STRBUF_INIT;
>  +          strbuf_addbuf(&buf, &pp->children[i].err);
>  +        	opts->duplicate_output(&buf, &pp->children[i].err,
>  +        		  opts->data,
>  +        		  pp->children[i].data);
>  +          strbuf_release(&buf);
>  +      }
>
> which also has the nice benefit of not touching the strbuf_read_once()
> line.

We should also use "size_t n" there, not "int n", which is what it
returns. It won't matter for overflow in this case, but let's not
truncate for no good reason, it makes spotting actual overflows in
compiler output harder.

And why does "&buf" exist at all? Why can't we just pass
&pp->children[i].err, and if this callback cares about the last thing we
read let's pass it an offset, so it can know what came from the
strbuf_read_once() (I don't know if it actually cares about that
either...).

That would avoid the copy entirely.