[PATCH 0/3] worktree: stop using "the_repository" in is_current_worktree()

[PATCH 0/3] worktree: stop using "the_repository" in is_current_worktree()

[Phillip Wood]

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

This is a follow up to pw/no-more-NULL-means-current-worktree that removes
"the_repository" from is_current_worktree() and get_worktree_git_dir().
The first patch removes the use of "the_repository" when determining
if a worktree is current. Patches 2 & 3 require a non-NULL worktree
when calling get_worktree_git_dir() to remove the last use of
"the_repository" in that function.

Base-Commit: 7f19e4e1b6a3ad259e2ed66033e01e03b8b74c5e
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fworktree-is-current-use-repo%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/7f19e4e1b...1151b5b30
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/worktree-is-current-use-repo/v1


Phillip Wood (3):
  worktree: remove "the_repository" from is_current_worktree()
  worktree add: stop reading ".git/HEAD"
  worktree: reject NULL worktree in get_worktree_git_dir()

 builtin/worktree.c      | 21 ++-------------------
 t/t2400-worktree-add.sh | 28 ++++++++++++----------------
 worktree.c              | 10 +++++-----
 3 files changed, 19 insertions(+), 40 deletions(-)

-- 
2.52.0.362.g884e03848a9

[PATCH 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

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

is_current_worktree() compares the gitdir of the worktree to the gitdir
of "the_repository" and returns true when they match. To get the gitdir
of the worktree it calls get_workree_git_dir() which also depends on
"the_repository". This has the effect that even if "wt->path" matches
"wt->repo->worktree" is_current_worktree(wt) will return false when
"wt->repo" is not "the_repository" which is confusing.

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). In both cases we have
a repository instance available so use that instead. This means
that a worktree "wt" is always considered current when "wt->path"
matches "wt->repo->worktree" and so the worktree returned by
get_worktree_from_repository() is always considered current.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/worktree.c b/worktree.c
index e9ff6e6ef2e..344ad0c031b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
 
 static int is_current_worktree(struct worktree *wt)
 {
-	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
+	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
 	char *wt_git_dir = get_worktree_git_dir(wt);
 	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
 	free(wt_git_dir);
@@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
 	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);
+	wt->is_current = true;
 	add_head_info(wt);
 
 	free(gitdir);
@@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
 	if (!wt)
 		return xstrdup(repo_get_git_dir(the_repository));
 	else if (!wt->id)
-		return xstrdup(repo_get_common_dir(the_repository));
+		return xstrdup(repo_get_common_dir(wt->repo));
 	else
