[PATCH v2 0/4] Link worktrees with relative paths

[PATCH v2 0/4] Link worktrees with relative paths

[Caleb White]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

This is a resubmission attempt to try to fix the damaged
patch noted in previous discussions.

Thanks!

Caleb White (4):
  worktree: refactor infer_backlink() to use *strbuf
  worktree: link worktrees with relative paths
  worktree: sync worktree paths after gitdir move
  worktree: prevent null pointer dereference

 builtin/worktree.c           |  17 +--
 setup.c                      |   2 +-
 t/t2408-worktree-relative.sh |  39 ++++++
 worktree.c                   | 235 +++++++++++++++++++++++++++--------
 worktree.h                   |  10 ++
 5 files changed, 240 insertions(+), 63 deletions(-)
 create mode 100755 t/t2408-worktree-relative.sh

-- 
2.46.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

[PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1: Type: text/plain, Size: 3498 bytes --]

This refactors the `infer_backlink` function to return an integer
result and use a pre-allocated `strbuf` for the inferred backlink
path, replacing the previous `char*` return type.

This lays the groundwork for the next patch, which needs the
resultant backlink as a `strbuf`. There was no need to go from
`strbuf -> char* -> strbuf` again. This change also aligns the
function's signature with other `strbuf`-based functions.

Signed-off-by: Caleb White <cdwhite3@pm.me>
---
 worktree.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/worktree.c b/worktree.c
index 0f032cc..c6d2ede 100644
--- a/worktree.c
+++ b/worktree.c
@@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)
  * be able to infer the gitdir by manually reading /path/to/worktree/.git,
  * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
  */
-static char *infer_backlink(const char *gitfile)
+static int infer_backlink(st
ruct strbuf *inferred, const char *gitfile)
 {
 	struct strbuf actual = STRBUF_INIT;
-	struct strbuf inferred = STRBUF_INIT;
 	const char *id;
 
 	if (strbuf_read_file(&actual, gitfile, 0) < 0)
@@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile)
 	id++; /* advance past '/' to point at <id> */
 	if (!*id)
 		goto error;
-	strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
-	if (!is_directory(inferred.buf))
+	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
+	if (!is_directory(inferred->buf))
 		goto error;
 
 	strbuf_release(&actual);
-	return strbuf_detach(&inferred, NULL);
+	return 0;
 
 error:
 	strbuf_release(&actual);
-	strbuf_release(&inferred);
-	return NULL;
+	return 1;
 }
 
 /*
@@ -680,9 +678,10 @@ void repair_worktree_at_path(const char *path,
 {
 	struct strbuf dotgit = STRBUF_INIT;
 	struct strbuf realdotgit = STRBUF_INIT;
+	struct strbuf backlink = STRBUF_INIT;
 	struct strbuf gitd
ir = STRBUF_INIT;
 	struct strbuf olddotgit = STRBUF_INIT;
-	char *backlink = NULL;
+	char *git_contents = NULL;
 	const char *repair = NULL;
 	int err;
 
@@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path,
 		goto done;
 	}
 
-	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
+	git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
 	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
 		fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
 		goto done;
 	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
-		if (!(backlink = infer_backlink(realdotgit.buf))) {
+		if (infer_backlink(&backlink, realdotgit.buf)) {
 			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
 			goto done;
 		}
 	} else if (err) {
 		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
 		goto done;
+	} else if (git_conte
nts) {
+		strbuf_addstr(&backlink, git_contents);
 	}
 
-	strbuf_addf(&gitdir, "%s/gitdir", backlink);
+	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
 	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
 		repair = _("gitdir unreadable");
 	else {
@@ -726,8 +727,9 @@ void repair_worktree_at_path(const char *path,
 		write_file(gitdir.buf, "%s", realdotgit.buf);
 	}
 done:
-	free(backlink);
+	free(git_contents);
 	strbuf_release(&olddotgit);
+	strbuf_release(&backlink);
 	strbuf_release(&gitdir);
 	strbuf_release(&realdotgit);
 	strbuf_release(&dotgit);
-- 
2.46.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

[PATCH v2 2/4] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1: Type: text/plain, Size: 13128 bytes --]

This modifies Git’s handling of worktree linking to use relative
paths instead of absolute paths. Previously, when creating a worktree,
Git would store the absolute paths to both the main repository and the
linked worktrees. These hardcoded absolute paths cause breakages such
as when the repository is moved to a different directory or during
containerized development where the absolute differs between systems.

By switching to relative paths, we help ensure that worktree links are
more resilient when the repository is moved. While links external to the
repository may still break, Git still automatically handles many common
scenarios, reducing the need for manual repair. This is particularly
useful in containerized or portable development environments, where the
absolute path to the repository can differ between systems. Developers
no longer need to reinitialize or repair worktrees after relocating the
repository, improving workflow efficiency and reducing breakages.

For self-contained repositories (such as using a bare repository with
worktrees), where both the repository and its worktrees are located
within the same directory structure, using relative paths guarantees all
links remain functional regardless of where the directory is located.

Signed-off-by: Caleb White <cdwhite3@pm.me>
---
 builtin/worktree.c           |  17 ++--
 t/t2408-worktree-relative.sh |  39 +++++++++
 worktree.c                   | 152 +++++++++++++++++++++++++----------
 3 files changed, 159 insertions(+), 49 deletions(-)
 create mode 100755 t/t2408-worktree-relative.sh

diff --git a/builtin/worktree.c b/builtin/worktree.c
index fc31d07..99cee56 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname,
 			const struct add_opts *opts)
 {
 	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
-	struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT;
+	struct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT;
 	const char *name;
 	struct strvec child_env = STRVEC_INIT;
 	unsigned int counter = 0;
@@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname,
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
-	strbuf_realpath(&realpath, sb_git.buf, 1);
-	write_file(sb.buf, "%s", realpath.buf);
-	strbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1);
-	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
-		   realpath.buf, name);
+	strbuf_realpath(&sb_path_realpath, path, 1);
+	strbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1);
+	write_file(sb.buf, "%s/.git", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp));
+	strbuf_reset(&sb_tmp);
+	write_file(sb_git.buf, "gitdir: %s", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
@@ -578,11 +579,13 @@ static int add_worktree(const char *path, const char *refname,
 
 	strvec_clear(&child_env);
 	strbuf_release(&sb);
+	strbuf_release(&sb_tmp);
 	strbuf_release(&symref);
 	strbuf_release(&sb_repo);
+	strbuf_release(&sb_repo_realpath);
 	strbuf_release(&sb_git);
+	strbuf_release(&sb_path_realpath);
 	strbuf_release(&sb_name);
-	strbuf_release(&realpath);
 	free_worktree(wt);
 	return ret;
 }
diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh
new file mode 100755
index 0000000..a3136db
--- /dev/null
+++ b/t/t2408-worktree-relative.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='test worktrees linked with relative paths'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'links worktrees with relative paths' '
+	test_when_finished rm -rf repo &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit initial &&
+		git worktree add wt1 &&
+		echo "../../../wt1/.git" >expected_gitdir &&
+		cat .git/worktrees/wt1/gitdir >actual_gitdir &&
+		echo "gitdir: ../.git/worktrees/wt1" >expected_git &&
+		cat wt1/.git >actual_git &&
+		test_cmp expected_gitdir actual_gitdir &&
+		test_cmp expected_git actual_git
+	)
+'
+
+test_expect_success 'move repo without breaking relative internal links' '
+	test_when_finished rm -rf repo moved &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit initial &&
+		git worktree add wt1 &&
+		cd .. &&
+		mv repo moved &&
+		cd moved/wt1 &&
+		git status >out 2>err &&
+		test_must_be_empty err
+	)
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index c6d2ede..fc14e9a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -110,6 +110,12 @@ struct worktree *get_linked_worktree(const char *id,
 	strbuf_rtrim(&worktree_path);
 	strbuf_strip_suffix(&worktree_path, "/.git");
 
+	if (!is_absolute_path(worktree_path.buf)) {
+	    strbuf_strip_suffix(&path, "gitdir");
+	    strbuf_addbuf(&path, &worktree_path);
+	    strbuf_realpath_forgiving(&worktree_path, path.buf, 0);
+	}
+
 	CALLOC_ARRAY(worktree, 1);
 	worktree->repo = the_repository;
 	worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 void update_worktree_location(struct worktree *wt, const char *path_)
 {
 	struct strbuf path = STRBUF_INIT;
+	struct strbuf repo = STRBUF_INIT;
+	struct strbuf tmp = STRBUF_INIT;
+	char *file = NULL;
 
 	if (is_main_worktree(wt))
 		BUG("can't relocate main worktree");
 
+	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
 	strbuf_realpath(&path, path_, 1);
 	if (fspathcmp(wt->path, path.buf)) {
-		write_file(git_common_path("worktrees/%s/gitdir", wt->id),
-			   "%s/.git", path.buf);
+		file = xstrfmt("%s/gitdir", repo.buf);
+		write_file(file, "%s/.git", relative_path(path.buf, repo.buf, &tmp));
+		free(file);
+		strbuf_reset(&tmp);
+		file = xstrfmt("%s/.git", path.buf);
+		write_file(file, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
+
 		free(wt->path);
 		wt->path = strbuf_detach(&path, NULL);
 	}
+	free(file);
 	strbuf_release(&path);
+	strbuf_release(&repo);
+	strbuf_release(&tmp);
 }
 
 int is_worktree_being_rebased(const struct worktree *wt,
@@ -564,38 +582,52 @@ static void repair_gitfile(struct worktree *wt,
 {
 	struct strbuf dotgit = STRBUF_INIT;
 	struct strbuf repo = STRBUF_INIT;
-	char *backlink;
+	struct strbuf backlink = STRBUF_INIT;
+	struct strbuf tmp = STRBUF_INIT;
+	char *git_contents = NULL;
 	const char *repair = NULL;
 	int err;
 
 	/* missing worktree can't be repaired */
 	if (!file_exists(wt->path))
