worktree repair: detect relative path in .git file correctly

From: Yoichi NAKAYAMA <[email protected]>

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.

---
worktree repair: detect relative path in .git file correctly

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2205%2Fyoichi%2Fworktree-repair-relative-path-handling-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2205/yoichi/worktree-repair-relative-path-handling-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2205

 builtin/init-db.c          |  2 +-
 setup.c                    | 13 ++++++---
 setup.h                    |  4 +--
 t/t2406-worktree-repair.sh | 54 +++++++++++++++++++++++++++++---------
 worktree.c                 | 28 ++++++--------------
 5 files changed, 62 insertions(+), 39 deletions(-)

index e96b1283b7..2369fcea1b 100644
@@ -210,7 +210,7 @@ int cmd_init_db(int argc,
 		const char *p;
 		struct strbuf sb = STRBUF_INIT;
 
-		p = read_gitfile_gently(git_dir, &err);
+		p = read_gitfile_gently(git_dir, NULL, &err);
 		if (p && get_common_dir(&sb, p)) {
 			struct strbuf mainwt = STRBUF_INIT;
 

index 95909e9603..febb3248a5 100644
@@ -458,7 +458,7 @@ int is_nonbare_repository_dir(struct strbuf *path)
 	assert(orig_path_len != 0);
 	strbuf_complete(path, '/');
 	strbuf_addstr(path, ".git");
-	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
+	if (read_gitfile_gently(path->buf, NULL, &gitfile_error) || is_git_directory(path->buf))
 		ret = 1;
 	if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
 	    gitfile_error == READ_GITFILE_ERR_READ_FAILED)

@@ -956,12 +956,15 @@ void read_gitfile_error_die(int error_code, const char *path)
  * return path to git directory if found. The return value comes from
  * a shared buffer.
  *
+ * On success, if absolute is not NULL, it will be set to whether the
+ * path in .git file is an absolute path.
+ *
  * On failure, if return_error_code is not NULL, return_error_code
  * will be set to an error code and NULL will be returned. If
  * return_error_code is NULL the function will die instead (for most
  * cases).
  */
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, bool *absolute, int *return_error_code)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;

@@ -1016,6 +1019,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	}
 	buf[len] = '\0';
 	dir = buf + 8;
+	if (absolute)
+		*absolute = is_absolute_path(dir);
 
 	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
 		size_t pathlen = slash+1 - path;

@@ -1615,7 +1620,7 @@ static enum discovery_result repo_discovery_find_dir(struct strbuf *dir,
 		if (offset > min_offset)
 			strbuf_addch(dir, '/');
 		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
-		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
+		gitdirenv = read_gitfile_gently(dir->buf, NULL, &error_code);
 		if (!gitdirenv) {
 			switch (error_code) {
 			case READ_GITFILE_ERR_MISSING:

@@ -2185,7 +2190,7 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
 {
 	if (is_git_directory(suspect))
 		return suspect;
-	return read_gitfile_gently(suspect, return_error_code);
+	return read_gitfile_gently(suspect, NULL, return_error_code);
 }
 
 /* if any standard file descriptor is missing open it to /dev/null */

index 654f10e059..018893b1d7 100644
@@ -39,8 +39,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_MISSING 9
 #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);
-#define read_gitfile(path) read_gitfile_gently((path), NULL)
+const char *read_gitfile_gently(const char *path, bool *absolute, int *return_error_code);
+#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
 const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
 

index cbf95328a3..86b599ed27 100644
@@ -409,7 +409,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 		goto done;
 	}
 
-	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
+	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, NULL, &err));
 	if (!path) {
 		strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
 				   wt_path.buf, err);

@@ -650,6 +650,7 @@ static void repair_gitfile(struct worktree *wt,
 	struct strbuf repo = STRBUF_INIT;
 	struct strbuf backlink = STRBUF_INIT;
 	char *dotgit_contents = NULL;
+	bool absolute;
 	const char *repair = NULL;
 	char *path = NULL;
 	int err;

@@ -667,16 +668,10 @@ static void repair_gitfile(struct worktree *wt,
 	strbuf_realpath(&repo, path, 1);
 	strbuf_addf(&dotgit, "%s/.git", wt->path);
 	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
-	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &absolute, &err));
 
-	if (dotgit_contents) {
-		if (is_absolute_path(dotgit_contents)) {
-			strbuf_addstr(&backlink, dotgit_contents);
-		} else {
-			strbuf_addf(&backlink, "%s/%s", wt->path, dotgit_contents);
-			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
-		}
-	}
+	if (dotgit_contents)
+		strbuf_addstr(&backlink, dotgit_contents);
 
 	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 		err == READ_GITFILE_ERR_IS_A_DIR)

