Re: [PATCH v2 7/8] fetch: introduce new `--output-format` option
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 3, 2023, 09:43 UTC
- Message-ID
- <ZFIsw94SBxRVkWmz@ncase>
- In-Reply-To
- <kl6l5y9f3ahf.fsf@chooglen-macbookpro.roam.corp.google.com>
On Fri, Apr 28, 2023 at 03:31:08PM -0700, Glen Choo wrote:
Show 61 quoted lines
> 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