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