-		return;
+		goto done;
 
 	if (!is_directory(wt->path)) {
 		fn(1, wt->path, _("not a directory"), cb_data);
-		return;
+		goto done;
 	}
 
 	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
 	strbuf_addf(&dotgit, "%s/.git", wt->path);
-	backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+
+	if (git_contents) {
+		if (is_absolute_path(git_contents)) {
+			strbuf_addstr(&backlink, git_contents);
+		} else {
+			strbuf_addf(&backlink, "%s/%s", wt->path, git_contents);
+			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
+		}
+	}
 
 	if (err == READ_GITFILE_ERR_NOT_A_FILE)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
-	else if (fspathcmp(backlink, repo.buf))
+	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");
 
 	if (repair) {
 		fn(0, wt->path, repair, cb_data);
-		write_file(dotgit.buf, "gitdir: %s", repo.buf);
+		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
 	}
 
-	free(backlink);
+done:
+	free(git_contents);
 	strbuf_release(&repo);
 	strbuf_release(&dotgit);
+	strbuf_release(&backlink);
+	strbuf_release(&tmp);
 }
 
 static void repair_noop(int iserr UNUSED,
@@ -681,6 +713,8 @@ void repair_worktree_at_path(const char *path,
 	struct strbuf backlink = STRBUF_INIT;
 	struct strbuf gitdir = STRBUF_INIT;
 	struct strbuf olddotgit = STRBUF_INIT;
+	struct strbuf realolddotgit = STRBUF_INIT;
+	struct strbuf tmp = STRBUF_INIT;
 	char *git_contents = NULL;
 	const char *repair = NULL;
 	int err;
@@ -710,97 +744,131 @@ void repair_worktree_at_path(const char *path,
 		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
 		goto done;
 	} else if (git_contents) {
-		strbuf_addstr(&backlink, git_contents);
+		if (is_absolute_path(git_contents)) {
+			strbuf_addstr(&backlink, git_contents);
+		} else {
+			strbuf_addbuf(&backlink, &realdotgit);
+			strbuf_strip_suffix(&backlink, ".git");
+			strbuf_addstr(&backlink, git_contents);
+		}
 	}
 
+	strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
 	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
 	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
 		repair = _("gitdir unreadable");
 	else {
 		strbuf_rtrim(&olddotgit);
-		if (fspathcmp(olddotgit.buf, realdotgit.buf))
+		if (is_absolute_path(olddotgit.buf)) {
+			strbuf_addbuf(&realolddotgit, &olddotgit);
+		} else {
+			strbuf_addf(&realolddotgit, "%s/%s", backlink.buf, olddotgit.buf);
+			strbuf_realpath_forgiving(&realolddotgit, realolddotgit.buf, 0);
+		}
+		if (fspathcmp(realolddotgit.buf, realdotgit.buf))
 			repair = _("gitdir incorrect");
 	}
 
 	if (repair) {
 		fn(0, gitdir.buf, repair, cb_data);
-		write_file(gitdir.buf, "%s", realdotgit.buf);
+		write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
 	}
 done:
 	free(git_contents);
 	strbuf_release(&olddotgit);
+	strbuf_release(&realolddotgit);
 	strbuf_release(&backlink);
 	strbuf_release(&gitdir);
 	strbuf_release(&realdotgit);
 	strbuf_release(&dotgit);
+	strbuf_release(&tmp);
 }
 
 int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)
 {
 	struct stat st;
-	char *path;
+	struct strbuf dotgit = STRBUF_INIT;
+	struct strbuf gitdir = STRBUF_INIT;
+	struct strbuf repo = STRBUF_INIT;
+	char *path = NULL;
+	char *file = NULL;
+	int rc = 0;
 	int fd;
 	size_t len;
 	ssize_t read_result;
 
 	*wtpath = NULL;
-	if (!is_directory(git_path("worktrees/%s", id))) {
+	strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1);
+	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
+	if (!is_directory(repo.buf)) {
 		strbuf_addstr(reason, _("not a valid directory"));
-		return 1;
+		rc = 1;
+		goto done;
 	}
-	if (file_exists(git_path("worktrees/%s/locked", id)))
-		return 0;
-	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
+	file = xstrfmt("%s/locked", repo.buf);
+	if (file_exists(file)) {
+		goto done;
+	}
+	if (stat(gitdir.buf, &st)) {
 		strbuf_addstr(reason, _("gitdir file does not exist"));
-		return 1;
+		rc = 1;
+		goto done;
 	}
-	fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
+	fd = open(gitdir.buf, O_RDONLY);
 	if (fd < 0) {
 		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
 			    strerror(errno));
-		return 1;
+		rc = 1;
+		goto done;
 	}
 	len = xsize_t(st.st_size);
 	path = xmallocz(len);
 
 	read_result = read_in_full(fd, path, len);
+	close(fd);
 	if (read_result < 0) {
 		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
 			    strerror(errno));
-		close(fd);
-		free(path);
-		return 1;
-	}
-	close(fd);
-
-	if (read_result != len) {
+		rc = 1;
+		goto done;
+	} else if (read_result != len) {
 		strbuf_addf(reason,
 			    _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
 			    (uintmax_t)len, (uintmax_t)read_result);
-		free(path);
-		return 1;
+		rc = 1;
+		goto done;
 	}
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
 		strbuf_addstr(reason, _("invalid gitdir file"));
-		free(path);
-		return 1;
+		rc = 1;
+		goto done;
 	}
 	path[len] = '\0';
-	if (!file_exists(path)) {
-		if (stat(git_path("worktrees/%s/index", id), &st) ||
-		    st.st_mtime <= expire) {
+	if (is_absolute_path(path)) {
+		strbuf_addstr(&dotgit, path);
+	} else {
+		strbuf_addf(&dotgit, "%s/%s", repo.buf, path);
+		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
+	}
+	if (!file_exists(dotgit.buf)) {
+		free(file);
+		file = xstrfmt("%s/index", repo.buf);
+		if (stat(file, &st) || st.st_mtime <= expire) {
 			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
-			free(path);
-			return 1;
-		} else {
-			*wtpath = path;
-			return 0;
+			rc = 1;
+			goto done;
 		}
 	}
-	*wtpath = path;
-	return 0;
+	*wtpath = strbuf_detach(&dotgit, NULL);
+done:
+	free(path);
+	free(file);
+	strbuf_release(&dotgit);
+	strbuf_release(&gitdir);
+	strbuf_release(&repo);
+	return rc;
 }
 
 static int move_config_setting(const char *key, const char *value,
-- 
2.46.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

[PATCH v2 3/4] worktree: sync worktree paths after gitdir move

[Caleb White]

[-- Attachment #1: Type: text/plain, Size: 4200 bytes --]

When re-initializing a repository with a separate gitdir (the original
gitdir is moved to a new location), any linked worktrees become broken
and must be repaired to reflect the new gitdir location. For absolute
paths, this breakage is one-way, but is both ways for relative paths
(the `<worktree>/.git` and the `<repo>/worktrees/<id>/gitdir`).

Previously, `repair_worktrees` was being called which loops through all
the worktrees in the repository and updates the `<worktree>/.git` files
to point to the new gitdir. However, when both sides of the worktrees
are broken, the previous gitdir location is required to reestablish the
link.

To fix this, the function `repair_worktrees_after_gitdir_move` is
introduced. It takes the old gitdir path as an argument and repairs both
sides of the worktree.

This change fixes the following test cases in t0001-init.sh:
- re-init to move gitdir with linked worktrees
- re-init to move gitdir within linked worktree

Signed-off-by: Caleb 
White <cdwhite3@pm.me>
---
 setup.c    |  2 +-
 worktree.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 10 ++++++++++
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 94e79b2..7b648de 100644
--- a/setup.c
+++ b/setup.c
@@ -2420,7 +2420,7 @@ static void separate_git_dir(const char *git_dir, const char *git_link)
 
 		if (rename(src, git_dir))
 			die_errno(_("unable to move %s to %s"), src, git_dir);
-		repair_worktrees(NULL, NULL);
+		repair_worktrees_after_gitdir_move(src);
 	}
 
 	write_file(git_link, "gitdir: %s", git_dir);
diff --git a/worktree.c b/worktree.c
index fc14e9a..b08ecce 100644
--- a/worktree.c
+++ b/worktree.c
@@ -650,6 +650,60 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data)
 	free_worktrees(worktrees);
 }
 