@@ -685,7 +680,7 @@ static void repair_gitfile(struct worktree *wt,
 		repair = _(".git file broken");
 	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");
-	else if (use_relative_paths == is_absolute_path(dotgit_contents))
+	else if (use_relative_paths == absolute)
 		repair = _(".git file absolute/relative path mismatch");
 
 	if (repair) {

@@ -855,16 +850,9 @@ void repair_worktree_at_path(struct repository *repo,
 
 	infer_backlink(repo, dotgit.buf, &inferred_backlink);
 	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
-	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, NULL, &err));
 	if (dotgit_contents) {
-		if (is_absolute_path(dotgit_contents)) {
-			strbuf_addstr(&backlink, dotgit_contents);
-		} else {
-			strbuf_addbuf(&backlink, &dotgit);
-			strbuf_strip_suffix(&backlink, ".git");
-			strbuf_addstr(&backlink, dotgit_contents);
-			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
-		}
+		strbuf_addstr(&backlink, dotgit_contents);
 	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);

base-commit: 11c6700f10234578d10523faf35656ca491425c9

gitgitgadget

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.

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.

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

From: Yoichi NAKAYAMA <[email protected]>

Given a state where 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, setting 'worktree.useRelativePaths=true' and running 'git worktree repair' within the main worktree converts them to relative paths.

On the other hand, given a state where the cross references are recorded using relative paths, one would expect (by symmetry) that setting 'worktree.useRelativePath=false' and running 'git worktree repair' would convert them to absolute paths. However, they remain as relative paths.

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.

---
worktree repair: detect relative path in .git file correctly

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2205%2Fyoichi%2Fworktree-repair-relative-path-handling-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2205/yoichi/worktree-repair-relative-path-handling-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2205

Range-diff vs v1:

1: c97b948565 ! 1: 5bcf19ef50 worktree repair: detect relative path in .git file correctly

     @@ Metadata
       ## Commit message ##
          worktree repair: detect relative path in .git file correctly
      
     -    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.
     +    Given a state where 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,
     +    setting 'worktree.useRelativePaths=true' and running 'git worktree
     +    repair' within the main worktree converts them to relative paths.
      
     -    Signed-off-by: Yoichi NAKAYAMA <[email protected]>
     +    On the other hand, given a state where the cross references are
     +    recorded using relative paths, one would expect (by symmetry) that
     +    setting 'worktree.useRelativePath=false' and running 'git worktree
     +    repair' would convert them to absolute paths. However, they remain as
     +    relative paths.
      
     - ## builtin/init-db.c ##
     -@@ builtin/init-db.c: int cmd_init_db(int argc,
     - 		const char *p;
     - 		struct strbuf sb = STRBUF_INIT;
     - 
     --		p = read_gitfile_gently(git_dir, &err);
     -+		p = read_gitfile_gently(git_dir, NULL, &err);
     - 		if (p && get_common_dir(&sb, p)) {
     - 			struct strbuf mainwt = STRBUF_INIT;
     - 
     +    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.
     +
     +    Signed-off-by: Yoichi NAKAYAMA <[email protected]>
      
       ## setup.c ##
     -@@ setup.c: int is_nonbare_repository_dir(struct strbuf *path)
     - 	assert(orig_path_len != 0);
     - 	strbuf_complete(path, '/');
     - 	strbuf_addstr(path, ".git");
     --	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
     -+	if (read_gitfile_gently(path->buf, NULL, &gitfile_error) || is_git_directory(path->buf))
     - 		ret = 1;
     - 	if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
     - 	    gitfile_error == READ_GITFILE_ERR_READ_FAILED)
      @@ setup.c: void read_gitfile_error_die(int error_code, const char *path)
     -  * return path to git directory if found. The return value comes from
     -  * a shared buffer.
     -  *
     -+ * On success, if absolute is not NULL, it will be set to whether the
     -+ * path in .git file is an absolute path.
     -+ *
     -  * On failure, if return_error_code is not NULL, return_error_code
     -  * will be set to an error code and NULL will be returned. If
     -  * return_error_code is NULL the function will die instead (for most
     -  * cases).
        */
     --const char *read_gitfile_gently(const char *path, int *return_error_code)
     -+const char *read_gitfile_gently(const char *path, bool *absolute, int *return_error_code)
     + const char *read_gitfile_gently(const char *path, int *return_error_code)
       {
     - 	const int max_file_size = 1 << 20;  /* 1MB */
     +-	const int max_file_size = 1 << 20;  /* 1MB */
       	int error_code = 0;
     + 	char *buf = NULL;
     +-	char *dir = NULL;
     + 	const char *slash;
     ++	static struct strbuf realpath = STRBUF_INIT;
     ++
     ++	buf = xstrdup_or_null(read_gitfile_raw(path, &error_code));
     ++	if (error_code)
     ++		goto cleanup_return;
     ++
     ++	if (!is_absolute_path(buf) && (slash = strrchr(path, '/'))) {
     ++		size_t pathlen = slash+1 - path;
     ++		char *dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
     ++				    (int)strlen(buf), buf);
     ++		free(buf);
     ++		buf = dir;
     ++	}
     ++	if (!is_git_directory(buf)) {
     ++		error_code = READ_GITFILE_ERR_NOT_A_REPO;
     ++		goto cleanup_return;
     ++	}
     ++
     ++	strbuf_realpath(&realpath, buf, 1);
     ++
     ++cleanup_return:
     ++	if (return_error_code)
     ++		*return_error_code = error_code;
     ++	else if (error_code)
     ++		read_gitfile_error_die(error_code, path);
     ++
     ++	free(buf);
     ++	return error_code ? NULL : realpath.buf;
     ++}
     ++
     ++const char *read_gitfile_raw(const char *path, int *return_error_code)
     ++{
     ++	const int max_file_size = 1 << 20;  /* 1MB */
     ++	int error_code = 0;
     ++	char *buf = NULL;
     + 	struct stat st;
     + 	int fd;
     + 	ssize_t len;
     +-	static struct strbuf realpath = STRBUF_INIT;
     ++	static struct strbuf contents = STRBUF_INIT;
     + 
     + 	if (stat(path, &st)) {
     + 		if (errno == ENOENT || errno == ENOTDIR)
      @@ setup.c: const char *read_gitfile_gently(const char *path, int *return_error_code)
     + 		error_code = READ_GITFILE_ERR_NO_PATH;
     + 		goto cleanup_return;
       	}
     - 	buf[len] = '\0';
     - 	dir = buf + 8;
     -+	if (absolute)
     -+		*absolute = is_absolute_path(dir);
     +-	buf[len] = '\0';
     +-	dir = buf + 8;
     +-
     +-	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
     +-		size_t pathlen = slash+1 - path;
     +-		dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
     +-			      (int)(len - 8), buf + 8);
     +-		free(buf);
     +-		buf = dir;
     +-	}
     +-	if (!is_git_directory(dir)) {
     +-		error_code = READ_GITFILE_ERR_NOT_A_REPO;
     +-		goto cleanup_return;
     +-	}
     +-
     +-	strbuf_realpath(&realpath, dir, 1);
     +-	path = realpath.buf;
     ++	strbuf_reset(&contents);
     ++	strbuf_add(&contents, buf+8, len-8);
       
     - 	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
     - 		size_t pathlen = slash+1 - path;
     -@@ setup.c: static enum discovery_result repo_discovery_find_dir(struct strbuf *dir,
     - 		if (offset > min_offset)
     - 			strbuf_addch(dir, '/');
     - 		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
     --		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
     -+		gitdirenv = read_gitfile_gently(dir->buf, NULL, &error_code);
     - 		if (!gitdirenv) {
     - 			switch (error_code) {
     - 			case READ_GITFILE_ERR_MISSING:
     -@@ setup.c: const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
     - {
     - 	if (is_git_directory(suspect))
     - 		return suspect;
     --	return read_gitfile_gently(suspect, return_error_code);
     -+	return read_gitfile_gently(suspect, NULL, return_error_code);
     + cleanup_return:
     +-	if (return_error_code)
     +-		*return_error_code = error_code;
     +-	else if (error_code)
     +-		read_gitfile_error_die(error_code, path);
     +-
     ++	*return_error_code = error_code;
     + 	free(buf);
     +-	return error_code ? NULL : path;
     ++	return error_code ? NULL : contents.buf;
       }
       
     - /* if any standard file descriptor is missing open it to /dev/null */
     + static void apply_gitdir_and_environment(struct repository *repo, const char *path)
      
       ## setup.h ##
      @@ setup.h: int is_nonbare_repository_dir(struct strbuf *path);
     - #define READ_GITFILE_ERR_MISSING 9
       #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);
     --#define read_gitfile(path) read_gitfile_gently((path), NULL)
     -+const char *read_gitfile_gently(const char *path, bool *absolute, int *return_error_code);
     -+#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
     + const char *read_gitfile_gently(const char *path, int *return_error_code);
     ++const char *read_gitfile_raw(const char *path, int *return_error_code);
     + #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)
     - 
      
       ## t/t2406-worktree-repair.sh ##
      @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative path with missing gitfile' '
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
       
      -test_expect_success 'repair absolute worktree to use relative paths' '
      -	test_when_finished "rm -rf main side sidemoved" &&
     -+test_expect_success 'repair absolute to relative in side worktree' '
     ++test_expect_success 'repair absolute to relative from side worktree' '
      +	test_when_finished "rm -rf main side" &&
       	test_create_repo main &&
       	test_commit -C main init &&
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
       	echo "gitdir: ../main/.git/worktrees/side" >expect-gitfile &&
      -	mv side sidemoved &&
      -	git -C main worktree repair --relative-paths ../sidemoved &&
     -+	git -C side worktree repair --relative-paths 2>main/err &&
     ++	git -C main worktree repair --relative-paths ../side 2>main/err &&
      +	test_grep "gitdir absolute/relative path mismatch" main/err &&
       	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
      -	test_cmp expect-gitfile sidemoved/.git
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
       
      -test_expect_success 'repair relative worktree to use absolute paths' '
      -	test_when_finished "rm -rf main side sidemoved" &&
     -+test_expect_success 'repair relative to absolute in side worktree' '
     ++test_expect_success 'repair relative to absolute from side worktree' '
      +	test_when_finished "rm -rf main side" &&
       	test_create_repo main &&
       	test_commit -C main init &&
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
       	echo "gitdir: $(pwd)/main/.git/worktrees/side" >expect-gitfile &&
      -	mv side sidemoved &&
      -	git -C main worktree repair ../sidemoved &&
     -+	git -C side worktree repair 2>main/err &&
     ++	git -C main worktree repair ../side 2>main/err &&
      +	test_grep "gitdir absolute/relative path mismatch" main/err &&
       	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
      -	test_cmp expect-gitfile sidemoved/.git
      +	test_cmp expect-gitfile side/.git
      +'
      +
     -+test_expect_success 'repair absolute to relative in main worktree' '
     ++test_expect_success 'repair absolute to relative from main worktree' '
      +	test_when_finished "rm -rf main side" &&
      +	test_create_repo main &&
      +	git -C main config worktree.useRelativePaths false &&
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
      +	test_cmp expect-gitfile side/.git
      +'
      +
     -+test_expect_success 'repair relative to absolute in main worktree' '
     ++test_expect_success 'repair relative to absolute from main worktree' '
      +	test_when_finished "rm -rf main side" &&
      +	test_create_repo main &&
      +	git -C main config worktree.useRelativePaths true &&
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
       test_done
      
       ## worktree.c ##
     -@@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
     - 		goto done;
     - 	}
     - 
     --	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
     -+	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, NULL, &err));
     - 	if (!path) {
     - 		strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
     - 				   wt_path.buf, err);
     -@@ worktree.c: static void repair_gitfile(struct worktree *wt,
     - 	struct strbuf repo = STRBUF_INIT;
     - 	struct strbuf backlink = STRBUF_INIT;
     - 	char *dotgit_contents = NULL;
     -+	bool absolute;
     - 	const char *repair = NULL;
     - 	char *path = NULL;
     - 	int err;
      @@ worktree.c: static void repair_gitfile(struct worktree *wt,
       	strbuf_realpath(&repo, path, 1);
       	strbuf_addf(&dotgit, "%s/.git", wt->path);
       	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
      -	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
     -+	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &absolute, &err));
     - 
     --	if (dotgit_contents) {
     --		if (is_absolute_path(dotgit_contents)) {
     --			strbuf_addstr(&backlink, dotgit_contents);
     --		} else {
     --			strbuf_addf(&backlink, "%s/%s", wt->path, dotgit_contents);
     --			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
     --		}
     --	}
     -+	if (dotgit_contents)
     -+		strbuf_addstr(&backlink, dotgit_contents);
     ++	dotgit_contents = xstrdup_or_null(read_gitfile_raw(dotgit.buf, &err));
       
     + 	if (dotgit_contents) {
     + 		if (is_absolute_path(dotgit_contents)) {
     +@@ worktree.c: static void repair_gitfile(struct worktree *wt,
       	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
       		err == READ_GITFILE_ERR_IS_A_DIR)
     -@@ worktree.c: static void repair_gitfile(struct worktree *wt,
     + 		fn(1, wt->path, _(".git is not a file"), cb_data);
     +-	else if (err)
     ++	else if (err || !is_git_directory(backlink.buf))
       		repair = _(".git file broken");
       	else if (fspathcmp(backlink.buf, repo.buf))
       		repair = _(".git file incorrect");
     --	else if (use_relative_paths == is_absolute_path(dotgit_contents))
     -+	else if (use_relative_paths == absolute)
     - 		repair = _(".git file absolute/relative path mismatch");
     - 
     - 	if (repair) {
      @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
     - 
     - 	infer_backlink(repo, dotgit.buf, &inferred_backlink);
       	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
     --	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
     -+	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, NULL, &err));
     + 	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
       	if (dotgit_contents) {
      -		if (is_absolute_path(dotgit_contents)) {
      -			strbuf_addstr(&backlink, dotgit_contents);

 setup.c                    | 66 +++++++++++++++++++++++---------------
 setup.h                    |  1 +
 t/t2406-worktree-repair.sh | 54 ++++++++++++++++++++++++-------
 worktree.c                 | 13 ++------
 4 files changed, 86 insertions(+), 48 deletions(-)

index 95909e9603..73111e08af 100644
@@ -963,15 +963,48 @@ void read_gitfile_error_die(int error_code, const char *path)
  */
 const char *read_gitfile_gently(const char *path, int *return_error_code)
 {
-	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
 	char *buf = NULL;
-	char *dir = NULL;
 	const char *slash;
+	static struct strbuf realpath = STRBUF_INIT;
+
+	buf = xstrdup_or_null(read_gitfile_raw(path, &error_code));
+	if (error_code)
+		goto cleanup_return;
+
+	if (!is_absolute_path(buf) && (slash = strrchr(path, '/'))) {
+		size_t pathlen = slash+1 - path;
+		char *dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
+				    (int)strlen(buf), buf);
+		free(buf);
+		buf = dir;
+	}
+	if (!is_git_directory(buf)) {
+		error_code = READ_GITFILE_ERR_NOT_A_REPO;
+		goto cleanup_return;
+	}
+
+	strbuf_realpath(&realpath, buf, 1);
+
+cleanup_return:
+	if (return_error_code)
+		*return_error_code = error_code;
+	else if (error_code)
+		read_gitfile_error_die(error_code, path);
+
+	free(buf);
+	return error_code ? NULL : realpath.buf;
+}
+
+const char *read_gitfile_raw(const char *path, int *return_error_code)
+{
+	const int max_file_size = 1 << 20;  /* 1MB */
+	int error_code = 0;
+	char *buf = NULL;
 	struct stat st;
 	int fd;
 	ssize_t len;
-	static struct strbuf realpath = STRBUF_INIT;
+	static struct strbuf contents = STRBUF_INIT;
 
 	if (stat(path, &st)) {
 		if (errno == ENOENT || errno == ENOTDIR)

@@ -1014,32 +1047,13 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NO_PATH;
 		goto cleanup_return;
 	}
-	buf[len] = '\0';
-	dir = buf + 8;
-
-	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
-		size_t pathlen = slash+1 - path;
-		dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
-			      (int)(len - 8), buf + 8);
-		free(buf);
-		buf = dir;
-	}
-	if (!is_git_directory(dir)) {
-		error_code = READ_GITFILE_ERR_NOT_A_REPO;
-		goto cleanup_return;
-	}
-
-	strbuf_realpath(&realpath, dir, 1);
-	path = realpath.buf;
+	strbuf_reset(&contents);
+	strbuf_add(&contents, buf+8, len-8);
 
 cleanup_return:
-	if (return_error_code)
-		*return_error_code = error_code;
-	else if (error_code)
-		read_gitfile_error_die(error_code, path);
-
+	*return_error_code = error_code;
 	free(buf);
-	return error_code ? NULL : path;
+	return error_code ? NULL : contents.buf;
 }
 
 static void apply_gitdir_and_environment(struct repository *repo, const char *path)

index 654f10e059..442acd8954 100644
@@ -40,6 +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);
 #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)

