On Tue, Oct 11 2022, Calvin Wan wrote:
(re-arranging the diff a bit for discussion)
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 46bac2bb70..d3c3df7960 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -52,6 +52,18 @@ static int no_job(struct child_process *cp,
> return 0;
> }
>
> +static void pipe_output(struct strbuf *out,
> + void *pp_cb,
> + void *pp_task_cb)
> +{
> + fprintf(stderr, "%s", out->buf);
> + /*
> + * Resetting output to show that piped output would print the
> + * same as other tests without the pipe_output() function set
> + */
> + strbuf_reset(out);
> +}
> +
This is still unaddressed from the last round:
https://lore.kernel.org/git/220927.86edvxytla.gmgdl@xxxxxxxxxxxxxxxxxxx/
Or well, it changed a bit, but the "reset" is still there.
Whatever you're aiming for here, this is apparently doing nothing,
because if I comment out the strbuf_reset() then all your tests pass,
which...
> int i = pp->output_owner;
>
> if (pp->children[i].state == GIT_CP_WORKING &&
> pp->children[i].err.len) {
> + if (opts->pipe_output)
> + opts->pipe_output(&pp->children[i].err, pp->data,
> + pp->children[i].data);
> strbuf_write(&pp->children[i].err, stderr);
> strbuf_reset(&pp->children[i].err);
> }
...we can see from here...
> @@ -1716,6 +1720,10 @@ static int pp_collect_finished(struct parallel_processes *pp,
>
> code = finish_command(&pp->children[i].process);
>
> + if (opts->pipe_output)
> + opts->pipe_output(&pp->children[i].err, pp->data,
> + pp->children[i].data);
> +
> if (opts->task_finished)
> code = opts->task_finished(code, opts->ungroup ? NULL :
> &pp->children[i].err, pp->data,
...and in particular here, where the "err" is subsequently passed to
"task_finished" *would* actually have an impact, but (again,
re-arranging he diff a bit) ...
> @@ -439,6 +451,12 @@ int cmd__run_command(int argc, const char **argv)
> opts.ungroup = 1;
> }
>
> + if (!strcmp(argv[1], "--pipe-output")) {
> + argv += 1;
> + argc -= 1;
> + opts.pipe_output = pipe_output;
> + }
> +
> jobs = atoi(argv[2]);
> strvec_clear(&proc.args);
> strvec_pushv(&proc.args, (const char **)argv + 3);
...here to test that we'd need our tests to combine "pipe_output" with
"task_finished".
Which we should do in either case, e.g. if you define both and append to
"err" in what order do we call them, and does "err" accumulate? (I think
it should), in any case...
> + /**
> + * pipe_output: See pipe_output_fn() above. This should be
> + * NULL unless process specific output is needed
> + */
> + pipe_output_fn pipe_output;
...since we're on the topic we really should document what the behavior
is. My understanding of this API has been that the "err" is not "const"
because the callback is supposed to *append errors*, and there's an
implicit contract to do nothing else than that. But looking at your new
API docs:
> + * This callback is periodically called while child processes are
> + * running and also when a child process finishes.
Whatever you think this should be doing isn't documented precisely,
i.e. is it called before/after some other callbacks, should the user
rely on that at all, and...
> + * "struct strbuf *out" contains the output collected from pp_task_cb
> + * since the last call of this function.
...this just seems wrong. We don't collect outptu "from pp_task_cb", do
you mean to say something, well, I was about to put words in your mouth,
but I honestly don't know what you expect, so ...
*I'd* this to be documented as something like (which every other
callback does):
See run_processes_parallel() below for a discussion of the "struct
strbuf *out" parameter.
Now *those* docs could be improved a bit, but how we handle "err/out"
really needs to be documented holistically, as it's shared betwene
callbacks.
> + *
> + * pp_cb is the callback cookie as passed into run_processes_parallel,
> + * pp_task_cb is the callback cookie as passed into get_next_task_fn.
> + *
> + * This function is incompatible with "ungroup"
I'll re-roll the base series, which has a good place to add a BUG() if
incompatible options are combined, which would be good to add, i.e.:
if (opts->pipe_output && opts->ungroup)
BUG(...);
> +for opt in '' '--pipe-output'
> +do
> + test_expect_success "run_command runs in parallel with more jobs available than tasks $opt" '
> + test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
> + test_must_be_empty out &&
> + test_cmp expect err
> + '
> +done
I take an earlier comment back, you do have tests that would change if
that strbuf_reset() was removed, you're just not running them. You
forgot to put an "$opt" into the "test-tool" invocations:
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index feabb3717b0..19a674480cd 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -162,7 +162,7 @@ test_expect_success 'run_command runs ungrouped in parallel with as many jobs as
for opt in '' '--pipe-output'
do
test_expect_success "run_command runs in parallel with more tasks than jobs available $opt" '
- test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test-tool run-command $opt run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_must_be_empty out &&
test_cmp expect err
'
@@ -186,7 +186,7 @@ EOF
for opt in '' '--pipe-output'
do
test_expect_success "run_command is asked to abort gracefully $opt" '
- test-tool run-command run-command-abort 3 false >out 2>err &&
+ test-tool run-command $opt run-command-abort 3 false >out 2>err &&
test_must_be_empty out &&
test_cmp expect err
'
@@ -205,7 +205,7 @@ EOF
for opt in '' '--pipe-output'
do
test_expect_success "run_command outputs $opt" '
- test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test-tool run-command $opt run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_must_be_empty out &&
test_cmp expect err
'
With that all except 1 of your tests pass, now if I remove
strbuf_reset() and tweak the test so we can see what output comes from
what, then:
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index ce246dfa4bc..690467e75c0 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -56,12 +56,11 @@ static void pipe_output(struct strbuf *out,
void *pp_cb,
void *pp_task_cb)
{
- fprintf(stderr, "%s", out->buf);
- /*
- * Resetting output to show that piped output would print the
- * same as other tests without the pipe_output() function set
- */
- strbuf_reset(out);
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_add_lines(&sb, "pipe_output: ", out->buf, out->len);
+ fputs(sb.buf, stderr);
+ strbuf_release(&sb);
}
static int task_finished(int result,
The first test we fail (but with the reset we don't fail this one) is:
+ diff -u expect err
--- expect 2022-10-12 08:22:58.032264828 +0000
+++ err 2022-10-12 08:22:58.068264515 +0000
@@ -1,12 +1,24 @@
+pipe_output: preloaded output of a child
+pipe_output: Hello
+pipe_output: World
preloaded output of a child
Hello
World
+pipe_output: preloaded output of a child
+pipe_output: Hello
+pipe_output: World
+pipe_output: preloaded output of a child
+pipe_output: Hello
+pipe_output: World
preloaded output of a child
Hello
World
preloaded output of a child
Hello
World
+pipe_output: preloaded output of a child
+pipe_output: Hello
+pipe_output: World
preloaded output of a child
Hello
World
So, the apparent reason for the strbuf_reset() is that at some point in
the past you passed $out, but wanted to "reset" it so that you could
re-use the existing test_cmp.
But that's just wrong, because we'd be sweeping under the rug exactly
what we want to be testing here. Instead of hiding this you should be
test_cmp-ing the full thing.