+void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path)
+{
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf repo = STRBUF_
INIT;
+	struct strbuf gitdir = STRBUF_INIT;
+	struct strbuf dotgit = STRBUF_INIT;
+	struct strbuf olddotgit = STRBUF_INIT;
+	struct strbuf tmp = STRBUF_INIT;
+
+	if (is_main_worktree(wt))
+		goto done;
+
+	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
+	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
+
+	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
+		goto done;
+
+	strbuf_rtrim(&olddotgit);
+	if (is_absolute_path(olddotgit.buf)) {
+		strbuf_addbuf(&dotgit, &olddotgit);
+	} else {
+		strbuf_addf(&dotgit, "%s/worktrees/%s/%s", old_path, wt->id, olddotgit.buf);
+		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
+	}
+
+	if (!file_exists(dotgit.buf))
+		goto done;
+
+	strbuf_addbuf(&path, &dotgit);
+	strbuf_strip_suffix(&path, "/.git");
+
+	write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
+	strbuf_reset(&tmp);
+	write_file(gitdir.buf, "%s", relative_path(dotgit.buf, repo.buf, &tmp));
+done:
+	strbu
f_release(&path);
+	strbuf_release(&repo);
+	strbuf_release(&gitdir);
+	strbuf_release(&dotgit);
+	strbuf_release(&olddotgit);
+	strbuf_release(&tmp);
+}
+
+void repair_worktrees_after_gitdir_move(const char *old_path)
+{
+	struct worktree **worktrees = get_worktrees_internal(1);
+	struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
+
+	for (; *wt; wt++)
+		repair_worktree_after_gitdir_move(*wt, old_path);
+	free_worktrees(worktrees);
+}
+
 static int is_main_worktree_path(const char *path)
 {
 	struct strbuf target = STRBUF_INIT;
diff --git a/worktree.h b/worktree.h
index 11279d0..e961186 100644
--- a/worktree.h
+++ b/worktree.h
@@ -131,6 +131,16 @@ typedef void (* worktree_repair_fn)(int iserr, const char *path,
  */
 void repair_worktrees(worktree_repair_fn, void *cb_data);
 
+/*
+ * Repair the linked worktrees after the gitdir has been moved.
+ */
+void repair_worktrees_after_gitdir_move(const char *old_path);
+
+/*
+ * Repair the l
inked worktree after the gitdir has been moved.
+ */
+void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path);
+
 /*
  * Repair administrative files corresponding to the worktree at the given path.
  * The worktree's .git file pointing at the repository must be intact for the
-- 
2.46.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

[PATCH v2 4/4] worktree: prevent null pointer dereference

[Caleb White]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

If worktrees is NULL, free_worktrees() should return immediately to
prevent a null pointer dereference.

Signed-off-by: Caleb White <cdwhite3@pm.me>
---
 worktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/worktree.c b/worktree.c
index b08ecce..1cf15b0 100644
--- a/worktree.c
+++ b/worktree.c
@@ -28,8 +28,9 @@ void free_worktree(struct worktree *worktree)
 
 void free_worktrees(struct worktree **worktrees)
 {
-	int i = 0;
-	for (i = 0; worktrees[i]; i++)
+	if  (!worktrees)
+		return;
+	for (int i = 0; worktrees[i]; i++)
 		free_worktree(worktrees[i]);
 	free (worktrees);
 }
-- 
2.46.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 0/4] Link worktrees with relative paths

[Eric Sunshine]

On Sun, Oct 6, 2024 at 2:00 AM Caleb White <cdwhite3@pm.me> wrote:
> This is a resubmission attempt to try to fix the damaged
> patch noted in previous discussions.

For the record, for those finding this message in the mailing list
archive, v1 is at:
https://lore.kernel.org/git/20241006045847.159937-1-cdwhite3@pm.me/

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[Eric Sunshine]

On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:
> This modifies Git’s handling of worktree linking to use relative
> paths instead of absolute paths. Previously, when creating a worktree,
> Git would store the absolute paths to both the main repository and the
> linked worktrees. These hardcoded absolute paths cause breakages such
> as when the repository is moved to a different directory or during
> containerized development where the absolute differs between systems.
>
> By switching to relative paths, we help ensure that worktree links are
> more resilient when the repository is moved. While links external to the
> repository may still break, Git still automatically handles many common
> scenarios, reducing the need for manual repair. This is particularly
> useful in containerized or portable development environments, where the
> absolute path to the repository can differ between systems. Developers
> no longer need to reinitialize or repair worktrees after relocating the
> repository, improving workflow efficiency and reducing breakages.
>
> For self-contained repositories (such as using a bare repository with
> worktrees), where both the repository and its worktrees are located
> within the same directory structure, using relative paths guarantees all
> links remain functional regardless of where the directory is located.
>
> Signed-off-by: Caleb White <cdwhite3@pm.me>

When you reroll, please extend the commit message to give a more
detailed overview of how this patch actually changes the behavior both
at a high level and at a low level. This is especially important since
this patch is sufficiently long and involved that it's not easy to
glean these details at-a-glance from the code changes themselves.

For instance, the cover letter talked about retaining correct behavior
for both absolute and relative paths, but this commit message mentions
only relative paths, so it's not obvious just from reading the commit
message whether absolute paths are still supported or, if they are,
what exactly that support looks like or how the user controls the
choice between the two path types. This sort of information should be
present in the commit message.

Similarly, if there is a chance that this change may break existing
tooling and workflows, then the commit message should talk about those
risks, as well, and what if any remediations are available. Also, if
there are such risks, then Documentation/git-worktree.txt may need to
be updated to warn users.

Regarding what you wrote above, there seems to be a good deal of
redundancy between the first two paragraphs; combining the paragraphs
and folding out the duplication might make the message more
streamlined. I do like the discussion about containerized environments
being used as (at least one) justification for employing relative
paths, and think that may be a good lead-in for the commit message.
Please see [1] for some helpful hints for composing a good commit
message.

I'm not going to review the code itself right now since I haven't been
able to apply the patches or play around with the functionality, but I
wanted to get the above written up since I think this series is going
to need to be rerolled anyhow.

[1]: https://lore.kernel.org/git/xmqqmsm6sc0q.fsf@gitster.g/

Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move

[Eric Sunshine]

On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:
> When re-initializing a repository with a separate gitdir (the original
> gitdir is moved to a new location), any linked worktrees become broken
> and must be repaired to reflect the new gitdir location. For absolute
> paths, this breakage is one-way, but is both ways for relative paths
> (the `<worktree>/.git` and the `<repo>/worktrees/<id>/gitdir`).
>
> Previously, `repair_worktrees` was being called which loops through all
> the worktrees in the repository and updates the `<worktree>/.git` files
> to point to the new gitdir. However, when both sides of the worktrees
> are broken, the previous gitdir location is required to reestablish the
> link.
>
> To fix this, the function `repair_worktrees_after_gitdir_move` is
> introduced. It takes the old gitdir path as an argument and repairs both
> sides of the worktree.
>
> This change fixes the following test cases in t0001-init.sh:
> - re-init to move gitdir with linked worktrees
> - re-init to move gitdir within linked worktree

If I understand correctly, this patch is fixing breakage resulting
from the preceding patch. Unfortunately, this approach is problematic
since it breaks bisectability of the project. In particular, it's not
sufficient for the full test suite to pass only at the end of the
patch series; rather, the entire test suite should continue to pass
after application of *each* patch in a series; we don't want one patch
to break the test suite and a subsequent patch to fix it again.

So, if my understanding is correct, please put some thought into how
to reorganize this patch series to ensure that the full test suite
passes for each patch.

Re: [PATCH v2 4/4] worktree: prevent null pointer dereference

[Eric Sunshine]

On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:
> If worktrees is NULL, free_worktrees() should return immediately to
> prevent a null pointer dereference.
>
> Signed-off-by: Caleb White <cdwhite3@pm.me>

Critical questions: It is not clear why this patch is needed,
especially coming at the end of the series. Is there some code in a
patch earlier in the series which might call free_worktrees() with
NULL? If so, then this patch should come before that patch. If not,
then why do we need this patch at all?

Devil's advocate question: Why is it the responsibility of
free_worktrees() to check this condition as opposed to being the
caller's responsibility?

The commit message should explain the need for this patch and answer
these questions, not just say what change is being made.

> diff --git a/worktree.c b/worktree.c
> @@ -28,8 +28,9 @@ void free_worktree(struct worktree *worktree)
>  void free_worktrees(struct worktree **worktrees)
>  {
> -       int i = 0;
> -       for (i = 0; worktrees[i]; i++)
> +       if  (!worktrees)
> +               return;
> +       for (int i = 0; worktrees[i]; i++)
>                 free_worktree(worktrees[i]);

Although it's true that this project has recently started allowing
declaration of the variable in the `for` statement, that change is
unrelated to the stated purpose in the commit message. True, it's a
minor thing in this case, but it causes a hiccup for reviewers when
unrelated changes are piggybacked like this with the "real" change
since it adds noise which obscures what the reviewer should really be
focusing on.

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:
> This refactors the `infer_backlink` function to return an integer
> result and use a pre-allocated `strbuf` for the inferred backlink
> path, replacing the previous `char*` return type.
> 
> This lays the groundwork for the next patch, which needs the
> resultant backlink as a `strbuf`. There was no need to go from
> `strbuf -> char* -> strbuf` again. This change also aligns the
> function's signature with other `strbuf`-based functions.
> 

I think we should first say why we need to add the change in the commit
message which means we should express our motivation in the first. It's
wired to say "I have done something" and then talk about the motivation
why we do this.

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>  worktree.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/worktree.c b/worktree.c
> index 0f032cc..c6d2ede 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)
>   * be able to infer the gitdir by manually reading /path/to/worktree/.git,
>   * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
>   */
> -static char *infer_backlink(const char *gitfile)
> +static int infer_backlink(st
> ruct strbuf *inferred, const char *gitfile)

This line is so strange. Why it generates a newline here?

>  {
>  	struct strbuf actual = STRBUF_INIT;
> -	struct strbuf inferred = STRBUF_INIT;
>  	const char *id;
>  
>  	if (strbuf_read_file(&actual, gitfile, 0) < 0)
> @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile)
>  	id++; /* advance past '/' to point at <id> */
>  	if (!*id)
>  		goto error;
> -	strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
> -	if (!is_directory(inferred.buf))
> +	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
> +	if (!is_directory(inferred->buf))
>  		goto error;
>  
>  	strbuf_release(&actual);
> -	return strbuf_detach(&inferred, NULL);
> +	return 0;
>  
>  error:
>  	strbuf_release(&actual);
> -	strbuf_release(&inferred);

Because we leave the life cycle of the "inferred" to be outside of this
function, we should not use "strbuf_release" to release the memory here.
Make sense.

> -	return NULL;
> +	return 1;
>  }
>  
>  /*
> @@ -680,9 +678,10 @@ void repair_worktree_at_path(const char *path,
>  {
>  	struct strbuf dotgit = STRBUF_INIT;
>  	struct strbuf realdotgit = STRBUF_INIT;
> +	struct strbuf backlink = STRBUF_INIT;

Here, we replace "char *backlink" to "struct strbuf backlink", we need
to align the changed "infer_backlink" function. That's OK.

>  	struct strbuf gitd
> ir = STRBUF_INIT;

Another strange newline here.

>  	struct strbuf olddotgit = STRBUF_INIT;
> -	char *backlink = NULL;
> +	char *git_contents = NULL;
>  	const char *repair = NULL;
>  	int err;
>  
> @@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path,
>  		goto done;
>  	}
>  
> -	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> +	git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));

So, we create a new variable "git_contents" here. I suspect this is a
poor design. In the original logic, we will do the following things for
"backlink".

  1. Call the "read_gitfile_gently" function. If it encounters error, it
     will return NULL and the "err" variable will be set to NON-zero.
  2. If the value of "err" is 0, we would simply execute the
     "strbuf_addstr(&gitdir, "%s/gitdir", backlink)".
  3. If not, and the "err" is "READ_GITFILE_ERR_NOT_A_REPO". We will
     call "infer_backlink" to set the "backlink".

Because now "backlink" is "struct strbuf", we cannot just assign it by
using "xstrdup_or_null(read_gitfile_gently(...))". So, we create a new
variable "git_contents" here.

And we will check whether "git_contents" is NULL to set the value for
the "backlink".

Why not simply do the following things here (I don't think
"git_contents" is a good name, however I am not familiar with the
worktree, I cannot give some advice here).

    const char *git_contents;
    git_contents = read_gitfile_gently(...);
    if (git_contents)
        strbuf_addstr(&backlink, git_contents);

Even more, we could enhance the logic here. If "git_contents" is not
NULL, there is no need for us to check the "err" variable.

>  	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
>  		fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
>  		goto done;
>  	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> -		if (!(backlink = infer_backlink(realdotgit.buf))) {
> +		if (infer_backlink(&backlink, realdotgit.buf)) {
>  			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
>  			goto done;
>  		}
>  	} else if (err) {
>  		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
>  		goto done;
> +	} else if (git_conte
> nts) {

Another strange newline here. As I have talked about above, we should
not check "git_contents" here.

> +		strbuf_addstr(&backlink, git_contents);
>  	}
>  
> -	strbuf_addf(&gitdir, "%s/gitdir", backlink);
> +	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
>  	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
>  		repair = _("gitdir unreadable");
>  	else {
> @@ -726,8 +727,9 @@ void repair_worktree_at_path(const char *path,
>  		write_file(gitdir.buf, "%s", realdotgit.buf);
>  	}
>  done:
> -	free(backlink);
> +	free(git_contents);
>  	strbuf_release(&olddotgit);
> +	strbuf_release(&backlink);
>  	strbuf_release(&gitdir);
>  	strbuf_release(&realdotgit);
>  	strbuf_release(&dotgit);
> -- 
> 2.46.2
> 

Thanks,
Jialuo

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Kristoffer Haugsbakk]

On Sun, Oct 6, 2024, at 17:09, shejialuo wrote:
> On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:
>> This refactors the `infer_backlink` function to return an integer
>> result and use a pre-allocated `strbuf` for the inferred backlink
>> path, replacing the previous `char*` return type.
>>
>> This lays the groundwork for the next patch, which needs the
>> resultant backlink as a `strbuf`. There was no need to go from
>> `strbuf -> char* -> strbuf` again. This change also aligns the
>> function's signature with other `strbuf`-based functions.
>>
>
> I think we should first say why we need to add the change in the commit
> message which means we should express our motivation in the first. It's
> wired to say "I have done something" and then talk about the motivation
> why we do this.
>
>> Signed-off-by: Caleb White <cdwhite3@pm.me>
>> ---
>>  worktree.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/worktree.c b/worktree.c
>> index 0f032cc..c6d2ede 100644
>> --- a/worktree.c
>> +++ b/worktree.c
>> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)
>>   * be able to infer the gitdir by manually reading /path/to/worktree/.git,
>>   * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
>>   */
>> -static char *infer_backlink(const char *gitfile)
>> +static int infer_backlink(st
>> ruct strbuf *inferred, const char *gitfile)
>
> This line is so strange. Why it generates a newline here?