-		return repo_common_path(the_repository, "worktrees/%s", wt->id);
+		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
 }
 
 static struct worktree *find_worktree_by_suffix(struct worktree **list,
-- 
2.52.0.362.g884e03848a9

[PATCH 2/3] worktree add: stop reading ".git/HEAD"

[Phillip Wood]

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

The function can_use_local_refs() prints a warning if there are no local
branches and HEAD is invalid or points to an unborn branch. As part of
the warning it prints the contents of ".git/HEAD". In a repository using
the reftable backend HEAD is not stored in the filesystem so reading
that file is pointless. In a repository using the files backend it is
unclear how useful printing it is - it would be better to diagnose the
problem for the user. For now, simplify the warning by not printing
the file contents and adjust the relevant test case accordingly. Also
fixup the test case to use test_grep so that anyone trying to debug a
test failure in the future is not met by a wall of silence.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/worktree.c      | 21 ++-------------------
 t/t2400-worktree-add.sh | 28 ++++++++++++----------------
 2 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index bc2d0d645ba..70410b53df3 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -692,25 +692,8 @@ static int can_use_local_refs(const struct add_opts *opts)
 	if (refs_head_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
 		return 1;
 	} else if (refs_for_each_branch_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
-		if (!opts->quiet) {
-			struct strbuf path = STRBUF_INIT;
-			struct strbuf contents = STRBUF_INIT;
-			char *wt_gitdir = get_worktree_git_dir(NULL);
-
-			strbuf_add_real_path(&path, wt_gitdir);
-			strbuf_addstr(&path, "/HEAD");
-			strbuf_read_file(&contents, path.buf, 64);
-			strbuf_stripspace(&contents, NULL);
-			strbuf_strip_suffix(&contents, "\n");
-
-			warning(_("HEAD points to an invalid (or orphaned) reference.\n"
-				  "HEAD path: '%s'\n"
-				  "HEAD contents: '%s'"),
-				  path.buf, contents.buf);
-			strbuf_release(&path);
-			strbuf_release(&contents);
-			free(wt_gitdir);
-		}
+		if (!opts->quiet)
+				warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
 		return 1;
 	}
 	return 0;
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 023e1301c8e..58b4445cc44 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -987,7 +987,7 @@ test_dwim_orphan () {
 				then
 					test_must_be_empty actual
 				else
-					grep "$info_text" actual
+					test_grep "$info_text" actual
 				fi
 			elif [ "$outcome" = "no_infer" ]
 			then
@@ -996,39 +996,35 @@ test_dwim_orphan () {
 				then
 					test_must_be_empty actual
 				else
-					! grep "$info_text" actual
+					test_grep ! "$info_text" actual
 				fi
 			elif [ "$outcome" = "fetch_error" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
-				grep "$fetch_error_text" actual
+				test_grep "$fetch_error_text" actual
 			elif [ "$outcome" = "fatal_orphan_bad_combo" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
-					! grep "$info_text" actual
+					test_grep ! "$info_text" actual
 				else
-					grep "$info_text" actual
+					test_grep "$info_text" actual
 				fi &&
-				grep "$bad_combo_regex" actual
+				test_grep "$bad_combo_regex" actual
 			elif [ "$outcome" = "warn_bad_head" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
-					grep "$invalid_ref_regex" actual &&
-					! grep "$orphan_hint" actual
+					test_grep "$invalid_ref_regex" actual &&
+					test_grep ! "$orphan_hint" actual
 				else
-					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
-					headcontents=$(cat "$headpath") &&
-					grep "HEAD points to an invalid (or orphaned) reference" actual &&
-					grep "HEAD path: .$headpath." actual &&
-					grep "HEAD contents: .$headcontents." actual &&
-					grep "$orphan_hint" actual &&
-					! grep "$info_text" actual
+					test_grep "HEAD points to an invalid (or orphaned) reference" actual &&
+					test_grep "$orphan_hint" actual &&
+					test_grep ! "$info_text" actual
 				fi &&
-				grep "$invalid_ref_regex" actual
+				test_grep "$invalid_ref_regex" actual
 			else
 				# Unreachable
 				false
-- 
2.52.0.362.g884e03848a9

Re: [PATCH 2/3] worktree add: stop reading ".git/HEAD"

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The function can_use_local_refs() prints a warning if there are no local
> branches and HEAD is invalid or points to an unborn branch. As part of
> the warning it prints the contents of ".git/HEAD". In a repository using
> the reftable backend HEAD is not stored in the filesystem so reading
> that file is pointless. In a repository using the files backend it is
> unclear how useful printing it is - it would be better to diagnose the
> problem for the user. For now, simplify the warning by not printing
> the file contents and adjust the relevant test case accordingly. Also
> fixup the test case to use test_grep so that anyone trying to debug a
> test failure in the future is not met by a wall of silence.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/worktree.c      | 21 ++-------------------
>  t/t2400-worktree-add.sh | 28 ++++++++++++----------------
>  2 files changed, 14 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index bc2d0d645ba..70410b53df3 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -692,25 +692,8 @@ static int can_use_local_refs(const struct add_opts *opts)
>  	if (refs_head_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
>  		return 1;
>  	} else if (refs_for_each_branch_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
> -		if (!opts->quiet) {
> -			struct strbuf path = STRBUF_INIT;
> -			struct strbuf contents = STRBUF_INIT;
> -			char *wt_gitdir = get_worktree_git_dir(NULL);
> -
> -			strbuf_add_real_path(&path, wt_gitdir);
> -			strbuf_addstr(&path, "/HEAD");
> -			strbuf_read_file(&contents, path.buf, 64);
> -			strbuf_stripspace(&contents, NULL);
> -			strbuf_strip_suffix(&contents, "\n");
> -
> -			warning(_("HEAD points to an invalid (or orphaned) reference.\n"
> -				  "HEAD path: '%s'\n"
> -				  "HEAD contents: '%s'"),
> -				  path.buf, contents.buf);
> -			strbuf_release(&path);
> -			strbuf_release(&contents);
> -			free(wt_gitdir);
> -		}
> +		if (!opts->quiet)
> +				warning(_("HEAD points to an invalid (or orphaned) reference.\n"));

This is indented one level too deep, it seems.

Other than that, I fully agree with the reasoning of the removal
explained in the proposed log message, and the updated test to use
test_grep does look much better.

We seem to use "[ a = b ]" instead of "test a = b" in this test
file, unlike everybody else, which I didn't notice before.  Of
course, this series has no need to touch them; it is just a tangent
I happened to have noticed.

Thanks.

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 023e1301c8e..58b4445cc44 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -987,7 +987,7 @@ test_dwim_orphan () {
>  				then
>  					test_must_be_empty actual
>  				else
> -					grep "$info_text" actual
> +					test_grep "$info_text" actual
>  				fi
>  			elif [ "$outcome" = "no_infer" ]
>  			then
> @@ -996,39 +996,35 @@ test_dwim_orphan () {
>  				then
>  					test_must_be_empty actual
>  				else
> -					! grep "$info_text" actual
> +					test_grep ! "$info_text" actual
>  				fi
>  			elif [ "$outcome" = "fetch_error" ]
>  			then
>  				test_must_fail git $dashc_args worktree add $args 2>actual &&
> -				grep "$fetch_error_text" actual
> +				test_grep "$fetch_error_text" actual
>  			elif [ "$outcome" = "fatal_orphan_bad_combo" ]
>  			then
>  				test_must_fail git $dashc_args worktree add $args 2>actual &&
>  				if [ $use_quiet -eq 1 ]
>  				then
> -					! grep "$info_text" actual
> +					test_grep ! "$info_text" actual
>  				else
> -					grep "$info_text" actual
> +					test_grep "$info_text" actual
>  				fi &&
> -				grep "$bad_combo_regex" actual
> +				test_grep "$bad_combo_regex" actual
>  			elif [ "$outcome" = "warn_bad_head" ]
>  			then
>  				test_must_fail git $dashc_args worktree add $args 2>actual &&
>  				if [ $use_quiet -eq 1 ]
>  				then
> -					grep "$invalid_ref_regex" actual &&
> -					! grep "$orphan_hint" actual
> +					test_grep "$invalid_ref_regex" actual &&
> +					test_grep ! "$orphan_hint" actual
>  				else
> -					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
> -					headcontents=$(cat "$headpath") &&
> -					grep "HEAD points to an invalid (or orphaned) reference" actual &&
> -					grep "HEAD path: .$headpath." actual &&
> -					grep "HEAD contents: .$headcontents." actual &&
> -					grep "$orphan_hint" actual &&
> -					! grep "$info_text" actual
> +					test_grep "HEAD points to an invalid (or orphaned) reference" actual &&
> +					test_grep "$orphan_hint" actual &&
> +					test_grep ! "$info_text" actual
>  				fi &&
> -				grep "$invalid_ref_regex" actual
> +				test_grep "$invalid_ref_regex" actual
>  			else
>  				# Unreachable
>  				false

[PATCH 3/3] worktree: reject NULL worktree in get_worktree_git_dir()

[Phillip Wood]

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

This removes the final dependence on "the_repository" in
get_worktree_git_dir(). The last commit removed only caller that
passed a NULL worktree.

get_worktree_git_dir() has the following callers:

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

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

 - builtin/receive-pack.c:update_worktree() which is called from
   update() only when "worktree" is non-NULL.

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

 - reachable.c:add_rebase_files() which loops over all worktrees.

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

 - worktree.c:is_current_worktree() which expects a non-NULL worktree.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/worktree.c b/worktree.c
index 344ad0c031b..1ed5e8c3cd2 100644
--- a/worktree.c
+++ b/worktree.c
@@ -227,7 +227,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));
+		BUG("%s() called with NULL worktree", __func__);
 	else if (!wt->id)
 		return xstrdup(repo_get_common_dir(wt->repo));
 	else
-- 
2.52.0.362.g884e03848a9

Re: [PATCH 3/3] worktree: reject NULL worktree in get_worktree_git_dir()

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This removes the final dependence on "the_repository" in
> get_worktree_git_dir(). The last commit removed only caller that
> passed a NULL worktree.
>
> get_worktree_git_dir() has the following callers:
>
>  - branch.c:prepare_checked_out_branches() which loops over all
>    worktrees.
>
>  - builtin/fsck.c:cmd_fsck() which loops over all worktrees.
>
>  - builtin/receive-pack.c:update_worktree() which is called from
>    update() only when "worktree" is non-NULL.
>
>  - builtin/worktree.c:validate_no_submodules() which is called from
>    check_clean_worktree() and move_worktree(), both of which supply
>    a non-NULL worktree.
>
>  - reachable.c:add_rebase_files() which loops over all worktrees.
>
>  - revision.c:add_index_objects_to_pending() which loops over all
>    worktrees.
>
>  - worktree.c:is_current_worktree() which expects a non-NULL worktree.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  worktree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/worktree.c b/worktree.c
> index 344ad0c031b..1ed5e8c3cd2 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -227,7 +227,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));
> +		BUG("%s() called with NULL worktree", __func__);
>  	else if (!wt->id)
>  		return xstrdup(repo_get_common_dir(wt->repo));
>  	else

<worktree.h> still has

    /*
     * Return git dir of the worktree. Note that the path may be relative.
     * If wt is NULL, git dir of current worktree is returned.
     */
    char *get_worktree_git_dir(const struct worktree *wt);

which needs a matching adjustment.

Re: [PATCH 3/3] worktree: reject NULL worktree in get_worktree_git_dir()

[Phillip Wood]

On 13/03/2026 21:42, Junio C Hamano wrote:
> 
> <worktree.h> still has
> 
>      /*
>       * Return git dir of the worktree. Note that the path may be relative.
>       * If wt is NULL, git dir of current worktree is returned.
>       */
>      char *get_worktree_git_dir(const struct worktree *wt);

Good catch, I'll fix that and send a re-roll

Thanks

Phillip

[PATCH v2 0/3] worktree: stop using "the_repository" in is_current_worktree()

[Phillip Wood]

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

This is a follow up to pw/no-more-NULL-means-current-worktree that removes
"the_repository" from is_current_worktree() and get_worktree_git_dir().
The first patch removes the use of "the_repository" when determining
if a worktree is current. Patches 2 & 3 require a non-NULL worktree
when calling get_worktree_git_dir() to remove the last use of
"the_repository" in that function.

Changes since V1

 - Patch 2: fixed indentation (thanks to Junio)
 - Patch 3: removed stale comment (thanks to Junio)

Base-Commit: 7f19e4e1b6a3ad259e2ed66033e01e03b8b74c5e
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fworktree-is-current-use-repo%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/7f19e4e1b...75eecc849
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/worktree-is-current-use-repo/v2


Phillip Wood (3):
  worktree: remove "the_repository" from is_current_worktree()
  worktree add: stop reading ".git/HEAD"
  worktree: reject NULL worktree in get_worktree_git_dir()

 builtin/worktree.c      | 21 ++-------------------
 t/t2400-worktree-add.sh | 28 ++++++++++++----------------
 worktree.c              | 10 +++++-----
 worktree.h              |  1 -
 4 files changed, 19 insertions(+), 41 deletions(-)

Range-diff against v1:
1:  075700a2256 = 1:  075700a2256 worktree: remove "the_repository" from is_current_worktree()
2:  ae2a368e7e7 ! 2:  c3c5767725d worktree add: stop reading ".git/HEAD"
    @@ builtin/worktree.c: static int can_use_local_refs(const struct add_opts *opts)
     -			free(wt_gitdir);
     -		}
     +		if (!opts->quiet)
    -+				warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
    ++			warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
      		return 1;
      	}
      	return 0;
3:  1151b5b3020 ! 3:  75eecc8492e worktree: reject NULL worktree in get_worktree_git_dir()
    @@ worktree.c: struct worktree **get_worktrees_without_reading_head(void)
      	else if (!wt->id)
      		return xstrdup(repo_get_common_dir(wt->repo));
      	else
    +
    + ## worktree.h ##
    +@@ worktree.h: int submodule_uses_worktrees(const char *path);
    + 
    + /*
    +  * Return git dir of the worktree. Note that the path may be relative.
    +- * If wt is NULL, git dir of current worktree is returned.
    +  */
    + char *get_worktree_git_dir(const struct worktree *wt);
    + 
-- 
2.52.0.362.g884e03848a9

[PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

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

is_current_worktree() compares the gitdir of the worktree to the gitdir
of "the_repository" and returns true when they match. To get the gitdir
of the worktree it calls get_workree_git_dir() which also depends on
"the_repository". This has the effect that even if "wt->path" matches
"wt->repo->worktree" is_current_worktree(wt) will return false when
"wt->repo" is not "the_repository" which is confusing.

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). In both cases we have
a repository instance available so use that instead. This means
that a worktree "wt" is always considered current when "wt->path"
matches "wt->repo->worktree" and so the worktree returned by
get_worktree_from_repository() is always considered current.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/worktree.c b/worktree.c
index e9ff6e6ef2e..344ad0c031b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
 
 static int is_current_worktree(struct worktree *wt)
 {
-	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
+	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
 	char *wt_git_dir = get_worktree_git_dir(wt);
 	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
 	free(wt_git_dir);
@@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
 	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);
+	wt->is_current = true;
 	add_head_info(wt);
 
 	free(gitdir);
@@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
 	if (!wt)
 		return xstrdup(repo_get_git_dir(the_repository));
 	else if (!wt->id)
-		return xstrdup(repo_get_common_dir(the_repository));
+		return xstrdup(repo_get_common_dir(wt->repo));
 	else
-		return repo_common_path(the_repository, "worktrees/%s", wt->id);
+		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
 }
 
 static struct worktree *find_worktree_by_suffix(struct worktree **list,
-- 
2.52.0.362.g884e03848a9

Re: [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Patrick Steinhardt]

On Sun, Mar 15, 2026 at 04:18:50PM +0000, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> is_current_worktree() compares the gitdir of the worktree to the gitdir
> of "the_repository" and returns true when they match. To get the gitdir
> of the worktree it calls get_workree_git_dir() which also depends on
> "the_repository". This has the effect that even if "wt->path" matches
> "wt->repo->worktree" is_current_worktree(wt) will return false when
> "wt->repo" is not "the_repository" which is confusing.
> 
> 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). In both cases we have
> a repository instance available so use that instead. This means
> that a worktree "wt" is always considered current when "wt->path"
> matches "wt->repo->worktree" and so the worktree returned by
> get_worktree_from_repository() is always considered current.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  worktree.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/worktree.c b/worktree.c
> index e9ff6e6ef2e..344ad0c031b 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
>  
>  static int is_current_worktree(struct worktree *wt)
>  {
> -	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
> +	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
>  	char *wt_git_dir = get_worktree_git_dir(wt);
>  	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
>  	free(wt_git_dir);

Hm, okay.

> @@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
>  	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);
> +	wt->is_current = true;
>  	add_head_info(wt);
>  
>  	free(gitdir);

I have been staring at this code for longer than I want to admit, and I
still haven't convinced myself that this is not a change in behaviour.
I think what I'm wondering about is what `is_current_worktree()` is
actually intended to do. In other words, what _do_ we consider to be
"current"?

Naively, I would consider the worktree "current" that the Git process
has been invoked in. So if I pass a repo other than `the_repository`, or
if I pass a worktree that is not the one that Git has been started in,
then I would expect the function to return `false`. With that naive
assumption your change would be breaking the existing logic.

But I have no idea whether my assumption is correct or not, as
there is not really any documentation of what the function or of the
`struct worktree::is_current` field. And having a look at a couple of
callers doesn't really make me all the wiser.

It would be great if you could shine some light on this and then also
add a bit of documentation to either the function, the field, or both :)

Thanks!

Patrick

Re: [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

Hi Patrick

On 16/03/2026 07:38, Patrick Steinhardt wrote:
> On Sun, Mar 15, 2026 at 04:18:50PM +0000, Phillip Wood wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> is_current_worktree() compares the gitdir of the worktree to the gitdir
>> of "the_repository" and returns true when they match. To get the gitdir
>> of the worktree it calls get_workree_git_dir() which also depends on
>> "the_repository". This has the effect that even if "wt->path" matches
>> "wt->repo->worktree" is_current_worktree(wt) will return false when
>> "wt->repo" is not "the_repository" which is confusing.
>>
>> 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). In both cases we have
>> a repository instance available so use that instead. This means
>> that a worktree "wt" is always considered current when "wt->path"
>> matches "wt->repo->worktree" and so the worktree returned by
>> get_worktree_from_repository() is always considered current.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   worktree.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/worktree.c b/worktree.c
>> index e9ff6e6ef2e..344ad0c031b 100644
>> --- a/worktree.c
>> +++ b/worktree.c
>> @@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
>>   
>>   static int is_current_worktree(struct worktree *wt)
>>   {
>> -	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
>> +	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
>>   	char *wt_git_dir = get_worktree_git_dir(wt);
>>   	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
>>   	free(wt_git_dir);
> 
> Hm, okay.
> 
>> @@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
>>   	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);
>> +	wt->is_current = true;
>>   	add_head_info(wt);
>>   
>>   	free(gitdir);
> 
> I have been staring at this code for longer than I want to admit, and I
> still haven't convinced myself that this is not a change in behaviour.
> I think what I'm wondering about is what `is_current_worktree()` is
> actually intended to do. In other words, what _do_ we consider to be
> "current"?

There was some discussion about that in [1] where reviewers were 
surprised that we needed to call is_current_worktree() here. This change 
is consistent with the rest of the patch but you're right that it is a 
change in behavior as at the moment the "current" worktree is set by the 
worktree that the process was started in. In practice we only ever have 
a single struct repository instance per process so there is no practical 
change in behavior here. At the moment, if you visit a set of submodules 
from the superproject by forking one process per submodule each 
submodule worktree is considered "current", but if you visit them by 
spinning up some threads in the current process they are not considered 
"current". That seems to me to be inconsistent as the process started by 
the user is in the superproject in both cases.

The "is_current" field was added in [1] without any discussion in the 
commit message. It seems to have been added to stop the same branch 
being checked out in multiple worktrees [2].

Thanks

Phillip

[1] 750e8a60d69 (worktree.c: mark current worktree, 2016-04-22)
[2] 
https://lore.kernel.org/git/CAJZYdzhG8h3s=Ep1fuGbam1cWhYkv0tW6tQ7pBGGj+fj6=Nrsw@mail.gmail.com/


> I would consider the worktree "current" that the Git process
> has been invoked in. So if I pass a repo other than `the_repository`, or
> if I pass a worktree that is not the one that Git has been started in,
> then I would expect the function to return `false`. With that naive
> assumption your change would be breaking the existing logic.
> 
> But I have no idea whether my assumption is correct or not, as
> there is not really any documentation of what the function or of the
> `struct worktree::is_current` field. And having a look at a couple of
> callers doesn't really make me all the wiser.
> 
> It would be great if you could shine some light on this and then also
> add a bit of documentation to either the function, the field, or both :)
> 
> Thanks!
> 
> Patrick
> 

