Re: [GSoC PATCH v7 5/5] repo: add the --format flag
From: Eric Sunshine <hidden>
Date: 2025-08-01 21:50:35
On Fri, Aug 1, 2025 at 9:11 AM Lucas Seiki Oshiro [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Add the --format flag to git-repo-info. By using this flag, the users can choose the format for obtaining the data they requested. Given that this command can be used for generating input for other applications and for being read by end users, it requires at least two formats: one for being read by humans and other for being read by machines. Some other Git commands also have two output formats, notably git-config which was the inspiration for the two formats that were chosen here: - keyvalue, where the retrieved data is printed one per line, using = for delimiting the key and the value. This is the default format, targeted for end users. - nul, where the retrieved data is separated by null characters, using the newline character for delimiting the key and the value. This format is targeted for being read by machines. Signed-off-by: Lucas Seiki Oshiro <redacted> ---diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh@@ -21,10 +21,17 @@ test_repo_info () { - test_expect_success "$label" ' - eval "$init_command $repo_name" && + test_expect_success "keyvalue: $label" ' + eval "$init_command keyvalue-$repo_name" && echo "$key=$expected_value" >expected && - git -C $repo_name repo info "$key" >actual && + git -C keyvalue-$repo_name repo info "$key" >actual && + test_cmp expected actual + ' + + test_expect_success "nul: $label" ' + eval "$init_command nul-$repo_name" && + printf "%s\n%s\0" "$key" "$expected_value" >expected && + git -C nul-$repo_name repo info --format=nul "$key" >actual && test_cmp expected actual '
Due to the embedded NUL's in this new "nul" test, I'm pretty sure you want to be using `test_cmp_bin` here as suggested previously[*]. [*]: https://lore.kernel.org/git/CAPig+cQn7c5+k06yHOD2jxYTGnny7is=fbo4tOw26eD+4zX-Jw@mail.gmail.com/ (local)
quoted hunk ↗ jump to hunk
@@ -45,6 +52,7 @@ test_repo_info 'shallow repository = false is retrieved correctly' ' test_repo_info 'shallow repository = true is retrieved correctly' ' + test_when_finished "rm -rf remote" && git init remote && echo x >remote/x && git -C remote add x &&
For what it's worth, it would be clearer to turn the removal of
"remote" into a "make sure we have a clean-slate for what we are about
to do" rather than making it an after-the-fact cleanup. That is:
test_repo_info 'shallow repository = true is retrieved correctly' '
rm -rf remote &&
git init remote &&
...
'
Alternatively, since the "remote" repository is static in the sense
that it is the same for both the "keyvalue" and "nul" cases, it would
be even clearer to just separate it out into its own "setup"-style
test:
test_expect_success 'setup remote' '
git init remote &&
echo x >remote/x &&
...
'
test_repo_info 'shallow repository = true is retrieved correctly' \
'git clone --depth 1 "file://$PWD/remote"' 'shallow'
'layout.shallow' 'true'
quoted hunk ↗ jump to hunk
@@ -79,4 +87,11 @@ test_expect_success 'output is returned correctly when two keys are requested' ' +test_expect_success 'git-repo-info aborts when requesting an invalid format' ' + test_when_finished "rm -f err expected" && + echo "fatal: invalid format '\'foo\''" >expected && + test_must_fail git repo info --format=foo 2>err && + test_cmp expected err +'
Do we need to perform this `test_when_finished` cleanup? As mentioned in earlier reviews, we don't usually perform cleanup unnecessarily since doing so slows down the test suite and makes it more difficult to debug a failing test. Also, didn't patch [2/5] already add this exact test?