worktree repair: detect relative path in .git file correctly

[yoichi]

cc: Yoichi Nakayama yoichi.nakayama@gmail.com

[yoichi]

/submit

[gitgitgadget]

Submitted as pull.2205.git.1786799480344.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2205/yoichi/worktree-repair-relative-path-handling-v1

To fetch this version to local tag pr-2205/yoichi/worktree-repair-relative-path-handling-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2205/yoichi/worktree-repair-relative-path-handling-v1

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> Since read_gitfile_gently() always returns an absolute path, the
> conversion from a relative path to an absolute path was not
> functioning and dead code existed.

This is ugly.  What problem is this really fixing?  What "conversion
from a relative path to an absolute path" does the above refer to?
What "dead code"?  Where in what file and what function?  Why does
the caller even care if it is absolute or relative?  Shouldn't they
work equally well as long as they point at the right location?

The proposed log message hides so many details to evaluate the claim
that this is a good change, and raises many unanswered questions.

Yes, read_gitfile_gently() always turns the gitfile it reads into an
absolute form.  Is there a caller A that wants the underlying
relative form, and if so why?  Is it to compare with some other path
that is relative?  How did the code B obtained the other path to be
compared that is relative?  If that code B used the helper that is
different from read_gitfile_gently() to obtain the other path that
is relative, perhaps the caller A can be changed to call it instead
of calling read_gitfile_gently() and the fix can be done without
churning so many existing call sites?

Stepping back a bit, why does "repair" even care if it is relative?
Is it considered a semi-error when a gitfile records its target as a
relative path?  If so, I wonder if a cleaner way may be to add a new
READ_GITFILE_ERR_RELATIVE_PATH constant that is treated as non-fatal
error by the read_gitfile_error_die() function?  If that approach
works, that may be the cleanest, as I suspect that "was it recorded
as an absolute path?" will not stay to be the only special case in
niche applications like "repair", but we need to audit callers of
the _gently() function and make sure they do not barf with the new
return code.

If not, perhaps introduce a separate function that returns the path
it read without any conversion, i.e.,

    char *read_raw_gitfile(const char *path);

that "repair" thing can use, and have it do the relateve-to-absolute
converaion itself, perhaps?  That function would be created by moving
most of the code from read_gitfile_gently() and read_gitfile_gently()
would become a very thin wrapper around that function.  Wouldn't that
be the least invasive and cleanest solution, if it works?

Thanks.

[gitgitgadget]

Yoichi Nakayama wrote on the Git mailing list (how to reply to this email):

On Tue, Aug 18, 2026 at 2:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> >
> > Since read_gitfile_gently() always returns an absolute path, the
> > conversion from a relative path to an absolute path was not
> > functioning and dead code existed.
>
> This is ugly.  What problem is this really fixing?  What "conversion
> from a relative path to an absolute path" does the above refer to?
> What "dead code"?  Where in what file and what function?  Why does
> the caller even care if it is absolute or relative?  Shouldn't they
> work equally well as long as they point at the right location?
>
> The proposed log message hides so many details to evaluate the claim
> that this is a good change, and raises many unanswered questions.

I'm sorry, the commit message lacked an explanation.
Let me explain the details of the issue I want to resolve.

When we create a worktree using default settings or with
`worktree.useRelativePaths=false`,
the cross references between the worktree and the repository
(specifically `worktree/id/gitdir`
in the main repository and the `.git` file in the worktree) are
recorded using absolute paths.

% mkdir repo
% cd repo
repo % git init
Initialized empty Git repository in /private/tmp/repo/.git/
repo % git commit --allow-empty -m init
[master (root-commit) bb4f6a1] init
repo % git config worktree.useRelativePaths
repo % git worktree add ../foo --detach
Preparing worktree (detached HEAD bb4f6a1)
HEAD is now at bb4f6a1 init
repo % cat .git/worktrees/foo/gitdir
/private/tmp/foo/.git
repo % cat ../foo/.git
gitdir: /private/tmp/repo/.git/worktrees/foo

In this situation, if we change the setting to
`worktree.useRelativePaths=true` and run
 `git worktree repair` within the main worktree, the cross references
are converted to
relative paths (this is an expected behavior).

repo % git config worktree.useRelativePaths true
repo % git worktree repair
repair: .git file absolute/relative path mismatch: /private/tmp/foo
repo % cat .git/worktrees/foo/gitdir
../../../../foo/.git
repo % cat ../foo/.git
gitdir: ../repo/.git/worktrees/foo

On the other hand, given a state where cross references are recorded
using relative paths,
one would expect (by symmetry) that changing
`worktree.useRelativePath` from `true` to `false`
and running `git worktree repair` would convert the cross references
to absolute paths. However,
no "absolute/relative path mismatch" is detected, and the cross
references remain as relative paths.
This is the problem I wanted to fix.

repo % cat .git/worktrees/foo/gitdir
../../../../foo/.git
repo % cat ../foo/.git
gitdir: ../repo/.git/worktrees/foo
repo % git config worktree.useRelativePaths false
repo % git worktree repair
repo % cat .git/worktrees/foo/gitdir
../../../../foo/.git
repo % cat ../foo/.git
gitdir: ../repo/.git/worktrees/foo