Re: [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

On 16/03/2026 16:22, Phillip Wood wrote:
> 
>> I have been staring at this code for longer than I want to admit, and I
>> still haven't convinced myself that this is not a change in behaviour.
>> I think what I'm wondering about is what `is_current_worktree()` is
>> actually intended to do. In other words, what _do_ we consider to be
>> "current"?
> 
> There was some discussion about that in [1] where reviewers were 
> surprised that we needed to call is_current_worktree() here. This change 
> is consistent with the rest of the patch but you're right that it is a 
> change in behavior as at the moment the "current" worktree is set by the 
> worktree that the process was started in. In practice we only ever have 
> a single struct repository instance per process so there is no practical 
> change in behavior here. At the moment, if you visit a set of submodules 
> from the superproject by forking one process per submodule each 
> submodule worktree is considered "current", but if you visit them by 
> spinning up some threads in the current process they are not considered 
> "current". That seems to me to be inconsistent as the process started by 
> the user is in the superproject in both cases.

To add another example get_worktree_ref_store() checks `wt->is_current` 
to see if it should use the ref store in `wt->repo` or create a new 
store. That means that if we use `the_repository` to define the current 
worktree we'll end up opening a copy of the ref store in `wt->repo` when 
`repo != the_repository` and `wt->path` matches `wt->repo->worktree` 
when we could be using the ref store that's already open. It is a bit 
like the bug fixed by 1339cb3c47a (worktree: don't store main worktree 
twice, 2024-06-06) but for multiple repositories.

I'll re-roll with a bit more description in the commit message but I'm 
going to be off the list for most of the rest of this week so it will 
probably be next week before I post a new version.

Thanks

Phillip

> The "is_current" field was added in [1] without any discussion in the 
> commit message. It seems to have been added to stop the same branch 
> being checked out in multiple worktrees [2].
> 
> Thanks
> 
> Phillip
> 
> [1] 750e8a60d69 (worktree.c: mark current worktree, 2016-04-22)
> [2] https://lore.kernel.org/git/ 
> CAJZYdzhG8h3s=Ep1fuGbam1cWhYkv0tW6tQ7pBGGj+fj6=Nrsw@mail.gmail.com/
> 
> 
>> I would consider the worktree "current" that the Git process
>> has been invoked in. So if I pass a repo other than `the_repository`, or
>> if I pass a worktree that is not the one that Git has been started in,
>> then I would expect the function to return `false`. With that naive
>> assumption your change would be breaking the existing logic.
>>
>> But I have no idea whether my assumption is correct or not, as
>> there is not really any documentation of what the function or of the
>> `struct worktree::is_current` field. And having a look at a couple of
>> callers doesn't really make me all the wiser.
>>
>> It would be great if you could shine some light on this and then also
>> add a bit of documentation to either the function, the field, or both :)
>>
>> Thanks!
>>
>> Patrick
>>
> 

