[PATCH] worktree repair: detect relative path in .git file correctly

[PATCH] worktree repair: detect relative path in .git file correctly

[Yoichi NAKAYAMA via GitGitGadget]

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.

Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
---
    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(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index e96b1283b7..2369fcea1b 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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;
 
diff --git a/setup.c b/setup.c
index 95909e9603..febb3248a5 100644
--- a/setup.c
+++ b/setup.c
@@ -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 */
diff --git a/setup.h b/setup.h
index 654f10e059..018893b1d7 100644
--- a/setup.h
+++ b/setup.h
@@ -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)
 
diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
index f5f19b3169..5f241c9878 100755
--- a/t/t2406-worktree-repair.sh
+++ b/t/t2406-worktree-repair.sh
@@ -228,30 +228,60 @@ test_expect_success 'repair worktree with relative path with missing gitfile' '
 	test_cmp expect wt/.git
 '
 
-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_when_finished "rm -rf main side" &&
 	test_create_repo main &&
 	test_commit -C main init &&
 	git -C main worktree add --detach ../side &&
-	echo "../../../../sidemoved/.git" >expect-gitdir &&
+	echo "../../../../side/.git" >expect-gitdir &&
 	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 &&
+	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 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_when_finished "rm -rf main side" &&
 	test_create_repo main &&
 	test_commit -C main init &&
 	git -C main worktree add --relative-paths --detach ../side &&
-	echo "$(pwd)/sidemoved/.git" >expect-gitdir &&
+	echo "$(pwd)/side/.git" >expect-gitdir &&
 	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 &&
+	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_when_finished "rm -rf main side" &&
+	test_create_repo main &&
+	git -C main config worktree.useRelativePaths false &&
+	test_commit -C main init &&
+	git -C main worktree add --detach ../side &&
+	echo "../../../../side/.git" >expect-gitdir &&
+	echo "gitdir: ../main/.git/worktrees/side" >expect-gitfile &&
+	git -C main config worktree.useRelativePaths true &&
+	git -C main worktree repair 2>main/err &&
+	test_grep ".git file absolute/relative path mismatch" main/err &&
+	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
+	test_cmp expect-gitfile side/.git
+'
+
+test_expect_success 'repair relative to absolute in main worktree' '
+	test_when_finished "rm -rf main side" &&
+	test_create_repo main &&
+	git -C main config worktree.useRelativePaths true &&
+	test_commit -C main init &&
+	git -C main worktree add --detach ../side &&
+	echo "$(pwd)/side/.git" >expect-gitdir &&
+	echo "gitdir: $(pwd)/main/.git/worktrees/side" >expect-gitfile &&
+	git -C main config worktree.useRelativePaths false &&
+	git -C main worktree repair 2>main/err &&
+	test_grep ".git file absolute/relative path mismatch" main/err &&
+	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
+	test_cmp expect-gitfile side/.git
 '
 
 test_done
diff --git a/worktree.c b/worktree.c
index cbf95328a3..86b599ed27 100644
--- a/worktree.c
+++ b/worktree.c
@@ -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

Re: [PATCH] worktree repair: detect relative path in .git file correctly

[Junio C Hamano]

"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.