The issue has been present since the initial implementation:
717af916cd (worktree: link worktrees with relative paths, 2024-10-07)
Although `dotgit_contents` (retrieved via `read_gitfile_gently()`) is
always an absolute path,
the implementations of `repair_gitfile()` and
`repair_worktree_at_path()` treat it as if the
actual contents of the `.git` file had been returned.

I have confirmed that the above issue can be reproduced even in the
v2.48.0 tag, which was
the first release to include that change.

> Yes, read_gitfile_gently() always turns the gitfile it reads into an
> absolute form.  Is there a caller A that wants the underlying
> relative form, and if so why?  Is it to compare with some other path
> that is relative?  How did the code B obtained the other path to be
> compared that is relative?  If that code B used the helper that is
> different from read_gitfile_gently() to obtain the other path that
> is relative, perhaps the caller A can be changed to call it instead
> of calling read_gitfile_gently() and the fix can be done without
> churning so many existing call sites?
>
> Stepping back a bit, why does "repair" even care if it is relative?
> Is it considered a semi-error when a gitfile records its target as a
> relative path?  If so, I wonder if a cleaner way may be to add a new
> READ_GITFILE_ERR_RELATIVE_PATH constant that is treated as non-fatal
> error by the read_gitfile_error_die() function?  If that approach
> works, that may be the cleanest, as I suspect that "was it recorded
> as an absolute path?" will not stay to be the only special case in
> niche applications like "repair", but we need to audit callers of
> the _gently() function and make sure they do not barf with the new
> return code.
>
> If not, perhaps introduce a separate function that returns the path
> it read without any conversion, i.e.,
>
>     char *read_raw_gitfile(const char *path);
>
> that "repair" thing can use, and have it do the relateve-to-absolute
> converaion itself, perhaps?  That function would be created by moving
> most of the code from read_gitfile_gently() and read_gitfile_gently()
> would become a very thin wrapper around that function.  Wouldn't that
> be the least invasive and cleanest solution, if it works?

You're right; changing the signature of `read_gitfile_gently` for a niche use
case like `worktree repair` isn't a good idea. I'll revise the
approach to introduce
something like the `read_raw_gitfile()` you suggested.

Thanks,
-- 
Yoichi NAKAYAMA

[gitgitgadget]

User Yoichi Nakayama <yoichi.nakayama@gmail.com> has been added to the cc: list.

[yoichi]

/submit

[gitgitgadget]

Submitted as pull.2205.v2.git.1787240760069.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2205/yoichi/worktree-repair-relative-path-handling-v2

To fetch this version to local tag pr-2205/yoichi/worktree-repair-relative-path-handling-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2205/yoichi/worktree-repair-relative-path-handling-v2

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This is because we wrongly use read_gitfile_gently() which always
> returns an absolute path. To fix this, introduce read_gitfile_raw()
> that is almost same as read_gitfile_gently(), but it skips existence
> check of the referenced repository and returns the unmodified path
> read from .git file.

This is more or less what I expected to see, but two function-scope
static variables are worse than one.  At least let us not
proliferate the bad pattern that makes the functions non-reentrant.

The attached patch updates read_gitfile_raw() in your patch to take
a caller-prepared strbuf to store the value read from the '.git'
file, returning the error code as an integer.  Ideally in the far
future, we would probably want to convert read_gitfile_gently() to
follow a similar function signature, but let us leave it as
#leftoverbits, as it has many more existing callers and all of them
would need adjusting.  On the other hand, it is easier to get the API
in read_gitfile_raw() right while it still has only two callers.


 setup.c | 9 +++------
 setup.h | 2 +-
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git c/setup.c w/setup.c
index af7601ff67..052c7d669b 100644
--- c/setup.c
+++ w/setup.c
@@ -996,7 +996,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	return error_code ? NULL : realpath.buf;
 }
 
-const char *read_gitfile_raw(const char *path, int *return_error_code)
+int read_gitfile_raw(struct strbuf *contents, const char *path)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
@@ -1004,7 +1004,6 @@ const char *read_gitfile_raw(const char *path, int *return_error_code)
 	struct stat st;
 	int fd;
 	ssize_t len;
-	static struct strbuf contents = STRBUF_INIT;
 
 	if (stat(path, &st)) {
 		if (errno == ENOENT || errno == ENOTDIR)
@@ -1047,13 +1046,11 @@ const char *read_gitfile_raw(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NO_PATH;
 		goto cleanup_return;
 	}
-	strbuf_reset(&contents);
-	strbuf_add(&contents, buf+8, len-8);
+	strbuf_add(contents, buf+8, len-8);
 
 cleanup_return:
-	*return_error_code = error_code;
 	free(buf);
-	return error_code ? NULL : contents.buf;
+	return error_code;
 }
 
 static void apply_gitdir_and_environment(struct repository *repo, const char *path)
diff --git c/setup.h w/setup.h
index 4c2fcbbeda..7394473e95 100644
--- c/setup.h
+++ w/setup.h
@@ -40,7 +40,7 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
-const char *read_gitfile_raw(const char *path, int *return_error_code);
+int read_gitfile_raw(struct strbuf *contents, const char *path);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
 const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)