Re: [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Shreyansh Paliwal]

> On 16/03/2026 16:22, Phillip Wood wrote:
> >
> >> I have been staring at this code for longer than I want to admit, and I
> >> still haven't convinced myself that this is not a change in behaviour.
> >> I think what I'm wondering about is what `is_current_worktree()` is
> >> actually intended to do. In other words, what _do_ we consider to be
> >> "current"?
> >
> > There was some discussion about that in [1] where reviewers were
> > surprised that we needed to call is_current_worktree() here. This change
> > is consistent with the rest of the patch but you're right that it is a
> > change in behavior as at the moment the "current" worktree is set by the
> > worktree that the process was started in. In practice we only ever have
> > a single struct repository instance per process so there is no practical
> > change in behavior here. At the moment, if you visit a set of submodules
> > from the superproject by forking one process per submodule each
> > submodule worktree is considered "current", but if you visit them by
> > spinning up some threads in the current process they are not considered
> > "current". That seems to me to be inconsistent as the process started by
> > the user is in the superproject in both cases.
>
> To add another example get_worktree_ref_store() checks `wt->is_current`
> to see if it should use the ref store in `wt->repo` or create a new
> store. That means that if we use `the_repository` to define the current
> worktree we'll end up opening a copy of the ref store in `wt->repo` when
> `repo != the_repository` and `wt->path` matches `wt->repo->worktree`
> when we could be using the ref store that's already open. It is a bit
> like the bug fixed by 1339cb3c47a (worktree: don't store main worktree
> twice, 2024-06-06) but for multiple repositories.
>
> I'll re-roll with a bit more description in the commit message but I'm
> going to be off the list for most of the rest of this week so it will
> probably be next week before I post a new version.
>
> Thanks
>
> Phillip
>
> > The "is_current" field was added in [1] without any discussion in the
> > commit message. It seems to have been added to stop the same branch
> > being checked out in multiple worktrees [2].
> >
> > Thanks
> >
> > Phillip
> >
> > [1] 750e8a60d69 (worktree.c: mark current worktree, 2016-04-22)
> > [2] https://lore.kernel.org/git/
> > CAJZYdzhG8h3s=Ep1fuGbam1cWhYkv0tW6tQ7pBGGj+fj6=Nrsw@mail.gmail.com/
> >
> >
> >> I would consider the worktree "current" that the Git process
> >> has been invoked in. So if I pass a repo other than `the_repository`, or
> >> if I pass a worktree that is not the one that Git has been started in,
> >> then I would expect the function to return `false`. With that naive
> >> assumption your change would be breaking the existing logic.
> >>
> >> But I have no idea whether my assumption is correct or not, as
> >> there is not really any documentation of what the function or of the
> >> `struct worktree::is_current` field. And having a look at a couple of
> >> callers doesn't really make me all the wiser.
> >>
> >> It would be great if you could shine some light on this and then also
> >> add a bit of documentation to either the function, the field, or both :)
> >>
> >> Thanks!
> >>
> >> Patrick
> >>
> >

Hi Phillip,

This may be slightly out of scope for this series. My understanding so far
has been that originally wt == NULL is used to represent the 'current worktree',
which eventually meant following the process-wide state (the_repository).
With the ongoing multi-repository work, the meaning is being changed to be
interpreted as 'the worktree associated with the repository that we are working in'.
However, in path.c there are some callers of repo_git_pathv() passing wt as 'NULL',
I know that there is not involvement of the_repository state but it would be create
less confusion if the semantics of worktrees are same everywhere. So if we replace
those NULL callers with the current worktree and update the checks of (!wt) to
(is_current_worktree(wt)), some tests are failing mostly related to refs of linked
worktrees, and I think the error is originating from this,

        if (!wt)
                adjust_git_path(repo, buf, gitdir_len);

So I am a bit confused to whether wt being NULL here could mean something else
behaviour wise ?

Best,
Shreyansh

