From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 11 Aug 2025 05:12:51 GMT
Subject: Re: [GSoC PATCH v9 2/5] repo: add the field references.format
Message-ID: <CAPig+cTA=UKbHd+BdCQUdTjvz1AzCNHi2XTREPhfUJfz_2Ny_A@mail.gmail.com>
In-Reply-To: <20250807150239.6987-3-lucasseikioshiro@gmail.com>

On Thu, Aug 7, 2025 at 11:04 AM Lucas Seiki Oshiro
<lucasseikioshiro@gmail.com> wrote:
> This commit is part of the series that introduces the new subcommand
> git-repo-info.
>
> The flag `--show-ref-format` from git-rev-parse is used for retrieving
> the reference format (i.e. `files` or `reftable`). This way, it is
> used for querying repository metadata, fitting in the purpose of
> git-repo-info.
>
> Add a new field `references.format` to the repo-info subcommand
> containing that information.
>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> @@ -22,6 +22,25 @@ COMMANDS
>         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 returned data is lexicographically sorted by the keys.
> ++
> +The output format consists of key-value pairs one per line using the `=`
> +character as the delimiter between the key and the value. Values containing
> +"unusual" characters are quoted as explained for the configuration variable
> +`core.quotePath` (see linkgit:git-config[1]). This is the default.

I don't see any alternative formats presented, so what does "This is
the default" mean here?

(I'm guessing that it might gain meaning in a later patch when NUL
output format is added, but lacking such context in this patch, the
sentence is more than a bit confusing.)

> diff --git a/builtin/repo.c b/builtin/repo.c
> @@ -1,17 +1,102 @@
> +/* repo_info_fields keys should be in lexicographical order */
> +static const struct field repo_info_fields[] = {
> +       { "references.format", get_references_format },
> +};

The comment ought to be more assertive: s/should/must/

> +static int print_fields(int argc, const char **argv, struct repository *repo)
> +{
> +       struct strbuf valbuf = STRBUF_INIT;
> +       struct strbuf quotbuf = STRBUF_INIT;
> +
> +       for (int i = 0; i < argc; i++) {
> +               get_value_fn *get_value;
> +               const char *key = argv[i];
> +
> +               strbuf_reset(&valbuf);
> +               strbuf_reset(&quotbuf);
> +
> +               if (!strcmp(key, last))
> +                       continue;
> +
> +               last = key;
> +               get_value = get_value_fn_for_key(key);
> +
> +               if (!get_value) {
> +                       ret = error(_("key '%s' not found"), key);
> +                       continue;
> +               }
> +
> +               get_value(repo, &valbuf);
> +               quote_c_style(valbuf.buf, &quotbuf, NULL, 0);
> +               printf("%s=%s\n", key, quotbuf.buf);
> +       }

Nit: To avoid unnecessary work in the two `continue` cases, I would
have placed the strbuf_reset() calls just before the call to
get_value() as illustrated in my earlier review[1]. Subjective and not
worth a reroll, though.

> diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh
> @@ -0,0 +1,57 @@
> +# Test whether a key-value pair is correctly returned
> +#
> +# Usage: test_repo_info <label> <init command> <key> <expected value>
> +#
> +# Arguments:
> +#   label: the label of the test
> +#   init_command: a command which creates a repository
> +#   repo_name: the name of the repository that will be created in init_command
> +#   key: the key of the field that is being tested
> +#   expected_value: the value that the field should contain

The "Usage" is still wrong (as mentioned earlier[1]). It shows only
four arguments despite the function taking five.

> +test_repo_info () {
> +       label=$1
> +       init_command=$2
> +       repo_name=$3
> +       key=$4
> +       expected_value=$5
> +
> +       test_expect_success "$label" '
> +               eval "$init_command $repo_name" &&
> +               echo "$key=$expected_value" >expected &&
> +               git -C $repo_name repo info "$key" >actual &&
> +               test_cmp expected actual
> +       '
> +}
> +
> +test_repo_info 'ref format files is retrieved correctly' '
> +       git init --ref-format=files' 'format-files' 'references.format' 'files'
> +
> +test_repo_info 'ref format reftable is retrieved correctly' '
> +       git init --ref-format=reftable' 'format-reftable' 'references.format' 'reftable'

The quote placement used in these calls to `test_repo_info` is still
unusual and confusing, as mentioned previously[2]. Calling the
function in the more traditional way would be preferable:

    test_repo_info 'ref format files is retrieved correctly' \
        'git init --ref-format=files' 'format-files' 'references.format' 'files'

> +test_expect_success 'git-repo-info fails if an invalid key is requested' '
> +       echo "error: key ${SQ}foo${SQ} not found" >expected_err &&
> +       test_must_fail git repo info foo 2>actual_err &&
> +       test_cmp expected_err actual_err
> +'
> +
> +test_expect_success 'git-repo-info outputs data even if there is an invalid field' '
> +       echo "references.format=$(test_detect_ref_format)" >expected &&
> +       test_must_fail git repo info foo references.format bar >actual &&
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'only one value is returned if the same key is requested twice' '
> +       val=$(git rev-parse --show-ref-format) &&
> +       echo "references.format=$val" >expect &&
> +       git repo info references.format references.format >actual &&
> +       test_cmp expect actual
> +'

In my previous review[1], I identified a problem in which the logic
would/could present a poor user-experience by emitting "key '%s' not
found" multiple times for a given unknown key, but I don't see a test
verifying that this problem has been fixed.

[1]: https://lore.kernel.org/git/CAPig+cTxNUPayO2SdCL-BPtjb2rfr3e3RK=BsQxAiiEAtpBaRg@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cR=vRu7GwGx_wpS_GZNdX7giosDK12K+qQdOW1va-6oWw@mail.gmail.com/