Re: [PATCH v3 1/7] repo: add path.toplevel with absolute and relative suffix formatting

Hey Justin,

Thanks for the detailed review.

On Tue, Jul 28, 2026 at 10:06 PM Justin Tobler [off-list ref] wrote:

On 26/07/26 04:13PM, K Jayatheerth wrote:

quoted

Scripts frequently need to find the root directory of a repository's
working tree. Currently, this requires using `git rev-parse --show-toplevel`
or inferring it from other path components.

Introduce `path.toplevel.absolute` and `path.toplevel.relative` keys
to `git repo info`. This allows scripts to retrieve the top-level
working tree path in a predictable, strictly formatted manner without
relying on `rev-parse`.

Ok, this seems like suitable information to also look up under
git-repo-info.

quoted

If requested in a bare repository where no working tree exists, the
command returns an empty string.

This matches the existing behavior in git-rev-parse(1). Makes sense.

Yes, that is correct.

quoted

+static int get_path_toplevel_absolute(struct repository *repo, struct strbuf *buf)
+{
+     const char *work_tree = repo_get_work_tree(repo);
+
+     if (!work_tree) {
+             strbuf_addstr(buf, "");

The strbuf here is already NULL-terminated when its initialized. I don't
think this should be necessary.

Very nice point indeed
I will change that everywhere I have added that.

quoted

+     if (!work_tree) {
+             strbuf_addstr(buf, "");

Same here.

quoted

+test_expect_success 'path.toplevel returns empty in a bare repository' '
+     test_when_finished "rm -rf bare.git" &&
+     git init --bare bare.git &&
+     (
+             cd bare.git &&
+             echo "path.toplevel.absolute=" >expect &&
+             git repo info path.toplevel.absolute >actual &&
+             test_cmp expect actual

In this test we are only checking the absolute path. It probably
wouldn't hurt to also check the relative path too.

Good catch. I'll add relative-path tests in both places.

quoted

Introduce `path.superproject-working-tree.absolute` and
`path.superproject-working-tree.relative` keys to `git repo info`.
This exposes the core submodule context via a scriptable config-like key
using standard format rules.

Ok, this also seems like a good fit to include as a key in
git-repo-info, but "superproject-working-tree" is a bit of a mouthful
IMO. An alternative could potentially be "superproject-root"? Maybe its
best to just be consistent with the option name in git-rev-parse(1) and
keep it the same though.

show-superproject-working-tree is what git rev parse uses.
I am a bit unclear about this.
Since it is meant to be a scripting command, I don't know if it being
a long name matters much.
At the same time, I don't think we can keep the exact
"show-superproject-working-tree" as the key's name.
As the "show" is indirectly covered by "path." so the "show" is just
redundant if you ask me (I might be wrong here).
That is actually why I kept the rest of the name similar and omitted
the prefix from the flag name.

In other words, I am fine with the name being either one.
That said, I am more inclined with the "superproject-root".
I think the name explains exactly what it does.
Whereas "superproject-working-tree" was not that clear to me when I
first read it (Might be subjective to me).

quoted

+static int get_path_superproject_absolute(struct repository *repo UNUSED, struct strbuf *buf)
+{
+     struct strbuf superproject = STRBUF_INIT;
+
+     if (!get_superproject_working_tree(&superproject)) {
+             strbuf_release(&superproject);
+             strbuf_addstr(buf, "");

Same comment here as in the previous patch...

quoted

+     struct strbuf superproject = STRBUF_INIT;
+
+     if (!get_superproject_working_tree(&superproject)) {
+             strbuf_release(&superproject);
+             strbuf_addstr(buf, "");

...and here as well...

quoted

+             return 0;
+     }
+
+     format_path(buf, superproject.buf, startup_info->prefix, PATH_FORMAT_RELATIVE);
+     strbuf_release(&superproject);
+     return 0;
+}
+
+test_expect_success 'path.superproject-working-tree returns empty when not in a submodule' '
+     test_when_finished "rm -rf repo" &&
+     git init repo &&
+     (
+             cd repo &&
+             echo "path.superproject-working-tree.absolute=" >expect &&
+             git repo info path.superproject-working-tree.absolute >actual &&
+             test_cmp expect actual

... and here as well. :)

Good catch. I'll add a relative-path test there as well.

quoted

Tools and deployment hooks frequently query the location of the object
database directory. Currently, this relies on legacy parsing methods or
manually inspecting `git rev-parse --git-path objects`.

"Tools and deployment hooks" seems a bit overly specific. Maybe instead
we could just say "Scripts operating on a repository may need to query
the location of the object database directory"?

I think your version is consistent with other patches as well.
I wanted to ask if it is not a good practice to write commit messages
which are very specific.
I also wanted to know what constitutes being overly specific so that I
can avoid doing it again.

Also, I'm not entirely sure what is meant by "legacy parsing methods"
here.

Something scripts have historically done: parse git rev-parse
--git-dir (or read .git/config by hand)
and manually append /objects, rather than asking for the object
directory directly
which breaks if GIT_OBJECT_DIRECTORY is set.
I'll reword the commit message to just say that explicitly instead of
the vague "legacy parsing methods" phrase.

quoted

Introduce `path.objects.absolute` and `path.objects.relative` keys to
`git repo info`. This allows tools to discover the object database
location safely while natively adhering to active `GIT_OBJECT_DIRECTORY`
environment variable overrides.

In the context of pluggable ODBs, this proposed key is a little more
interesting because a non-"files" ODB source in the future may not even
have a filesystem path to an objects directory. When this becomes more
relevant, we could just adapt these keys to return an empty string in
such cases, but it does also make me question whether it is information
that we should further expose in the first place if it does eventually
becomes an internal detail of a specific ODB source.

It probably doesn't matter too much, but I've CC'd Patrick for his
thoughts too.

Thanks for looping in Patrick!

That's a fair concern.
My thinking is that the "files" backend is going to be the reality for
the vast majority of
repositories for a long time yet, and scripts asking for path.objects
today are almost certainly
assuming a filesystem-backed ODB anyway (that's the whole reason
they'd want the path).
So I don't think we're locking ourselves into a bad abstraction by
adding it now.

If/when a non-filesystem ODB source becomes real,
we'd have two reasonable options:
return an empty string for that key (same pattern we already use for
path.toplevel/superproject-working-tree when the concept doesn't
apply),
or deprecate the key outright if it turns out to be meaningless in that world.
Either way it doesn't block adding it now, worst case we're removing
or narrowing a key later, not stuck with something actively wrong.

quoted

+static int get_path_objects_absolute(struct repository *repo, struct strbuf *buf)
+{
+     const char *obj_dir = repo_get_object_directory(repo);
+
+     if (!obj_dir)
+             return error(_("unable to get object directory"));
+
+     format_path(buf, obj_dir, startup_info->prefix, PATH_FORMAT_CANONICAL);

For the absolute path, do we actually need to provide the prefix? It
might make it more clear that its the aboslute path if we just pass ""
instead?

This point was raised in the foundational patch as well.
It won't change the output for sure, so I will pass "" instead.


On Wed, Jul 29, 2026 at 12:30 AM Justin Tobler [off-list ref] wrote:

On 26/07/26 04:13PM, K Jayatheerth wrote:

quoted

External tool integrations and validation systems need a stable way to
identify where the repository hooks are stored. Currently, this involves
relying on `git rev-parse --git-path hooks` or querying `core.hooksPath`
manually.

Similar to the comment in the previous patch, "External tool
intergations and validation systems" seems overly specific IMO. Also,
"need" is a bit strongly worded as I'm sure its not a requirement for
every external script/tool.

Got it, I will try it to be not overly specific as well.
Maybe something like:
Scripts operating on a repository may want a stable way to identify
where the repository hooks are stored. Currently, this involves
relying on `git rev-parse --git-path hooks` or querying
`core.hooksPath` manually.

Introduce `path.hooks.absolute` and `path.hooks.relative` keys to
`git repo info`. This allows scripts to discover the active hooks
location, ensuring proper resolution regardless of whether Git is
using the standard `.git/hooks` structure or a custom
`core.hooksPath` setup.

quoted

Introduce `path.hooks.absolute` and `path.hooks.relative` keys to
`git repo info`. This allows tools to discover the active hooks location
natively, ensuring proper resolution regardless of whether Git is using
the standard `.git/hooks` structure or a custom `core.hooksPath` setup.

Per hooks path documentation:

  The path can be either absolute or relative. A relative path is taken
  as relative to the directory where the hooks are run.

  ...

  You  can also disable all hooks entirely by setting core.hooksPath to
  /dev/null.

Should we handle this /dev/null case specially? It looks like:

  $ git -c core.hooksPath=/dev/null rev-parse --git-path hooks

just prints '/dev/null'. I do wonder if this makes much sense though in
context of the relative path version of this key. From some quick
testing, it appears the git-rev-parse(1) version of this option always
prints the absolute path if that is what is configured (it appears to
ignore --path-format). Maybe we should just special case /dev/null and
return an empty string? I'm not entirely sure what the best route is
here though.

After thinking about it
I would lean towards not special-casing it. A few reasons:

/dev/null is just a valid (if unusual) value of core.hooksPath as far
as Git is concerned, the field is documented to reflect
whatever core.hooksPath resolves to, and rev-parse --git-path hooks
already treats it as an ordinary path.
If we special-case it to an empty string, we'd be the only place in
Git that treats /dev/null as meaningfully different from any other
hooksPath value.

Empty string is already used by other keys in this series
to mean "this concept doesn't apply here" (no working tree, not a submodule).
Using empty string for "hooks are disabled" would overload that
meaning with something different, "the path exists but you shouldn't
use it"
and a script can't tell the two apart without also knowing the
hooksPath convention, which defeats the purpose of adding a structured
key in the first place.

Any script that's checking for /dev/null already has to know that
convention when
using the existing rev-parse interface, so we're not creating new
surprises, just carrying forward a pre-existing quirk of
core.hooksPath semantics.

For the relative version:
I'll let format_path/relative_path do whatever it already does for a
path that isn't under the prefix
(I'd guess it falls back to printing the absolute path, similar to how
rev-parse seems to ignore --path-format for this case).
That keeps behavior consistent with the existing tool rather than
introducing new special-casing for path.hooks.relative specifically.

I'll add a test covering core.hooksPath=/dev/null to lock in whichever
behavior we land on,
and I can add a doc note under path.hooks.absolute/path.hooks.relative
mentioning that /dev/null is passed through unchanged if that's the
value in use,
similar to how the existing hooksPath docs describe it.
Let me know if you still think empty-string is preferable despite the
above,  happy to go either way once we agree on the reasoning.

Thanks again for the detailed review!

Regards,
- K Jayatheerth