Re: [GSoC PATCH v7 3/5] repo: add the field layout.bare
From: Eric Sunshine <hidden>
Date: 2025-08-01 21:21:53
On Fri, Aug 1, 2025 at 9:11 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 --is-bare-repository from git-rev-parse is used for retrieving whether the current repository is bare. This way, it is used for querying repository metadata, fitting in the purpose of git-repo-info. Then, add a new field layout.bare to the git-repo-info subcommand containing that information. Signed-off-by: Lucas Seiki Oshiro <redacted> ---diff --git a/builtin/repo.c b/builtin/repo.c@@ -16,6 +19,13 @@ struct field { +static int get_layout_bare(struct repository *repo UNUSED, struct strbuf *buf) +{ + strbuf_addstr(buf, + is_bare_repository() ? "true" : "false"); + return 0; +}
Nit: You can drop the unnecessary line wrapping:
strbuf_addstr(buf, is_bare_repository() ? "true" : "false");
But don't re-roll just for this.
quoted hunk ↗ jump to hunk
diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh@@ -35,6 +35,12 @@ test_repo_info 'ref format files is retrieved correctly' ' +test_repo_info 'bare repository = false is retrieved correctly' ' + git init' 'bare' 'layout.bare' 'false' + +test_repo_info 'bare repository = true is retrieved correctly' ' + git init --bare' 'nonbare' 'layout.bare' 'true'
The quote placement used in these calls to `test_repo_info` is quite
unusual and more than a little confusing. I'm guessing you did it this
way to avoid having to use a backslash to continue the line or did it
to mimic how `test_expect/fail` is called, but it makes the function
call more difficult to understand than it ought to be. Instead, call
the function in the more traditional way:
test_repo_info 'bare repository = true is retrieved correctly' \
'git init --bare' 'nonbare' 'layout.bare' 'true'
This comment applies to the previous patch, as well, but I didn't
notice the issue when reviewing that patch.
quoted hunk ↗ jump to hunk
@@ -54,4 +60,12 @@ test_expect_success 'only one value is returned if the same key is requested twi +test_expect_success 'output is returned correctly when two keys are requested' ' + cat >expect <<-\EOF && + layout.bare=false + references.format=files + EOF + git init --ref-format=files two-keys && + git -C two-keys repo info layout.bare references.format +'
It's good to see use of the heredoc as suggested in the previous review, but isn't this test missing something important? Namely, it's never comparing the actual output to the expected output; in fact, it's never even capturing the actual output.