index cbf95328a3..48e77636cf 100644
@@ -667,7 +667,7 @@ static void repair_gitfile(struct worktree *wt,
 	strbuf_realpath(&repo, path, 1);
 	strbuf_addf(&dotgit, "%s/.git", wt->path);
 	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
-	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	dotgit_contents = xstrdup_or_null(read_gitfile_raw(dotgit.buf, &err));
 
 	if (dotgit_contents) {
 		if (is_absolute_path(dotgit_contents)) {

@@ -681,7 +681,7 @@ static void repair_gitfile(struct worktree *wt,
 	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 		err == READ_GITFILE_ERR_IS_A_DIR)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
-	else if (err)
+	else if (err || !is_git_directory(backlink.buf))
 		repair = _(".git file broken");
 	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");

@@ -857,14 +857,7 @@ void repair_worktree_at_path(struct repository *repo,
 	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
 	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
 	if (dotgit_contents) {
-		if (is_absolute_path(dotgit_contents)) {
-			strbuf_addstr(&backlink, dotgit_contents);
-		} else {
-			strbuf_addbuf(&backlink, &dotgit);
-			strbuf_strip_suffix(&backlink, ".git");
-			strbuf_addstr(&backlink, dotgit_contents);
-			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
-		}
+		strbuf_addstr(&backlink, dotgit_contents);
 	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0

gitgitgadget

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(-)

index af7601ff67..052c7d669b 100644
@@ -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)

index 4c2fcbbeda..7394473e95 100644
@@ -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)

From: Yoichi NAKAYAMA <[email protected]>

Given a state in which 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, setting 'worktree.useRelativePaths=true' and running 'git worktree repair' within the main worktree converts them to relative paths.

Conversely, given a state in which the cross-references are recorded using relative paths, one would expect that setting 'worktree.useRelativePaths=false' and running 'git worktree repair' would convert them to absolute paths. However, they remain as relative paths.

This is because we incorrectly use read_gitfile_gently(), which always returns an absolute path. To fix this, introduce read_gitfile_raw(), which is almost identical to read_gitfile_gently(), but skips checking the existence of the referenced repository and returns the path as-is from the .git file.

---
worktree repair: detect relative path in .git file correctly

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2205%2Fyoichi%2Fworktree-repair-relative-path-handling-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2205/yoichi/worktree-repair-relative-path-handling-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2205

Range-diff vs v2:

1: 5bcf19ef50 ! 1: 1cd25e315e worktree repair: detect relative path in .git file correctly

     @@ Metadata
       ## Commit message ##
          worktree repair: detect relative path in .git file correctly
      
     -    Given a state where 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,
     +    Given a state in which 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,
          setting 'worktree.useRelativePaths=true' and running 'git worktree
          repair' within the main worktree converts them to relative paths.
      
     -    On the other hand, given a state where the cross references are
     -    recorded using relative paths, one would expect (by symmetry) that
     -    setting 'worktree.useRelativePath=false' and running 'git worktree
     -    repair' would convert them to absolute paths. However, they remain as
     -    relative paths.
     +    Conversely, given a state in which the cross-references are recorded
     +    using relative paths, one would expect that setting
     +    'worktree.useRelativePaths=false' and running 'git worktree repair'
     +    would convert them to absolute paths. However, they remain as relative
     +    paths.
      
     -    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 because we incorrectly use read_gitfile_gently(), which always
     +    returns an absolute path. To fix this, introduce read_gitfile_raw(),
     +    which is almost identical to read_gitfile_gently(), but skips checking
     +    the existence of the referenced repository and returns the path as-is
     +    from the .git file.
      
          Signed-off-by: Yoichi NAKAYAMA <[email protected]>
      
       ## setup.c ##
      @@ setup.c: void read_gitfile_error_die(int error_code, const char *path)
     +  * cases).
        */
       const char *read_gitfile_gently(const char *path, int *return_error_code)
     - {
     --	const int max_file_size = 1 << 20;  /* 1MB */
     - 	int error_code = 0;
     - 	char *buf = NULL;
     --	char *dir = NULL;
     - 	const char *slash;
     ++{
     ++	int error_code = 0;
     ++	const char *slash;
     ++	struct strbuf contents = STRBUF_INIT;
      +	static struct strbuf realpath = STRBUF_INIT;
      +
     -+	buf = xstrdup_or_null(read_gitfile_raw(path, &error_code));
     ++	error_code = read_gitfile_raw(&contents, path);
      +	if (error_code)
      +		goto cleanup_return;
      +
     -+	if (!is_absolute_path(buf) && (slash = strrchr(path, '/'))) {
     ++	if (!is_absolute_path(contents.buf) && (slash = strrchr(path, '/'))) {
      +		size_t pathlen = slash+1 - path;
     -+		char *dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
     -+				    (int)strlen(buf), buf);
     -+		free(buf);
     -+		buf = dir;
     ++		char *dir = xstrfmt("%.*s%s", (int)pathlen, path, contents.buf);
     ++		strbuf_reset(&contents);
     ++		strbuf_addstr(&contents, dir);
     ++		free(dir);
      +	}
     -+	if (!is_git_directory(buf)) {
     ++	if (!is_git_directory(contents.buf)) {
      +		error_code = READ_GITFILE_ERR_NOT_A_REPO;
      +		goto cleanup_return;
      +	}
      +
     -+	strbuf_realpath(&realpath, buf, 1);
     ++	strbuf_realpath(&realpath, contents.buf, 1);
      +
      +cleanup_return:
      +	if (return_error_code)
     @@ setup.c: void read_gitfile_error_die(int error_code, const char *path)
      +	else if (error_code)
      +		read_gitfile_error_die(error_code, path);
      +
     -+	free(buf);
     ++	strbuf_release(&contents);
      +	return error_code ? NULL : realpath.buf;
      +}
      +
     -+const char *read_gitfile_raw(const char *path, int *return_error_code)
     -+{
     -+	const int max_file_size = 1 << 20;  /* 1MB */
     -+	int error_code = 0;
     -+	char *buf = NULL;
     ++int read_gitfile_raw(struct strbuf *contents, const char *path)
     + {
     + 	const int max_file_size = 1 << 20;  /* 1MB */
     + 	int error_code = 0;
     + 	char *buf = NULL;
     +-	char *dir = NULL;
     +-	const char *slash;
       	struct stat st;
       	int fd;
       	ssize_t len;
      -	static struct strbuf realpath = STRBUF_INIT;
     -+	static struct strbuf contents = STRBUF_INIT;
       
       	if (stat(path, &st)) {
       		if (errno == ENOENT || errno == ENOTDIR)
     @@ setup.c: const char *read_gitfile_gently(const char *path, int *return_error_cod
      -
      -	strbuf_realpath(&realpath, dir, 1);
      -	path = realpath.buf;
     -+	strbuf_reset(&contents);
     -+	strbuf_add(&contents, buf+8, len-8);
     ++	strbuf_add(contents, buf+8, len-8);
       
       cleanup_return:
      -	if (return_error_code)
     @@ setup.c: const char *read_gitfile_gently(const char *path, int *return_error_cod
      -	else if (error_code)
      -		read_gitfile_error_die(error_code, path);
      -
     -+	*return_error_code = error_code;
       	free(buf);
      -	return error_code ? NULL : path;
     -+	return error_code ? NULL : contents.buf;
     ++	return error_code;
       }
       
       static void apply_gitdir_and_environment(struct repository *repo, const char *path)
     @@ setup.h: 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)
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair worktree with relative p
       test_done
      
       ## worktree.c ##
     +@@ worktree.c: static void repair_gitfile(struct worktree *wt,
     + 	struct strbuf gitdir = STRBUF_INIT;
     + 	struct strbuf repo = STRBUF_INIT;
     + 	struct strbuf backlink = STRBUF_INIT;
     +-	char *dotgit_contents = NULL;
     ++	struct strbuf contents = STRBUF_INIT;
     ++	const char *dotgit_contents = NULL;
     + 	const char *repair = NULL;
     + 	char *path = NULL;
     + 	int err;
      @@ worktree.c: static void repair_gitfile(struct worktree *wt,
       	strbuf_realpath(&repo, path, 1);
       	strbuf_addf(&dotgit, "%s/.git", wt->path);
       	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
      -	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
     -+	dotgit_contents = xstrdup_or_null(read_gitfile_raw(dotgit.buf, &err));
     ++	err = read_gitfile_raw(&contents, dotgit.buf);
     ++	if (!err)
     ++		dotgit_contents = contents.buf;
       
       	if (dotgit_contents) {
       		if (is_absolute_path(dotgit_contents)) {
     @@ worktree.c: static void repair_gitfile(struct worktree *wt,
       		repair = _(".git file broken");
       	else if (fspathcmp(backlink.buf, repo.buf))
       		repair = _(".git file incorrect");
     +@@ worktree.c: static void repair_gitfile(struct worktree *wt,
     + 	}
     + 
     + done:
     +-	free(dotgit_contents);
     + 	free(path);
     + 	strbuf_release(&repo);
     + 	strbuf_release(&dotgit);
     + 	strbuf_release(&gitdir);
     + 	strbuf_release(&backlink);
     ++	strbuf_release(&contents);
     + }
     + 
     + static void repair_noop(int iserr UNUSED,
      @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
       	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
       	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));

 setup.c                    | 63 ++++++++++++++++++++++----------------
 setup.h                    |  1 +
 t/t2406-worktree-repair.sh | 54 ++++++++++++++++++++++++--------
 worktree.c                 | 20 +++++-------
 4 files changed, 88 insertions(+), 50 deletions(-)

index 95909e9603..9041827336 100644
@@ -962,16 +962,48 @@ void read_gitfile_error_die(int error_code, const char *path)
  * cases).
  */
 const char *read_gitfile_gently(const char *path, int *return_error_code)
+{
+	int error_code = 0;
+	const char *slash;
+	struct strbuf contents = STRBUF_INIT;
+	static struct strbuf realpath = STRBUF_INIT;
+
+	error_code = read_gitfile_raw(&contents, path);
+	if (error_code)
+		goto cleanup_return;
+
+	if (!is_absolute_path(contents.buf) && (slash = strrchr(path, '/'))) {
+		size_t pathlen = slash+1 - path;
+		char *dir = xstrfmt("%.*s%s", (int)pathlen, path, contents.buf);
+		strbuf_reset(&contents);
+		strbuf_addstr(&contents, dir);
+		free(dir);
+	}
+	if (!is_git_directory(contents.buf)) {
+		error_code = READ_GITFILE_ERR_NOT_A_REPO;
+		goto cleanup_return;
+	}
+
+	strbuf_realpath(&realpath, contents.buf, 1);
+
+cleanup_return:
+	if (return_error_code)
+		*return_error_code = error_code;
+	else if (error_code)
+		read_gitfile_error_die(error_code, path);
+
+	strbuf_release(&contents);
+	return error_code ? NULL : realpath.buf;
+}
+
+int read_gitfile_raw(struct strbuf *contents, const char *path)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
 	char *buf = NULL;
-	char *dir = NULL;
-	const char *slash;
 	struct stat st;
 	int fd;
 	ssize_t len;
-	static struct strbuf realpath = STRBUF_INIT;
 
 	if (stat(path, &st)) {
 		if (errno == ENOENT || errno == ENOTDIR)

@@ -1014,32 +1046,11 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NO_PATH;
 		goto cleanup_return;
 	}
-	buf[len] = '\0';
-	dir = buf + 8;
-
-	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
-		size_t pathlen = slash+1 - path;
-		dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
-			      (int)(len - 8), buf + 8);
-		free(buf);
-		buf = dir;
-	}
-	if (!is_git_directory(dir)) {
-		error_code = READ_GITFILE_ERR_NOT_A_REPO;
-		goto cleanup_return;
-	}
-
-	strbuf_realpath(&realpath, dir, 1);
-	path = realpath.buf;
+	strbuf_add(contents, buf+8, len-8);
 
 cleanup_return:
-	if (return_error_code)
-		*return_error_code = error_code;
-	else if (error_code)
-		read_gitfile_error_die(error_code, path);
-
 	free(buf);
-	return error_code ? NULL : path;
+	return error_code;
 }
 
 static void apply_gitdir_and_environment(struct repository *repo, const char *path)

index 654f10e059..e6e71bda3d 100644
@@ -40,6 +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);
+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)

index cbf95328a3..8cb8637b18 100644
@@ -649,7 +649,8 @@ static void repair_gitfile(struct worktree *wt,
 	struct strbuf gitdir = STRBUF_INIT;
 	struct strbuf repo = STRBUF_INIT;
 	struct strbuf backlink = STRBUF_INIT;
-	char *dotgit_contents = NULL;
+	struct strbuf contents = STRBUF_INIT;
+	const char *dotgit_contents = NULL;
 	const char *repair = NULL;
 	char *path = NULL;
 	int err;

@@ -667,7 +668,9 @@ static void repair_gitfile(struct worktree *wt,
 	strbuf_realpath(&repo, path, 1);
 	strbuf_addf(&dotgit, "%s/.git", wt->path);
 	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
-	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	err = read_gitfile_raw(&contents, dotgit.buf);
+	if (!err)
+		dotgit_contents = contents.buf;
 
 	if (dotgit_contents) {
 		if (is_absolute_path(dotgit_contents)) {

@@ -681,7 +684,7 @@ static void repair_gitfile(struct worktree *wt,
 	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 		err == READ_GITFILE_ERR_IS_A_DIR)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
-	else if (err)
+	else if (err || !is_git_directory(backlink.buf))
 		repair = _(".git file broken");
 	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");

@@ -695,12 +698,12 @@ static void repair_gitfile(struct worktree *wt,
 	}
 
 done:
-	free(dotgit_contents);
 	free(path);
 	strbuf_release(&repo);
 	strbuf_release(&dotgit);
 	strbuf_release(&gitdir);
 	strbuf_release(&backlink);
+	strbuf_release(&contents);
 }
 
 static void repair_noop(int iserr UNUSED,

@@ -857,14 +860,7 @@ void repair_worktree_at_path(struct repository *repo,
 	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
 	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
 	if (dotgit_contents) {
-		if (is_absolute_path(dotgit_contents)) {
-			strbuf_addstr(&backlink, dotgit_contents);
-		} else {
-			strbuf_addbuf(&backlink, &dotgit);
-			strbuf_strip_suffix(&backlink, ".git");
-			strbuf_addstr(&backlink, dotgit_contents);
-			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
-		}
+		strbuf_addstr(&backlink, dotgit_contents);
 	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0

gitgitgadget

Excellent observation of the problem addressed by the patch. I wish everybody wrote his or her proposed log message this clearly.

Actually, I take half of that back. If we pretend the leading part of the "path", which could be absolute, the result will lose the relative-ness of the original. Keeping the tweaking of the relative path in read_gitfile_gently() is reasonable. As is_git_directory() needs to be called on a usable path, if the relative path tweaking cannot be done inside read_gitfile_raw(), it cannot check if the directory is is_git_directory(), either.

So, the change to setup.c is fine as is. I didn't look at the changes to worktree.c, though.

Thanks.

If we were to keep the call to is_git_directory() inside read_gitfile_raw(), it is necessary to calculate the absolute path of the candidate. While it is possible to calculate the path in read_gitfile_raw(), call is_git_directory(), and then discard the calculated path,
I felt it was wasteful to calculate the absolute path twice when read_gitfile_raw() is called from read_gitfile_gently().

From another perspective, while the function name read_gitfile_*() suggests its role is simply to read the .git file, I felt that verifying whether the resulting path is a valid git directory went beyond that scope.

I understand the desire to minimize the functional differences between read_gitfile_raw() and read_gitfile_gently(), but for the reasons mentioned above, I have moved the check performed by is_git_directory() to the caller of read_gitfile_raw() within worktree.c.

I have moved the is_git_directory() call to worktree.c so as not to alter the behavior when a .git file points to a location other than any git directory, but I didn't mention it in the commit message.

Are you concerned about the lack of explanation in the commit message, or about the functional differences between read_gitfile_raw() and read_gitfile_gently()?

Thanks,

Yoichi NAKAYAMA

Mostly the former, i.e., the commit message too sketchy.

I now understand that these two functions need to be more different than just the _raw() not calling strbuf_realpath() and the other calling strbuf_realpath(). The functional split is fine.

Thanks.

From: Yoichi NAKAYAMA <[email protected]>

Given a state in which 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, setting 'worktree.useRelativePaths=true' and running 'git worktree repair' within the main worktree converts them to relative paths.

Conversely, given a state in which the cross-references are recorded using relative paths, one would expect that setting 'worktree.useRelativePaths=false' and running 'git worktree repair' would convert them to absolute paths. However, they remain as relative paths.

This is because we incorrectly use read_gitfile_gently(), which always returns an absolute path. To fix this, introduce read_gitfile_raw(), which reads the path from the .git file without resolving it to an absolute path.

Because read_gitfile_raw() does not validate the path with is_git_directory(), repair_gitfile() performs this validation to preserve the existing behavior.

---
worktree repair: detect relative path in .git file correctly

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2205%2Fyoichi%2Fworktree-repair-relative-path-handling-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2205/yoichi/worktree-repair-relative-path-handling-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2205

Range-diff vs v3:

1: 1cd25e315e ! 1: abff315880 worktree repair: detect relative path in .git file correctly
@@ Commit message

This is because we incorrectly use read_gitfile_gently(), which always returns an absolute path. To fix this, introduce read_gitfile_raw(),
- which is almost identical to read_gitfile_gently(), but skips checking
- the existence of the referenced repository and returns the path as-is
- from the .git file.
+ which reads the path from the .git file without resolving it to an
+ absolute path.
+
+ Because read_gitfile_raw() does not validate the path with
+ is_git_directory(), repair_gitfile() performs this validation to
+ preserve the existing behavior.

@@ setup.c: void read_gitfile_error_die(int error_code, const char *path)
+ return error_code ? NULL : realpath.buf;
+}
+
++/*
++ * Read the path following "gitdir: " from the .git file into strbuf. ++ *
++ * Unlike read_gitfile_gently(), this function does not resolve a ++ * relative path or validate it using is_git_directory(). ++ */
+int read_gitfile_raw(struct strbuf *contents, const char *path)
{
const int max_file_size = 1 << 20; /* 1MB */

 setup.c                    | 69 ++++++++++++++++++++++++--------------
 setup.h                    |  1 +
 t/t2406-worktree-repair.sh | 54 ++++++++++++++++++++++-------
 worktree.c                 | 20 +++++------
 4 files changed, 94 insertions(+), 50 deletions(-)

index 95909e9603..d6a00cdbb4 100644
@@ -962,16 +962,54 @@ void read_gitfile_error_die(int error_code, const char *path)
  * cases).
  */
 const char *read_gitfile_gently(const char *path, int *return_error_code)
+{
+	int error_code = 0;
+	const char *slash;
+	struct strbuf contents = STRBUF_INIT;
+	static struct strbuf realpath = STRBUF_INIT;
+
+	error_code = read_gitfile_raw(&contents, path);
+	if (error_code)
+		goto cleanup_return;
+
+	if (!is_absolute_path(contents.buf) && (slash = strrchr(path, '/'))) {
+		size_t pathlen = slash+1 - path;
+		char *dir = xstrfmt("%.*s%s", (int)pathlen, path, contents.buf);
+		strbuf_reset(&contents);
+		strbuf_addstr(&contents, dir);
+		free(dir);
+	}
+	if (!is_git_directory(contents.buf)) {
+		error_code = READ_GITFILE_ERR_NOT_A_REPO;
+		goto cleanup_return;
+	}
+
+	strbuf_realpath(&realpath, contents.buf, 1);
+
+cleanup_return:
+	if (return_error_code)
+		*return_error_code = error_code;
+	else if (error_code)
+		read_gitfile_error_die(error_code, path);
+
+	strbuf_release(&contents);
+	return error_code ? NULL : realpath.buf;
+}
+
+/*
+ * Read the path following "gitdir: " from the .git file into strbuf.
+ *
+ * Unlike read_gitfile_gently(), this function does not resolve a
+ * relative path or validate it using is_git_directory().
+ */
+int read_gitfile_raw(struct strbuf *contents, const char *path)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
 	char *buf = NULL;
-	char *dir = NULL;
-	const char *slash;
 	struct stat st;
 	int fd;
 	ssize_t len;
-	static struct strbuf realpath = STRBUF_INIT;
 
 	if (stat(path, &st)) {
 		if (errno == ENOENT || errno == ENOTDIR)

@@ -1014,32 +1052,11 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NO_PATH;
 		goto cleanup_return;
 	}
-	buf[len] = '\0';
-	dir = buf + 8;
-
-	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
-		size_t pathlen = slash+1 - path;
-		dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
-			      (int)(len - 8), buf + 8);
-		free(buf);
-		buf = dir;
-	}
-	if (!is_git_directory(dir)) {
-		error_code = READ_GITFILE_ERR_NOT_A_REPO;
-		goto cleanup_return;
-	}
-
-	strbuf_realpath(&realpath, dir, 1);
-	path = realpath.buf;
+	strbuf_add(contents, buf+8, len-8);
 
 cleanup_return:
-	if (return_error_code)
-		*return_error_code = error_code;
-	else if (error_code)
-		read_gitfile_error_die(error_code, path);
-
 	free(buf);
-	return error_code ? NULL : path;
+	return error_code;
 }
 
 static void apply_gitdir_and_environment(struct repository *repo, const char *path)

index 654f10e059..e6e71bda3d 100644
@@ -40,6 +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);
+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)

index cbf95328a3..8cb8637b18 100644
@@ -649,7 +649,8 @@ static void repair_gitfile(struct worktree *wt,
 	struct strbuf gitdir = STRBUF_INIT;
 	struct strbuf repo = STRBUF_INIT;
 	struct strbuf backlink = STRBUF_INIT;
-	char *dotgit_contents = NULL;
+	struct strbuf contents = STRBUF_INIT;
+	const char *dotgit_contents = NULL;
 	const char *repair = NULL;
 	char *path = NULL;
 	int err;

@@ -667,7 +668,9 @@ static void repair_gitfile(struct worktree *wt,
 	strbuf_realpath(&repo, path, 1);
 	strbuf_addf(&dotgit, "%s/.git", wt->path);
 	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
-	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	err = read_gitfile_raw(&contents, dotgit.buf);
+	if (!err)
+		dotgit_contents = contents.buf;
 
 	if (dotgit_contents) {
 		if (is_absolute_path(dotgit_contents)) {

@@ -681,7 +684,7 @@ static void repair_gitfile(struct worktree *wt,
 	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 		err == READ_GITFILE_ERR_IS_A_DIR)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
-	else if (err)
+	else if (err || !is_git_directory(backlink.buf))
 		repair = _(".git file broken");
 	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");

@@ -695,12 +698,12 @@ static void repair_gitfile(struct worktree *wt,
 	}
 
 done:
-	free(dotgit_contents);
 	free(path);
 	strbuf_release(&repo);
 	strbuf_release(&dotgit);
 	strbuf_release(&gitdir);
 	strbuf_release(&backlink);
+	strbuf_release(&contents);
 }
 
 static void repair_noop(int iserr UNUSED,

@@ -857,14 +860,7 @@ void repair_worktree_at_path(struct repository *repo,
 	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
 	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
 	if (dotgit_contents) {
-		if (is_absolute_path(dotgit_contents)) {
-			strbuf_addstr(&backlink, dotgit_contents);
-		} else {
-			strbuf_addbuf(&backlink, &dotgit);
-			strbuf_strip_suffix(&backlink, ".git");
-			strbuf_addstr(&backlink, dotgit_contents);
-			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
-		}
+		strbuf_addstr(&backlink, dotgit_contents);
 	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
 			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0

gitgitgadget

This iteration looks very good to me. Will mark for 'next' unless there are comments that say otherwise in the next few days.

Thanks.