The patches got corrupted by something.  See the emails from Eric
Sunshine.

This resubmit didn’t fix the issue.

https://lore.kernel.org/git/CAPig+cTB-sA-g4cdhfEjWwY1mnbWJ41e=bOCNwp=Y8JKvpmpRg@mail.gmail.com/

-- 
Kristoffer Haugsbakk

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[shejialuo]

On Sun, Oct 06, 2024 at 06:01:17AM +0000, Caleb White wrote:
> This modifies Git’s handling of worktree linking to use relative
> paths instead of absolute paths. Previously, when creating a worktree,
> Git would store the absolute paths to both the main repository and the
> linked worktrees. These hardcoded absolute paths cause breakages such
> as when the repository is moved to a different directory or during
> containerized development where the absolute differs between systems.
> 
> By switching to relative paths, we help ensure that worktree links are
> more resilient when the repository is moved. While links external to the
> repository may still break, Git still automatically handles many common
> scenarios, reducing the need for manual repair. This is particularly
> useful in containerized or portable development environments, where the
> absolute path to the repository can differ between systems. Developers
> no longer need to reinitialize or repair worktrees after relocating the
> repository, improving workflow efficiency and reducing breakages.
> 
> For self-contained repositories (such as using a bare repository with
> worktrees), where both the repository and its worktrees are located
> within the same directory structure, using relative paths guarantees all
> links remain functional regardless of where the directory is located.
> 

Eric has already commented on this commit message. I think this commit
has done a lot of things which make the review painful.

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>  builtin/worktree.c           |  17 ++--
>  t/t2408-worktree-relative.sh |  39 +++++++++
>  worktree.c                   | 152 +++++++++++++++++++++++++----------
>  3 files changed, 159 insertions(+), 49 deletions(-)
>  create mode 100755 t/t2408-worktree-relative.sh
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index fc31d07..99cee56 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname,
>  			const struct add_opts *opts)
>  {
>  	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
> -	struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;
> +	struct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT;
> +	struct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT;
>  	const char *name;
>  	struct strvec child_env = STRVEC_INIT;
>  	unsigned int counter = 0;
> @@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname,
>  
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
> -	strbuf_realpath(&realpath, sb_git.buf, 1);
> -	write_file(sb.buf, "%s", realpath.buf);
> -	strbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1);
> -	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
> -		   realpath.buf, name);
> +	strbuf_realpath(&sb_path_realpath, path, 1);
> +	strbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1);
> +	write_file(sb.buf, "%s/.git", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp));
> +	strbuf_reset(&sb_tmp);

Do we need reset the "sb_tmp"? I guess we do not need, "relative_path"
does not care about "sb_tmp". It will always reset the value of the
"sb_tmp". So, we may delete this line.

[snip]

