Re: [GSoC PATCH v9 2/5] repo: add the field references.format
From: Eric Sunshine <hidden>
Date: 2025-08-11 05:13:04
On Thu, Aug 7, 2025 at 11:04 AM Lucas Seiki Oshiro [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> ---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.)
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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/ (local) [2]: https://lore.kernel.org/git/CAPig+cR=vRu7GwGx_wpS_GZNdX7giosDK12K+qQdOW1va-6oWw@mail.gmail.com/ (local)