[RFC][PATCH 0/2] worktree: change representation and usage of primary worktree

[RFC][PATCH 0/2] worktree: change representation and usage of primary worktree

[Shreyansh Paliwal]

While working on reducing global state in wt-status [1], it became clear
that some cleanup in the worktree API would be helpful before moving ahead.

As of now Primary worktree is represented in three ways,

* by passing NULL as a worktree
* by a struct worktree whose wt->id is NULL, and
* in the ref-store map, by using "/" as the key.

This creates ambiguity, callers and helpers often need extra checks for a
NULL worktree, and the implicit NULL form makes it harder to rely on
fields like wt->repo without falling back to global state.

So this patch series involves making the worktree API more robust by
removing the usage of NULL as the representation and using wt->id = '/'
as the marker for a primary worktree.

In patch 1/2, change the internal representation in worktree.c so that the
main worktree is identified by '/' instead of NULL.

In patch 2/2, update the API usage by modifying callers to obtain the
primary worktree via helper and pass an actual struct worktree rather than
NULL.

I would like to get feedback whether this implementation is in the right direction,
and if there is anything else that I should be doing in this cleanup series.

[1]- https://lore.kernel.org/git/20260209134439.14492-1-shreyanshpaliwalcmsmn@gmail.com/T/#u

Shreyansh Paliwal (2):
  worktree: represent the primary worktree with "/" instead of NULL
  worktree: stop passing NULL as primary worktree

 builtin/fsck.c     |  2 +-
 builtin/worktree.c | 10 +++++++---
 path.c             | 27 +++++++++++++--------------
 path.h             |  9 +++------
 refs.c             |  4 ++--
 revision.c         |  6 ++++--
 worktree.c         | 43 +++++++++++++++++++++++++------------------
 worktree.h         |  7 +++++++
 wt-status.c        | 22 ++++++++++++----------
 9 files changed, 74 insertions(+), 56 deletions(-)

--
2.53.0

[RFC][PATCH 1/2] worktree: represent the primary worktree with '/' instead of NULL

[Shreyansh Paliwal]

The worktree API uses NULL to represent the primary worktree. As a result,
many callers pass NULL to implicitly refer to it, which in turn requires
additional checks to ensure that `wt` is defined before use.

Represent the main worktree explicitly by setting `wt->id` to "/" in
`get_main_worktree()` and update `is_main_worktree()` accordingly. Replace
checks for wt to be defined in worktree.c functions with calls to
`is_main_worktree()` (or strcmp(wt->id, "/")).

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 worktree.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/worktree.c b/worktree.c
index 9308389cb6..b29934407f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -101,6 +101,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
 
 	CALLOC_ARRAY(worktree, 1);
 	worktree->repo = the_repository;
+	worktree->id = xstrdup("/");
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->is_current = is_current_worktree(worktree);
 	worktree->is_bare = (is_bare_repository_cfg == 1) ||
