Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel

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

On Mon, Sep 26 2022, Calvin Wan wrote:

>> On the implementation:
>>
>> > + * If the "pipe_output" option is specified, the output will be piped
>> > + * to task_finished_fn in the "struct strbuf *out" variable. The output
>> > + * will still be printed unless the callback resets the strbuf. The
>> > + * "pipe_output" option can be enabled by setting the global
>> > + * "run_processes_parallel_pipe_output" to "1" before invoking
>> > + * run_processes_parallel(), it will be set back to "0" as soon as the
>> > + * API reads that setting.
>>
>> ...okey, but...
>>
>> > +static int task_finished_pipe_output(int result,
>> > +                      struct strbuf *err,
>> > +                      void *pp_cb,
>> > +                      void *pp_task_cb)
>> > +{
>> > +     if (err && pipe_output) {
>> > +             fprintf(stderr, "%s", err->buf);
>> > +             strbuf_reset(err);
>>
>> ...my memory's hazy, and I haven't re-logged in any detail, but is it
>> really the API interface here that the "output" callback function is
>> responsible for resetting the strbuf that the API gives to it?
>>
>> That seems backwards to me, and e.g. a look at "start_failure" shows
>> that we strbuf_reset() the "err".
>>
>> What's the point of doing it in the API consumer? If it doesn't do it
>> we'll presumably keep accumulating output. Is there a use-case for that?
>>
>> Or perhaps it's not needed & this is really just misleading boilerplate?
>
> Ultimately it is not needed -- I added it as an example to showcase that
> the output is correctly being piped to "task_finished_pipe_output". The
> reset is necessary in this case to prevent the output from being printed
> twice. I'm not sure how exactly else I would go about testing "pipe_output".

If that's the intent then having that reset there seems to me to be
doing the exact opposite of what you want.

If the API is broken and passing the output along twice without clearing
it in-between the two calls your strbuf_reset() would be sweeping that
issue under the rug, that API brokenness would be "repaired" by your
test.

Whereas if you remove the strbuf_reset() it should behave as it does
now, and if it doesn't the API itself is broken, i.e. after calling the
callback it should be resetting the buffer.