> diff --git a/worktree.c b/worktree.c
> index c6d2ede..fc14e9a 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
>  void update_worktree_location(struct worktree *wt, const char *path_)
>  {
>  	struct strbuf path = STRBUF_INIT;
> +	struct strbuf repo = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
> +	char *file = NULL;
>  
>  	if (is_main_worktree(wt))
>  		BUG("can't relocate main worktree");
>  
> +	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
>  	strbuf_realpath(&path, path_, 1);
>  	if (fspathcmp(wt->path, path.buf)) {
> -		write_file(git_common_path("worktrees/%s/gitdir", wt->id),
> -			   "%s/.git", path.buf);
> +		file = xstrfmt("%s/gitdir", repo.buf);
> +		write_file(file, "%s/.git", relative_path(path.buf, repo.buf, &tmp));
> +		free(file);
> +		strbuf_reset(&tmp);
> +		file = xstrfmt("%s/.git", path.buf);

Still, we do not need to call "strbuf_reset" again for "tmp". But there
is another question here. Should we define the "file" just in this "if"
block and free "file" also in the block?

And I don't think it's a good idea to use "xstrfmt". Here, we need to
allocate two times and free two times. Why not just define a "struct
strbuf" and the use "strbuf_*" method here?

> +		write_file(file, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> +
>  		free(wt->path);
>  		wt->path = strbuf_detach(&path, NULL);
>  	}
> +	free(file);
>  	strbuf_release(&path);
> +	strbuf_release(&repo);
> +	strbuf_release(&tmp);
>  }
>  
> @@ -564,38 +582,52 @@ static void repair_gitfile(struct worktree *wt,
>  {
>  

[snip]

>  	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
>  	strbuf_addf(&dotgit, "%s/.git", wt->path);
> -	backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> +	git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));

Why here we need to use "xstrdup_or_null". The life cycle of the
"git_contents" variable is in the "repair_gitfile" function.

[snip]

I omit the review for the later code, there are too many codes to
review. And due to my limited knowledge about worktree, the overhead
will be too big for me.

Thanks,
Jialuo

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Eric Sunshine]

On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:
> This refactors the `infer_backlink` function to return an integer
> result and use a pre-allocated `strbuf` for the inferred backlink
> path, replacing the previous `char*` return type.
>
> This lays the groundwork for the next patch, which needs the
> resultant backlink as a `strbuf`. There was no need to go from
> `strbuf -> char* -> strbuf` again. This change also aligns the
> function's signature with other `strbuf`-based functions.

Regarding aligning the signature with other strbuf-based functions...

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)
> -static char *infer_backlink(const char *gitfile)
> +static int infer_backlink(struct strbuf *inferred, const char *gitfile)

... it's subjective, but I find that placing the strbuf as the first
argument makes the purpose of the function less clear. The direct
purpose of all (or the majority of) functions in strbuf.h is to
operate on strbufs, thus it makes sense for the strbuf to be the first
argument (just like `this` is the invisible first argument of instance
methods in C++ which operate on an instance of the class). However,
infer_backlink()'s purpose isn't to operate on a strbuf; the fact that
the original signature neither accepted nor returned a strbuf bears
that out. The really important input to this function is `gitfile`,
thus it makes sense for this argument to come first. The strbuf which
this patch makes it use is a mere implementation detail (a
micro-optimization) which doesn't otherwise change the function's
original purpose, thus it belongs in a less prominent position in the
argument list.

> @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile)
> -       strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
> -       if (!is_directory(inferred.buf))
> +       strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
> +       if (!is_directory(inferred->buf))
>                 goto error;
>         strbuf_release(&actual);
> -       return strbuf_detach(&inferred, NULL);
> +       return 0;

On success, we now return zero...

>  error:
>         strbuf_release(&actual);
> -       strbuf_release(&inferred);
> -       return NULL;
> +       return 1;

... and on failure we return 1. To avoid confusing readers who are
familiar with project idioms, it would probably be better to instead
follow the convention used in most of the rest of the project (and in
Unix system calls in general) of returning -1.

However...

> @@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path,
> -               if (!(backlink = infer_backlink(realdotgit.buf))) {
> +               if (infer_backlink(&backlink, realdotgit.buf)) {
>                         fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
>                         goto done;
>                 }

... this code now becomes more than a little confusing to read. It
says "if infer_backlink then signal error", which sounds rather
backward and leaves the reader scratching his or her head. ("Why
signal error if the function succeeded?"). Hence, infer_backlink()
should probably return 1 on success and 0 on failure, which will make
this code read more idiomatically:

    if (!infer_backlink(realdotgit.buf, &backlink)) {
        ...signal error...

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Eric Sunshine]

On Sun, Oct 6, 2024 at 11:09 AM shejialuo <shejialuo@gmail.com> wrote:
> On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:
> > -     backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> > +     git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
>
> So, we create a new variable "git_contents" here. I suspect this is a
> poor design. In the original logic, we will do the following things for
> "backlink".
>
>   1. Call the "read_gitfile_gently" function. If it encounters error, it
>      will return NULL and the "err" variable will be set to NON-zero.
>   2. If the value of "err" is 0, we would simply execute the
>      "strbuf_addstr(&gitdir, "%s/gitdir", backlink)".
>   3. If not, and the "err" is "READ_GITFILE_ERR_NOT_A_REPO". We will
>      call "infer_backlink" to set the "backlink".
>
> Because now "backlink" is "struct strbuf", we cannot just assign it by
> using "xstrdup_or_null(read_gitfile_gently(...))". So, we create a new
> variable "git_contents" here.
>
> And we will check whether "git_contents" is NULL to set the value for
> the "backlink".

Thanks for thinking through this logic. I have a few additional comments...

> Why not simply do the following things here (I don't think
> "git_contents" is a good name, however I am not familiar with the
> worktree, I cannot give some advice here).

I found the name "git_contents" clear enough and understood its
purpose at-a-glance, so I think it's a reasonably good choice. A
slightly better name might be "gitfile_contents" or perhaps
"dotgit_contents" for consistency with other similarly-named variables
in this function.

>     const char *git_contents;
>     git_contents = read_gitfile_gently(...);
>     if (git_contents)
>         strbuf_addstr(&backlink, git_contents);
>
> Even more, we could enhance the logic here.

Upon reading this patch, I had a similar thought about this, that it
would be more reflective of the original code to set "backlink" early
here rather than waiting until the end of the if-else-cascade.
However, upon reflection, I don't mind that setting "backlink" is
delayed until after the if-else-chain, though I think it deserves at
least one change which I will explain below.

> If "git_contents" is not
> NULL, there is no need for us to check the "err" variable.

I'm not sure we would want to change this; the existing logic seems
clear enough.

> >       if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> >               fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> >               goto done;
> >       } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> > -             if (!(backlink = infer_backlink(realdotgit.buf))) {
> > +             if (infer_backlink(&backlink, realdotgit.buf)) {
> >                       fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> >                       goto done;
> >               }
> >       } else if (err) {
> >               fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> >               goto done;
> > +     } else if (git_contents) {
> > +             strbuf_addstr(&backlink, git_contents);
> >       }

It certainly makes sense to check whether "git_contents" is NULL
before trying to copy it into the "backlink" strbuf, however, if
"git_contents" is NULL here, then what does that mean? What does it
mean to leave "backlink" empty? The only way (presumably) we get this
far is if read_gitfile_gently() succeeded, so (presumably)
"git_contents" should not be NULL. Thus, rather than conditionally
copying into "backlink", we should instead indicate clearly via BUG()
that it should be impossible for "git_contents" to be NULL. So, rather
than making this part of the existing if-else-cascade, we should do
this as a standalone `if`:

    if (!git_contents)
        BUG(...);
    strbuf_addstr(&backlink, git_contents);

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1100 bytes --]

On Sunday, October 6th, 2024 at 06:05, Eric Sunshine <sunshine@sunshineco.com> wrote:

> When you reroll, please extend the commit message to give a more
> detailed overview of how this patch actually changes the behavior both
> at a high level and at a low level. This is especially important since
> this patch is sufficiently long and involved that it's not easy to
> glean these details at-a-glance from the code changes themselves.

I will do that, I was not sure how much low level detail I should dive into.

> Regarding what you wrote above, there seems to be a good deal of
> redundancy between the first two paragraphs; combining the paragraphs
> and folding out the duplication might make the message more
> streamlined. I do like the discussion about containerized environments
> being used as (at least one) justification for employing relative
> paths, and think that may be a good lead-in for the commit message.
> Please see [1] for some helpful hints for composing a good commit
> message.

Thanks, I will clean up the redundancy and add more detail to the commit.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1085 bytes --]

On Sunday, October 6th, 2024 at 06:12, Eric Sunshine <sunshine@sunshineco.com> wrote:
> If I understand correctly, this patch is fixing breakage resulting
> from the preceding patch. Unfortunately, this approach is problematic
> since it breaks bisectability of the project. In particular, it's not
> sufficient for the full test suite to pass only at the end of the
> patch series; rather, the entire test suite should continue to pass
> after application of each patch in a series; we don't want one patch
> to break the test suite and a subsequent patch to fix it again.
>
> So, if my understanding is correct, please put some thought into how
> to reorganize this patch series to ensure that the full test suite
> passes for each patch.

