Re: [PATCH v3] repo: add paths.toplevel to repo info
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 9, 2026, 14:42 UTC
- Message-ID
- <xmqq8qaww5y1.fsf@gitster.g>
- In-Reply-To
- <pull.2264.v3.git.git.1775714492944.gitgitgadget@gmail.com>
"Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 18 quoted lines
> From: Jayesh Daga <jayeshdaga99@gmail.com> > > 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.
Show 17 quoted lines
> 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?).
Show 9 quoted lines
> 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.
Show 19 quoted lines
> -@@ 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 --<opt> repo info <field>
> ++ *
> ++ * 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 <missing>
> ++ * show-prefix <missing>
> ++ *
> ++ * Some <missing> 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?
Show 6 quoted lines
> +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; > +}
Show 11 quoted lines
> 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
Show 16 quoted lines
> + 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