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

From: Glen Choo <chooglen@google.com>
Date: Apr 28, 2023, 22:31 UTC
Message-ID: <kl6l5y9f3ahf.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <0335e5eeb4ded336c5ff7c8888c8aab9dfed2505.1682593865.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 7 quoted lines

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