From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 27 Oct 2025 00:22:25 GMT
Subject: Re: [PATCH v3 2/2] repo: add --all to git-repo-info
Message-ID: <CAPig+cSOE1Du52Y8-s=b7HvKQyk-XHdGh3XzZR_tVktut9R1YA@mail.gmail.com>
In-Reply-To: <20251026225409.46647-3-lucasseikioshiro@gmail.com>

On Sun, Oct 26, 2025 at 6:54 PM Lucas Seiki Oshiro
<lucasseikioshiro@gmail.com> wrote:
> Add a new flag `--all` to git-repo-info for requesting values for all
> the available keys. By using this flag, the user can retrieve all the
> values instead of searching what are the desired keys for what they
> wants.
>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> @@ -18,13 +18,13 @@ THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> +`info [--format=(keyvalue|nul)] [-z] [--all | <key>...]`::
>         Retrieve metadata-related information about the current repository. Only
>         the requested data will be returned based on their keys (see "INFO KEYS"
>         section below).
>  +
>  The values are returned in the same order in which their respective keys were
> -requested.
> +requested. The `--all` flag requests the values for all the available keys.

I'm getting mixed signals from this patch. The documentation says that
it requests all *values*, but... (continued far below)

> diff --git a/builtin/repo.c b/builtin/repo.c
> @@ -124,6 +124,24 @@ static int print_fields(int argc, const char **argv,
> +static void print_all_fields(struct repository *repo,
> +                            enum output_format format)
> +{
> +       struct strbuf valbuf = STRBUF_INIT;
> +       struct strbuf quotbuf = STRBUF_INIT;
> +
> +       for (unsigned long i = 0; i < ARRAY_SIZE(repo_info_fields); i++) {
> +               struct field field = repo_info_fields[i];

Why do we need to make a copy of the field record? Can't we just use a
const pointer?

    struct field *field = &repo_info_fields[i];

or:

   struct field *field = repo_info_fields + i;

> +               strbuf_reset(&valbuf);
> +               field.get_value(repo, &valbuf);
> +               print_field(format, field.key, &valbuf, &quotbuf);
> +       }
> +
> +       strbuf_release(&valbuf);
> +       strbuf_release(&quotbuf);
> +}
> @@ -153,11 +172,20 @@ static int repo_info(int argc, const char **argv, const char *prefix,
> +               OPT_BOOL(0, "all", &all_keys, N_("return all keys")),

(continued from above) ...this gives the impression that it's only
returning *keys*, but not necessarily the values associated with those
keys. Also, "return" is a bit unusual in this context; perhaps say
"request all keys/values" or "print all keys/values" or something.

>                 OPT_END()
>         };
>
>         argc = parse_options(argc, argv, prefix, options, repo_usage, 0);
>
> +       if (all_keys) {
> +               if (argc)
> +                       die(_("--all and <key> cannot be used together"));
> +
> +               print_all_fields(repo, format);
> +               return 0;
> +       }
> +
>         return print_fields(argc, argv, repo, format);
>  }
>
> diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh
> @@ -4,6 +4,15 @@ test_description='test git repo-info'
> +# git-repo-info keys. It must contain the same keys listed in the const
> +# repo_info_fields, in lexicographical order.
> +REPO_INFO_KEYS='
> +       layout.bare
> +       layout.shallow
> +       object.format
> +       references.format
> +'

I'm not a fan of this since it is so brittle. However, I can't think
of a better alternative at the moment, and we can always revisit it
later if it becomes a maintenance burden.