Yes, there was one edge case that broke and this patch fixed. But I
understand what you mean about the bisectability. I was trying to come
up with ways to split up the commits and this seemed like a good spot as
it just introduced new functions with minimal changes elsewhere. But
this can be squashed into the previous patch.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move

[Eric Sunshine]

On Sun, Oct 6, 2024 at 6:41 PM Caleb White <cdwhite3@pm.me> wrote:
> On Sunday, October 6th, 2024 at 06:12, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > So, if my understanding is correct, please put some thought into how
> > to reorganize this patch series to ensure that the full test suite
> > passes for each patch.
>
> Yes, there was one edge case that broke and this patch fixed. But I
> understand what you mean about the bisectability. I was trying to come
> up with ways to split up the commits and this seemed like a good spot as
> it just introduced new functions with minimal changes elsewhere. But
> this can be squashed into the previous patch.

I haven't yet pored over the code in-depth, so I don't know if it is
even possible, but it's typically very much preferred by reviewers if
you can present a series as smaller, simpler, easier-to-digest patches
than large monolithic ones. So, it would be ideal if you could figure
out some good split points (especially since patch [2/4] is already
uncomfortably large for a reviewer). But sometimes it's just not
possible to find good splits, so a large patch may be the only choice.

Re: [PATCH v2 4/4] worktree: prevent null pointer dereference

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 2295 bytes --]

On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@sunshineco.com> wrote: 

> Critical questions: It is not clear why this patch is needed,
> especially coming at the end of the series. Is there some code in a
> patch earlier in the series which might call free_worktrees() with
> NULL? If so, then this patch should come before that patch. If not,
> then why do we need this patch at all?

When I was working through different solution for the 3rd patch, I
encountered this issue and this was the fix for that (granted, I
could've handled this on the caller side). It turned out that I had
to go a different direction and this was no longer needed, but I
figured it wouldn't hurt to leave this in which is why it's the
last patch. However, I can just drop this if you want.

> Devil's advocate question: Why is it the responsibility of
> free_worktrees() to check this condition as opposed to being the
> caller's responsibility?

Sometimes it's cleaner to write a check once on the callee side
instead of all callers having to duplicate the same check. This
also follows the same pattern as free_worktree():

```
void free_worktree(struct worktree *worktree)
{
	if (!worktree)
		return;
```


> The commit message should explain the need for this patch and answer
> these questions, not just say what change is being made.

Will do (unless we decide to drop this).

> Although it's true that this project has recently started allowing
> declaration of the variable in the `for` statement, that change is
> unrelated to the stated purpose in the commit message. True, it's a
> minor thing in this case, but it causes a hiccup for reviewers when
> unrelated changes are piggybacked like this with the "real" change
> since it adds noise which obscures what the reviewer should really be
> focusing on.

I didn't change this originally, but then the build process threw errors
that I had code before declarations (because I placed the if condition at
the start of the function). I could've moved the if condition past the
declaration, but it seemed weird to need to declare a variable if the
function is just going to immediately return due to a NULL pointer.

If we decide to keep this patch then I can keep the separate declaration.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1052 bytes --]

On Sunday, October 6th, 2024 at 17:48, Eric Sunshine <sunshine@sunshineco.com> wrote:
> I haven't yet pored over the code in-depth, so I don't know if it is
> even possible, but it's typically very much preferred by reviewers if
> you can present a series as smaller, simpler, easier-to-digest patches
> than large monolithic ones. So, it would be ideal if you could figure
> out some good split points (especially since patch [2/4] is already
> uncomfortably large for a reviewer). But sometimes it's just not
> possible to find good splits, so a large patch may be the only choice.

There's really not any other good split points because it's
an all or nothing kind of thing. All of these changes need to be in place
at the same time or there's some edge cases that are going to fail.

I suppose I could try to split the *reading* of the absolute/relative paths
separate from the *writing* of the relative paths. However, I'm not
sure if this would be worth the trouble as most places that read from
the files also write to the files.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 4/4] worktree: prevent null pointer dereference

[Eric Sunshine]

On Sun, Oct 6, 2024 at 7:03 PM Caleb White <cdwhite3@pm.me> wrote:
> On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Critical questions: It is not clear why this patch is needed,
> > especially coming at the end of the series. Is there some code in a
> > patch earlier in the series which might call free_worktrees() with
> > NULL? If so, then this patch should come before that patch. If not,
> > then why do we need this patch at all?
>
> When I was working through different solution for the 3rd patch, I
> encountered this issue and this was the fix for that (granted, I
> could've handled this on the caller side). It turned out that I had
> to go a different direction and this was no longer needed, but I
> figured it wouldn't hurt to leave this in which is why it's the
> last patch. However, I can just drop this if you want.

Reviewers are a limited resource on this project[1], so it's ideal if
submissions can be as reviewer-friendly as possible. Extraneous
patches, unnecessary or unrelated changes to surrounding code, etc.
all make a patch series more onerous to review, thus are best avoided.
(This concern prompted all the review comments I left on this patch.)
So, let's drop this patch since it adds no value to either this series
or to the existing codebase. If someone needs such a change later on,
they can resurrect the change.

[1] There are far more people submitting patches to the project than
reviewing them. For instance, according to Junio's latest "What's
Cooking" report[2], the patch I submitted[3] a couple weeks ago to fix
"git worktree repair" to handle manual copy operations is still
awaiting review. (Since you've now been living in the worktree code a
bit and have had to digest the "repair" logic, perhaps you'd be
interested in reviewing that patch?)

[2]: see the "es/worktree-repair-copied" entry at
https://lore.kernel.org/git/xmqq7cancmoj.fsf@gitster.g/

[3]: https://lore.kernel.org/git/20240923075416.54289-1-ericsunshine@charter.net/

> > Although it's true that this project has recently started allowing
> > declaration of the variable in the `for` statement, that change is
> > unrelated to the stated purpose in the commit message. True, it's a
> > minor thing in this case, but it causes a hiccup for reviewers when
> > unrelated changes are piggybacked like this with the "real" change
> > since it adds noise which obscures what the reviewer should really be
> > focusing on.
>
> I didn't change this originally, but then the build process threw errors
> that I had code before declarations (because I placed the if condition at
> the start of the function). I could've moved the if condition past the
> declaration, but it seemed weird to need to declare a variable if the
> function is just going to immediately return due to a NULL pointer.

Since this project is still following older style rules about
declaring all variables up-front, it's not unusual to declare
variables which don't necessarily get used in some code path (even
though it might feel weird for someone who habitually declares
variables only as and where needed).

Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move

[Eric Sunshine]

On Sun, Oct 6, 2024 at 7:13 PM Caleb White <cdwhite3@pm.me> wrote:
> On Sunday, October 6th, 2024 at 17:48, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > I haven't yet pored over the code in-depth, so I don't know if it is
> > even possible, but it's typically very much preferred by reviewers if
> > you can present a series as smaller, simpler, easier-to-digest patches
> > than large monolithic ones. So, it would be ideal if you could figure
> > out some good split points (especially since patch [2/4] is already
> > uncomfortably large for a reviewer). But sometimes it's just not
> > possible to find good splits, so a large patch may be the only choice.
>
> There's really not any other good split points because it's
> an all or nothing kind of thing. All of these changes need to be in place
> at the same time or there's some edge cases that are going to fail.
>
> I suppose I could try to split the *reading* of the absolute/relative paths
> separate from the *writing* of the relative paths. However, I'm not
> sure if this would be worth the trouble as most places that read from
> the files also write to the files.

If that's the case, then it probably wouldn't help to split it up in
that fashion. Artificial splits like the one you describe are very
likely to be more confusing for reviewers than helpful.

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 2383 bytes --]

On Sunday, October 6th, 2024 at 10:09, shejialuo <shejialuo@gmail.com> wrote:
> I think we should first say why we need to add the change in the commit
> message which means we should express our motivation in the first. It's
> wired to say "I have done something" and then talk about the motivation
> why we do this.

I can reword this commit message.

> Because we leave the life cycle of the "inferred" to be outside of this
> function, we should not use "strbuf_release" to release the memory here.
> Make sense.

Yes, however, I just realized that I should likely reset the strbuf when it
enters the function (or before it is written to) which is similar to what
the relative_path() function does.

> So, we create a new variable "git_contents" here. I suspect this is a
> poor design. In the original logic, we will do the following things for
> "backlink".
>
> 1. Call the "read_gitfile_gently" function. If it encounters error, it
> will return NULL and the "err" variable will be set to NON-zero.
> 2. If the value of "err" is 0, we would simply execute the
> "strbuf_addstr(&gitdir, "%s/gitdir", backlink)".
> 3. If not, and the "err" is "READ_GITFILE_ERR_NOT_A_REPO". We will
> call "infer_backlink" to set the "backlink".
>
> Because now "backlink" is "struct strbuf", we cannot just assign it by
> using "xstrdup_or_null(read_gitfile_gently(...))". So, we create a new
> variable "git_contents" here.
>
> And we will check whether "git_contents" is NULL to set the value for
> the "backlink".
>
> Why not simply do the following things here (I don't think
> "git_contents" is a good name, however I am not familiar with the
> worktree, I cannot give some advice here).
>
> const char *git_contents;
> git_contents = read_gitfile_gently(...);
> if (git_contents)
> strbuf_addstr(&backlink, git_contents);
>
> Even more, we could enhance the logic here. If "git_contents" is not
> NULL, there is no need for us to check the "err" variable.

