On Thu, Aug 7, 2025 at 11:04 AM Lucas Seiki Oshiro
<lucasseikioshiro@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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("buf);
> +
> + 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, "buf, 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@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/git/CAPig+cR=vRu7GwGx_wpS_GZNdX7giosDK12K+qQdOW1va-6oWw@xxxxxxxxxxxxxx/
