On Wed, May 03, 2023 at 11:43:31AM +0200, Patrick Steinhardt wrote:
> On Fri, Apr 28, 2023 at 03:31:08PM -0700, Glen Choo wrote:
> > Patrick Steinhardt <ps@xxxxxx> 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
I've gone with a slightly different variant of the first option that is
inspired by `--negotiate-only`: instead of refusing to run, we disable
submodule-fetches unless explicitly specified on the command line. In
that case we return an error.
Patrick
Attachment:
signature.asc
Description: PGP signature
