On Mon, Nov 28, 2022 at 12:45 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> Looking at this patch, since this new option is incompatible with "ungroup",
> I would have expected that the new functionality be in a place that already
> contains an "if (ungroup)", and thus would go into the "else" block. Looking at
> the code, it seems like a reasonable place would be in pp_collect_finished().
The code lives in pp_buffer_stderr(), which if you go one level higher, you'll
notice that the call to pp_buffer_stderr() is in the "else" block of an
"if (ungroup)".
> Is the reason this is not there because we only want the output of the child
> process, not anything that the callback functions might write to the out
> strbuf? If yes, is there a reason for that? If not, I think the code would
> be simpler if we did what I suggested. (Maybe this has already been discussed
> previously - if that is the case, the reason for doing it this way should be in
> the commit message.)
Yes, inside of pp_output(), you'll see that if the process is the output_owner,
then "pp->children[i].err" is printed and reset, which is why the code
lives before
pp_output(). The caller already has access to the callback functions and knows
what will be written to the out strbuf -- the goal of this patch is to
provide access
to all of the child output.
>
> > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> > index 3ecb830f4a..40dd329e02 100644
> > --- a/t/helper/test-run-command.c
> > +++ b/t/helper/test-run-command.c
> > @@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,
> > return 0;
> > }
> >
> > +static void duplicate_output(struct strbuf *process_out,
> > + struct strbuf *out,
> > + void *pp_cb,
> > + void *pp_task_cb)
> > +{
> > + struct string_list list = STRING_LIST_INIT_DUP;
> > +
> > + string_list_split(&list, process_out->buf, '\n', -1);
> > + for (size_t i = 0; i < list.nr; i++) {
> > + if (strlen(list.items[i].string) > 0)
> > + fprintf(stderr, "duplicate_output: %s\n", list.items[i].string);
> > + }
> > + string_list_clear(&list, 0);
> > +}
> > +
> > static int task_finished(int result,
> > struct strbuf *err,
> > void *pp_cb,
> > @@ -439,6 +454,12 @@ int cmd__run_command(int argc, const char **argv)
> > opts.ungroup = 1;
> > }
> >
> > + if (!strcmp(argv[1], "--duplicate-output")) {
> > + argv += 1;
> > + argc -= 1;
> > + opts.duplicate_output = duplicate_output;
> > + }
>
> In the tests, can we also write things from the callback functions? Whether we think that callback output should be
> duplicated or not, we should test what happens to them.
ack.