I was trying to not make huge changes to the logic flow, but I suppose I
could revisit this if desired. I can likely move the `if(git_contents)`
to the start instead of being at the end. I was not aware that if an err
occurred that the function returned NULL, I thought that perhaps there was
the possibility of git_contents being filled with something and an err still
occurring.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1838 bytes --]

On Sunday, October 6th, 2024 at 10:37, shejialuo <shejialuo@gmail.com> wrote
> Eric has already commented on this commit message. I think this commit
> has done a lot of things which make the review painful.

My apologies, I will do better on this commit message.

> Do we need reset the "sb_tmp"? I guess we do not need, "relative_path"
> does not care about "sb_tmp". It will always reset the value of the
> "sb_tmp". So, we may delete this line.

You are right, this is reset by relative_path(). I originally encountered
a bug and I thought not resetting this strbuf between relative_path() calls
was the cause but it must have been something else that I fixed.

> Still, we do not need to call "strbuf_reset" again for "tmp". But there
> is another question here. Should we define the "file" just in this "if"
> block and free "file" also in the block?

The style this code uses seems to place most / all of the declarations at
the top of the function and frees at the bottom so I think this fits in.

> And I don't think it's a good idea to use "xstrfmt". Here, we need to
> allocate two times and free two times. Why not just define a "struct
> strbuf" and the use "strbuf_*" method here?

I can use strbufs, I just wasn't sure if I really needed a strbuf for
each of the paths and was just trying to reuse a var.

> > strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> > strbuf_addf(&dotgit, "%s/.git", wt->path);
> > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> 

> 

> Why here we need to use "xstrdup_or_null". The life cycle of the
> "git_contents" variable is in the "repair_gitfile" function.

This what the existing code used and I saw no reason to change it...

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1378 bytes --]

On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote:
> I found the name "git_contents" clear enough and understood its
> purpose at-a-glance, so I think it's a reasonably good choice. A
> slightly better name might be "gitfile_contents" or perhaps
> "dotgit_contents" for consistency with other similarly-named variables
> in this function.

I will rename to `dotgit_contents`.

> It certainly makes sense to check whether "git_contents" is NULL
> before trying to copy it into the "backlink" strbuf, however, if
> "git_contents" is NULL here, then what does that mean? What does it
> mean to leave "backlink" empty? The only way (presumably) we get this
> far is if read_gitfile_gently() succeeded, so (presumably)
> "git_contents" should not be NULL. Thus, rather than conditionally
> copying into "backlink", we should instead indicate clearly via BUG()
> that it should be impossible for "git_contents" to be NULL. So, rather
> than making this part of the existing if-else-cascade, we should do
> this as a standalone `if`:
> 

> if (!git_contents)
> BUG(...);
> strbuf_addstr(&backlink, git_contents);

We can't use BUG because this is handled as part of the err
conditions. The contents can be NULL and `backlink` could be
filled with the inferred backlink. I moved this to the top
and I think it reads better.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1760 bytes --]

On Sunday, October 6th, 2024 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
> ... it's subjective, but I find that placing the strbuf as the first
> argument makes the purpose of the function less clear. The direct
> purpose of all (or the majority of) functions in strbuf.h is to
> operate on strbufs, thus it makes sense for the strbuf to be the first
> argument (just like `this` is the invisible first argument of instance
> methods in C++ which operate on an instance of the class). However,
> infer_backlink()'s purpose isn't to operate on a strbuf; the fact that
> the original signature neither accepted nor returned a strbuf bears
> that out. The really important input to this function is `gitfile`,
> thus it makes sense for this argument to come first. The strbuf which
> this patch makes it use is a mere implementation detail (a
> micro-optimization) which doesn't otherwise change the function's
> original purpose, thus it belongs in a less prominent position in the
> argument list.

I can reverse the arguments.

> ... this code now becomes more than a little confusing to read. It
> says "if infer_backlink then signal error", which sounds rather
> backward and leaves the reader scratching his or her head. ("Why
> signal error if the function succeeded?"). Hence, infer_backlink()
> should probably return 1 on success and 0 on failure, which will make
> this code read more idiomatically:
> 

> if (!infer_backlink(realdotgit.buf, &backlink)) {
> ...signal error...

This was my first thought, however, on unix it is fairly standard
to return 0 if successful and a non-zero int if there's an error.
I don't mind updating, but I want to follow what makes the most
sense and would be most expected.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 4/4] worktree: prevent null pointer dereference

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1145 bytes --]

On Sunday, October 6th, 2024 at 18:24, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Reviewers are a limited resource on this project[1], so it's ideal if
> submissions can be as reviewer-friendly as possible. Extraneous
> patches, unnecessary or unrelated changes to surrounding code, etc.
> all make a patch series more onerous to review, thus are best avoided.
> (This concern prompted all the review comments I left on this patch.)
> So, let's drop this patch since it adds no value to either this series
> or to the existing codebase. If someone needs such a change later on,
> they can resurrect the change.

Sounds good, dropped.

> [1] There are far more people submitting patches to the project than
> reviewing them. For instance, according to Junio's latest "What's
> Cooking" report[2], the patch I submitted[3] a couple weeks ago to fix
> "git worktree repair" to handle manual copy operations is still
> awaiting review. (Since you've now been living in the worktree code a
> bit and have had to digest the "repair" logic, perhaps you'd be
> interested in reviewing that patch?)

I'd be happy to take a look!

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Eric Sunshine]

On Sun, Oct 6, 2024 at 10:42 PM Caleb White <cdwhite3@pm.me> wrote:
> On Sunday, October 6th, 2024 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > ... this code now becomes more than a little confusing to read. It
> > says "if infer_backlink then signal error", which sounds rather
> > backward and leaves the reader scratching his or her head. ("Why
> > signal error if the function succeeded?"). Hence, infer_backlink()
> > should probably return 1 on success and 0 on failure, which will make
> > this code read more idiomatically:
> >
> > if (!infer_backlink(realdotgit.buf, &backlink)) {
> > ...signal error...
>
> This was my first thought, however, on unix it is fairly standard
> to return 0 if successful and a non-zero int if there's an error.
> I don't mind updating, but I want to follow what makes the most
> sense and would be most expected.

I mentioned it because it made my reading hiccup, but I don't feel too
strongly about it one way or the other considering that this is an
internal function.

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 285 bytes --]

On Sunday, October 6th, 2024 at 22:26, Eric Sunshine <sunshine@sunshineco.com> wrote:
> I mentioned it because it made my reading hiccup, but I don't feel too
> strongly about it one way or the other considering that this is an
> internal function.

I reversed the flags :thumsup:

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[shejialuo]

On Sun, Oct 06, 2024 at 11:57:22PM +0000, Caleb White wrote:

[snip]

> > Still, we do not need to call "strbuf_reset" again for "tmp". But there
> > is another question here. Should we define the "file" just in this "if"
> > block and free "file" also in the block?
> 
> The style this code uses seems to place most / all of the declarations at
> the top of the function and frees at the bottom so I think this fits in.
> 

Yes, as you have said, the code style places most / all of the
declarations at the top and free at the bottom. But the trouble here is
we will free the "file" in the "if" block.

    char *file = NULL;

    if (...) {
        file = xstrfmt(...);
        free(file);
        file = xstrfmt(...);
    }

    free(file);

If we want to follow the original code style, should we create two
variables at the top and free them at the bottom?

> > And I don't think it's a good idea to use "xstrfmt". Here, we need to
> > allocate two times and free two times. Why not just define a "struct
> > strbuf" and the use "strbuf_*" method here?
> 
> I can use strbufs, I just wasn't sure if I really needed a strbuf for
> each of the paths and was just trying to reuse a var.
> 

You don't need to create a new "strbuf" for each of the paths. You could
just use "strbuf_reset" for only one "struct strbuf".

    struct strbuf file = STRBUF_INIT;

    if (...) {
        strbuf_addf(...);
        strbuf_reset(&file);
        strbuf_addf(...);

    }

    strbuf_release(&file);

> > > strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> > > strbuf_addf(&dotgit, "%s/.git", wt->path);
> > > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> > > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> > 
> 
> > 
> 
> > Why here we need to use "xstrdup_or_null". The life cycle of the
> > "git_contents" variable is in the "repair_gitfile" function.
> 
> This what the existing code used and I saw no reason to change it...

Actually, I somehow understand why in the original code, it will use
"xstrdup_or_null" to initialize the "backlink". Because in
"read_gitfile_gently", we will use a static "struct strbuf" as the
buffer.

I guess the intention of the original code is that if we call again
"read_gitfile_gently" in this function or we have another thread which
calls this function, the content of the buffer will be changed. So we
explicitly use "xstrdup_or_null" to create a copy here to avoid this.

But I wonder whether we really need to consider above problem?

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Sun, Oct 06, 2024 at 02:41:22PM -0400, Eric Sunshine wrote:

[snip]

> > Why not simply do the following things here (I don't think
> > "git_contents" is a good name, however I am not familiar with the
> > worktree, I cannot give some advice here).
> 
> I found the name "git_contents" clear enough and understood its
> purpose at-a-glance, so I think it's a reasonably good choice. A
> slightly better name might be "gitfile_contents" or perhaps
> "dotgit_contents" for consistency with other similarly-named variables
> in this function.
> 

Thanks, I don't know the context here.

> > If "git_contents" is not
> > NULL, there is no need for us to check the "err" variable.
> 
> I'm not sure we would want to change this; the existing logic seems
> clear enough.
> 

The reason why I don't think we need to check the "err" variable is that
the "git_contents" and "err" is relevant. If "git_contents" is not NULL,
the "err" must be zero unless there are bugs in "read_gitfile_gently".
So, if we already check "git_contents", why do we need to check again
for "err"?

But, I agree with you that the existing logic is clear enough. I just
explain further what I mean here.

Thanks,
Jialuo

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1043 bytes --]

