Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts

From: Calvin Wan <calvinwan@google.com>
Date: Nov 30, 2022, 18:46 UTC
Message-ID: <CAFySSZB6XBzRedrp5+9r+xB9XmXR7Q09j+kkD-8Ym6B7bu69sQ@mail.gmail.com>
In-Reply-To: <20221128204538.2748977-1-jonathantanmy@google.com>

On Mon, Nov 28, 2022 at 12:45 PM Jonathan Tan <jonathantanmy@google.com> 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)".

Show 6 quoted lines

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

Show 39 quoted lines

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