From: Junio C Hamano Date: Thu, 09 Apr 2026 14:42:46 GMT Subject: Re: [PATCH v3] repo: add paths.toplevel to repo info Message-ID: In-Reply-To: "Jayesh Daga via GitGitGadget" writes: > From: Jayesh Daga > > repo info currently does not expose the repository's > working tree root, even though this information is > available via `repo_get_work_tree()` and > `git rev-parse --show-toplevel`. > > Add a new field `paths.toplevel` to expose this value. > > While doing so, document the correspondence between > `git rev-parse` options and `repo info` fields to make > it easier to identify missing or future additions. > > For bare repositories, this value is empty, consistent > with other non-applicable fields. > > Signed-off-by: Jayesh Daga jayeshdaga99@gmail.com > --- So "document the correspondence" added to the v2 iteration is still up there. I expect to see such a change in the patch body. > repo: add paths.toplevel to repo info > > repo info currently does not expose the repository's working tree root, > even though this information is available via repo_get_work_tree(). > > This makes it harder for scripts to retrieve the repository root through > a structured interface, often requiring the use of git rev-parse > --show-toplevel. > > Add a new field paths.toplevel to git repo info that returns the working > tree root. For bare repositories, this value is empty, consistent with > other non-applicable fields. > > This provides a consistent and script-friendly way to query repository > paths without invoking additional commands. > > Signed-off-by: Jayesh Daga jayeshdaga99@gmail.com This space below the three-dash is not where reiterate what you already wrote in the commit log message. Rather, this is a place to give additional explanation that should not go in the commit log message, like "the previous iteration botched X and Y which have been corrected in this version", etc. I see this mistake often in patches from GGG users---please double check how to use that tool (I am not a GGG user, but I vaguely recall that GGG duplicates what you wrote in the pull request message after the three-dashes, so perhaps you are expected to rewrite the pull request message every time to summarize the differences since the last iteration before you submit a new iteration, or something?). > Range-diff vs v2: > > 1: 05e34bfe2c ! 1: 8a58b6ce03 repo: add paths.toplevel to repo info > @@ Commit message > For bare repositories, this value is empty, consistent > with other non-applicable fields. > > - Signed-off-by: Jayesh Daga [jayeshdaga99@gmail.com](mailto:jayeshdaga99@gmail.com) > + Signed-off-by: Jayesh Daga jayeshdaga99@gmail.com OK. > -@@ builtin/repo.c: static const struct repo_info_field repo_info_field[] = { > +@@ builtin/repo.c: static int get_references_format(struct repository *repo, struct strbuf *buf) > + return 0; > + } > + > ++/* > ++ * rev-parse -- repo info > ++ * > ++ * is-bare-repository layout.bare > ++ * is-shallow-repository layout.shallow > ++ * show-object-format object.format > ++ * show-ref-format references.format > ++ * show-toplevel paths.toplevel > ++ * > ++ * show-cdup > ++ * show-prefix > ++ * > ++ * Some entries may be candidates for future > ++ * implementation, while others may not be needed. It is a surprisingly small set. Did you do your own research, or did you only copied what I gave as examples? > +static int get_paths_toplevel(struct repository *repo, struct strbuf *buf) > +{ > + const char *wt = repo_get_work_tree(repo); > + > + if (!wt) > + return -1; /* match existing error style */ That is not a very helpful comment. It sounds more like "I do not understand why I am supposed to return -1 here upon error, but I am just blindly following what everybody else does" excuse, than "I return -1 upon an error because the caller is prepared to act on it and do X, which is what we want to see when repo_get_work_tree(repo) returns NULL, which signals condition Y that should be reported as such" explanation. If the reason why returning -1 is so obvious that we do not even have to say it (which I suspect is the case), then future readers would appreciate if you didn't add such a comment. If the reason why this needs to return -1 (and not -2 or -999 or whatever) is so subtle and needs to be explained, then the comment does not give even a single bit of information to help them understand why. > + strbuf_addstr(buf, wt); > + return 0; > +} > diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh > index 39bb77dda0..470e06e8c2 100755 > --- a/t/t1900-repo-info.sh > +++ b/t/t1900-repo-info.sh > @@ -155,4 +155,20 @@ test_expect_success 'git repo info -h shows only repo info usage' ' > test_grep ! "git repo structure" actual > ' > > +test_expect_success 'repo info paths.toplevel' ' > + git repo info paths.toplevel >actual && > + echo "paths.toplevel=$(git rev-parse --show-toplevel)" >expected && These seem to be SP*4 indented; one level of indent is supposed to be a single tab, not 4-column. You may have copied it from other tests, but the above is done in an unusual order and I found it harder to read. We prepare an expected output in a file called "expect" first, and then store the output of the command we are testing in a file called "actual", and then compare "test_cmp expect actual", in this order. If you deliberately misspell the command to pass "--shaw-toplabel", does it break this "echo" invocation, or will we only notice the breakage/mistake when we check the output in the "expect" file against > + test_cmp expected actual > +' > + > +test_expect_success 'repo info paths.toplevel (bare repo)' ' > + git init --bare bare.git && > + ( > + cd bare.git && > + git repo info paths.toplevel >actual && > + echo "paths.toplevel=" >expected && > + test_cmp expected actual > + ) > +' > + > test_done > > base-commit: 256554692df0685b45e60778b08802b720880c50