Re: [PATCH v2 02/18] setup: stop using `the_repository` in `is_inside_worktree()`

[Patrick Steinhardt <ps@pks.im>]

On Tue, May 19, 2026 at 01:23:21AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The function `is_inside_worktree()` verifies whether or not the current
> 
> Nit: Here and the subject s/is_inside_worktree/is_inside_work_tree/
> 
> Honestly, I would prefer `worktree` as that's what we've used through
> out the code base, but that is probably out of scope here.

Oh, good eyes.

> > Note that there is one small gotcha: we sometimes may end up with
> > relative directory paths, and if so `is_inside_dir()` might fail. This
> > wasn't an issue before because of how we proactively set the cached
> > value during repository discovery. Now that we stop doing that it
> > becomes a problem though, but it is worked around by resolving the
> > repository directory via `realpath()`.
> >
> 
> Curious if you discovered this via a test. But running the tests
> without `realpath()` didn't trigger any failures. Is this something we
> can add a test for?

No, I wasn't able to come up with a test for this. I think overall the
paragraph could use some tweaking as it's warning about something that
cannot happen right now.

Originally, I had the order of `is_inside_work_tree()` and
`is_inside_git_dir()` reversed, and I had most of the commit message in
`is_inside_git_dir()`. So this explanation here was originally part of
the gitdir commit, and here it is correct: when setting the repo's
gitdir we don't resolve it to its real path. And not using the real path
does make tests fail in this case.

But this is different for the work tree: the only caller that sets
the repository's worktree is `repo_set_worktree()`, and that function
does use `real_pathdup()` to resolve the path. So we know that the
worktree is always the absolute resolved path, and consequently it's
unnecessary to resolve the variable again.

So there's two ways to handle that:

  - We could be defensive about it and continue to allow a relative or
    non-resolved worktree path here and manually resolve.

  - We could rely on the fact that currently the worktree path is always
    resolved anyway.

I think a world where we only ever use absolute paths is preferable, but
I honestly don't trust our repository setup infrastructure enough to say
that this assumption will never break. Furthermore, it would create some
asymmetry between `is_inside_git_dir()` and `is_inside_work_tree()` that
is overall a bit puzzling.

I guess the proper fix for this would be to also change the repository's
gitdir to be an absolute path. But that would result in a bunch of
user-visible changes, both in error messages and in commands like `git
rev-parse --git-common-dir`.

Meanwhile I'm more leaning into the direction of keeping the code
as-is, but correct the commit message.

Thanks!

Patrick