> 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@xxxxxxxxx/ > > 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?