Re: [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

On 23/03/2026 09:41, Shreyansh Paliwal wrote:
> 
> This may be slightly out of scope for this series. My understanding so far
> has been that originally wt == NULL is used to represent the 'current worktree',
> which eventually meant following the process-wide state (the_repository).
> With the ongoing multi-repository work, the meaning is being changed to be
> interpreted as 'the worktree associated with the repository that we are working in'.
> However, in path.c there are some callers of repo_git_pathv() passing wt as 'NULL',
> I know that there is not involvement of the_repository state but it would be create
> less confusion if the semantics of worktrees are same everywhere. So if we replace
> those NULL callers with the current worktree and update the checks of (!wt) to
> (is_current_worktree(wt)), some tests are failing mostly related to refs of linked
> worktrees, and I think the error is originating from this,
> 
>          if (!wt)
>                  adjust_git_path(repo, buf, gitdir_len);
> 
> So I am a bit confused to whether wt being NULL here could mean something else
> behaviour wise ?

That line comes from 543107333b3 (path: worktree_git_path() should not 
use file relocation, 2017-06-22) which explains why we don't adjust the 
patch when wt is non-NULL. worktree_git_path() is called from 
builtin/fsck.c in a loop over all worktrees so changing 'if (!wt)' 'if 
(!wt->is_current)' will change the behavior for the current worktree. 
While it might be nice to clean this up in the future, it is an internal 
helper function so I'm less worried it than if it were a public function.

Thanks

Phillip

Re: [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree()

[Shreyansh Paliwal]

> On 23/03/2026 09:41, Shreyansh Paliwal wrote:
> >
> > This may be slightly out of scope for this series. My understanding so far
> > has been that originally wt == NULL is used to represent the 'current worktree',
> > which eventually meant following the process-wide state (the_repository).
> > With the ongoing multi-repository work, the meaning is being changed to be
> > interpreted as 'the worktree associated with the repository that we are working in'.
> > However, in path.c there are some callers of repo_git_pathv() passing wt as 'NULL',
> > I know that there is not involvement of the_repository state but it would be create
> > less confusion if the semantics of worktrees are same everywhere. So if we replace
> > those NULL callers with the current worktree and update the checks of (!wt) to
> > (is_current_worktree(wt)), some tests are failing mostly related to refs of linked
> > worktrees, and I think the error is originating from this,
> >
> >          if (!wt)
> >                  adjust_git_path(repo, buf, gitdir_len);
> >
> > So I am a bit confused to whether wt being NULL here could mean something else
> > behaviour wise ?
>
> That line comes from 543107333b3 (path: worktree_git_path() should not
> use file relocation, 2017-06-22) which explains why we don't adjust the
> patch when wt is non-NULL. worktree_git_path() is called from
> builtin/fsck.c in a loop over all worktrees so changing 'if (!wt)' 'if
> (!wt->is_current)' will change the behavior for the current worktree.
> While it might be nice to clean this up in the future, it is an internal
> helper function so I'm less worried it than if it were a public function.

Hmm, I see. That would end up calling adjust_git_path() for the current worktree
in that list of worktrees, which wasn’t happening before, that makes sense.
Thanks.

[PATCH v2 2/3] worktree add: stop reading ".git/HEAD"

[Phillip Wood]

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

The function can_use_local_refs() prints a warning if there are no local
branches and HEAD is invalid or points to an unborn branch. As part of
the warning it prints the contents of ".git/HEAD". In a repository using
the reftable backend HEAD is not stored in the filesystem so reading
that file is pointless. In a repository using the files backend it is
unclear how useful printing it is - it would be better to diagnose the
problem for the user. For now, simplify the warning by not printing
the file contents and adjust the relevant test case accordingly. Also
fixup the test case to use test_grep so that anyone trying to debug a
test failure in the future is not met by a wall of silence.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/worktree.c      | 21 ++-------------------
 t/t2400-worktree-add.sh | 28 ++++++++++++----------------
 2 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index bc2d0d645ba..9170b2e8981 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -692,25 +692,8 @@ static int can_use_local_refs(const struct add_opts *opts)
 	if (refs_head_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
 		return 1;
 	} else if (refs_for_each_branch_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
-		if (!opts->quiet) {
-			struct strbuf path = STRBUF_INIT;
-			struct strbuf contents = STRBUF_INIT;
-			char *wt_gitdir = get_worktree_git_dir(NULL);
-
-			strbuf_add_real_path(&path, wt_gitdir);
-			strbuf_addstr(&path, "/HEAD");
-			strbuf_read_file(&contents, path.buf, 64);
-			strbuf_stripspace(&contents, NULL);
-			strbuf_strip_suffix(&contents, "\n");
-
-			warning(_("HEAD points to an invalid (or orphaned) reference.\n"
-				  "HEAD path: '%s'\n"
-				  "HEAD contents: '%s'"),
-				  path.buf, contents.buf);
-			strbuf_release(&path);
-			strbuf_release(&contents);
-			free(wt_gitdir);
-		}
+		if (!opts->quiet)
+			warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
 		return 1;
 	}
 	return 0;
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 023e1301c8e..58b4445cc44 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -987,7 +987,7 @@ test_dwim_orphan () {
 				then
 					test_must_be_empty actual
 				else
-					grep "$info_text" actual
+					test_grep "$info_text" actual
 				fi
 			elif [ "$outcome" = "no_infer" ]
 			then
@@ -996,39 +996,35 @@ test_dwim_orphan () {
 				then
 					test_must_be_empty actual
 				else
-					! grep "$info_text" actual
+					test_grep ! "$info_text" actual
 				fi
 			elif [ "$outcome" = "fetch_error" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
-				grep "$fetch_error_text" actual
+				test_grep "$fetch_error_text" actual
 			elif [ "$outcome" = "fatal_orphan_bad_combo" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
-					! grep "$info_text" actual
+					test_grep ! "$info_text" actual
 				else
-					grep "$info_text" actual
+					test_grep "$info_text" actual
 				fi &&
-				grep "$bad_combo_regex" actual
+				test_grep "$bad_combo_regex" actual
 			elif [ "$outcome" = "warn_bad_head" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
-					grep "$invalid_ref_regex" actual &&
-					! grep "$orphan_hint" actual
+					test_grep "$invalid_ref_regex" actual &&
+					test_grep ! "$orphan_hint" actual
 				else
-					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
-					headcontents=$(cat "$headpath") &&
-					grep "HEAD points to an invalid (or orphaned) reference" actual &&
-					grep "HEAD path: .$headpath." actual &&
-					grep "HEAD contents: .$headcontents." actual &&
-					grep "$orphan_hint" actual &&
-					! grep "$info_text" actual
+					test_grep "HEAD points to an invalid (or orphaned) reference" actual &&
+					test_grep "$orphan_hint" actual &&
+					test_grep ! "$info_text" actual
 				fi &&
-				grep "$invalid_ref_regex" actual
+				test_grep "$invalid_ref_regex" actual
 			else
 				# Unreachable
 				false
-- 
2.52.0.362.g884e03848a9

Re: [PATCH v2 2/3] worktree add: stop reading ".git/HEAD"

[Patrick Steinhardt]

On Sun, Mar 15, 2026 at 04:18:51PM +0000, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> The function can_use_local_refs() prints a warning if there are no local
> branches and HEAD is invalid or points to an unborn branch. As part of
> the warning it prints the contents of ".git/HEAD". In a repository using
> the reftable backend HEAD is not stored in the filesystem so reading
> that file is pointless. In a repository using the files backend it is
> unclear how useful printing it is - it would be better to diagnose the
> problem for the user. For now, simplify the warning by not printing
> the file contents and adjust the relevant test case accordingly. Also
> fixup the test case to use test_grep so that anyone trying to debug a
> test failure in the future is not met by a wall of silence.

Oh, interesting, and good catch. I fully agree that removing this makes
sense.

Patrick

[PATCH v2 3/3] worktree: reject NULL worktree in get_worktree_git_dir()

[Phillip Wood]

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

This removes the final dependence on "the_repository" in
get_worktree_git_dir(). The last commit removed only caller that
passed a NULL worktree.

get_worktree_git_dir() has the following callers:

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

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

 - builtin/receive-pack.c:update_worktree() which is called from
   update() only when "worktree" is non-NULL.

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

 - reachable.c:add_rebase_files() which loops over all worktrees.

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

 - worktree.c:is_current_worktree() which expects a non-NULL worktree.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c | 2 +-
 worktree.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/worktree.c b/worktree.c
index 344ad0c031b..1ed5e8c3cd2 100644
--- a/worktree.c
+++ b/worktree.c
@@ -227,7 +227,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));
+		BUG("%s() called with NULL worktree", __func__);
 	else if (!wt->id)
 		return xstrdup(repo_get_common_dir(wt->repo));
 	else
diff --git a/worktree.h b/worktree.h
index e450d1a3317..85d634c36c0 100644
--- a/worktree.h
+++ b/worktree.h
@@ -51,7 +51,6 @@ int submodule_uses_worktrees(const char *path);
 
 /*
  * Return git dir of the worktree. Note that the path may be relative.
- * If wt is NULL, git dir of current worktree is returned.
  */
 char *get_worktree_git_dir(const struct worktree *wt);
 
-- 
2.52.0.362.g884e03848a9

Re: [PATCH v2 0/3] worktree: stop using "the_repository" in is_current_worktree()

[Junio C Hamano]

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

> Range-diff against v1:
> 1:  075700a2256 = 1:  075700a2256 worktree: remove "the_repository" from is_current_worktree()
> 2:  ae2a368e7e7 ! 2:  c3c5767725d worktree add: stop reading ".git/HEAD"
>     @@ builtin/worktree.c: static int can_use_local_refs(const struct add_opts *opts)
>      -			free(wt_gitdir);
>      -		}
>      +		if (!opts->quiet)
>     -+				warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
>     ++			warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
>       		return 1;
>       	}
>       	return 0;
> 3:  1151b5b3020 ! 3:  75eecc8492e worktree: reject NULL worktree in get_worktree_git_dir()
>     @@ worktree.c: struct worktree **get_worktrees_without_reading_head(void)
>       	else if (!wt->id)
>       		return xstrdup(repo_get_common_dir(wt->repo));
>       	else
>     +
>     + ## worktree.h ##
>     +@@ worktree.h: int submodule_uses_worktrees(const char *path);
>     + 
>     + /*
>     +  * Return git dir of the worktree. Note that the path may be relative.
>     +- * If wt is NULL, git dir of current worktree is returned.
>     +  */
>     + char *get_worktree_git_dir(const struct worktree *wt);
>     + 

Ah, I somehow expected to see that we say "passing NULL to wt is an
error", but that is misleading.  When something expects a worktree
instance, and it accepts NULL as a special case, then that is worth
commenting, but otherwise, it is not worth mentioning.

All look good.  Will replace.  Thanks.

[PATCH v3 0/3] worktree: stop using "the_repository" in is_current_worktree()

[Phillip Wood]

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

This is a follow up to pw/no-more-NULL-means-current-worktree that removes
"the_repository" from is_current_worktree() and get_worktree_git_dir().
The first patch removes the use of "the_repository" when determining
if a worktree is current. Patches 2 & 3 require a non-NULL worktree
when calling get_worktree_git_dir() to remove the last use of
"the_repository" in that function.

Changes since V2

 - Patch 1: expanded commit message and added a comment to is_current
            member of struct worktree. (thanks to Patrick)

Changes since V1

 - Patch 2: fixed indentation (thanks to Junio)
 - Patch 3: removed stale comment (thanks to Junio)

Base-Commit: 7f19e4e1b6a3ad259e2ed66033e01e03b8b74c5e
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fworktree-is-current-use-repo%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/7f19e4e1b...c33290280
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/worktree-is-current-use-repo/v3


Phillip Wood (3):
  worktree: remove "the_repository" from is_current_worktree()
  worktree add: stop reading ".git/HEAD"
  worktree: reject NULL worktree in get_worktree_git_dir()

 builtin/worktree.c      | 21 ++-------------------
 t/t2400-worktree-add.sh | 28 ++++++++++++----------------
 worktree.c              | 10 +++++-----
 worktree.h              |  3 +--
 4 files changed, 20 insertions(+), 42 deletions(-)

Range-diff against v2:
1:  075700a2256 ! 1:  5357c0dd53e worktree: remove "the_repository" from is_current_worktree()
    @@ Metadata
      ## Commit message ##
         worktree: remove "the_repository" from is_current_worktree()
     
    -    is_current_worktree() compares the gitdir of the worktree to the gitdir
    -    of "the_repository" and returns true when they match. To get the gitdir
    -    of the worktree it calls get_workree_git_dir() which also depends on
    -    "the_repository". This has the effect that even if "wt->path" matches
    +    The "is_current" member of struct worktree was added in 750e8a60d69
    +    (worktree.c: mark current worktree, 2016-04-22) and was used in
    +    8d9fdd7087d (worktree.c: check whether branch is rebased in another
    +    worktree, 2016-04-22) to optionally skip the current worktree when
    +    seeing if a branch is already checked out in die_if_checked_out().
    +
    +    To determine if a worktree is "current" is_current_worktree() compares
    +    the gitdir of the worktree to the gitdir of "the_repository"
    +    and returns true when they match. To get the gitdir of the
    +    worktree it calls get_workree_git_dir() which also depends on
    +    "the_repository". This means that even if "wt->path" matches
         "wt->repo->worktree" is_current_worktree(wt) will return false when
    -    "wt->repo" is not "the_repository" which is confusing.
    +    "wt->repo" is not "the_repository". Consequently die_if_checked_out()
    +    will fail to skip such a worktree when checking if a branch is already
    +    checked out and may die errounously. Fix this by using the worktree's
    +    repository instance instead of "the_repository" when comparing gitdirs.
     
         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). In both cases we have
    -    a repository instance available so use that instead. This means
    -    that a worktree "wt" is always considered current when "wt->path"
    -    matches "wt->repo->worktree" and so the worktree returned by
    -    get_worktree_from_repository() is always considered current.
    +    in favor of `repo_common_path()`, 2025-02-07). In both cases the
    +    replacements appear to have been mechanical.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    @@ worktree.c: char *get_worktree_git_dir(const struct worktree *wt)
      }
      
      static struct worktree *find_worktree_by_suffix(struct worktree **list,
    +
    + ## worktree.h ##
    +@@ worktree.h: struct worktree {
    + 	struct object_id head_oid;
    + 	int is_detached;
    + 	int is_bare;
    +-	int is_current;
    ++	int is_current;		/* does `path` match `repo->worktree` */
    + 	int lock_reason_valid; /* private */
    + 	int prune_reason_valid; /* private */
    + };
2:  c3c5767725d = 2:  4d50e6bcb2e worktree add: stop reading ".git/HEAD"
3:  75eecc8492e = 3:  c3329028010 worktree: reject NULL worktree in get_worktree_git_dir()
-- 
2.52.0.362.g884e03848a9.dirty

[PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

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

The "is_current" member of struct worktree was added in 750e8a60d69
(worktree.c: mark current worktree, 2016-04-22) and was used in
8d9fdd7087d (worktree.c: check whether branch is rebased in another
worktree, 2016-04-22) to optionally skip the current worktree when
seeing if a branch is already checked out in die_if_checked_out().

To determine if a worktree is "current" is_current_worktree() compares
the gitdir of the worktree to the gitdir of "the_repository"
and returns true when they match. To get the gitdir of the
worktree it calls get_workree_git_dir() which also depends on
"the_repository". This means that even if "wt->path" matches
"wt->repo->worktree" is_current_worktree(wt) will return false when
"wt->repo" is not "the_repository". Consequently die_if_checked_out()
will fail to skip such a worktree when checking if a branch is already
checked out and may die errounously. Fix this by using the worktree's
repository instance instead of "the_repository" when comparing gitdirs.

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). In both cases the
replacements appear to have been mechanical.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c | 8 ++++----
 worktree.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/worktree.c b/worktree.c
index e9ff6e6ef2e..344ad0c031b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
 
 static int is_current_worktree(struct worktree *wt)
 {
-	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
+	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
 	char *wt_git_dir = get_worktree_git_dir(wt);
 	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
 	free(wt_git_dir);
@@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
 	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);
+	wt->is_current = true;
 	add_head_info(wt);
 
 	free(gitdir);
@@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
 	if (!wt)
 		return xstrdup(repo_get_git_dir(the_repository));
 	else if (!wt->id)
-		return xstrdup(repo_get_common_dir(the_repository));
+		return xstrdup(repo_get_common_dir(wt->repo));
 	else
-		return repo_common_path(the_repository, "worktrees/%s", wt->id);
+		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
 }
 
 static struct worktree *find_worktree_by_suffix(struct worktree **list,
diff --git a/worktree.h b/worktree.h
index e450d1a3317..94ae58db973 100644
--- a/worktree.h
+++ b/worktree.h
@@ -16,7 +16,7 @@ struct worktree {
 	struct object_id head_oid;
 	int is_detached;
 	int is_bare;
-	int is_current;
+	int is_current;		/* does `path` match `repo->worktree` */
 	int lock_reason_valid; /* private */
 	int prune_reason_valid; /* private */
 };
-- 
2.52.0.362.g884e03848a9.dirty

Re: [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()

[Junio C Hamano]

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

> ... it calls get_workree_git_dir() which also depends on
> "the_repository". This means that even if "wt->path" matches
> "wt->repo->worktree" is_current_worktree(wt) will return false when
> "wt->repo" is not "the_repository". Consequently die_if_checked_out()
> will fail to skip such a worktree when checking if a branch is already
> checked out and may die errounously. Fix this by using the worktree's
> repository instance instead of "the_repository" when comparing gitdirs.

It sounds like the above is something we can write in a test to make
sure the code with this fix won't regress in the future.  Can we add
one?

> 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). In both cases the
> replacements appear to have been mechanical.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  worktree.c | 8 ++++----
>  worktree.h | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index e9ff6e6ef2e..344ad0c031b 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
>  
>  static int is_current_worktree(struct worktree *wt)
>  {
> -	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
> +	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
>  	char *wt_git_dir = get_worktree_git_dir(wt);
>  	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
>  	free(wt_git_dir);
> @@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
>  	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);
> +	wt->is_current = true;
>  	add_head_info(wt);
>  
>  	free(gitdir);

I think I found the semantics of get_worktree_from_repository()
unclear when we discussed a different patch series, so I may have
asked the same question already, but what exactly does it mean to
"get worktree from repository", i.e., this function does?  A
repository can have one or more worktrees attached to it (that was
the whole point of introducing the worktree mechanism), so "I have
this repository, give me the worktree for it" is not a sensible
request.  The function's comment in <worktree.h> talks only at the
implementation level "construct a worktree struct from repo->gitdir
and repo->worktree" as if it is so obvious what the resulting
worktree struct means at a higher layer's point of view, which does
not help, either.

I am asking the above because I find this unconditional assignment
of "true" utterly confusing.  Is it because "we created a brand new
worktree structure that by definition is current out of a repository
structure, so as far as the caller is concerned by definition it is
what it is currently working on"?

Stepping back a bit, how do "is_current_worktree(wt)" and
"wt->is_current" relate to each other?  Here is what happens in the
former:

        static int is_current_worktree(struct worktree *wt)
        {
                char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
                char *wt_git_dir = get_worktree_git_dir(wt);
                int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
                free(wt_git_dir);
                free(git_dir);
                return is_current;
        }

and given the definition of get_worktree_git_dir(), which gives
either the "common" .git directory for the primary worktree, or the
worktree specific .git directory for the secondaries, the is_current
being computed here sounds more like "is wt the primary worktree
among the ones that are attached to the same repository?"

But given that there are API functions with "main" in their names,
like is_main_worktree() and internal function get_main_worktree(),
what I said apparently is not the intention of whoever built the
worktree subsystem.  I am still confused... X-<.

> @@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
>  	if (!wt)
>  		return xstrdup(repo_get_git_dir(the_repository));
>  	else if (!wt->id)
> -		return xstrdup(repo_get_common_dir(the_repository));
> +		return xstrdup(repo_get_common_dir(wt->repo));
>  	else
> -		return repo_common_path(the_repository, "worktrees/%s", wt->id);
> +		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
>  }
>  
>  static struct worktree *find_worktree_by_suffix(struct worktree **list,
> diff --git a/worktree.h b/worktree.h
> index e450d1a3317..94ae58db973 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -16,7 +16,7 @@ struct worktree {
>  	struct object_id head_oid;
>  	int is_detached;
>  	int is_bare;
> -	int is_current;
> +	int is_current;		/* does `path` match `repo->worktree` */

This new comment also talks only at the lowest implementation level.
What significance does it have to the upper layer that calls into
the API if `path` matches or does not match `repo->worktree` is
totally unclear, at least to me.

>  	int lock_reason_valid; /* private */
>  	int prune_reason_valid; /* private */
>  };

Thanks.

Re: [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()

[Phillip Wood]

On 26/03/2026 15:48, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... it calls get_workree_git_dir() which also depends on
>> "the_repository". This means that even if "wt->path" matches
>> "wt->repo->worktree" is_current_worktree(wt) will return false when
>> "wt->repo" is not "the_repository". Consequently die_if_checked_out()
>> will fail to skip such a worktree when checking if a branch is already
>> checked out and may die errounously. Fix this by using the worktree's
>> repository instance instead of "the_repository" when comparing gitdirs.
> 
> It sounds like the above is something we can write in a test to make
> sure the code with this fix won't regress in the future.  Can we add
> one?

We'd need to create a struct worktree instance with a repository 
instance that is not "the_repository" - I'm not sure we can do that in a 
script.

>> -	wt->is_current = is_current_worktree(wt);
>> +	wt->is_current = true;
>>   	add_head_info(wt);
>>   
>>   	free(gitdir);
> 
> I think I found the semantics of get_worktree_from_repository()
> unclear when we discussed a different patch series, so I may have
> asked the same question already, but what exactly does it mean to
> "get worktree from repository", i.e., this function does?  A
> repository can have one or more worktrees attached to it (that was
> the whole point of introducing the worktree mechanism), so "I have
> this repository, give me the worktree for it" is not a sensible
> request.

A repository can have more that one worktree but a "struct repository" 
instance has "gitdir" and "worktree" members that point to a specific 
worktree within that repository. For example

	repo_get_oid(repo, "HEAD", &oid);

reads "HEAD" from a specific worktree within the repository.

> The function's comment in <worktree.h> talks only at the
> implementation level "construct a worktree struct from repo->gitdir
> and repo->worktree" as if it is so obvious what the resulting
> worktree struct means at a higher layer's point of view, which does
> not help, either.

That comes from me thinking of a struct repository as referring to a 
specific worktree - would calling it 
"get_worktree_from_repository_instance" be clearer?

> I am asking the above because I find this unconditional assignment
> of "true" utterly confusing.  Is it because "we created a brand new
> worktree structure that by definition is current out of a repository
> structure, so as far as the caller is concerned by definition it is
> what it is currently working on"?

Yes

> Stepping back a bit, how do "is_current_worktree(wt)" and
> "wt->is_current" relate to each other?

"wt->current" is the cached return value of is_current_worktree() (see 
the implementation of get_main_worktree() and get_linked_worktree())

>  Here is what happens in the
> former:
> 
>          static int is_current_worktree(struct worktree *wt)
>          {
>                  char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
>                  char *wt_git_dir = get_worktree_git_dir(wt);
>                  int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
>                  free(wt_git_dir);
>                  free(git_dir);
>                  return is_current;
>          }
> 
> and given the definition of get_worktree_git_dir(), which gives
> either the "common" .git directory for the primary worktree, or the
> worktree specific .git directory for the secondaries, the is_current
> being computed here sounds more like "is wt the primary worktree
> among the ones that are attached to the same repository?"

No, the primary worktree (which the code calls the "main" worktree) is 
orthogonal to the "current" worktree. The main worktree is the one whose 
gitdir is $GIT_COMMON_DIR i.e. the worktree created by "git init". The 
"current" worktree is the one whose gitdir is wt->repo->gitdir and may 
or may not be the "main" worktree.

> But given that there are API functions with "main" in their names,
> like is_main_worktree() and internal function get_main_worktree(),
> what I said apparently is not the intention of whoever built the
> worktree subsystem.  I am still confused... X-<.
> 
>> @@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
>>   	if (!wt)
>>   		return xstrdup(repo_get_git_dir(the_repository));
>>   	else if (!wt->id)
>> -		return xstrdup(repo_get_common_dir(the_repository));
>> +		return xstrdup(repo_get_common_dir(wt->repo));
>>   	else
>> -		return repo_common_path(the_repository, "worktrees/%s", wt->id);
>> +		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
>>   }
>>   
>>   static struct worktree *find_worktree_by_suffix(struct worktree **list,
>> diff --git a/worktree.h b/worktree.h
>> index e450d1a3317..94ae58db973 100644
>> --- a/worktree.h
>> +++ b/worktree.h
>> @@ -16,7 +16,7 @@ struct worktree {
>>   	struct object_id head_oid;
>>   	int is_detached;
>>   	int is_bare;
>> -	int is_current;
>> +	int is_current;		/* does `path` match `repo->worktree` */
> 
> This new comment also talks only at the lowest implementation level.
> What significance does it have to the upper layer that calls into
> the API if `path` matches or does not match `repo->worktree` is
> totally unclear, at least to me.

Does it help to think about how "is_current" is used by functions like 
get_worktree_ref_store()? For the current worktree it can use the 
refstore from wt->repo, if it is not it needs to open the store for that 
worktree.

I feel I'm struggling to explain this clearly - I find this whole 
discussion gets confusing because we have "struct worktree" and also a 
"worktree" member of "struct repository" which means a "struct 
repository" instance is tied to a specific worktree within the 
repository. If "struct repository" only had a "commondir" member and no 
"gitdir" or "worktree" members and we instead used "sturct worktree" to 
refer to a specific worktree within a repository with functions like

	worktree_get_oid(wt, "HEAD", &oid);

instead of

	repo_get_oid(repo, "HEAD", &oid);

it might be clearer but that would be a very big change.

Thanks

Phillip

>>   	int lock_reason_valid; /* private */
>>   	int prune_reason_valid; /* private */
>>   };
> 
> Thanks.

Re: [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()

[Junio C Hamano]

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

> On 26/03/2026 15:48, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> ... it calls get_workree_git_dir() which also depends on
>>> "the_repository". This means that even if "wt->path" matches
>>> "wt->repo->worktree" is_current_worktree(wt) will return false when
>>> "wt->repo" is not "the_repository". Consequently die_if_checked_out()
>>> will fail to skip such a worktree when checking if a branch is already
>>> checked out and may die errounously. Fix this by using the worktree's
>>> repository instance instead of "the_repository" when comparing gitdirs.
>> 
>> It sounds like the above is something we can write in a test to make
>> sure the code with this fix won't regress in the future.  Can we add
>> one?
>
> We'd need to create a struct worktree instance with a repository 
> instance that is not "the_repository" - I'm not sure we can do that in a 
> script.

Ah, so this is more of "futureproofing" than "fix"?  That is fine.
Thanks for clarifying.

>>> -	wt->is_current = is_current_worktree(wt);
>>> +	wt->is_current = true;
>>>   	add_head_info(wt);
>>>   
>>>   	free(gitdir);
>> 
>> I think I found the semantics of get_worktree_from_repository()
>> unclear when we discussed a different patch series, so I may have
>> asked the same question already, but what exactly does it mean to
>> "get worktree from repository", i.e., this function does?  A
>> repository can have one or more worktrees attached to it (that was
>> the whole point of introducing the worktree mechanism), so "I have
>> this repository, give me the worktree for it" is not a sensible
>> request.
>
> A repository can have more that one worktree but a "struct repository" 
> instance has "gitdir" and "worktree" members that point to a specific 
> worktree within that repository. For example
>
> 	repo_get_oid(repo, "HEAD", &oid);
>
> reads "HEAD" from a specific worktree within the repository.

And...?

I _think_ what I am frustrated about is the lack of description on
"what it means to be the worktree among many that is pointed by via
the .worktree member in the repository struct".  Does it correspond to
the worktree being "the current worktree the codepath is working on?"

>> The function's comment in <worktree.h> talks only at the
>> implementation level "construct a worktree struct from repo->gitdir
>> and repo->worktree" as if it is so obvious what the resulting
>> worktree struct means at a higher layer's point of view, which does
>> not help, either.
>
> That comes from me thinking of a struct repository as referring to a 
> specific worktree - would calling it 
> "get_worktree_from_repository_instance" be clearer?

It does not change the descriptive value of the name in any
meaningful way, so let's not do that.  If the answer to my "what
frustrates me" comment above is "yeah, we are getting the current
worktree", then renaming the function to include "current" in its
name would add descriptive value vastly, though.

> I feel I'm struggling to explain this clearly - I find this whole 
> discussion gets confusing because we have "struct worktree" and also a 
> "worktree" member of "struct repository" which means a "struct 
> repository" instance is tied to a specific worktree within the 
> repository. If "struct repository" only had a "commondir" member and no 
> "gitdir" or "worktree" members and we instead used "sturct worktree" to 
> refer to a specific worktree within a repository with functions like
>
> 	worktree_get_oid(wt, "HEAD", &oid);
>
> instead of
>
> 	repo_get_oid(repo, "HEAD", &oid);
>
> it might be clearer but that would be a very big change.

In short, am I hearing the worktree subsystem is not conceptually
clean and it would be a huge undertaking to clean it up?

[PATCH v3 2/3] worktree add: stop reading ".git/HEAD"

[Phillip Wood]

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

The function can_use_local_refs() prints a warning if there are no local
branches and HEAD is invalid or points to an unborn branch. As part of
the warning it prints the contents of ".git/HEAD". In a repository using
the reftable backend HEAD is not stored in the filesystem so reading
that file is pointless. In a repository using the files backend it is
unclear how useful printing it is - it would be better to diagnose the
problem for the user. For now, simplify the warning by not printing
the file contents and adjust the relevant test case accordingly. Also
fixup the test case to use test_grep so that anyone trying to debug a
test failure in the future is not met by a wall of silence.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/worktree.c      | 21 ++-------------------
 t/t2400-worktree-add.sh | 28 ++++++++++++----------------
 2 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index bc2d0d645ba..9170b2e8981 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -692,25 +692,8 @@ static int can_use_local_refs(const struct add_opts *opts)
 	if (refs_head_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
 		return 1;
 	} else if (refs_for_each_branch_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
-		if (!opts->quiet) {
-			struct strbuf path = STRBUF_INIT;
-			struct strbuf contents = STRBUF_INIT;
-			char *wt_gitdir = get_worktree_git_dir(NULL);
-
-			strbuf_add_real_path(&path, wt_gitdir);
-			strbuf_addstr(&path, "/HEAD");
-			strbuf_read_file(&contents, path.buf, 64);
-			strbuf_stripspace(&contents, NULL);
-			strbuf_strip_suffix(&contents, "\n");
-
-			warning(_("HEAD points to an invalid (or orphaned) reference.\n"
-				  "HEAD path: '%s'\n"
-				  "HEAD contents: '%s'"),
-				  path.buf, contents.buf);
-			strbuf_release(&path);
-			strbuf_release(&contents);
-			free(wt_gitdir);
-		}
+		if (!opts->quiet)
+			warning(_("HEAD points to an invalid (or orphaned) reference.\n"));
 		return 1;
 	}
 	return 0;
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 023e1301c8e..58b4445cc44 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -987,7 +987,7 @@ test_dwim_orphan () {
 				then
 					test_must_be_empty actual
 				else
-					grep "$info_text" actual
+					test_grep "$info_text" actual
 				fi
 			elif [ "$outcome" = "no_infer" ]
 			then
@@ -996,39 +996,35 @@ test_dwim_orphan () {
 				then
 					test_must_be_empty actual
 				else
-					! grep "$info_text" actual
+					test_grep ! "$info_text" actual
 				fi
 			elif [ "$outcome" = "fetch_error" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
-				grep "$fetch_error_text" actual
+				test_grep "$fetch_error_text" actual
 			elif [ "$outcome" = "fatal_orphan_bad_combo" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
-					! grep "$info_text" actual
+					test_grep ! "$info_text" actual
 				else
-					grep "$info_text" actual
+					test_grep "$info_text" actual
 				fi &&
-				grep "$bad_combo_regex" actual
+				test_grep "$bad_combo_regex" actual
 			elif [ "$outcome" = "warn_bad_head" ]
 			then
 				test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
-					grep "$invalid_ref_regex" actual &&
-					! grep "$orphan_hint" actual
+					test_grep "$invalid_ref_regex" actual &&
+					test_grep ! "$orphan_hint" actual
 				else
-					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
-					headcontents=$(cat "$headpath") &&
-					grep "HEAD points to an invalid (or orphaned) reference" actual &&
-					grep "HEAD path: .$headpath." actual &&
-					grep "HEAD contents: .$headcontents." actual &&
-					grep "$orphan_hint" actual &&
-					! grep "$info_text" actual
+					test_grep "HEAD points to an invalid (or orphaned) reference" actual &&
+					test_grep "$orphan_hint" actual &&
+					test_grep ! "$info_text" actual
 				fi &&
-				grep "$invalid_ref_regex" actual
+				test_grep "$invalid_ref_regex" actual
 			else
 				# Unreachable
 				false
-- 
2.52.0.362.g884e03848a9.dirty

[PATCH v3 3/3] worktree: reject NULL worktree in get_worktree_git_dir()

[Phillip Wood]

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

This removes the final dependence on "the_repository" in
get_worktree_git_dir(). The last commit removed only caller that
passed a NULL worktree.

get_worktree_git_dir() has the following callers:

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

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

 - builtin/receive-pack.c:update_worktree() which is called from
   update() only when "worktree" is non-NULL.

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

 - reachable.c:add_rebase_files() which loops over all worktrees.

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

 - worktree.c:is_current_worktree() which expects a non-NULL worktree.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 worktree.c | 2 +-
 worktree.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/worktree.c b/worktree.c
index 344ad0c031b..1ed5e8c3cd2 100644
--- a/worktree.c
+++ b/worktree.c
@@ -227,7 +227,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));
+		BUG("%s() called with NULL worktree", __func__);
 	else if (!wt->id)
 		return xstrdup(repo_get_common_dir(wt->repo));
 	else
diff --git a/worktree.h b/worktree.h
index 94ae58db973..400b614f133 100644
--- a/worktree.h
+++ b/worktree.h
@@ -51,7 +51,6 @@ int submodule_uses_worktrees(const char *path);
 
 /*
  * Return git dir of the worktree. Note that the path may be relative.
- * If wt is NULL, git dir of current worktree is returned.
  */
 char *get_worktree_git_dir(const struct worktree *wt);
 
-- 
2.52.0.362.g884e03848a9.dirty