@@ -127,6 +128,8 @@ struct worktree *get_linked_worktree(const char *id,
 
 	if (!id)
 		die("Missing linked worktree name");
+	if (!strcmp(id, "/"))
+		die("'/' is reserved for primary worktree");
 
 	repo_common_path_append(the_repository, &path, "worktrees/%s/gitdir", id);
 	if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
@@ -206,9 +209,7 @@ struct worktree **get_worktrees_without_reading_head(void)
 
 char *get_worktree_git_dir(const struct worktree *wt)
 {
-	if (!wt)
-		return xstrdup(repo_get_git_dir(the_repository));
-	else if (!wt->id)
+	if (is_main_worktree(wt))
 		return xstrdup(repo_get_common_dir(the_repository));
 	else
 		return repo_common_path(the_repository, "worktrees/%s", wt->id);
@@ -277,7 +278,7 @@ struct worktree *find_worktree_by_path(struct worktree **list, const char *p)
 
 int is_main_worktree(const struct worktree *wt)
 {
-	return !wt->id;
+	return !strcmp(wt->id, "/");
 }
 
 const char *worktree_lock_reason(struct worktree *wt)
@@ -566,7 +567,7 @@ void strbuf_worktree_ref(const struct worktree *wt,
 {
 	if (parse_worktree_ref(refname, NULL, NULL, NULL) ==
 		    REF_WORKTREE_CURRENT &&
-	    wt && !wt->is_current) {
+	    !wt->is_current) {
 		if (is_main_worktree(wt))
 			strbuf_addstr(sb, "main-worktree/");
 		else
@@ -629,6 +630,9 @@ static void repair_gitfile(struct worktree *wt,
 	char *path = NULL;
 	int err;
 
+	if (is_main_worktree(wt))
+		goto done;
+
 	/* missing worktree can't be repaired */
 	if (!file_exists(wt->path))
 		goto done;
-- 
2.53.0

[RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Shreyansh Paliwal]

Some functions like repo_git_pathv() in path.c, wt_status_check_rebase()
and wt_status_check_bisect() in wt-status.c, get_worktree_git_dir() in
builtin/worktree.c are called with a NULL worktree. This makes it difficult
to access fields like `wt->repo` and add extra handling for checking wt is
defined.

Add a helper function of get_worktrees_internal() as get_worktrees_repo()
and pass struct repository down the callstack to get_main_worktree()
function so that the primary worktree of a specific repository can be
accessed instead of just the_repository.

Access the primary worktree by the help of get_worktrees_repo() and pass it
to the functions as struct worktree instead of passing NULL.

Further `worktree_git_path()` no longer needs a separate
`struct repository *` parameter. Use `wt->repo` instead and update its
callers accordingly.

while at, replace any unecessary checks for wt to be defined with
is_main_worktree(wt).

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 builtin/fsck.c     |  2 +-
 builtin/worktree.c | 10 +++++++---
 path.c             | 27 +++++++++++++--------------
 path.h             |  9 +++------
 refs.c             |  4 ++--
 revision.c         |  6 ++++--
 worktree.c         | 29 ++++++++++++++++-------------
 worktree.h         |  7 +++++++
 wt-status.c        | 22 ++++++++++++----------
 9 files changed, 65 insertions(+), 51 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0512f78a87..42ba0afb91 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1137,7 +1137,7 @@ int cmd_fsck(int argc,
 			 * and may get overwritten by other calls
 			 * while we're examining the index.
 			 */
-			path = xstrdup(worktree_git_path(the_repository, wt, "index"));
+			path = xstrdup(worktree_git_path(wt, "index"));
 			wt_gitdir = get_worktree_git_dir(wt);
 
 			read_index_from(&istate, path, wt_gitdir);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index fbdaf2eb2e..27c5889c89 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -328,6 +328,8 @@ static void check_candidate_path(const char *path,
 	wt = find_worktree_by_path(worktrees, path);
 	if (!wt)
 		return;
+	if(is_main_worktree(wt))
+		die(_("'%s' is the main worktree"), path);
 
 	locked = !!worktree_lock_reason(wt);
 	if ((!locked && force) || (locked && force > 1)) {
@@ -660,7 +662,8 @@ static int can_use_local_refs(const struct add_opts *opts)
 		if (!opts->quiet) {
 			struct strbuf path = STRBUF_INIT;
 			struct strbuf contents = STRBUF_INIT;
-			char *wt_gitdir = get_worktree_git_dir(NULL);
+			struct worktree **worktrees = get_worktrees_repo(the_repository);
+			char *wt_gitdir = get_worktree_git_dir(worktrees[0]);
 
 			strbuf_add_real_path(&path, wt_gitdir);
 			strbuf_addstr(&path, "/HEAD");
@@ -675,6 +678,7 @@ static int can_use_local_refs(const struct add_opts *opts)
 			strbuf_release(&path);
 			strbuf_release(&contents);
 			free(wt_gitdir);
+			free_worktrees(worktrees);
 		}
 		return 1;
 	}
@@ -1191,14 +1195,14 @@ static void validate_no_submodules(const struct worktree *wt)
 
 	wt_gitdir = get_worktree_git_dir(wt);
 
-	if (is_directory(worktree_git_path(the_repository, wt, "modules"))) {
+	if (is_directory(worktree_git_path(wt, "modules"))) {
 		/*
 		 * There could be false positives, e.g. the "modules"
 		 * directory exists but is empty. But it's a rare case and
 		 * this simpler check is probably good enough for now.
 		 */
 		found_submodules = 1;
-	} else if (read_index_from(&istate, worktree_git_path(the_repository, wt, "index"),
+	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),
 				   wt_gitdir) > 0) {
 		for (i = 0; i < istate.cache_nr; i++) {
 			struct cache_entry *ce = istate.cache[i];
diff --git a/path.c b/path.c
index d726537622..4ac86e1e58 100644
--- a/path.c
+++ b/path.c
@@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
 				   const struct repository *repo,
 				   const struct worktree *wt)
 {
-	if (!wt)
-		strbuf_addstr(buf, repo->gitdir);
-	else if (!wt->id)
+	if (is_main_worktree(wt))
 		strbuf_addstr(buf, repo->commondir);
 	else
 		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
@@ -426,7 +424,7 @@ static void repo_git_pathv(struct repository *repo,
 		strbuf_addch(buf, '/');
 	gitdir_len = buf->len;
 	strbuf_vaddf(buf, fmt, args);
-	if (!wt)
+	if (is_main_worktree(wt))
 		adjust_git_path(repo, buf, gitdir_len);
 	strbuf_cleanup_path(buf);
 }
@@ -437,8 +435,10 @@ char *repo_git_path(struct repository *repo,
 	struct strbuf path = STRBUF_INIT;
 	va_list args;
 	va_start(args, fmt);
-	repo_git_pathv(repo, NULL, &path, fmt, args);
+	struct worktree **worktrees = get_worktrees_repo(repo);
+	repo_git_pathv(repo, worktrees[0], &path, fmt, args);
 	va_end(args);
+	free_worktrees(worktrees);
 	return strbuf_detach(&path, NULL);
 }
 
@@ -448,8 +448,10 @@ const char *repo_git_path_append(struct repository *repo,
 {
 	va_list args;
 	va_start(args, fmt);
-	repo_git_pathv(repo, NULL, sb, fmt, args);
+	struct worktree **worktrees = get_worktrees_repo(repo);
+	repo_git_pathv(repo, worktrees[0], sb, fmt, args);
 	va_end(args);
+	free_worktrees(worktrees);
 	return sb->buf;
 }
 
@@ -460,8 +462,10 @@ const char *repo_git_path_replace(struct repository *repo,
 	va_list args;
 	strbuf_reset(sb);
 	va_start(args, fmt);
-	repo_git_pathv(repo, NULL, sb, fmt, args);
+	struct worktree **worktrees = get_worktrees_repo(repo);
+	repo_git_pathv(repo, worktrees[0], sb, fmt, args);
 	va_end(args);
+	free_worktrees(worktrees);
 	return sb->buf;
 }
 
@@ -486,17 +490,12 @@ const char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname->buf);
 }
 
-const char *worktree_git_path(struct repository *r,
-			      const struct worktree *wt, const char *fmt, ...)
+const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
 {
 	struct strbuf *pathname = get_pathname();
 	va_list args;
-
-	if (wt && wt->repo != r)
-		BUG("worktree not connected to expected repository");
-
 	va_start(args, fmt);
-	repo_git_pathv(r, wt, pathname, fmt, args);
+	repo_git_pathv(wt->repo, wt, pathname, fmt, args);
 	va_end(args);
 	return pathname->buf;
 }
diff --git a/path.h b/path.h
index 0ec95a0b07..54e09b2883 100644
--- a/path.h
+++ b/path.h
@@ -66,13 +66,10 @@ const char *repo_git_path_replace(struct repository *repo,
 
 /*
  * Similar to repo_git_path() but can produce paths for a specified
- * worktree instead of current one. When no worktree is given, then the path is
- * computed relative to main worktree of the given repository.
+ * worktree instead of current one.
  */
-const char *worktree_git_path(struct repository *r,
-			      const struct worktree *wt,
-			      const char *fmt, ...)
-	__attribute__((format (printf, 3, 4)));
+const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
 
 /*
  * The `repo_worktree_path` family of functions will construct a path into a
diff --git a/refs.c b/refs.c
index 627b7f8698..98df2235e7 100644
--- a/refs.c
+++ b/refs.c
@@ -2324,12 +2324,12 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
 	if (wt->is_current)
 		return get_main_ref_store(wt->repo);
 
-	id = wt->id ? wt->id : "/";
+	id = wt->id;
 	refs = lookup_ref_store_map(&wt->repo->worktree_ref_stores, id);
 	if (refs)
 		return refs;
 
-	if (wt->id) {
+	if (!is_main_worktree(wt)) {
 		struct strbuf common_path = STRBUF_INIT;
 		repo_common_path_append(wt->repo, &common_path,
 					"worktrees/%s", wt->id);
diff --git a/revision.c b/revision.c
index 9b131670f7..a9d4f796ed 100644
--- a/revision.c
+++ b/revision.c
@@ -1730,12 +1730,14 @@ void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)
 
 	cb.all_revs = revs;
 	cb.all_flags = flags;
-	cb.wt = NULL;
+	struct worktree **worktrees = get_worktrees_repo(the_repository);
+	cb.wt = worktrees[0];
 	refs_for_each_reflog(get_main_ref_store(the_repository),
 			     handle_one_reflog, &cb);
 
 	if (!revs->single_worktree)
 		add_other_reflogs_to_pending(&cb);
+	free_worktrees(worktrees);
 }
 
 static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
@@ -1847,7 +1849,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 		wt_gitdir = get_worktree_git_dir(wt);
 
 		if (read_index_from(&istate,
-				    worktree_git_path(the_repository, wt, "index"),
+				    worktree_git_path(wt, "index"),
 				    wt_gitdir) > 0)
 			do_add_index_objects_to_pending(revs, &istate, flags);
 
diff --git a/worktree.c b/worktree.c
index b29934407f..1059c18115 100644
--- a/worktree.c
+++ b/worktree.c
@@ -91,16 +91,16 @@ static int is_main_worktree_bare(struct repository *repo)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(int skip_reading_head)
+static struct worktree *get_main_worktree(struct repository *repo, int skip_reading_head)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf worktree_path = STRBUF_INIT;
 
-	strbuf_add_real_path(&worktree_path, repo_get_common_dir(the_repository));
+	strbuf_add_real_path(&worktree_path, repo_get_common_dir(repo));
 	strbuf_strip_suffix(&worktree_path, "/.git");
 
 	CALLOC_ARRAY(worktree, 1);
-	worktree->repo = the_repository;
+	worktree->repo = repo;
 	worktree->id = xstrdup("/");
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->is_current = is_current_worktree(worktree);
@@ -112,7 +112,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
 		 * This check is unnecessary if we're currently in the main worktree,
 		 * as prior checks already consulted all configs of the current worktree.
 		 */
-		(!worktree->is_current && is_main_worktree_bare(the_repository));
+		(!worktree->is_current && is_main_worktree_bare(repo));
 
 	if (!skip_reading_head)
 		add_head_info(worktree);
@@ -165,7 +165,7 @@ struct worktree *get_linked_worktree(const char *id,
  * retrieving worktree metadata that could be used when the worktree is known
  * to not be in a healthy state, e.g. when creating or repairing it.
  */
-static struct worktree **get_worktrees_internal(int skip_reading_head)
+static struct worktree **get_worktrees_internal(struct repository *repo, int skip_reading_head)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -175,9 +175,9 @@ static struct worktree **get_worktrees_internal(int skip_reading_head)
 
 	ALLOC_ARRAY(list, alloc);
 
-	list[counter++] = get_main_worktree(skip_reading_head);
+	list[counter++] = get_main_worktree(repo, skip_reading_head);
 
-	strbuf_addf(&path, "%s/worktrees", repo_get_common_dir(the_repository));
+	strbuf_addf(&path, "%s/worktrees", repo_get_common_dir(repo));
 	dir = opendir(path.buf);
 	strbuf_release(&path);
 	if (dir) {
@@ -199,12 +199,15 @@ static struct worktree **get_worktrees_internal(int skip_reading_head)
 
 struct worktree **get_worktrees(void)
 {
-	return get_worktrees_internal(0);
+	return get_worktrees_repo(the_repository);
+}
+struct worktree **get_worktrees_repo(struct repository *repo)
+{
+	return get_worktrees_internal(repo, 0);
 }
-
 struct worktree **get_worktrees_without_reading_head(void)
 {
-	return get_worktrees_internal(1);
+	return get_worktrees_internal(the_repository, 1);
 }
 
 char *get_worktree_git_dir(const struct worktree *wt)
@@ -289,7 +292,7 @@ const char *worktree_lock_reason(struct worktree *wt)
 	if (!wt->lock_reason_valid) {
 		struct strbuf path = STRBUF_INIT;
 
-		strbuf_addstr(&path, worktree_git_path(the_repository, wt, "locked"));
+		strbuf_addstr(&path, worktree_git_path(wt, "locked"));
 		if (file_exists(path.buf)) {
 			struct strbuf lock_reason = STRBUF_INIT;
 			if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)
@@ -690,7 +693,7 @@ static void repair_noop(int iserr UNUSED,
 
 void repair_worktrees(worktree_repair_fn fn, void *cb_data, int use_relative_paths)
 {
-	struct worktree **worktrees = get_worktrees_internal(1);
+	struct worktree **worktrees = get_worktrees_internal(the_repository, 1);
 	struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
 
 	if (!fn)
@@ -735,7 +738,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path
 
 void repair_worktrees_after_gitdir_move(const char *old_path)
 {
-	struct worktree **worktrees = get_worktrees_internal(1);
+	struct worktree **worktrees = get_worktrees_internal(the_repository, 1);
 	struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
 
 	for (; *wt; wt++)
diff --git a/worktree.h b/worktree.h
index e4bcccdc0a..fc5f87b5b1 100644
--- a/worktree.h
+++ b/worktree.h
@@ -30,6 +30,13 @@ struct worktree {
  */
 struct worktree **get_worktrees(void);
 
+/*
+ * Like `get_worktrees`, but it returns the worktrees for the given repository.
+ * This is useful for cases where local repository context is used rather than
+ * the_repository global.
+ */
+struct worktree **get_worktrees_repo(struct repository *repo);
+
 /*
  * Like `get_worktrees`, but does not read HEAD. Skip reading HEAD allows to
  * get the worktree without worrying about failures pertaining to parsing
diff --git a/wt-status.c b/wt-status.c
index e12adb26b9..11aea61c81 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1624,7 +1624,7 @@ static char *get_branch(const struct worktree *wt, const char *path)
 	struct object_id oid;
 	const char *branch_name;
 
-	if (strbuf_read_file(&sb, worktree_git_path(the_repository, wt, "%s", path), 0) <= 0)
+	if (strbuf_read_file(&sb, worktree_git_path(wt, "%s", path), 0) <= 0)
 		goto got_nothing;
 
 	while (sb.len && sb.buf[sb.len - 1] == '\n')
@@ -1723,18 +1723,18 @@ int wt_status_check_rebase(const struct worktree *wt,
 {
 	struct stat st;
 
-	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
-		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
+	if (!stat(worktree_git_path(wt, "rebase-apply"), &st)) {
+		if (!stat(worktree_git_path(wt, "rebase-apply/applying"), &st)) {
 			state->am_in_progress = 1;
-			if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/patch"), &st) && !st.st_size)
+			if (!stat(worktree_git_path(wt, "rebase-apply/patch"), &st) && !st.st_size)
 				state->am_empty_patch = 1;
 		} else {
 			state->rebase_in_progress = 1;
 			state->branch = get_branch(wt, "rebase-apply/head-name");
 			state->onto = get_branch(wt, "rebase-apply/onto");
 		}
-	} else if (!stat(worktree_git_path(the_repository, wt, "rebase-merge"), &st)) {
-		if (!stat(worktree_git_path(the_repository, wt, "rebase-merge/interactive"), &st))
+	} else if (!stat(worktree_git_path(wt, "rebase-merge"), &st)) {
+		if (!stat(worktree_git_path(wt, "rebase-merge/interactive"), &st))
 			state->rebase_interactive_in_progress = 1;
 		else
 			state->rebase_in_progress = 1;
@@ -1750,7 +1750,7 @@ int wt_status_check_bisect(const struct worktree *wt,
 {
 	struct stat st;
 
-	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
+	if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
 		state->bisecting_from = get_branch(wt, "BISECT_START");
 		return 1;
@@ -1795,18 +1795,19 @@ void wt_status_get_state(struct repository *r,
 	struct stat st;
 	struct object_id oid;
 	enum replay_action action;
+	struct worktree **worktrees = get_worktrees_repo(r);
 
 	if (!stat(git_path_merge_head(r), &st)) {
-		wt_status_check_rebase(NULL, state);
+		wt_status_check_rebase(worktrees[0], state);
 		state->merge_in_progress = 1;
-	} else if (wt_status_check_rebase(NULL, state)) {
+	} else if (wt_status_check_rebase(worktrees[0], state)) {
 		;		/* all set */
 	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 		   !repo_get_oid(r, "CHERRY_PICK_HEAD", &oid)) {
 		state->cherry_pick_in_progress = 1;
 		oidcpy(&state->cherry_pick_head_oid, &oid);
 	}
-	wt_status_check_bisect(NULL, state);
+	wt_status_check_bisect(worktrees[0], state);
 	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") &&
 	    !repo_get_oid(r, "REVERT_HEAD", &oid)) {
 		state->revert_in_progress = 1;
@@ -1824,6 +1825,7 @@ void wt_status_get_state(struct repository *r,
 	if (get_detached_from)
 		wt_status_get_detached_from(r, state);
 	wt_status_check_sparse_checkout(r, state);
+	free_worktrees(worktrees);
 }
 
 static void wt_longstatus_print_state(struct wt_status *s)
-- 
2.53.0

Re: [RFC][PATCH 1/2] worktree: represent the primary worktree with '/' instead of NULL

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> diff --git a/worktree.c b/worktree.c
> index 9308389cb6..b29934407f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -101,6 +101,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
>  
>  	CALLOC_ARRAY(worktree, 1);
>  	worktree->repo = the_repository;
> +	worktree->id = xstrdup("/");
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
>  	worktree->is_current = is_current_worktree(worktree);
>  	worktree->is_bare = (is_bare_repository_cfg == 1) ||

Presumably we left .id = NULL from CALLOC_ARRAY(), so this looks
sensible.  When releasing resources from an instance of worktree,
we'd blindly free(worktree->id) and in the old world, free(NULL)
turned into no-op, and this xstrdup()'d copy will be freed in the
new world, so there is nothing funny here, I hope?  This one, and
the change to is_main_worktree() go together.

> @@ -127,6 +128,8 @@ struct worktree *get_linked_worktree(const char *id,
>  
>  	if (!id)
>  		die("Missing linked worktree name");
> +	if (!strcmp(id, "/"))
> +		die("'/' is reserved for primary worktree");

Makes me wonder if this is a BUG not die; where does id come from?

	... goes and looks ...

The only caller is worktree.c:get_worktrees_internal() and it is
feeding d->d_name that came from readdir_skip_dot_and_dotdot(), so
it cannot be "/".

By the way, I suspect that get_linked_worktree() should become
file-scope static, as there is no other caller.

> @@ -206,9 +209,7 @@ struct worktree **get_worktrees_without_reading_head(void)
>  
>  char *get_worktree_git_dir(const struct worktree *wt)
>  {
> -	if (!wt)
> -		return xstrdup(repo_get_git_dir(the_repository));
> -	else if (!wt->id)
> +	if (is_main_worktree(wt))
>  		return xstrdup(repo_get_common_dir(the_repository));
>  	else
>  		return repo_common_path(the_repository, "worktrees/%s", wt->id);

Good spotting.  This series needs to spot any and all places that
use these other conventions (i.e. wt->id == NULL) to identify the
primary worktree and rewrite them to call is_main_worktree(), which
may be a chore, but once it is done, it would become a lot easier to
follow the resulting code.

> @@ -277,7 +278,7 @@ struct worktree *find_worktree_by_path(struct worktree **list, const char *p)
>  
>  int is_main_worktree(const struct worktree *wt)
>  {
> -	return !wt->id;
> +	return !strcmp(wt->id, "/");
>  }

OK.

> @@ -566,7 +567,7 @@ void strbuf_worktree_ref(const struct worktree *wt,
>  {
>  	if (parse_worktree_ref(refname, NULL, NULL, NULL) ==
>  		    REF_WORKTREE_CURRENT &&
> -	    wt && !wt->is_current) {
> +	    !wt->is_current) {

OK.

> @@ -629,6 +630,9 @@ static void repair_gitfile(struct worktree *wt,
>  	char *path = NULL;
>  	int err;
>  
> +	if (is_main_worktree(wt))
> +		goto done;

This is a bit new.

The original did not say 

	if (!wt || !wt->id || !strcmp(wt->id, "/"))
		goto done;

The only caller is iterating over the resulting list of worktrees
returned from get_worktrees_internal(1) *BUT* it already skips the
primary worktree (the function MUST return the primary one as the
first one, and the callers MUST be aware of the convention).

So I am not sure if the new check is even needed.  Or rather, this ...

	if (!wt || !wt->id || !strcmp(wt->id, "/"))
		BUG("why are you feeding me the primary worktree???");
	
... might be more appropriate, perhaps?  I dunno.


>  	/* missing worktree can't be repaired */
>  	if (!file_exists(wt->path))
>  		goto done;

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> -			path = xstrdup(worktree_git_path(the_repository, wt, "index"));
> +			path = xstrdup(worktree_git_path(wt, "index"));

We'll understand this change when we read the changes to
worktree_git_path() later in this patch, I guess.  I will not
comment on the same changes around worktree_git_path() callers.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index fbdaf2eb2e..27c5889c89 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -328,6 +328,8 @@ static void check_candidate_path(const char *path,
>  	wt = find_worktree_by_path(worktrees, path);
>  	if (!wt)
>  		return;
> +	if(is_main_worktree(wt))
> +		die(_("'%s' is the main worktree"), path);

Style (missing SP between "if" and "(condition)").

Earlier, a failure from find_worktree_by_path(), presumably meaning
"you gave me this path, but that is not where any of our worktrees
live" gave wt==NULL and it silently returned.  Could the function
returned wt==NULL to signal that the path is where the primary
worktree is?  I guess not (it seems to give us a worktree instance
with wt->id==NULL).

So, we never cared about wt being the primary worktree, but now we
care.  Why do we need to?

> @@ -660,7 +662,8 @@ static int can_use_local_refs(const struct add_opts *opts)
>  		if (!opts->quiet) {
>  			struct strbuf path = STRBUF_INIT;
>  			struct strbuf contents = STRBUF_INIT;
> -			char *wt_gitdir = get_worktree_git_dir(NULL);
> +			struct worktree **worktrees = get_worktrees_repo(the_repository);
> +			char *wt_gitdir = get_worktree_git_dir(worktrees[0]);

We used to pass NULL to get_worktree_git_dir() to ask about the
primary working tree, but the convention is no longer available.  So
we use get_worktrees_repo(), presumably is a new function, that
gives all the worktrees honoring the "first one in the resulting
list is the primary one" convention, only to use the first element
in the list.

I wonder if making worktree.c:get_main_worktree(), which is a file
scope static in worktree.c, available would allow us express this
logic more directly?

>  			strbuf_add_real_path(&path, wt_gitdir);
>  			strbuf_addstr(&path, "/HEAD");
> @@ -675,6 +678,7 @@ static int can_use_local_refs(const struct add_opts *opts)
>  			strbuf_release(&path);
>  			strbuf_release(&contents);
>  			free(wt_gitdir);
> +			free_worktrees(worktrees);

Anyway, we need to release the list of worktrees here.

> @@ -1191,14 +1195,14 @@ static void validate_no_submodules(const struct worktree *wt)
>  
>  	wt_gitdir = get_worktree_git_dir(wt);
>  
> -	if (is_directory(worktree_git_path(the_repository, wt, "modules"))) {
> +	if (is_directory(worktree_git_path(wt, "modules"))) {
> -	} else if (read_index_from(&istate, worktree_git_path(the_repository, wt, "index"),
> +	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),

Ditto on worktree_git_path().

> diff --git a/path.c b/path.c
> index d726537622..4ac86e1e58 100644
> --- a/path.c
> +++ b/path.c
> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
>  				   const struct repository *repo,
>  				   const struct worktree *wt)
>  {
> -	if (!wt)
> -		strbuf_addstr(buf, repo->gitdir);
> -	else if (!wt->id)
> +	if (is_main_worktree(wt))
>  		strbuf_addstr(buf, repo->commondir);
>  	else
>  		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);

This is curious.

We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
use repo->commondir for both.  For the primary worktree, it ought to
be the same as repo->gitdir, so it should not matter, but makes me
wonder what the reason behind this difference in the original.

We have been assuming that wt==NULL and wt->id==NULL both meant the
same thing: "we are talking about the primary worktree".  But the
code around here before this patch seems to behave differently.  Is
our assumption incorrect and are we making a mistake by conflating
these two conditions into one?

> @@ -437,8 +435,10 @@ char *repo_git_path(struct repository *repo,
>  	struct strbuf path = STRBUF_INIT;
>  	va_list args;
>  	va_start(args, fmt);
> -	repo_git_pathv(repo, NULL, &path, fmt, args);
> +	struct worktree **worktrees = get_worktrees_repo(repo);
> +	repo_git_pathv(repo, worktrees[0], &path, fmt, args);
>  	va_end(args);
> +	free_worktrees(worktrees);

The same "to pass the primary worktree, we need to grab everybody
and pass the first one" pattern is repeated here.

> @@ -448,8 +448,10 @@ const char *repo_git_path_append(struct repository *repo,
>  {
>  	va_list args;
>  	va_start(args, fmt);
> -	repo_git_pathv(repo, NULL, sb, fmt, args);
> +	struct worktree **worktrees = get_worktrees_repo(repo);
> +	repo_git_pathv(repo, worktrees[0], sb, fmt, args);
>  	va_end(args);
> +	free_worktrees(worktrees);

And again here.

> @@ -460,8 +462,10 @@ const char *repo_git_path_replace(struct repository *repo,
>  	va_list args;
>  	strbuf_reset(sb);
>  	va_start(args, fmt);
> -	repo_git_pathv(repo, NULL, sb, fmt, args);
> +	struct worktree **worktrees = get_worktrees_repo(repo);
> +	repo_git_pathv(repo, worktrees[0], sb, fmt, args);
>  	va_end(args);
> +	free_worktrees(worktrees);

And again here.

> @@ -486,17 +490,12 @@ const char *mkpath(const char *fmt, ...)
>  	return cleanup_path(pathname->buf);
>  }
>  
> -const char *worktree_git_path(struct repository *r,
> -			      const struct worktree *wt, const char *fmt, ...)
> +const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
>  {

Since we no longer use wt==NULL as the sign to work on the primary
working tree, we can rely on wt->repo being a valid repository the
caller wants to work with.

>  	struct strbuf *pathname = get_pathname();
>  	va_list args;
> -
> -	if (wt && wt->repo != r)
> -		BUG("worktree not connected to expected repository");
> -
>  	va_start(args, fmt);
> -	repo_git_pathv(r, wt, pathname, fmt, args);
> +	repo_git_pathv(wt->repo, wt, pathname, fmt, args);
>  	va_end(args);
>  	return pathname->buf;
>  }
> diff --git a/path.h b/path.h
> index 0ec95a0b07..54e09b2883 100644
> --- a/path.h
> +++ b/path.h
> @@ -66,13 +66,10 @@ const char *repo_git_path_replace(struct repository *repo,
>  
>  /*
>   * Similar to repo_git_path() but can produce paths for a specified
> - * worktree instead of current one. When no worktree is given, then the path is
> - * computed relative to main worktree of the given repository.
> + * worktree instead of current one.
>   */
> -const char *worktree_git_path(struct repository *r,
> -			      const struct worktree *wt,
> -			      const char *fmt, ...)
> -	__attribute__((format (printf, 3, 4)));
> +const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
> +	__attribute__((format (printf, 2, 3)));

Good.

> diff --git a/refs.c b/refs.c
> index 627b7f8698..98df2235e7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2324,12 +2324,12 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
>  	if (wt->is_current)
>  		return get_main_ref_store(wt->repo);
>  
> -	id = wt->id ? wt->id : "/";
> +	id = wt->id;
>  	refs = lookup_ref_store_map(&wt->repo->worktree_ref_stores, id);
>  	if (refs)
>  		return refs;
>  
> -	if (wt->id) {
> +	if (!is_main_worktree(wt)) {
>  		struct strbuf common_path = STRBUF_INIT;
>  		repo_common_path_append(wt->repo, &common_path,
>  					"worktrees/%s", wt->id);

Good.  The original uses local "id" as a pathname component (which
could be "/"), and wt->id==NULL as the sign that it is the primary.
The updated code is a faithful conversion of it to the new world
order.

> diff --git a/revision.c b/revision.c
> index 9b131670f7..a9d4f796ed 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1730,12 +1730,14 @@ void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)
>  
>  	cb.all_revs = revs;
>  	cb.all_flags = flags;
> -	cb.wt = NULL;
> +	struct worktree **worktrees = get_worktrees_repo(the_repository);
> +	cb.wt = worktrees[0];
>  	refs_for_each_reflog(get_main_ref_store(the_repository),
>  			     handle_one_reflog, &cb);
>  
>  	if (!revs->single_worktree)
>  		add_other_reflogs_to_pending(&cb);
> +	free_worktrees(worktrees);

The same "to pass the primary worktree, we need to grab everybody
and pass the first one" pattern strikes again.

> diff --git a/worktree.c b/worktree.c
> index b29934407f..1059c18115 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -91,16 +91,16 @@ static int is_main_worktree_bare(struct repository *repo)
>  /**
>   * get the main worktree
>   */
> -static struct worktree *get_main_worktree(int skip_reading_head)
> +static struct worktree *get_main_worktree(struct repository *repo, int skip_reading_head)
>  {
>  	struct worktree *worktree = NULL;
>  	struct strbuf worktree_path = STRBUF_INIT;
>  
> -	strbuf_add_real_path(&worktree_path, repo_get_common_dir(the_repository));
> +	strbuf_add_real_path(&worktree_path, repo_get_common_dir(repo));
>  	strbuf_strip_suffix(&worktree_path, "/.git");
>  
>  	CALLOC_ARRAY(worktree, 1);
> -	worktree->repo = the_repository;
> +	worktree->repo = repo;
>  	worktree->id = xstrdup("/");
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
>  	worktree->is_current = is_current_worktree(worktree);
> @@ -112,7 +112,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
>  		 * This check is unnecessary if we're currently in the main worktree,
>  		 * as prior checks already consulted all configs of the current worktree.
>  		 */
> -		(!worktree->is_current && is_main_worktree_bare(the_repository));
> +		(!worktree->is_current && is_main_worktree_bare(repo));
>  
>  	if (!skip_reading_head)
>  		add_head_info(worktree);

Weaning the code from depending on the_repository is mixed into the
refactoring, which makes me wonder if it is better done in a
separate patch.  We seriously should consider making this function
externally visible, as so many callers want to get hold of it.

I also wonder if "struct repository" wants to have a member that
points at the primary worktree instance, but I think I am getting
way ahead of myself.

> @@ -199,12 +199,15 @@ static struct worktree **get_worktrees_internal(int skip_reading_head)
>  
>  struct worktree **get_worktrees(void)
>  {
> -	return get_worktrees_internal(0);
> +	return get_worktrees_repo(the_repository);
> +}
> +struct worktree **get_worktrees_repo(struct repository *repo)
> +{
> +	return get_worktrees_internal(repo, 0);
>  }

OK.

> @@ -1795,18 +1795,19 @@ void wt_status_get_state(struct repository *r,
>  	struct stat st;
>  	struct object_id oid;
>  	enum replay_action action;
> +	struct worktree **worktrees = get_worktrees_repo(r);
>  
>  	if (!stat(git_path_merge_head(r), &st)) {
> -		wt_status_check_rebase(NULL, state);
> +		wt_status_check_rebase(worktrees[0], state);
>  		state->merge_in_progress = 1;
> -	} else if (wt_status_check_rebase(NULL, state)) {
> +	} else if (wt_status_check_rebase(worktrees[0], state)) {
>  		;		/* all set */
>  	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
>  		   !repo_get_oid(r, "CHERRY_PICK_HEAD", &oid)) {
>  		state->cherry_pick_in_progress = 1;
>  		oidcpy(&state->cherry_pick_head_oid, &oid);
>  	}
> -	wt_status_check_bisect(NULL, state);
> +	wt_status_check_bisect(worktrees[0], state);
>  	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") &&
>  	    !repo_get_oid(r, "REVERT_HEAD", &oid)) {
>  		state->revert_in_progress = 1;
> @@ -1824,6 +1825,7 @@ void wt_status_get_state(struct repository *r,
>  	if (get_detached_from)
>  		wt_status_get_detached_from(r, state);
>  	wt_status_check_sparse_checkout(r, state);
> +	free_worktrees(worktrees);
>  }

The same "to pass the primary worktree, we need to grab everybody
and pass the first one" pattern strikes yet another time.

Thanks.

Re: [RFC][PATCH 1/2] worktree: represent the primary worktree with '/' instead of NULL

[Shreyansh Paliwal]

> > diff --git a/worktree.c b/worktree.c
> > index 9308389cb6..b29934407f 100644
> > --- a/worktree.c
> > +++ b/worktree.c
> > @@ -101,6 +101,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
> >
> >  	CALLOC_ARRAY(worktree, 1);
> >  	worktree->repo = the_repository;
> > +	worktree->id = xstrdup("/");
> >  	worktree->path = strbuf_detach(&worktree_path, NULL);
> >  	worktree->is_current = is_current_worktree(worktree);
> >  	worktree->is_bare = (is_bare_repository_cfg == 1) ||
>
> Presumably we left .id = NULL from CALLOC_ARRAY(), so this looks
> sensible.  When releasing resources from an instance of worktree,
> we'd blindly free(worktree->id) and in the old world, free(NULL)
> turned into no-op, and this xstrdup()'d copy will be freed in the
> new world, so there is nothing funny here, I hope?  This one, and
> the change to is_main_worktree() go together.

Hmm, I don't think free(worktree->id) should cause any issue in this case.

> > @@ -127,6 +128,8 @@ struct worktree *get_linked_worktree(const char *id
> >
> >  	if (!id)
> >  		die("Missing linked worktree name");
> > +	if (!strcmp(id, "/"))
> > +		die("'/' is reserved for primary worktree");
>
> Makes me wonder if this is a BUG not die; where does id come from?
>
> 	... goes and looks ...
>
> The only caller is worktree.c:get_worktrees_internal() and it is
> feeding d->d_name that came from readdir_skip_dot_and_dotdot(), so
> it cannot be "/".
>
> By the way, I suspect that get_linked_worktree() should become
> file-scope static, as there is no other caller.

Actually get_linked_worktee(), along with worktree.c: get_worktrees_internal()
is also called from builtin/worktree.c: add_worktree().
So at this point we should prefer die(), and if were to make
get_linked_worktree() static, then we can add a helper for external uses
maybe using a struct repository* instead of the_repository in the future.

> > @@ -629,6 +630,9 @@ static void repair_gitfile(struct worktree *wt,
> >  	char *path = NULL;
> >  	int err;
> >
> > +	if (is_main_worktree(wt))
> > +		goto done;
>
> This is a bit new.
>
> The original did not say
>
> 	if (!wt || !wt->id || !strcmp(wt->id, "/"))
> 		goto done;
>
> The only caller is iterating over the resulting list of worktrees
> returned from get_worktrees_internal(1) *BUT* it already skips the
> primary worktree (the function MUST return the primary one as the
> first one, and the callers MUST be aware of the convention).
>
> So I am not sure if the new check is even needed.  Or rather, this ...
>
> 	if (!wt || !wt->id || !strcmp(wt->id, "/"))
> 		BUG("why are you feeding me the primary worktree???");
>
> ... might be more appropriate, perhaps?  I dunno.

Actually this new check is to prevent accidently feeding '/' as wt->id in,

	path = repo_common_path(the_repository, "worktrees/%s", wt->id);

but you are right as of now its only caller skips the primary worktree
so we can just put a precautionary check, for the case if this function
is used somewhere in future like this,

    if(is_main_worktree(wt))
        BUG("repair_gitfile() called for the main worktree");

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Shreyansh Paliwal]

> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index fbdaf2eb2e..27c5889c89 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -328,6 +328,8 @@ static void check_candidate_path(const char *path,
> >  	wt = find_worktree_by_path(worktrees, path);
> >  	if (!wt)
> >  		return;
> > +	if(is_main_worktree(wt))
> > +		die(_("'%s' is the main worktree"), path);
>
> Style (missing SP between "if" and "(condition)").

my bad, will fix that.

> Earlier, a failure from find_worktree_by_path(), presumably meaning
> "you gave me this path, but that is not where any of our worktrees
> live" gave wt==NULL and it silently returned.  Could the function
> returned wt==NULL to signal that the path is where the primary
> worktree is?  I guess not (it seems to give us a worktree instance
> with wt->id==NULL).
>
> So, we never cared about wt being the primary worktree, but now we
> care.  Why do we need to?

The function check_candidate_path() is basically to check the path we can
add a worktree, so if we are receiving primary worktree it shouldn't proceed
and further in the function we are calling delete_git_dir(wt->id)
below which then further calls repo_common_path_append() with id.
So to prevent feeding '/' to this we need to check if the worktree is
main or not.

> > @@ -660,7 +662,8 @@ static int can_use_local_refs(const struct add_opts *opts)
> >  		if (!opts->quiet) {
> >  			struct strbuf path = STRBUF_INIT;
> >  			struct strbuf contents = STRBUF_INIT;
> > -			char *wt_gitdir = get_worktree_git_dir(NULL);
> > +			struct worktree **worktrees = get_worktrees_repo(the_repository);
> > +			char *wt_gitdir = get_worktree_git_dir(worktrees[0]);
>
> We used to pass NULL to get_worktree_git_dir() to ask about the
> primary working tree, but the convention is no longer available.  So
> we use get_worktrees_repo(), presumably is a new function, that
> gives all the worktrees honoring the "first one in the resulting
> list is the primary one" convention, only to use the first element
> in the list.
>
> I wonder if making worktree.c:get_main_worktree(), which is a file
> scope static in worktree.c, available would allow us express this
> logic more directly?

Actually I thought about making get_main_worktree() available but then
I saw helpers like get_worktrees() so I thought that get_worktrees_repo()
would be better for every use case.
But I agree since there are so many sites to access the main worktree, we can
make get_main_worktree() available and use it instead of introducing
get_worktrees_repo().

> > diff --git a/path.c b/path.c
> > index d726537622..4ac86e1e58 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
> >  				   const struct repository *repo,
> >  				   const struct worktree *wt)
> >  {
> > -	if (!wt)
> > -		strbuf_addstr(buf, repo->gitdir);
> > -	else if (!wt->id)
> > +	if (is_main_worktree(wt))
> >  		strbuf_addstr(buf, repo->commondir);
> >  	else
> >  		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
>
> This is curious.
>
> We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
> use repo->commondir for both.  For the primary worktree, it ought to
> be the same as repo->gitdir, so it should not matter, but makes me
> wonder what the reason behind this difference in the original.
>
> We have been assuming that wt==NULL and wt->id==NULL both meant the
> same thing: "we are talking about the primary worktree".  But the
> code around here before this patch seems to behave differently.  Is
> our assumption incorrect and are we making a mistake by conflating
> these two conditions into one?

Yes it came into my mind as well. So if we check were strbuf_worktree_gitdir()
is called from, there is only one function repo_git_pathv() which is mostly
called with a NULL wt (primary worktree) and called once with an actual
worktreee once inside worktree_git_path() which had some NULL indirect callers
inside wt-status.c which again meant wt being primary.
I think different usecases for wt and wt->id being NULL was just an oversight
at this particular instance and both of them should refer to the main worktree.

> > diff --git a/worktree.c b/worktree.c
> > index b29934407f..1059c18115 100644
> > --- a/worktree.c
> > +++ b/worktree.c
> > @@ -91,16 +91,16 @@ static int is_main_worktree_bare(struct repository *repo)
> >  /**
> >   * get the main worktree
> >   */
> > -static struct worktree *get_main_worktree(int skip_reading_head)
> > +static struct worktree *get_main_worktree(struct repository *repo, int skip_reading_head)
> >  {
> >  	struct worktree *worktree = NULL;
> >  	struct strbuf worktree_path = STRBUF_INIT;
> >
> > -	strbuf_add_real_path(&worktree_path, repo_get_common_dir(the_repository));
> > +	strbuf_add_real_path(&worktree_path, repo_get_common_dir(repo));
> >  	strbuf_strip_suffix(&worktree_path, "/.git");
> >
> >  	CALLOC_ARRAY(worktree, 1);
> > -	worktree->repo = the_repository;
> > +	worktree->repo = repo;
> >  	worktree->id = xstrdup("/");
> >  	worktree->path = strbuf_detach(&worktree_path, NULL);
> >  	worktree->is_current = is_current_worktree(worktree);
> > @@ -112,7 +112,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
> >  		 * This check is unnecessary if we're currently in the main worktree,
> >  		 * as prior checks already consulted all configs of the current worktree.
> >  		 */
> > -		(!worktree->is_current && is_main_worktree_bare(the_repository));
> > +		(!worktree->is_current && is_main_worktree_bare(repo));
> >
> >  	if (!skip_reading_head)
> >  		add_head_info(worktree);
>
> Weaning the code from depending on the_repository is mixed into the
> refactoring, which makes me wonder if it is better done in a
> separate patch.  We seriously should consider making this function
> externally visible, as so many callers want to get hold of it.
>
> I also wonder if "struct repository" wants to have a member that
> points at the primary worktree instance, but I think I am getting
> way ahead of myself.

Yes agreed. I will in a seperate patch make get_main_worktree() available
with a struct repository * argument instead of the_repository and then
we can use it in all the places where we need to access the main worktree.

And I think that if we are making get_main_worktree() with a
struct repository * argument available, then we can skip adding a new member
to struct repository and just call get_main_worktree() whenever we need to
access the main worktree with respect to whatever instance of repository we
are working with.

Thanks for reviewing.

Best,
Shreyansh

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Phillip Wood]

I've cc'd Eric for a second opinion

On 13/02/2026 22:29, Junio C Hamano wrote:
> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> 
>> diff --git a/path.c b/path.c
>> index d726537622..4ac86e1e58 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
>>   				   const struct repository *repo,
>>   				   const struct worktree *wt)
>>   {
>> -	if (!wt)
>> -		strbuf_addstr(buf, repo->gitdir);
>> -	else if (!wt->id)
>> +	if (is_main_worktree(wt))
>>   		strbuf_addstr(buf, repo->commondir);
>>   	else
>>   		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
> 
> This is curious.
> 
> We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
> use repo->commondir for both.  For the primary worktree, it ought to
> be the same as repo->gitdir, so it should not matter, but makes me
> wonder what the reason behind this difference in the original.
> 
> We have been assuming that wt==NULL and wt->id==NULL both meant the
> same thing: "we are talking about the primary worktree".  But the
> code around here before this patch seems to behave differently.  Is
> our assumption incorrect and are we making a mistake by conflating
> these two conditions into one?

My understanding is that wt==NULL means "use the current worktree" and 
wt->id==NULL means "this is the main worktree". That would explain why 
we use repo->gitdir above when wt==NULL and repo->commondir when 
wt->id==NULL, as repo->gitdir is the gitdir of the current worktree and 
repo->commondir will be the gitdir of the main worktree. If we look at 
the code in wt-status.c that's passing a NULL worktree it wants to know 
about the status of the current worktree, not the main worktree.

I think that we should add a new function

struct worktree *get_current_worktree(struct repository*);

to worktree.c that constructs a struct worktree using repo->gitdir etc. 
The worktree id is the last path component of repo->gitdir when the 
repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we 
can use that function to get the current worktree rather than passing 
NULL when we call wt_status_check_{rebase,bisect} from 
wt_status_get_state(). We should also think about whether we should 
change wt_status_get_state() to take a "struct worktree*" rather than a 
"struct repository*" instead (I've not looked at the callers to see if 
that's sensible).

With that, we can gradually clean up uses of wt==NULL in the rest of the 
codebase overtime and eventually remove support for it from worktree.c 
rather than having a big flag-day patch. I don't think we need to change 
uses of wt-id==NULL.

Thanks

Phillip

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I've cc'd Eric for a second opinion
>
> On 13/02/2026 22:29, Junio C Hamano wrote:
>> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
>> 
>>> diff --git a/path.c b/path.c
>>> index d726537622..4ac86e1e58 100644
>>> --- a/path.c
>>> +++ b/path.c
>>> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
>>>   				   const struct repository *repo,
>>>   				   const struct worktree *wt)
>>>   {
>>> -	if (!wt)
>>> -		strbuf_addstr(buf, repo->gitdir);
>>> -	else if (!wt->id)
>>> +	if (is_main_worktree(wt))
>>>   		strbuf_addstr(buf, repo->commondir);
>>>   	else
>>>   		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
>> 
>> This is curious.
>> 
>> We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
>> use repo->commondir for both.  For the primary worktree, it ought to
>> be the same as repo->gitdir, so it should not matter, but makes me
>> wonder what the reason behind this difference in the original.
>> 
>> We have been assuming that wt==NULL and wt->id==NULL both meant the
>> same thing: "we are talking about the primary worktree".  But the
>> code around here before this patch seems to behave differently.  Is
>> our assumption incorrect and are we making a mistake by conflating
>> these two conditions into one?
>
> My understanding is that wt==NULL means "use the current worktree" and 
> wt->id==NULL means "this is the main worktree". That would explain why 
> we use repo->gitdir above when wt==NULL and repo->commondir when 
> wt->id==NULL, as repo->gitdir is the gitdir of the current worktree and 
> repo->commondir will be the gitdir of the main worktree. If we look at 
> the code in wt-status.c that's passing a NULL worktree it wants to know 
> about the status of the current worktree, not the main worktree.

Oh, boy.  If that is what wt==NULL means, the above confusion about
the original is perfectly cleared.  We have been operating under a
totally wrong assumption.

> I think that we should add a new function
>
> struct worktree *get_current_worktree(struct repository*);
>
> to worktree.c that constructs a struct worktree using repo->gitdir etc. 

Certainly.

> We should also think about whether we should 
> change wt_status_get_state() to take a "struct worktree *" rather than a 
> "struct repository *" instead (I've not looked at the callers to see if 
> that's sensible).
>
> With that, we can gradually clean up uses of wt==NULL in the rest of the 
> codebase overtime and eventually remove support for it from worktree.c 
> rather than having a big flag-day patch. I don't think we need to change 
> uses of wt-id==NULL.

OK.  I think wt->id==NULL vs wt->id=="/" is about correcting
inconsistencies between the worktree.c and refs.c and certainly can
be done in a separate patch.

We probably should think about how often we use the current one
(presumably almost all the time, given that even wt-status.c API
functions seem to take one), and if the current implementation is
the best way to signal, among a list of worktrees, which one is the
current and which one is the primary.

I do not mind too much about "the primary sits at the beginning of
the resulting list all the time" convention, but wt->is_current bit
looks like a disaster waiting to happen.  To find the current one,
you need to construct a full list and then iterate over the list to
find one with that bit on?  What if there is nobody with the bit or
more than one?  Are callers prepared to notice and report such bugs?

As you said, comparison between gitdir and commondir is sufficient,
then we can lose that bit.  One fewer thing that can go out of sync
takes us one step closer to a cleaner world.

Thanks.

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Shreyansh Paliwal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3738 bytes --]

> I've cc'd Eric for a second opinion
>
> On 13/02/2026 22:29, Junio C Hamano wrote:
> > Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> >
> >> diff --git a/path.c b/path.c
> >> index d726537622..4ac86e1e58 100644
> >> --- a/path.c
> >> +++ b/path.c
> >> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
> >>   				   const struct repository *repo,
> >>   				   const struct worktree *wt)
> >>   {
> >> -	if (!wt)
> >> -		strbuf_addstr(buf, repo->gitdir);
> >> -	else if (!wt->id)
> >> +	if (is_main_worktree(wt))
> >>   		strbuf_addstr(buf, repo->commondir);
> >>   	else
> >>   		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
> >
> > This is curious.
> >
> > We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
> > use repo->commondir for both.  For the primary worktree, it ought to
> > be the same as repo->gitdir, so it should not matter, but makes me
> > wonder what the reason behind this difference in the original.
> >
> > We have been assuming that wt==NULL and wt->id==NULL both meant the
> > same thing: "we are talking about the primary worktree".  But the
> > code around here before this patch seems to behave differently.  Is
> > our assumption incorrect and are we making a mistake by conflating
> > these two conditions into one?
>
> My understanding is that wt==NULL means "use the current worktree" and
> wt->id==NULL means "this is the main worktree". That would explain why
> we use repo->gitdir above when wt==NULL and repo->commondir when
> wt->id==NULL, as repo->gitdir is the gitdir of the current worktree and
> repo->commondir will be the gitdir of the main worktree. If we look at
> the code in wt-status.c that's passing a NULL worktree it wants to know
> about the status of the current worktree, not the main worktree.
>
> I think that we should add a new function
>
> struct worktree *get_current_worktree(struct repository*);
>
> to worktree.c that constructs a struct worktree using repo->gitdir etc.
> The worktree id is the last path component of repo->gitdir when the
> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
> can use that function to get the current worktree rather than passing
> NULL when we call wt_status_check_{rebase,bisect} from
> wt_status_get_state(). We should also think about whether we should
> change wt_status_get_state() to take a "struct worktree*" rather than a
> "struct repository*" instead (I've not looked at the callers to see if
> that's sensible).
>
> With that, we can gradually clean up uses of wt==NULL in the rest of the
> codebase overtime and eventually remove support for it from worktree.c
> rather than having a big flag-day patch. I don't think we need to change
> uses of wt-id==NULL.

Thanks a lot for clarifying. This helps solve the doubt regarding the
different usage of !wt and !wt->id in strbuf_worktree_gitdir(). I realize
we have been under the wrong assumption about what wt == NULL represents.

But I still have a few points where I’m a bit confused,

If wt == NULL is meant to represent the current worktree, then what role
wt->is_current plays in the present implementation, and if they both
represent the same thing then wt->is_current wouldn't make sense if wt is
already NULL in the case of a current worktree.

Beyond representation, I’m not quite understanding on how call sites are
logically differentiating on whether the intent is to 'operate on the
worktree we are in' or 'operate on the primary one'.

And I think if we included both in struct repository (r->main_wt, r->current_wt)
so accessing either of them would be a whole lot easier and also would
prevent confusion in the future.

Let me know what you think.

Best,
Shreyansh

[PATCH 0/2] worktree_git_path(): remove repository argument

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

On 14/02/2026 14:30, Phillip Wood wrote:
>
> I think that we should add a new function
>
> struct worktree *get_current_worktree(struct repository*);
>
> to worktree.c that constructs a struct worktree using repo->gitdir etc.
> The worktree id is the last path component of repo->gitdir when the
> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
> can use that function to get the current worktree rather than passing
> NULL when we call wt_status_check_{rebase,bisect} from
> wt_status_get_state().

Here's what that looks like, the first patch adds
get_worktree_from_repository() and uses it to avoid passing a NULL
worktree to worktree_git_path(). The second patch then removes the
repository argument from that function and always uses wt->repo instead.

Shreyansh - I think your patches to clean up wt-status.c can probably proceed
separately to these if you remove the changes to
wt_status_check_{bisect,rebase}().

Base-Commit: 852829b3dd2fe4e7c7fc4d8badde644cf1b66c74
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fget-current-worktree%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/852829b3d...23b8a355b
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/get-current-worktree/v1


Phillip Wood (2):
  wt-status: avoid passing NULL worktree
  path: remove repository argument from worktree_git_path()

 builtin/fsck.c     |  2 +-
 builtin/worktree.c |  4 ++--
 path.c             |  9 ++++-----
 path.h             |  8 +++-----
 revision.c         |  2 +-
 worktree.c         | 22 +++++++++++++++++++++-
 worktree.h         |  5 ++++-
 wt-status.c        | 29 +++++++++++++++++++----------
 8 files changed, 55 insertions(+), 26 deletions(-)

-- 
2.52.0.362.g884e03848a9

[PATCH 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In preparation for removing the repository argument from
worktree_git_path() add a function to construct a "struct worktree"
from a "struct repository" and use that to avoid passing a NULL
worktree to wt_status_check_bisect() and wt_status_check_rebase().

wt_status_check_bisect() and wt_status_check_rebase() have the following
callers:

 - branch.c:prepare_checked_out_branches() which loops over all
   worktrees.

 - worktree.c:is_worktree_being_rebased() which is called from
   builtin/branch.c:reject_rebase_or_bisect_branch() that loops over all
   worktrees and worktree.c:is_shared_symref() which dereferences wt
   earlier in the function.

 - wt-status:wt_status_get_state() which is updated to avoid passing a
   NULL worktree by this patch.

This updates the only callers that pass a NULL worktree to
worktree_git_path().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c  | 20 ++++++++++++++++++++
 worktree.h  |  5 ++++-
 wt-status.c | 15 ++++++++++++---
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/worktree.c b/worktree.c
index 9308389cb6f..fd182c319b7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -66,6 +66,26 @@ static int is_current_worktree(struct worktree *wt)
 	return is_current;
 }
 
+struct worktree *get_worktree_from_repository(struct repository *repo)
+{
+	struct worktree *wt = xcalloc(1, sizeof(*wt));
+	char *gitdir = absolute_pathdup(repo->gitdir);
+	char *commondir = absolute_pathdup(repo->commondir);
+
+	wt->repo = repo;
+	if (repo->worktree)
+		wt->path = absolute_pathdup(repo->worktree);
+	wt->is_bare = !!repo->worktree;
+	if (fspathcmp(gitdir, commondir))
+		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);
+	wt->is_current = is_current_worktree(wt);
+	add_head_info(wt);
+
+	free(gitdir);
+	free(commondir);
+	return wt;
+}
+
 /*
 * When in a secondary worktree, and when extensions.worktreeConfig
 * is true, only $commondir/config and $commondir/worktrees/<id>/
diff --git a/worktree.h b/worktree.h
index e4bcccdc0ae..b162bbabd50 100644
--- a/worktree.h
+++ b/worktree.h
@@ -38,7 +38,10 @@ struct worktree **get_worktrees(void);
  */
 struct worktree **get_worktrees_without_reading_head(void);
 
-/*
+/* Construct a struct worktree from a struct repository */
+struct worktree *get_worktree_from_repository(struct repository *repo);
+
+ /*
  * Returns 1 if linked worktrees exist, 0 otherwise.
  */
 int submodule_uses_worktrees(const char *path);
diff --git a/wt-status.c b/wt-status.c
index 95942399f8c..2debda534c1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1747,6 +1747,9 @@ int wt_status_check_rebase(const struct worktree *wt,
 {
 	struct stat st;
 
+	if (!wt)
+		BUG("wt_status_check_rebase() called with NULL worktree");
+
 	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
 		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
 			state->am_in_progress = 1;
@@ -1774,6 +1777,9 @@ int wt_status_check_bisect(const struct worktree *wt,
 {
 	struct stat st;
 
+	if (!wt)
+		BUG("wt_status_check_bisect() called with NULL worktree");
+
 	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
 		state->bisecting_from = get_branch(wt, "BISECT_START");
@@ -1819,18 +1825,19 @@ void wt_status_get_state(struct repository *r,
 	struct stat st;
 	struct object_id oid;
 	enum replay_action action;
+	struct worktree *wt = get_worktree_from_repository(r);
 
 	if (!stat(git_path_merge_head(r), &st)) {
-		wt_status_check_rebase(NULL, state);
+		wt_status_check_rebase(wt, state);
 		state->merge_in_progress = 1;
-	} else if (wt_status_check_rebase(NULL, state)) {
+	} else if (wt_status_check_rebase(wt, state)) {
 		;		/* all set */
 	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 		   !repo_get_oid(r, "CHERRY_PICK_HEAD", &oid)) {
 		state->cherry_pick_in_progress = 1;
 		oidcpy(&state->cherry_pick_head_oid, &oid);
 	}
-	wt_status_check_bisect(NULL, state);
+	wt_status_check_bisect(wt, state);
 	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") &&
 	    !repo_get_oid(r, "REVERT_HEAD", &oid)) {
 		state->revert_in_progress = 1;
@@ -1848,6 +1855,8 @@ void wt_status_get_state(struct repository *r,
 	if (get_detached_from)
 		wt_status_get_detached_from(r, state);
 	wt_status_check_sparse_checkout(r, state);
+
+	free_worktree(wt);
 }
 
 static void wt_longstatus_print_state(struct wt_status *s)
-- 
2.52.0.362.g884e03848a9

[PATCH 2/2] path: remove repository argument from worktree_git_path()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

worktree_git_path() takes a struct repository and a struct worktree
which also contains a struct repository. The repository argument
was added by a973f60dc7c (path: stop relying on `the_repository` in
`worktree_git_path()`, 2024-08-13) and exists because the worktree
argument is optional. Having two ways of passing a repository is
a potential foot-gun as if the the worktree argument is present the
repository argument must match the worktree's repository member. Since
the last commit there are no callers that pass a NULL worktree so lets
remove the repository argument. This removes the potential confusion
and lets us delete a number of uses of "the_repository".

worktree_git_path() has the following callers:

 - builtin/worktree.c:validate_no_submodules() which is called from
   check_clean_worktree() and move_worktree(), both of which supply
   a non-NULL worktree.

 - builtin/fsck.c:cmd_fsck() which loops over all worktrees.

 - revision.c:add_index_objects_to_pending() which loops over all
   worktrees.

 - worktree.c:worktree_lock_reason() which dereferences wt before
   calling worktree_git_path().

 - wt-status.c:wt_status_check_bisect() and wt_status_check_rebase()
   which are always called with a non-NULL worktree after the last
   commit.

 - wt-status.c:git_branch() which is only called by
   wt_status_check_bisect() and wt_status_check_rebase().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/fsck.c     |  2 +-
 builtin/worktree.c |  4 ++--
 path.c             |  9 ++++-----
 path.h             |  8 +++-----
 revision.c         |  2 +-
 worktree.c         |  2 +-
 wt-status.c        | 14 +++++++-------
 7 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0512f78a87f..42ba0afb91a 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1137,7 +1137,7 @@ int cmd_fsck(int argc,
 			 * and may get overwritten by other calls
 			 * while we're examining the index.
 			 */
-			path = xstrdup(worktree_git_path(the_repository, wt, "index"));
+			path = xstrdup(worktree_git_path(wt, "index"));
 			wt_gitdir = get_worktree_git_dir(wt);
 
 			read_index_from(&istate, path, wt_gitdir);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 3d6547c23b4..62fd4642e5d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1191,14 +1191,14 @@ static void validate_no_submodules(const struct worktree *wt)
 
 	wt_gitdir = get_worktree_git_dir(wt);
 
-	if (is_directory(worktree_git_path(the_repository, wt, "modules"))) {
+	if (is_directory(worktree_git_path(wt, "modules"))) {
 		/*
 		 * There could be false positives, e.g. the "modules"
 		 * directory exists but is empty. But it's a rare case and
 		 * this simpler check is probably good enough for now.
 		 */
 		found_submodules = 1;
-	} else if (read_index_from(&istate, worktree_git_path(the_repository, wt, "index"),
+	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),
 				   wt_gitdir) > 0) {
 		for (i = 0; i < istate.cache_nr; i++) {
 			struct cache_entry *ce = istate.cache[i];
diff --git a/path.c b/path.c
index d726537622c..073f631b914 100644
--- a/path.c
+++ b/path.c
@@ -486,17 +486,16 @@ const char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname->buf);
 }
 
-const char *worktree_git_path(struct repository *r,
-			      const struct worktree *wt, const char *fmt, ...)
+const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
 {
 	struct strbuf *pathname = get_pathname();
 	va_list args;
 
-	if (wt && wt->repo != r)
-		BUG("worktree not connected to expected repository");
+	if (!wt)
+		BUG("%s() called with NULL worktree", __func__);
 
 	va_start(args, fmt);
-	repo_git_pathv(r, wt, pathname, fmt, args);
+	repo_git_pathv(wt->repo, wt, pathname, fmt, args);
 	va_end(args);
 	return pathname->buf;
 }
diff --git a/path.h b/path.h
index 0ec95a0b079..cbcad254a0a 100644
--- a/path.h
+++ b/path.h
@@ -66,13 +66,11 @@ const char *repo_git_path_replace(struct repository *repo,
 
 /*
  * Similar to repo_git_path() but can produce paths for a specified
- * worktree instead of current one. When no worktree is given, then the path is
- * computed relative to main worktree of the given repository.
+ * worktree instead of current one.
  */
-const char *worktree_git_path(struct repository *r,
-			      const struct worktree *wt,
+const char *worktree_git_path(const struct worktree *wt,
 			      const char *fmt, ...)
-	__attribute__((format (printf, 3, 4)));
+	__attribute__((format (printf, 2, 3)));
 
 /*
  * The `repo_worktree_path` family of functions will construct a path into a
diff --git a/revision.c b/revision.c
index 29972c3a198..ca3481c1902 100644
--- a/revision.c
+++ b/revision.c
@@ -1847,7 +1847,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 		wt_gitdir = get_worktree_git_dir(wt);
 
 		if (read_index_from(&istate,
-				    worktree_git_path(the_repository, wt, "index"),
+				    worktree_git_path(wt, "index"),
 				    wt_gitdir) > 0)
 			do_add_index_objects_to_pending(revs, &istate, flags);
 
diff --git a/worktree.c b/worktree.c
index fd182c319b7..efd2b75608d 100644
--- a/worktree.c
+++ b/worktree.c
@@ -308,7 +308,7 @@ const char *worktree_lock_reason(struct worktree *wt)
 	if (!wt->lock_reason_valid) {
 		struct strbuf path = STRBUF_INIT;
 
-		strbuf_addstr(&path, worktree_git_path(the_repository, wt, "locked"));
+		strbuf_addstr(&path, worktree_git_path(wt, "locked"));
 		if (file_exists(path.buf)) {
 			struct strbuf lock_reason = STRBUF_INIT;
 			if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)
diff --git a/wt-status.c b/wt-status.c
index 2debda534c1..68257d6dfd2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1648,7 +1648,7 @@ static char *get_branch(const struct worktree *wt, const char *path)
 	struct object_id oid;
 	const char *branch_name;
 
-	if (strbuf_read_file(&sb, worktree_git_path(the_repository, wt, "%s", path), 0) <= 0)
+	if (strbuf_read_file(&sb, worktree_git_path(wt, "%s", path), 0) <= 0)
 		goto got_nothing;
 
 	while (sb.len && sb.buf[sb.len - 1] == '\n')
@@ -1750,18 +1750,18 @@ int wt_status_check_rebase(const struct worktree *wt,
 	if (!wt)
 		BUG("wt_status_check_rebase() called with NULL worktree");
 
-	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
-		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
+	if (!stat(worktree_git_path(wt, "rebase-apply"), &st)) {
+		if (!stat(worktree_git_path(wt, "rebase-apply/applying"), &st)) {
 			state->am_in_progress = 1;
-			if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/patch"), &st) && !st.st_size)
+			if (!stat(worktree_git_path(wt, "rebase-apply/patch"), &st) && !st.st_size)
 				state->am_empty_patch = 1;
 		} else {
 			state->rebase_in_progress = 1;
 			state->branch = get_branch(wt, "rebase-apply/head-name");
 			state->onto = get_branch(wt, "rebase-apply/onto");
 		}
-	} else if (!stat(worktree_git_path(the_repository, wt, "rebase-merge"), &st)) {
-		if (!stat(worktree_git_path(the_repository, wt, "rebase-merge/interactive"), &st))
+	} else if (!stat(worktree_git_path(wt, "rebase-merge"), &st)) {
+		if (!stat(worktree_git_path(wt, "rebase-merge/interactive"), &st))
 			state->rebase_interactive_in_progress = 1;
 		else
 			state->rebase_in_progress = 1;
@@ -1780,7 +1780,7 @@ int wt_status_check_bisect(const struct worktree *wt,
 	if (!wt)
 		BUG("wt_status_check_bisect() called with NULL worktree");
 
-	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
+	if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
 		state->bisecting_from = get_branch(wt, "BISECT_START");
 		return 1;
-- 
2.52.0.362.g884e03848a9

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Phillip Wood]

On 15/02/2026 08:56, Shreyansh Paliwal wrote:
>> I've cc'd Eric for a second opinion
>>
>> On 13/02/2026 22:29, Junio C Hamano wrote:
>>> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
>>>
>>>> diff --git a/path.c b/path.c
>>>> index d726537622..4ac86e1e58 100644
>>>> --- a/path.c
>>>> +++ b/path.c
>>>> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
>>>>    				   const struct repository *repo,
>>>>    				   const struct worktree *wt)
>>>>    {
>>>> -	if (!wt)
>>>> -		strbuf_addstr(buf, repo->gitdir);
>>>> -	else if (!wt->id)
>>>> +	if (is_main_worktree(wt))
>>>>    		strbuf_addstr(buf, repo->commondir);
>>>>    	else
>>>>    		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
>>>
>>> This is curious.
>>>
>>> We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
>>> use repo->commondir for both.  For the primary worktree, it ought to
>>> be the same as repo->gitdir, so it should not matter, but makes me
>>> wonder what the reason behind this difference in the original.
>>>
>>> We have been assuming that wt==NULL and wt->id==NULL both meant the
>>> same thing: "we are talking about the primary worktree".  But the
>>> code around here before this patch seems to behave differently.  Is
>>> our assumption incorrect and are we making a mistake by conflating
>>> these two conditions into one?
>>
>> My understanding is that wt==NULL means "use the current worktree" and
>> wt->id==NULL means "this is the main worktree". That would explain why
>> we use repo->gitdir above when wt==NULL and repo->commondir when
>> wt->id==NULL, as repo->gitdir is the gitdir of the current worktree and
>> repo->commondir will be the gitdir of the main worktree. If we look at
>> the code in wt-status.c that's passing a NULL worktree it wants to know
>> about the status of the current worktree, not the main worktree.
>>
>> I think that we should add a new function
>>
>> struct worktree *get_current_worktree(struct repository*);
>>
>> to worktree.c that constructs a struct worktree using repo->gitdir etc.
>> The worktree id is the last path component of repo->gitdir when the
>> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
>> can use that function to get the current worktree rather than passing
>> NULL when we call wt_status_check_{rebase,bisect} from
>> wt_status_get_state(). We should also think about whether we should
>> change wt_status_get_state() to take a "struct worktree*" rather than a
>> "struct repository*" instead (I've not looked at the callers to see if
>> that's sensible).
>>
>> With that, we can gradually clean up uses of wt==NULL in the rest of the
>> codebase overtime and eventually remove support for it from worktree.c
>> rather than having a big flag-day patch. I don't think we need to change
>> uses of wt-id==NULL.
> 
> Thanks a lot for clarifying. This helps solve the doubt regarding the
> different usage of !wt and !wt->id in strbuf_worktree_gitdir(). I realize
> we have been under the wrong assumption about what wt == NULL represents.
> 
> But I still have a few points where I’m a bit confused,
> 
> If wt == NULL is meant to represent the current worktree, then what role
> wt->is_current plays in the present implementation, and if they both
> represent the same thing then wt->is_current wouldn't make sense if wt is
> already NULL in the case of a current worktree.

wt == NULL is a shorthand that callers can use if they don't have a 
struct worktree to pass, it does not replace wt->is_current when listing 
all worktrees with get_worktrees() which returns a NULL terminated list.

> Beyond representation, I’m not quite understanding on how call sites are
> logically differentiating on whether the intent is to 'operate on the
> worktree we are in' or 'operate on the primary one'.

We're nearly always interested in the current one. The primary worktree 
is special in that it cannot be moved or deleted with "git worktree" but 
git commands generally operate on the current worktree and occasionally 
check the state of other worktrees (for example to avoid checking out 
the same branch in two different worktrees).

> And I think if we included both in struct repository (r->main_wt, r->current_wt)
> so accessing either of them would be a whole lot easier and also would
> prevent confusion in the future.

It might be worth adding the current worktree (or probably the worktree 
that the struct repository refers to) to struct repository in the future 
but I think that is outside the scope of cleaning up wt-status.c

Thanks

Phillip

> Let me know what you think.
> 
> Best,
> Shreyansh
> 

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> It might be worth adding the current worktree (or probably the worktree 
> that the struct repository refers to) to struct repository in the future 
> but I think that is outside the scope of cleaning up wt-status.c

Thanks for being conservative.  I would agree that we would need
further thought before making such a change, and if we do not need
it if we have "give me the current worktree struct" call to clean up
the wt-status.c where passing NULL to indicate the "current" is
problematic.

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

On 16/02/2026 16:18, Phillip Wood wrote:
> 
> +struct worktree *get_worktree_from_repository(struct repository *repo)
> +{
> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
> +	char *gitdir = absolute_pathdup(repo->gitdir);
> +	char *commondir = absolute_pathdup(repo->commondir);
> +
> +	wt->repo = repo;
> +	if (repo->worktree)
> +		wt->path = absolute_pathdup(repo->worktree);
> +	wt->is_bare = !!repo->worktree;
> +	if (fspathcmp(gitdir, commondir))
> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);

Oops s/commondir/gitdir/ - I'll wait to see if there are any other 
comments before re-rolling (perhaps with a test that runs git status on 
a rebase in a linked worktree)

Thanks

Phillip

> +	wt->is_current = is_current_worktree(wt);
> +	add_head_info(wt);
> +
> +	free(gitdir);
> +	free(commondir);
> +	return wt;
> +}
> +
>   /*
>   * When in a secondary worktree, and when extensions.worktreeConfig
>   * is true, only $commondir/config and $commondir/worktrees/<id>/
> diff --git a/worktree.h b/worktree.h
> index e4bcccdc0ae..b162bbabd50 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -38,7 +38,10 @@ struct worktree **get_worktrees(void);
>    */
>   struct worktree **get_worktrees_without_reading_head(void);
>   
> -/*
> +/* Construct a struct worktree from a struct repository */
> +struct worktree *get_worktree_from_repository(struct repository *repo);
> +
> + /*
>    * Returns 1 if linked worktrees exist, 0 otherwise.
>    */
>   int submodule_uses_worktrees(const char *path);
> diff --git a/wt-status.c b/wt-status.c
> index 95942399f8c..2debda534c1 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1747,6 +1747,9 @@ int wt_status_check_rebase(const struct worktree *wt,
>   {
>   	struct stat st;
>   
> +	if (!wt)
> +		BUG("wt_status_check_rebase() called with NULL worktree");
> +
>   	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
>   		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
>   			state->am_in_progress = 1;
> @@ -1774,6 +1777,9 @@ int wt_status_check_bisect(const struct worktree *wt,
>   {
>   	struct stat st;
>   
> +	if (!wt)
> +		BUG("wt_status_check_bisect() called with NULL worktree");
> +
>   	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
>   		state->bisect_in_progress = 1;
>   		state->bisecting_from = get_branch(wt, "BISECT_START");
> @@ -1819,18 +1825,19 @@ void wt_status_get_state(struct repository *r,
>   	struct stat st;
>   	struct object_id oid;
>   	enum replay_action action;
> +	struct worktree *wt = get_worktree_from_repository(r);
>   
>   	if (!stat(git_path_merge_head(r), &st)) {
> -		wt_status_check_rebase(NULL, state);
> +		wt_status_check_rebase(wt, state);
>   		state->merge_in_progress = 1;
> -	} else if (wt_status_check_rebase(NULL, state)) {
> +	} else if (wt_status_check_rebase(wt, state)) {
>   		;		/* all set */
>   	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
>   		   !repo_get_oid(r, "CHERRY_PICK_HEAD", &oid)) {
>   		state->cherry_pick_in_progress = 1;
>   		oidcpy(&state->cherry_pick_head_oid, &oid);
>   	}
> -	wt_status_check_bisect(NULL, state);
> +	wt_status_check_bisect(wt, state);
>   	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") &&
>   	    !repo_get_oid(r, "REVERT_HEAD", &oid)) {
>   		state->revert_in_progress = 1;
> @@ -1848,6 +1855,8 @@ void wt_status_get_state(struct repository *r,
>   	if (get_detached_from)
>   		wt_status_get_detached_from(r, state);
>   	wt_status_check_sparse_checkout(r, state);
> +
> +	free_worktree(wt);
>   }
>   
>   static void wt_longstatus_print_state(struct wt_status *s)

Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Shreyansh Paliwal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 4836 bytes --]

> On 15/02/2026 08:56, Shreyansh Paliwal wrote:
> >> I've cc'd Eric for a second opinion
> >>
> >> On 13/02/2026 22:29, Junio C Hamano wrote:
> >>> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> >>>
> >>>> diff --git a/path.c b/path.c
> >>>> index d726537622..4ac86e1e58 100644
> >>>> --- a/path.c
> >>>> +++ b/path.c
> >>>> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
> >>>>    				   const struct repository *repo,
> >>>>    				   const struct worktree *wt)
> >>>>    {
> >>>> -	if (!wt)
> >>>> -		strbuf_addstr(buf, repo->gitdir);
> >>>> -	else if (!wt->id)
> >>>> +	if (is_main_worktree(wt))
> >>>>    		strbuf_addstr(buf, repo->commondir);
> >>>>    	else
> >>>>    		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
> >>>
> >>> This is curious.
> >>>
> >>> We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
> >>> use repo->commondir for both.  For the primary worktree, it ought to
> >>> be the same as repo->gitdir, so it should not matter, but makes me
> >>> wonder what the reason behind this difference in the original.
> >>>
> >>> We have been assuming that wt==NULL and wt->id==NULL both meant the
> >>> same thing: "we are talking about the primary worktree".  But the
> >>> code around here before this patch seems to behave differently.  Is
> >>> our assumption incorrect and are we making a mistake by conflating
> >>> these two conditions into one?
> >>
> >> My understanding is that wt==NULL means "use the current worktree" and
> >> wt->id==NULL means "this is the main worktree". That would explain why
> >> we use repo->gitdir above when wt==NULL and repo->commondir when
> >> wt->id==NULL, as repo->gitdir is the gitdir of the current worktree and
> >> repo->commondir will be the gitdir of the main worktree. If we look at
> >> the code in wt-status.c that's passing a NULL worktree it wants to know
> >> about the status of the current worktree, not the main worktree.
> >>
> >> I think that we should add a new function
> >>
> >> struct worktree *get_current_worktree(struct repository*);
> >>
> >> to worktree.c that constructs a struct worktree using repo->gitdir etc.
> >> The worktree id is the last path component of repo->gitdir when the
> >> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
> >> can use that function to get the current worktree rather than passing
> >> NULL when we call wt_status_check_{rebase,bisect} from
> >> wt_status_get_state(). We should also think about whether we should
> >> change wt_status_get_state() to take a "struct worktree*" rather than a
> >> "struct repository*" instead (I've not looked at the callers to see if
> >> that's sensible).
> >>
> >> With that, we can gradually clean up uses of wt==NULL in the rest of the
> >> codebase overtime and eventually remove support for it from worktree.c
> >> rather than having a big flag-day patch. I don't think we need to change
> >> uses of wt-id==NULL.
> >
> > Thanks a lot for clarifying. This helps solve the doubt regarding the
> > different usage of !wt and !wt->id in strbuf_worktree_gitdir(). I realize
> > we have been under the wrong assumption about what wt == NULL represents.
> >
> > But I still have a few points where I’m a bit confused,
> >
> > If wt == NULL is meant to represent the current worktree, then what role
> > wt->is_current plays in the present implementation, and if they both
> > represent the same thing then wt->is_current wouldn't make sense if wt is
> > already NULL in the case of a current worktree.
>
> wt == NULL is a shorthand that callers can use if they don't have a
> struct worktree to pass, it does not replace wt->is_current when listing
> all worktrees with get_worktrees() which returns a NULL terminated list.

Ah, yes got it.

> > Beyond representation, I’m not quite understanding on how call sites are
> > logically differentiating on whether the intent is to 'operate on the
> > worktree we are in' or 'operate on the primary one'.
>
> We're nearly always interested in the current one. The primary worktree
> is special in that it cannot be moved or deleted with "git worktree" but
> git commands generally operate on the current worktree and occasionally
> check the state of other worktrees (for example to avoid checking out
> the same branch in two different worktrees).

Hmm. Understood.

> > And I think if we included both in struct repository (r->main_wt, r->current_wt)
> > so accessing either of them would be a whole lot easier and also would
> > prevent confusion in the future.
>
> It might be worth adding the current worktree (or probably the worktree
> that the struct repository refers to) to struct repository in the future
> but I think that is outside the scope of cleaning up wt-status.c

Right.

Thanks for clearing it out :)

Best,
Shreyansh

Re: [PATCH 0/2] worktree_git_path(): remove repository argument

[Shreyansh Paliwal]

> On 14/02/2026 14:30, Phillip Wood wrote:
> >
> > I think that we should add a new function
> >
> > struct worktree *get_current_worktree(struct repository*);
> >
> > to worktree.c that constructs a struct worktree using repo->gitdir etc.
> > The worktree id is the last path component of repo->gitdir when the
> > repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
> > can use that function to get the current worktree rather than passing
> > NULL when we call wt_status_check_{rebase,bisect} from
> > wt_status_get_state().
>
> Here's what that looks like, the first patch adds
> get_worktree_from_repository() and uses it to avoid passing a NULL
> worktree to worktree_git_path(). The second patch then removes the
> repository argument from that function and always uses wt->repo instead.
>
> Shreyansh - I think your patches to clean up wt-status.c can probably proceed
> separately to these if you remove the changes to
> wt_status_check_{bisect,rebase}().

Cool. I'll send a revised version on the original thread.

Best,
Shreyansh

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Shreyansh Paliwal]

> On 16/02/2026 16:18, Phillip Wood wrote:
> >
> > +struct worktree *get_worktree_from_repository(struct repository *repo)
> > +{
> > +	struct worktree *wt = xcalloc(1, sizeof(*wt));
> > +	char *gitdir = absolute_pathdup(repo->gitdir);
> > +	char *commondir = absolute_pathdup(repo->commondir);
> > +
> > +	wt->repo = repo;
> > +	if (repo->worktree)
> > +		wt->path = absolute_pathdup(repo->worktree);
> > +	wt->is_bare = !!repo->worktree;
> > +	if (fspathcmp(gitdir, commondir))
> > +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);
>
> Oops s/commondir/gitdir/ - I'll wait to see if there are any other
> comments before re-rolling (perhaps with a test that runs git status on
> a rebase in a linked worktree)
>
> Thanks
>
> Phillip

I wanted to just check for my understanding: the NULL usage of worktree in
get_worktree_git_dir() caller, repo_git_pathv() callers and inside function
add_reflogs_to_pending() is intentionally left unchanged for now,
and is meant for a follow-up once this gets gets finalized.
or it is out of scope wrt this cleanup?

Best,
Shreyansh

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

On 17/02/2026 10:18, Shreyansh Paliwal wrote:
> 
> I wanted to just check for my understanding: the NULL usage of worktree in
> get_worktree_git_dir() caller, repo_git_pathv() callers and inside function
> add_reflogs_to_pending() is intentionally left unchanged for now,
> and is meant for a follow-up once this gets gets finalized.
> or it is out of scope wrt this cleanup?

I left those out as they're not needed for cleaning up wt-status.c. They 
can be cleaned up separately if you're still interesting in working on 
that. It would certainly be worth removing "the_repository" from 
get_worktree_git_dir(). The others are not quite so bad as they don't 
use "the_repository".

Thanks

Phillip

Re: [PATCH 0/2] worktree_git_path(): remove repository argument

[Phillip Wood]

On 17/02/2026 10:12, Shreyansh Paliwal wrote:
>> On 14/02/2026 14:30, Phillip Wood wrote:
>>>
>>> I think that we should add a new function
>>>
>>> struct worktree *get_current_worktree(struct repository*);
>>>
>>> to worktree.c that constructs a struct worktree using repo->gitdir etc.
>>> The worktree id is the last path component of repo->gitdir when the
>>> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
>>> can use that function to get the current worktree rather than passing
>>> NULL when we call wt_status_check_{rebase,bisect} from
>>> wt_status_get_state().
>>
>> Here's what that looks like, the first patch adds
>> get_worktree_from_repository() and uses it to avoid passing a NULL
>> worktree to worktree_git_path(). The second patch then removes the
>> repository argument from that function and always uses wt->repo instead.
>>
>> Shreyansh - I think your patches to clean up wt-status.c can probably proceed
>> separately to these if you remove the changes to
>> wt_status_check_{bisect,rebase}().
> 
> Cool. I'll send a revised version on the original thread.

Great, I hope I'm not stepping on your toes posting these patches. By 
the time I'd worked out what was needed and checked all the callers were 
passing a non-NULL worktree argument I had the code changes and commit 
messages so I thought I'd post them.

Thanks

Phillip

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Shreyansh Paliwal]

> On 17/02/2026 10:18, Shreyansh Paliwal wrote:
> >
> > I wanted to just check for my understanding: the NULL usage of worktree in
> > get_worktree_git_dir() caller, repo_git_pathv() callers and inside function
> > add_reflogs_to_pending() is intentionally left unchanged for now,
> > and is meant for a follow-up once this gets gets finalized.
> > or it is out of scope wrt this cleanup?
>
> I left those out as they're not needed for cleaning up wt-status.c. They
> can be cleaned up separately if you're still interesting in working on
> that. It would certainly be worth removing "the_repository" from
> get_worktree_git_dir(). The others are not quite so bad as they don't
> use "the_repository".

Thanks, that makes sense.
I will send a patch for get_worktree_git_dir() sometime later.

Best,
Shreyansh

Re: [PATCH 0/2] worktree_git_path(): remove repository argument

[Shreyansh Paliwal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1635 bytes --]

> On 17/02/2026 10:12, Shreyansh Paliwal wrote:
> >> On 14/02/2026 14:30, Phillip Wood wrote:
> >>>
> >>> I think that we should add a new function
> >>>
> >>> struct worktree *get_current_worktree(struct repository*);
> >>>
> >>> to worktree.c that constructs a struct worktree using repo->gitdir etc.
> >>> The worktree id is the last path component of repo->gitdir when the
> >>> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we
> >>> can use that function to get the current worktree rather than passing
> >>> NULL when we call wt_status_check_{rebase,bisect} from
> >>> wt_status_get_state().
> >>
> >> Here's what that looks like, the first patch adds
> >> get_worktree_from_repository() and uses it to avoid passing a NULL
> >> worktree to worktree_git_path(). The second patch then removes the
> >> repository argument from that function and always uses wt->repo instead.
> >>
> >> Shreyansh - I think your patches to clean up wt-status.c can probably proceed
> >> separately to these if you remove the changes to
> >> wt_status_check_{bisect,rebase}().
> >
> > Cool. I'll send a revised version on the original thread.
>
> Great, I hope I'm not stepping on your toes posting these patches. By
> the time I'd worked out what was needed and checked all the callers were
> passing a non-NULL worktree argument I had the code changes and commit
> messages so I thought I'd post them.

Not at all, I’m glad you posted them. They clarified the right direction
and are logically more fit.
I got to learn a lot about the worktree API while working on this,
so that was very helpful. Thanks :)

Best,
Shreyansh

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Karthik Nayak]

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> In preparation for removing the repository argument from
> worktree_git_path() add a function to construct a "struct worktree"
> from a "struct repository" and use that to avoid passing a NULL
> worktree to wt_status_check_bisect() and wt_status_check_rebase().
>

Okay this makes sense, I'm curious how 'wt->id = NULL' is going to be
handled. Let's see

> wt_status_check_bisect() and wt_status_check_rebase() have the following
> callers:
>
>  - branch.c:prepare_checked_out_branches() which loops over all
>    worktrees.
>
>  - worktree.c:is_worktree_being_rebased() which is called from
>    builtin/branch.c:reject_rebase_or_bisect_branch() that loops over all
>    worktrees and worktree.c:is_shared_symref() which dereferences wt
>    earlier in the function.
>
>  - wt-status:wt_status_get_state() which is updated to avoid passing a
>    NULL worktree by this patch.
>
> This updates the only callers that pass a NULL worktree to
> worktree_git_path().
>

I was thinking surely there must be other places where we also pass NULL
for worktree, but doesn't seem like there are any such instances.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  worktree.c  | 20 ++++++++++++++++++++
>  worktree.h  |  5 ++++-
>  wt-status.c | 15 ++++++++++++---
>  3 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index 9308389cb6f..fd182c319b7 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -66,6 +66,26 @@ static int is_current_worktree(struct worktree *wt)
>  	return is_current;
>  }
>
> +struct worktree *get_worktree_from_repository(struct repository *repo)
> +{
> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
> +	char *gitdir = absolute_pathdup(repo->gitdir);
> +	char *commondir = absolute_pathdup(repo->commondir);
> +
> +	wt->repo = repo;
> +	if (repo->worktree)
> +		wt->path = absolute_pathdup(repo->worktree);

Shouldn't this always be set? I guess my question is, will
`repo->worktree` ever be NULL?

> +	wt->is_bare = !!repo->worktree;
> +	if (fspathcmp(gitdir, commondir))
> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);

So here we continue to treat NULL as the main worktree. Okay.

> +	wt->is_current = is_current_worktree(wt);

Since we're getting the worktree from the repo, shouldn't this be
'true'?

> +	add_head_info(wt);
> +
> +	free(gitdir);
> +	free(commondir);
> +	return wt;
> +}
> +
>  /*
>  * When in a secondary worktree, and when extensions.worktreeConfig
>  * is true, only $commondir/config and $commondir/worktrees/<id>/
> diff --git a/worktree.h b/worktree.h
> index e4bcccdc0ae..b162bbabd50 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -38,7 +38,10 @@ struct worktree **get_worktrees(void);
>   */
>  struct worktree **get_worktrees_without_reading_head(void);
>
> -/*
> +/* Construct a struct worktree from a struct repository */
> +struct worktree *get_worktree_from_repository(struct repository *repo);
> +
> + /*

Nit: extra space?

>   * Returns 1 if linked worktrees exist, 0 otherwise.
>   */
>  int submodule_uses_worktrees(const char *path);

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/2] path: remove repository argument from worktree_git_path()

[Karthik Nayak]

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> worktree_git_path() takes a struct repository and a struct worktree
> which also contains a struct repository. The repository argument
> was added by a973f60dc7c (path: stop relying on `the_repository` in
> `worktree_git_path()`, 2024-08-13) and exists because the worktree
> argument is optional. Having two ways of passing a repository is
> a potential foot-gun as if the the worktree argument is present the
> repository argument must match the worktree's repository member. Since
> the last commit there are no callers that pass a NULL worktree so lets
> remove the repository argument. This removes the potential confusion
> and lets us delete a number of uses of "the_repository".
>
> worktree_git_path() has the following callers:
>
>  - builtin/worktree.c:validate_no_submodules() which is called from
>    check_clean_worktree() and move_worktree(), both of which supply
>    a non-NULL worktree.
>
>  - builtin/fsck.c:cmd_fsck() which loops over all worktrees.
>
>  - revision.c:add_index_objects_to_pending() which loops over all
>    worktrees.
>
>  - worktree.c:worktree_lock_reason() which dereferences wt before
>    calling worktree_git_path().
>
>  - wt-status.c:wt_status_check_bisect() and wt_status_check_rebase()
>    which are always called with a non-NULL worktree after the last
>    commit.
>
>  - wt-status.c:git_branch() which is only called by
>    wt_status_check_bisect() and wt_status_check_rebase().
>

Nice. Well explained, the patch looks good to me :)

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Oops s/commondir/gitdir/ - I'll wait to see if there are any other 
> comments before re-rolling (perhaps with a test that runs git status on 
> a rebase in a linked worktree)

Such a test that exposes behaviour difference would be very much
appreciated.

Thanks.

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> In preparation for removing the repository argument from
> worktree_git_path() add a function to construct a "struct worktree"
> from a "struct repository" and use that to avoid passing a NULL
> worktree to wt_status_check_bisect() and wt_status_check_rebase().

Hmph, I am afraid that

    "Construct a struct worktree from a struct repository"

is not quite sufficient.  A repository can have more than one
worktrees, so if you give a repository as a parameter, there needs a
way for the implementation of this helper function to identify which
one of them to construct a struct worktree for, and more importantly
for you as the caller to be able to expect which one the implementation
would pick, and what that particular worktree among many _means_ to you.

I know that the implementation uses repo->worktree but what does
that path mean in the world-view of the worktree API set?

I am guessing that it is what the worktree API calls "current", but
if so, perhaps the function should be explained with that word in
it, and the function name should also contain that word, no?

> +struct worktree *get_worktree_from_repository(struct repository *repo)
> +{
> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
> +	char *gitdir = absolute_pathdup(repo->gitdir);
> +	char *commondir = absolute_pathdup(repo->commondir);
> +
> +	wt->repo = repo;
> +	if (repo->worktree)
> +		wt->path = absolute_pathdup(repo->worktree);

So, if the repository instance knows where the worktree is, we use
that to wt->path.  Otherwise wt->path is left NULL.

> +	wt->is_bare = !!repo->worktree;

I may be confused but don't we have one ! too many?  If we have a
worktree directory, "git checkout" would check the files there, and
that is not quite a "bare" repository, no?

> +	if (fspathcmp(gitdir, commondir))
> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);

OK.  So gitdir and commondir would be the same for the primary and
for everybody else we'd have "id" as the last directory component of
the commondir.

> +	wt->is_current = is_current_worktree(wt);

Oh, so I guessed wrong and this is not about "current" worktree?
What does the directory pointed at by repo->worktree mean to the
callers of this function?  I somehow thought that is_current would
be always 1 here,.

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

On 17/02/2026 18:47, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> In preparation for removing the repository argument from
>> worktree_git_path() add a function to construct a "struct worktree"
>> from a "struct repository" and use that to avoid passing a NULL
>> worktree to wt_status_check_bisect() and wt_status_check_rebase().
> 
> Hmph, I am afraid that
> 
>      "Construct a struct worktree from a struct repository"
> 
> is not quite sufficient.  A repository can have more than one
> worktrees, so if you give a repository as a parameter, there needs a
> way for the implementation of this helper function to identify which
> one of them to construct a struct worktree for, and more importantly
> for you as the caller to be able to expect which one the implementation
> would pick, and what that particular worktree among many _means_ to you.
> 
> I know that the implementation uses repo->worktree but what does
> that path mean in the world-view of the worktree API set?

While a repository can have multiple worktrees, a "struct repository" 
points to a particular worktree within that repository via the gitdir 
and worktree members. I'll try and make it clearer that the function 
returns a struct worktree corresponding to those members.

> I am guessing that it is what the worktree API calls "current", but
> if so, perhaps the function should be explained with that word in
> it, and the function name should also contain that word, no?

That's what I thought initially. However is_current_worktree() is 
defined in terms of "the_repository" rather than "wt->repo". That means 
all the struct worktrees within a single process agree on the "current" 
worktree but it is suprising that if "wt->path" matches 
"wt->repo->worktree" it is not necessarily the "current" worktree. I'm 
not sure if we want to change the definition of is_current_worktree() to 
use "wt->repo" rather than "the_repository", but if we do I think we can 
do that separately.

>> +struct worktree *get_worktree_from_repository(struct repository *repo)
>> +{
>> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
>> +	char *gitdir = absolute_pathdup(repo->gitdir);
>> +	char *commondir = absolute_pathdup(repo->commondir);
>> +
>> +	wt->repo = repo;
>> +	if (repo->worktree)
>> +		wt->path = absolute_pathdup(repo->worktree);
> 
> So, if the repository instance knows where the worktree is, we use
> that to wt->path.  Otherwise wt->path is left NULL.

That's actually a bug, we should be using repo->gitdir when the 
repository is bare.

>> +	wt->is_bare = !!repo->worktree;
> 
> I may be confused but don't we have one ! too many?  If we have a
> worktree directory, "git checkout" would check the files there, and
> that is not quite a "bare" repository, no?

Yes, it should be "wt->is_bare = !repo->worktree;"

>> +	if (fspathcmp(gitdir, commondir))
>> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);
> 
> OK.  So gitdir and commondir would be the same for the primary and
> for everybody else we'd have "id" as the last directory component of
> the commondir.
> 
>> +	wt->is_current = is_current_worktree(wt);
> 
> Oh, so I guessed wrong and this is not about "current" worktree?
> What does the directory pointed at by repo->worktree mean to the
> callers of this function?  I somehow thought that is_current would
> be always 1 here,.

As explained above "repo->worktree" means nothing to 
is_current_worktree() because it uses "the_repository" instead of 
"wt->repo".

I'll re-roll with the fixes above and a bit more detail in the commit 
message about is_current_worktree() and the that the "struct worktree" 
instance corresponds to worktree that uses repo->gitdir

Thanks

Phillip

Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

On 17/02/2026 17:46, Karthik Nayak wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> This updates the only callers that pass a NULL worktree to
>> worktree_git_path().
>>
> 
> I was thinking surely there must be other places where we also pass NULL
> for worktree, but doesn't seem like there are any such instances.

Yes, I was pleasantly surprised there weren't more sites to convert.

>> +struct worktree *get_worktree_from_repository(struct repository *repo)
>> +{
>> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
>> +	char *gitdir = absolute_pathdup(repo->gitdir);
>> +	char *commondir = absolute_pathdup(repo->commondir);
>> +
>> +	wt->repo = repo;
>> +	if (repo->worktree)
>> +		wt->path = absolute_pathdup(repo->worktree);
> 
> Shouldn't this always be set? I guess my question is, will
> `repo->worktree` ever be NULL?

Oh, wt->path should never be NULL. repo->worktree is NULL in bare 
repositories but then we should use repo->gitdir as the worktree path.

>> +	wt->is_bare = !!repo->worktree;
>> +	if (fspathcmp(gitdir, commondir))
>> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);
> 
> So here we continue to treat NULL as the main worktree. Okay.
> 
>> +	wt->is_current = is_current_worktree(wt);
> 
> Since we're getting the worktree from the repo, shouldn't this be
> 'true'?

That's what I thought initially. However is_current_worktree() compares 
"repo->gitdir" to "the_repository->gitdir" so the "current" worktree is 
the one that the process was started in, which is not necessarily the 
same as the one matching "wt->repo->gitdir". It's possible that we will 
want to change that definition in the future but I opted to make this 
function consistent with the status quo.

>> -/*
>> +/* Construct a struct worktree from a struct repository */
>> +struct worktree *get_worktree_from_repository(struct repository *repo);
>> +
>> + /*
> 
> Nit: extra space?

Good spot I'll fix it

Thanks for the review

Phillip

[PATCH v2 0/2] worktree_git_path(): remove repository argument

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

These patches remove the repository argument from worktree_git_path()
in favor of using the repository in the "sturct worktree" argument.
This enables us to remove some uses of "the_repository". The first
patch adds a new function git_worktree_from_repository() to construct
a "struct worktree" based on the repository's worktree and uses it
to avoid passing a NULL worktree to worktree_git_path(). The second
patch then removes the repository argument from that function and
always uses the repository in the worktree argument instead.

Thanks to Karthik and Junio for their comments, here are the changes
since V1:
 - always set worktree path - for bare repositories the worktree path
   is repo->gitdir
 - fix the worktree bareness (there were too many negations)
 - fix the wortkree id (it comes from repo->gitdir not repo->commondir)
 - add a test for "git status" on a rebase in a linked worktree.
 - expand the commit message to explain
   (a) that we use the "gitdir" and "worktree" members of "struct
       repository" to construct the "struct worktree"
   (b) how the "current" worktree is determined

Base-Commit: 852829b3dd2fe4e7c7fc4d8badde644cf1b66c74
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fget-current-worktree%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/852829b3d...db9d519cb
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/get-current-worktree/v2


Phillip Wood (2):
  wt-status: avoid passing NULL worktree
  path: remove repository argument from worktree_git_path()

 builtin/fsck.c         |  2 +-
 builtin/worktree.c     |  4 ++--
 path.c                 |  9 ++++-----
 path.h                 |  8 +++-----
 revision.c             |  2 +-
 t/t7512-status-help.sh |  9 +++++++++
 worktree.c             | 22 +++++++++++++++++++++-
 worktree.h             |  6 ++++++
 wt-status.c            | 29 +++++++++++++++++++----------
 9 files changed, 66 insertions(+), 25 deletions(-)

Range-diff against v1:
1:  409871a7d52 ! 1:  902295b8714 wt-status: avoid passing NULL worktree
    @@ Commit message
     
         In preparation for removing the repository argument from
         worktree_git_path() add a function to construct a "struct worktree"
    -    from a "struct repository" and use that to avoid passing a NULL
    -    worktree to wt_status_check_bisect() and wt_status_check_rebase().
    +    from a "struct repository" using its "gitdir" and "worktree"
    +    members. This function is then used to avoid passing a NULL worktree to
    +    wt_status_check_bisect() and wt_status_check_rebase(). In general the
    +    "struct worktree" returned may not correspond to the "current" worktree
    +    defined by is_current_worktree() as that function uses "the_repository"
    +    rather than "wt->repo" when deciding which worktree is "current". In
    +    practice the "struct repository" we pass corresponds to "the_repository"
    +    as we only ever operate on a single repository at the moment.
     
         wt_status_check_bisect() and wt_status_check_rebase() have the following
         callers:
    @@ Commit message
            NULL worktree by this patch.
     
         This updates the only callers that pass a NULL worktree to
    -    worktree_git_path().
    +    worktree_git_path(). A new test is added to check that "git status"
    +    detects a rebase in a linked worktree.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    + ## t/t7512-status-help.sh ##
    +@@ t/t7512-status-help.sh: EOF
    + 	test_cmp expected actual
    + '
    + 
    ++test_expect_success 'rebase in a linked worktree' '
    ++	test_might_fail git rebase --abort &&
    ++	git worktree add wt &&
    ++	test_when_finished "test_might_fail git -C wt rebase --abort;
    ++				git worktree remove wt" &&
    ++	GIT_SEQUENCE_EDITOR="echo break >" git -C wt rebase -i HEAD &&
    ++	git -C wt status >actual &&
    ++	test_grep "interactive rebase in progress" actual
    ++'
    + 
    + test_expect_success 'prepare am_session' '
    + 	git reset --hard main &&
    +
      ## worktree.c ##
     @@ worktree.c: static int is_current_worktree(struct worktree *wt)
      	return is_current;
    @@ worktree.c: static int is_current_worktree(struct worktree *wt)
     +	char *commondir = absolute_pathdup(repo->commondir);
     +
     +	wt->repo = repo;
    -+	if (repo->worktree)
    -+		wt->path = absolute_pathdup(repo->worktree);
    -+	wt->is_bare = !!repo->worktree;
    ++	wt->path = absolute_pathdup(repo->worktree ? repo->worktree
    ++						   : repo->gitdir);
    ++	wt->is_bare = !repo->worktree;
     +	if (fspathcmp(gitdir, commondir))
    -+		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);
    ++		wt->id = xstrdup(find_last_dir_sep(gitdir) + 1);
     +	wt->is_current = is_current_worktree(wt);
     +	add_head_info(wt);
     +
    @@ worktree.h: struct worktree **get_worktrees(void);
       */
      struct worktree **get_worktrees_without_reading_head(void);
      
    --/*
    -+/* Construct a struct worktree from a struct repository */
    ++/*
    ++ * Construct a struct worktree corresponding to repo->gitdir and
    ++ * repo->worktree.
    ++ */
     +struct worktree *get_worktree_from_repository(struct repository *repo);
     +
    -+ /*
    + /*
       * Returns 1 if linked worktrees exist, 0 otherwise.
       */
    - int submodule_uses_worktrees(const char *path);
     
      ## wt-status.c ##
     @@ wt-status.c: int wt_status_check_rebase(const struct worktree *wt,
2:  23b8a355b41 = 2:  db9d519cbda path: remove repository argument from worktree_git_path()
-- 
2.52.0.362.g884e03848a9

[PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In preparation for removing the repository argument from
worktree_git_path() add a function to construct a "struct worktree"
from a "struct repository" using its "gitdir" and "worktree"
members. This function is then used to avoid passing a NULL worktree to
wt_status_check_bisect() and wt_status_check_rebase(). In general the
"struct worktree" returned may not correspond to the "current" worktree
defined by is_current_worktree() as that function uses "the_repository"
rather than "wt->repo" when deciding which worktree is "current". In
practice the "struct repository" we pass corresponds to "the_repository"
as we only ever operate on a single repository at the moment.

wt_status_check_bisect() and wt_status_check_rebase() have the following
callers:

 - branch.c:prepare_checked_out_branches() which loops over all
   worktrees.

 - worktree.c:is_worktree_being_rebased() which is called from
   builtin/branch.c:reject_rebase_or_bisect_branch() that loops over all
   worktrees and worktree.c:is_shared_symref() which dereferences wt
   earlier in the function.

 - wt-status:wt_status_get_state() which is updated to avoid passing a
   NULL worktree by this patch.

This updates the only callers that pass a NULL worktree to
worktree_git_path(). A new test is added to check that "git status"
detects a rebase in a linked worktree.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t7512-status-help.sh |  9 +++++++++
 worktree.c             | 20 ++++++++++++++++++++
 worktree.h             |  6 ++++++
 wt-status.c            | 15 ++++++++++++---
 4 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 25e8e9711f8..08e82f79140 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -594,6 +594,15 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'rebase in a linked worktree' '
+	test_might_fail git rebase --abort &&
+	git worktree add wt &&
+	test_when_finished "test_might_fail git -C wt rebase --abort;
+				git worktree remove wt" &&
+	GIT_SEQUENCE_EDITOR="echo break >" git -C wt rebase -i HEAD &&
+	git -C wt status >actual &&
+	test_grep "interactive rebase in progress" actual
+'
 
 test_expect_success 'prepare am_session' '
 	git reset --hard main &&
diff --git a/worktree.c b/worktree.c
index 9308389cb6f..218c332a66d 100644
--- a/worktree.c
+++ b/worktree.c
@@ -66,6 +66,26 @@ static int is_current_worktree(struct worktree *wt)
 	return is_current;
 }
 
+struct worktree *get_worktree_from_repository(struct repository *repo)
+{
+	struct worktree *wt = xcalloc(1, sizeof(*wt));
+	char *gitdir = absolute_pathdup(repo->gitdir);
+	char *commondir = absolute_pathdup(repo->commondir);
+
+	wt->repo = repo;
+	wt->path = absolute_pathdup(repo->worktree ? repo->worktree
+						   : repo->gitdir);
+	wt->is_bare = !repo->worktree;
+	if (fspathcmp(gitdir, commondir))
+		wt->id = xstrdup(find_last_dir_sep(gitdir) + 1);
+	wt->is_current = is_current_worktree(wt);
+	add_head_info(wt);
+
+	free(gitdir);
+	free(commondir);
+	return wt;
+}
+
 /*
 * When in a secondary worktree, and when extensions.worktreeConfig
 * is true, only $commondir/config and $commondir/worktrees/<id>/
diff --git a/worktree.h b/worktree.h
index e4bcccdc0ae..06efe26b835 100644
--- a/worktree.h
+++ b/worktree.h
@@ -38,6 +38,12 @@ struct worktree **get_worktrees(void);
  */
 struct worktree **get_worktrees_without_reading_head(void);
 
+/*
+ * Construct a struct worktree corresponding to repo->gitdir and
+ * repo->worktree.
+ */
+struct worktree *get_worktree_from_repository(struct repository *repo);
+
 /*
  * Returns 1 if linked worktrees exist, 0 otherwise.
  */
diff --git a/wt-status.c b/wt-status.c
index 95942399f8c..2debda534c1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1747,6 +1747,9 @@ int wt_status_check_rebase(const struct worktree *wt,
 {
 	struct stat st;
 
+	if (!wt)
+		BUG("wt_status_check_rebase() called with NULL worktree");
+
 	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
 		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
 			state->am_in_progress = 1;
@@ -1774,6 +1777,9 @@ int wt_status_check_bisect(const struct worktree *wt,
 {
 	struct stat st;
 
+	if (!wt)
+		BUG("wt_status_check_bisect() called with NULL worktree");
+
 	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
 		state->bisecting_from = get_branch(wt, "BISECT_START");
@@ -1819,18 +1825,19 @@ void wt_status_get_state(struct repository *r,
 	struct stat st;
 	struct object_id oid;
 	enum replay_action action;
+	struct worktree *wt = get_worktree_from_repository(r);
 
 	if (!stat(git_path_merge_head(r), &st)) {
-		wt_status_check_rebase(NULL, state);
+		wt_status_check_rebase(wt, state);
 		state->merge_in_progress = 1;
-	} else if (wt_status_check_rebase(NULL, state)) {
+	} else if (wt_status_check_rebase(wt, state)) {
 		;		/* all set */
 	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 		   !repo_get_oid(r, "CHERRY_PICK_HEAD", &oid)) {
 		state->cherry_pick_in_progress = 1;
 		oidcpy(&state->cherry_pick_head_oid, &oid);
 	}
-	wt_status_check_bisect(NULL, state);
+	wt_status_check_bisect(wt, state);
 	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") &&
 	    !repo_get_oid(r, "REVERT_HEAD", &oid)) {
 		state->revert_in_progress = 1;
@@ -1848,6 +1855,8 @@ void wt_status_get_state(struct repository *r,
 	if (get_detached_from)
 		wt_status_get_detached_from(r, state);
 	wt_status_check_sparse_checkout(r, state);
+
+	free_worktree(wt);
 }
 
 static void wt_longstatus_print_state(struct wt_status *s)
-- 
2.52.0.362.g884e03848a9

[PATCH v2 2/2] path: remove repository argument from worktree_git_path()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

worktree_git_path() takes a struct repository and a struct worktree
which also contains a struct repository. The repository argument
was added by a973f60dc7c (path: stop relying on `the_repository` in
`worktree_git_path()`, 2024-08-13) and exists because the worktree
argument is optional. Having two ways of passing a repository is
a potential foot-gun as if the the worktree argument is present the
repository argument must match the worktree's repository member. Since
the last commit there are no callers that pass a NULL worktree so lets
remove the repository argument. This removes the potential confusion
and lets us delete a number of uses of "the_repository".

worktree_git_path() has the following callers:

 - builtin/worktree.c:validate_no_submodules() which is called from
   check_clean_worktree() and move_worktree(), both of which supply
   a non-NULL worktree.

 - builtin/fsck.c:cmd_fsck() which loops over all worktrees.

 - revision.c:add_index_objects_to_pending() which loops over all
   worktrees.

 - worktree.c:worktree_lock_reason() which dereferences wt before
   calling worktree_git_path().

 - wt-status.c:wt_status_check_bisect() and wt_status_check_rebase()
   which are always called with a non-NULL worktree after the last
   commit.

 - wt-status.c:git_branch() which is only called by
   wt_status_check_bisect() and wt_status_check_rebase().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/fsck.c     |  2 +-
 builtin/worktree.c |  4 ++--
 path.c             |  9 ++++-----
 path.h             |  8 +++-----
 revision.c         |  2 +-
 worktree.c         |  2 +-
 wt-status.c        | 14 +++++++-------
 7 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0512f78a87f..42ba0afb91a 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1137,7 +1137,7 @@ int cmd_fsck(int argc,
 			 * and may get overwritten by other calls
 			 * while we're examining the index.
 			 */
-			path = xstrdup(worktree_git_path(the_repository, wt, "index"));
+			path = xstrdup(worktree_git_path(wt, "index"));
 			wt_gitdir = get_worktree_git_dir(wt);
 
 			read_index_from(&istate, path, wt_gitdir);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 3d6547c23b4..62fd4642e5d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1191,14 +1191,14 @@ static void validate_no_submodules(const struct worktree *wt)
 
 	wt_gitdir = get_worktree_git_dir(wt);
 
-	if (is_directory(worktree_git_path(the_repository, wt, "modules"))) {
+	if (is_directory(worktree_git_path(wt, "modules"))) {
 		/*
 		 * There could be false positives, e.g. the "modules"
 		 * directory exists but is empty. But it's a rare case and
 		 * this simpler check is probably good enough for now.
 		 */
 		found_submodules = 1;
-	} else if (read_index_from(&istate, worktree_git_path(the_repository, wt, "index"),
+	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),
 				   wt_gitdir) > 0) {
 		for (i = 0; i < istate.cache_nr; i++) {
 			struct cache_entry *ce = istate.cache[i];
diff --git a/path.c b/path.c
index d726537622c..073f631b914 100644
--- a/path.c
+++ b/path.c
@@ -486,17 +486,16 @@ const char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname->buf);
 }
 
-const char *worktree_git_path(struct repository *r,
-			      const struct worktree *wt, const char *fmt, ...)
+const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
 {
 	struct strbuf *pathname = get_pathname();
 	va_list args;
 
-	if (wt && wt->repo != r)
-		BUG("worktree not connected to expected repository");
+	if (!wt)
+		BUG("%s() called with NULL worktree", __func__);
 
 	va_start(args, fmt);
-	repo_git_pathv(r, wt, pathname, fmt, args);
+	repo_git_pathv(wt->repo, wt, pathname, fmt, args);
 	va_end(args);
 	return pathname->buf;
 }
diff --git a/path.h b/path.h
index 0ec95a0b079..cbcad254a0a 100644
--- a/path.h
+++ b/path.h
@@ -66,13 +66,11 @@ const char *repo_git_path_replace(struct repository *repo,
 
 /*
  * Similar to repo_git_path() but can produce paths for a specified
- * worktree instead of current one. When no worktree is given, then the path is
- * computed relative to main worktree of the given repository.
+ * worktree instead of current one.
  */
-const char *worktree_git_path(struct repository *r,
-			      const struct worktree *wt,
+const char *worktree_git_path(const struct worktree *wt,
 			      const char *fmt, ...)
-	__attribute__((format (printf, 3, 4)));
+	__attribute__((format (printf, 2, 3)));
 
 /*
  * The `repo_worktree_path` family of functions will construct a path into a
diff --git a/revision.c b/revision.c
index 29972c3a198..ca3481c1902 100644
--- a/revision.c
+++ b/revision.c
@@ -1847,7 +1847,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 		wt_gitdir = get_worktree_git_dir(wt);
 
 		if (read_index_from(&istate,
-				    worktree_git_path(the_repository, wt, "index"),
+				    worktree_git_path(wt, "index"),
 				    wt_gitdir) > 0)
 			do_add_index_objects_to_pending(revs, &istate, flags);
 
diff --git a/worktree.c b/worktree.c
index 218c332a66d..6e2f0f78283 100644
--- a/worktree.c
+++ b/worktree.c
@@ -308,7 +308,7 @@ const char *worktree_lock_reason(struct worktree *wt)
 	if (!wt->lock_reason_valid) {
 		struct strbuf path = STRBUF_INIT;
 
-		strbuf_addstr(&path, worktree_git_path(the_repository, wt, "locked"));
+		strbuf_addstr(&path, worktree_git_path(wt, "locked"));
 		if (file_exists(path.buf)) {
 			struct strbuf lock_reason = STRBUF_INIT;
 			if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)
diff --git a/wt-status.c b/wt-status.c
index 2debda534c1..68257d6dfd2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1648,7 +1648,7 @@ static char *get_branch(const struct worktree *wt, const char *path)
 	struct object_id oid;
 	const char *branch_name;
 
-	if (strbuf_read_file(&sb, worktree_git_path(the_repository, wt, "%s", path), 0) <= 0)
+	if (strbuf_read_file(&sb, worktree_git_path(wt, "%s", path), 0) <= 0)
 		goto got_nothing;
 
 	while (sb.len && sb.buf[sb.len - 1] == '\n')
@@ -1750,18 +1750,18 @@ int wt_status_check_rebase(const struct worktree *wt,
 	if (!wt)
 		BUG("wt_status_check_rebase() called with NULL worktree");
 
-	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
-		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
+	if (!stat(worktree_git_path(wt, "rebase-apply"), &st)) {
+		if (!stat(worktree_git_path(wt, "rebase-apply/applying"), &st)) {
 			state->am_in_progress = 1;
-			if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/patch"), &st) && !st.st_size)
+			if (!stat(worktree_git_path(wt, "rebase-apply/patch"), &st) && !st.st_size)
 				state->am_empty_patch = 1;
 		} else {
 			state->rebase_in_progress = 1;
 			state->branch = get_branch(wt, "rebase-apply/head-name");
 			state->onto = get_branch(wt, "rebase-apply/onto");
 		}
-	} else if (!stat(worktree_git_path(the_repository, wt, "rebase-merge"), &st)) {
-		if (!stat(worktree_git_path(the_repository, wt, "rebase-merge/interactive"), &st))
+	} else if (!stat(worktree_git_path(wt, "rebase-merge"), &st)) {
+		if (!stat(worktree_git_path(wt, "rebase-merge/interactive"), &st))
 			state->rebase_interactive_in_progress = 1;
 		else
 			state->rebase_in_progress = 1;
@@ -1780,7 +1780,7 @@ int wt_status_check_bisect(const struct worktree *wt,
 	if (!wt)
 		BUG("wt_status_check_bisect() called with NULL worktree");
 
-	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
+	if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
 		state->bisecting_from = get_branch(wt, "BISECT_START");
 		return 1;
-- 
2.52.0.362.g884e03848a9

Re: [PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> In general the "struct worktree" returned may not correspond to
> the "current" worktree defined by is_current_worktree() as that
> function uses "the_repository" rather than "wt->repo" when
> deciding which worktree is "current".  In practice the "struct
> repository" we pass corresponds to "the_repository" as we only
> ever operate on a single repository at the moment.

This may technically be a correct description, but feels very
unsatisfactory, as it fails to answer this very simple question:

    what does it mean when is_current_worktree() says "no" to the
    worktree instance returned by this function?  In other words,
    what are the sample sequences that can lead to such a worktree?

We start a Git process in a directory which is part of a set of
worktrees governed by a single repository.  That repository becomes
the_repository and the worktree instance that represents our
directory would satisfy is_current_worktree().  Then we visit
another directory that is one of a set of worktrees goverend by a
separate and different repository.  We now have a repository
instance that is different from our the_repository.  Perhaps our
in-core submodule code may do that, and that different repository is
the submodule in question.  Running this function will yield the
worktree instance, whose path is a subdirectory of our current
directory where the submodule is checked out?  It may be the current
worktree if we asked is_current_worktree() about that worktree in
the context of the submodule, but it is not in the context of our
superproject repository.  In fact, none of the worktrees governed by
the submodule repository can be "current", as they are not our
checkout, from the viewpoint of our superproject repository.

Is that what is going on here?  

A related question that is much more relevant is this:

    What is the significance of the worktree, relative to our
    process, returned by this function for a given repo?  What is so
    special about this worktree, among others that are also linked
    to the same repository?

    What does it mean for a worktree to "corresponds to"
    repo->{gitdir,worktree}?  Why does the currently running Git
    process want to grab such a worktree?

If the answer were "it is the current worktree", then we would have
a nice and very understandable name "get-current-worktree-for-repo"
for the function, but because I do not think of a good answer to the
question (and you already explained why it is not the "current"
worktree), I cannot improve on "get _A_ worktree from repository",
the name given by the patch, which leaves the "which one of the
worktrees are we talking about?  Why did we pick that particular one
instead of other ones" unanswered.

Or perhaps "the current worktree" is not a per-Git-process concept,
but is a per-process-per-repo concept?

In other words, the function is_current_worktree(wt) may not take a
repository and always compute things relative to the_repository, but
once we wean ourselves off of the_repository, we would/should have
repo_is_current_worktree(repo, wt), making is_current_worktree(wt) a
thin wrapper for repo_is_current_worktree(the_repository, wt)?

> diff --git a/worktree.h b/worktree.h
> index e4bcccdc0ae..06efe26b835 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -38,6 +38,12 @@ struct worktree **get_worktrees(void);
>   */
>  struct worktree **get_worktrees_without_reading_head(void);
>  
> +/*
> + * Construct a struct worktree corresponding to repo->gitdir and
> + * repo->worktree.
> + */
> +struct worktree *get_worktree_from_repository(struct repository *repo);
> +

Re: [PATCH v2 2/2] path: remove repository argument from worktree_git_path()

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> worktree_git_path() takes a struct repository and a struct worktree
> which also contains a struct repository. The repository argument
> was added by a973f60dc7c (path: stop relying on `the_repository` in
> `worktree_git_path()`, 2024-08-13) and exists because the worktree
> argument is optional. Having two ways of passing a repository is
> a potential foot-gun as if the the worktree argument is present the
> repository argument must match the worktree's repository member. Since
> the last commit there are no callers that pass a NULL worktree so lets
> remove the repository argument. This removes the potential confusion
> and lets us delete a number of uses of "the_repository".
>
> worktree_git_path() has the following callers:
>
>  - builtin/worktree.c:validate_no_submodules() which is called from
>    check_clean_worktree() and move_worktree(), both of which supply
>    a non-NULL worktree.
>
>  - builtin/fsck.c:cmd_fsck() which loops over all worktrees.
>
>  - revision.c:add_index_objects_to_pending() which loops over all
>    worktrees.
>
>  - worktree.c:worktree_lock_reason() which dereferences wt before
>    calling worktree_git_path().
>
>  - wt-status.c:wt_status_check_bisect() and wt_status_check_rebase()
>    which are always called with a non-NULL worktree after the last
>    commit.
>
>  - wt-status.c:git_branch() which is only called by
>    wt_status_check_bisect() and wt_status_check_rebase().
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/fsck.c     |  2 +-
>  builtin/worktree.c |  4 ++--
>  path.c             |  9 ++++-----
>  path.h             |  8 +++-----
>  revision.c         |  2 +-
>  worktree.c         |  2 +-
>  wt-status.c        | 14 +++++++-------
>  7 files changed, 19 insertions(+), 22 deletions(-)

Thank you for working on this clean-up.  Very well reasoned.

Re: [PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In other words, the function is_current_worktree(wt) may not take a
> repository and always compute things relative to the_repository, but
> once we wean ourselves off of the_repository, we would/should have
> repo_is_current_worktree(repo, wt), making is_current_worktree(wt) a
> thin wrapper for repo_is_current_worktree(the_repository, wt)?

Eh, in light of 2/2 of this series, since wt knows which repository
it belongs to, what I wrote above does not make much sense.
Allowing callers to give repo that is different from wt->repo to
that function is a potential foot-gun.  In other words, isn't
is_current_worktree(wt) using the_worktree and not wt->repo a bug
already, I have to wonder?

Re: [PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

On 19/02/2026 20:37, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> In other words, the function is_current_worktree(wt) may not take a
>> repository and always compute things relative to the_repository, but
>> once we wean ourselves off of the_repository, we would/should have
>> repo_is_current_worktree(repo, wt), making is_current_worktree(wt) a
>> thin wrapper for repo_is_current_worktree(the_repository, wt)?
> 
> Eh, in light of 2/2 of this series, since wt knows which repository
> it belongs to, what I wrote above does not make much sense.
> Allowing callers to give repo that is different from wt->repo to
> that function is a potential foot-gun.  In other words, isn't
> is_current_worktree(wt) using the_worktree and not wt->repo a bug
> already, I have to wonder?

I wonder that too. You, Karthik and me all initially assumed that the 
current worktree would be defined by wt->repo->worktree matching 
wt->path (the code actually compares the git_dir of the worktree and the 
repository to accommodate bare repositories but the same principle 
applies). The use of "the_repository" in is_current_wortree() comes from 
replacing get_git_dir() with repo_get_git_dir() in 246deeac951 
(environment: make `get_git_dir()` accept a repository, 2024-09-12). In
get_worktree_git_dir() it comes from replacing git_common_path() with 
repo_common_path() in 07242c2a5af (path: drop `git_common_path()` in 
favor of `repo_common_path()`, 2025-02-07). I suspect the replacements 
were mechanical and not much thought went into considering whether, in a 
world where there can be more than one repository per process, they 
should use the local repository instance instead of "the_repository".

The current situation seems counter intuitive and I don't see what the 
benefit is in defining the current worktree to be per-process rather 
than per-struct-repository instance.

This series isn't in next yet - shall I re-roll with an extra 
preparatory patch changing is_current_worktree() and 
git_worktree_git_dir() to use wt->repo, or are you happy to have that as 
a separate follow up on top of these patches?

Thanks

Phillip

Re: [PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 19/02/2026 20:37, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>>> In other words, the function is_current_worktree(wt) may not take a
>>> repository and always compute things relative to the_repository, but
>>> once we wean ourselves off of the_repository, we would/should have
>>> repo_is_current_worktree(repo, wt), making is_current_worktree(wt) a
>>> thin wrapper for repo_is_current_worktree(the_repository, wt)?
>> 
>> Eh, in light of 2/2 of this series, since wt knows which repository
>> it belongs to, what I wrote above does not make much sense.
>> Allowing callers to give repo that is different from wt->repo to
>> that function is a potential foot-gun.  In other words, isn't
>> is_current_worktree(wt) using the_worktree and not wt->repo a bug
>> already, I have to wonder?
>
> I wonder that too. You, Karthik and me all initially assumed that the 
> current worktree would be defined by wt->repo->worktree matching 
> wt->path (the code actually compares the git_dir of the worktree and the 
> repository to accommodate bare repositories but the same principle 
> applies). The use of "the_repository" in is_current_wortree() comes from 
> replacing get_git_dir() with repo_get_git_dir() in 246deeac951 
> (environment: make `get_git_dir()` accept a repository, 2024-09-12). In
> get_worktree_git_dir() it comes from replacing git_common_path() with 
> repo_common_path() in 07242c2a5af (path: drop `git_common_path()` in 
> favor of `repo_common_path()`, 2025-02-07). I suspect the replacements 
> were mechanical and not much thought went into considering whether, in a 
> world where there can be more than one repository per process, they 
> should use the local repository instance instead of "the_repository".
>
> The current situation seems counter intuitive and I don't see what the 
> benefit is in defining the current worktree to be per-process rather 
> than per-struct-repository instance.
>
> This series isn't in next yet - shall I re-roll with an extra 
> preparatory patch changing is_current_worktree() and 
> git_worktree_git_dir() to use wt->repo, or are you happy to have that as 
> a separate follow up on top of these patches?

Thanks for investigating how we got here.  I do not have strong
preference in the order, as long as we eventually get there.

Re: [PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Phillip Wood]

On 25/02/2026 17:11, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>> This series isn't in next yet - shall I re-roll with an extra
>> preparatory patch changing is_current_worktree() and
>> git_worktree_git_dir() to use wt->repo, or are you happy to have that as
>> a separate follow up on top of these patches?
> 
> Thanks for investigating how we got here.  I do not have strong
> preference in the order, as long as we eventually get there.

In that case I'll send a follow-up series next week.

Thanks

Phillip

Re: [PATCH v2 1/2] wt-status: avoid passing NULL worktree

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 25/02/2026 17:11, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>
>>> This series isn't in next yet - shall I re-roll with an extra
>>> preparatory patch changing is_current_worktree() and
>>> git_worktree_git_dir() to use wt->repo, or are you happy to have that as
>>> a separate follow up on top of these patches?
>> 
>> Thanks for investigating how we got here.  I do not have strong
>> preference in the order, as long as we eventually get there.
>
> In that case I'll send a follow-up series next week.

OK, then let's merge what we have to 'next'.