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

[Calvin Wan <calvinwan@google.com>]

> An earlier version of that series simply changed the API to pass an
> "opts" struct instead:
> https://lore.kernel.org/git/patch-v2-2.8-5f0a6e9925f-20220518T195858Z-avarab@gmail.com/
>
> I really should have submitted those post-release cleanup patches
> already, and I'm not sure whether the right thing at this point is to
> take this & do the cleanup for "ungroup" *and* this new argument later.
>
> But maybe you're interested in cherry-picking & adjusting the relevant
> part of that series for this one? I.e. we're not in some post-release
> regression hurry, so rather than extending the use of this odd interface
> we could (and maybe should) just fix how we're doing it first.

I'll go ahead and give this a try. I was also a little bit surprised that
"ungroup" was set this way, but didn't realize it was for a quick fix.

>
> 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".

>
> > @@ -140,6 +140,11 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail
> >       test_line_count = 4 err
> >  '
> >
> > +test_expect_success 'run_command runs pipe_output in parallel with more jobs available than tasks' '
> > +     test-tool run-command --pipe-output run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
> > +     test_cmp expect actual
> > +'
> > +
>
> Like the global argument, the copy/pasting for "ungroup" was mostly a
> matter of expediency.
>
> But at least in that case we have a different assertion (test_cmp
> v.s. test_line_count).
>
> But here this test case seems to be exactly the same as for the
> "vanilla" version.
>
> So can't we make this some:
>
>         for opt in '' '--pipe-output'
>         do
>                 test_expect_success ...
>         done
>
> ?

Yes we can -- but I may need to rethink how instead I should be testing
this option?