Re: [PATCH v2 7/8] fetch: introduce new `--output-format` option

> On Fri, Apr 28, 2023 at 03:31:08PM -0700, Glen Choo wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > @@ -1894,6 +1902,9 @@ static int fetch_multiple(struct string_list *list, int max_children)
> > >  		     "--no-write-commit-graph", NULL);
> > >  	add_options_to_argv(&argv);
> > >  
> > > +	if (format != DISPLAY_FORMAT_UNKNOWN)
> > > +		strvec_pushf(&argv, "--output-format=%s", display_formats[format]);
> > > +
> > 
> > I think these lines belong inside add_options_to_argv(), since that's
> > also used to prepare argv for fetch_submodules(), so we'd also get
> > support for --recurse-submodules. (I wish I had spotted that in v1,
> > sorry. Thankfully they use the same helper function, so we only have to
> > do this once.)
> > 
> > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
> >   diff --git a/builtin/fetch.c b/builtin/fetch.c
> >   index 422e29a914..7aa385aed5 100644
> >   --- a/builtin/fetch.c
> >   +++ b/builtin/fetch.c
> >   @@ -1796,8 +1796,11 @@ static int add_remote_or_group(const char *name, struct string_list *list)
> >     return 1;
> >   }
> > 
> >   -static void add_options_to_argv(struct strvec *argv)
> >   +static void add_options_to_argv(struct strvec *argv,
> >   +				enum display_format format)
> >   {
> >   /* Maybe this shouldn't be first, idk */
> >   +	if (format != DISPLAY_FORMAT_UNKNOWN)
> >   +		strvec_pushf(argv, "--output-format=%s", display_formats[format]);
> >     if (dry_run)
> >       strvec_push(argv, "--dry-run");
> >     if (prune != -1)
> >   @@ -1908,10 +1911,7 @@ static int fetch_multiple(struct string_list *list, int max_children,
> >     strvec_pushl(&argv, "-c", "fetch.bundleURI=",
> >           "fetch", "--append", "--no-auto-gc",
> >           "--no-write-commit-graph", NULL);
> >   -	add_options_to_argv(&argv);
> >   -
> >   -	if (format != DISPLAY_FORMAT_UNKNOWN)
> >   -		strvec_pushf(&argv, "--output-format=%s", display_formats[format]);
> >   +	add_options_to_argv(&argv, format);
> > 
> >     if (max_children != 1 && list->nr != 1) {
> >       struct parallel_fetch_state state = { argv.v, list, 0, 0 };
> >   @@ -2403,7 +2403,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> >       if (max_children < 0)
> >         max_children = fetch_parallel_config;
> > 
> >   -		add_options_to_argv(&options);
> >   +		add_options_to_argv(&options, display_format);
> >       result = fetch_submodules(the_repository,
> >               &options,
> >               submodule_prefix,
> > 
> > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
> > 
> > I tested the result of that locally with --recurse-submodules, and
> > it works.
> 
> Unfortunately it doesn't quite work alright: while the porcelain format
> does indeed get inherited to the child process correctly, the parallel
> process API will cause us to group output per submodule-fetch. This has
> the consequence that stdout will be redirected into stderr, and that
> then breaks the assumption that all machine-parseable output goes to
> stdout.
> 
> My initial reflex is to just outright reject porcelain mode when
> submodule fetches are enabled. But that would require the caller to
> always explicitly pass `--recurse-submodules=off`, which isn't exactly
> great usability-wise.
> 
> The alternative would be to ungroup the output so that we can continue
> to print to the correct output streams. That works alright, and I've got
> a working version that does exactly that. But now we have the issue that
> the porcelain output is misleading: you cannot tell whether a specific
> reference update happens in the parent repository or in the submodule as
> that information is not part of the output.
> 
> I consider the second option to be much worse than the first option
> because it can cause scripts do to the wrong thing. So I'll send v3 with
> the first option, even though it's kind of an awful workaround. I'd be
> happy to hear any alternative proposals though.
> 
> Patrick