On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote:
> The reason why I don't think we need to check the "err" variable is that
> the "git_contents" and "err" is relevant. If "git_contents" is not NULL,
> the "err" must be zero unless there are bugs in "read_gitfile_gently".
> So, if we already check "git_contents", why do we need to check again
> for "err"?

There are two other error conditions we check, and one of them we try
to find the inferred backlink (so it is not a failure path):

```
} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
	fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
	goto done;
} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
	if (!infer_backlink(realdotgit.buf, &backlink)) {
		fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
		goto done;
	}
} else if (err) {
	fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
	goto done;
}

```

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[Eric Sunshine]

On Sun, Oct 6, 2024 at 11:45 PM shejialuo <shejialuo@gmail.com> wrote:
> Actually, I somehow understand why in the original code, it will use
> "xstrdup_or_null" to initialize the "backlink". Because in
> "read_gitfile_gently", we will use a static "struct strbuf" as the
> buffer.
>
> I guess the intention of the original code is that if we call again
> "read_gitfile_gently" in this function or we have another thread which
> calls this function, the content of the buffer will be changed. So we
> explicitly use "xstrdup_or_null" to create a copy here to avoid this.

That's correct. The code is being defensive in case the static strbuf
inside read_gitfile_gently() gets overwritten.

> But I wonder whether we really need to consider above problem?

It's easier to reason about the code in this function if we don't have
to worry about the underlying static strbuf. Also, this is not a hot
path so the extra allocation and string copy is not a concern. As
such, it makes sense for the code to remain robust (by creating the
copy) rather than becoming potentially more fragile by eliminating the
string copy.

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Mon, Oct 07, 2024 at 02:26:51AM +0000, Caleb White wrote:
> On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > I found the name "git_contents" clear enough and understood its
> > purpose at-a-glance, so I think it's a reasonably good choice. A
> > slightly better name might be "gitfile_contents" or perhaps
> > "dotgit_contents" for consistency with other similarly-named variables
> > in this function.
> 
> I will rename to `dotgit_contents`.
> 
> > It certainly makes sense to check whether "git_contents" is NULL
> > before trying to copy it into the "backlink" strbuf, however, if
> > "git_contents" is NULL here, then what does that mean? What does it
> > mean to leave "backlink" empty? The only way (presumably) we get this
> > far is if read_gitfile_gently() succeeded, so (presumably)
> > "git_contents" should not be NULL. Thus, rather than conditionally
> > copying into "backlink", we should instead indicate clearly via BUG()
> > that it should be impossible for "git_contents" to be NULL. So, rather
> > than making this part of the existing if-else-cascade, we should do
> > this as a standalone `if`:
> > 
> 
> > if (!git_contents)
> > BUG(...);
> > strbuf_addstr(&backlink, git_contents);

I agree with the idea that Eric proposed. It's truly we should raise a
bug here. That's would be much more clear.

> We can't use BUG because this is handled as part of the err
> conditions. The contents can be NULL and `backlink` could be
> filled with the inferred backlink. I moved this to the top
> and I think it reads better.

That's not correct. It is true that the contents can be NULL and
`backlink` could be filled with the infer_backlink. But do you notice
that if `backlink` was filled with the infer_backlink, it will jump
to the "done" label.

What Erie means is the following:

    git_contents = read_gitfile_gently(...);
    if (err) {
        if (...) {

        } else if (err == RAED_GITFILE_ERR_NOT_A_REPO) {
            // handle backlink
            goto done;

        }
    }

    if (!git_contents)
        BUG(...);
    strbuf_addstr(&backlink, git_contents);

    done:
        // cleanup operations

Thanks,
Jialuo

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1375 bytes --]

On Sunday, October 6th, 2024 at 23:12, shejialuo <shejialuo@gmail.com> wrote:
> That's not correct. It is true that the contents can be NULL and
> `backlink` could be filled with the infer_backlink. But do you notice
> that if `backlink` was filled with the infer_backlink, it will jump
> to the "done" label.

This is not correct, if backlink is filled with `infer_backlink` it
continues on with the processing. I have made this more clear:

    dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
    if (dotgit_contents) {
	if (is_absolute_path(dotgit_contents)) {
	    strbuf_addstr(&backlink, dotgit_contents);
        } else {
	    strbuf_addbuf(&backlink, &realdotgit);
	    strbuf_strip_suffix(&backlink, ".git");
	    strbuf_addstr(&backlink, dotgit_contents);
        }
    } else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
	fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
	goto done;
    } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
	if (!infer_backlink(realdotgit.buf, &backlink)) {
	    fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
	    goto done;
        }
    } else if (err) {
        fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
        goto done;
    }



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Mon, Oct 07, 2024 at 04:01:43AM +0000, Caleb White wrote:
> On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote:
> > The reason why I don't think we need to check the "err" variable is that
> > the "git_contents" and "err" is relevant. If "git_contents" is not NULL,
> > the "err" must be zero unless there are bugs in "read_gitfile_gently".
> > So, if we already check "git_contents", why do we need to check again
> > for "err"?
> 
> There are two other error conditions we check, and one of them we try
> to find the inferred backlink (so it is not a failure path):
> 
> ```
> } else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> 	fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> 	goto done;
> } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> 	if (!infer_backlink(realdotgit.buf, &backlink)) {
> 		fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> 		goto done;
> 	}
> } else if (err) {
> 	fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> 	goto done;
> }
> 
> ```

I am sorry that my words make you not clear here. I want to express that
if "git_contents" is not NULL and there is no need for us to check
"err". This means we could use the following flows:

    if (git_contents && !err) {
        ...
    } else if (err == xxx) {
        ...
    }

However, from my perspective, the way proposed by Eric where we could
use "BUG" is more robust. Because the current method assumes that
"read_gitfile_gently" works as we want.

Thanks,
Jialuo

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Mon, Oct 07, 2024 at 04:19:22AM +0000, Caleb White wrote:
> On Sunday, October 6th, 2024 at 23:12, shejialuo <shejialuo@gmail.com> wrote:
> > That's not correct. It is true that the contents can be NULL and
> > `backlink` could be filled with the infer_backlink. But do you notice
> > that if `backlink` was filled with the infer_backlink, it will jump
> > to the "done" label.
> 
> This is not correct, if backlink is filled with `infer_backlink` it
> continues on with the processing. I have made this more clear:
> 
>     dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
>     if (dotgit_contents) {
> 	if (is_absolute_path(dotgit_contents)) {
> 	    strbuf_addstr(&backlink, dotgit_contents);
>         } else {
> 	    strbuf_addbuf(&backlink, &realdotgit);
> 	    strbuf_strip_suffix(&backlink, ".git");
> 	    strbuf_addstr(&backlink, dotgit_contents);
>         }
>     } else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> 	fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> 	goto done;
>     } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> 	if (!infer_backlink(realdotgit.buf, &backlink)) {
> 	    fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> 	    goto done;
>         }
>     } else if (err) {
>         fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
>         goto done;
>     }
> 
> 

Thanks, you are correct here. We cannot use "BUG" here. And I think
currently this is OK.

Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 419 bytes --]

On Sunday, October 6th, 2024 at 23:28, shejialuo <shejialuo@gmail.com> wrote:
> > } else if (err) {
> > fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> > goto done;
> > }
> 

> 

> Thanks, you are correct here. We cannot use "BUG" here. And I think
> currently this is OK.

Granted, that last `else if` could just become an `else` statement now,
I'll make that change.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 465 bytes --]

On Sunday, October 6th, 2024 at 22:45, shejialuo <shejialuo@gmail.com> wrote:
> You don't need to create a new "strbuf" for each of the paths. You could
> just use "strbuf_reset" for only one "struct strbuf".
> 

> struct strbuf file = STRBUF_INIT;
> 

> if (...) {
> strbuf_addf(...);
> strbuf_reset(&file);
> strbuf_addf(...);
> 

> }
> 

> strbuf_release(&file);

Ah, this is a better design---I've updated this to use a single strbuf, thanks!

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]