[PATCH] worktree: teach find_shared_symref to ignore current worktree

[PATCH] worktree: teach find_shared_symref to ignore current worktree

[Rubén Justo]

We prevent some operations from being executed on a branch checked out
in a worktree other than the current one.  An example of this was
introduced in b5cabb4 (rebase: refuse to switch to branch already
checked out elsewhere, 2020-02-23).

"find_shared_symref()" is sometimes used to find the worktree in which a
branch is checked out.  It performs its search starting with the current
worktree.

As we allow to have the same branch checked out in multiple worktrees
simultaneously...

	$ git worktree add foo
	$ git worktree add -f bar foo
	$ git checkout --ignore-other-worktrees foo

... if the branch checked out in the current worktree is also checked
out in another worktree, with "find_shared_symref()" we will not notice
this "other" working tree.

Let's teach "find_shared_symref()" to ignore the current worktree in the
search, based on the caller's needs.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c               | 4 ++--
 builtin/notes.c        | 2 +-
 builtin/receive-pack.c | 2 +-
 t/t3400-rebase.sh      | 3 +++
 worktree.c             | 6 +++++-
 worktree.h             | 3 ++-
 6 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/branch.c b/branch.c
index d182756827..2508e94add 100644
--- a/branch.c
+++ b/branch.c
@@ -822,8 +822,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
 	struct worktree **worktrees = get_worktrees();
 	const struct worktree *wt;
 
-	wt = find_shared_symref(worktrees, "HEAD", branch);
-	if (wt && (!ignore_current_worktree || !wt->is_current)) {
+	wt = find_shared_symref(worktrees, "HEAD", branch, ignore_current_worktree);
+	if (wt) {
 		skip_prefix(branch, "refs/heads/", &branch);
 		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
 	}
diff --git a/builtin/notes.c b/builtin/notes.c
index 80d9dfd25c..80326bdaab 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -869,7 +869,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
 		worktrees = get_worktrees();
 		wt = find_shared_symref(worktrees, "NOTES_MERGE_REF",
-					default_notes_ref());
+					default_notes_ref(), 0);
 		if (wt)
 			die(_("a notes merge into %s is already in-progress at %s"),
 			    default_notes_ref(), wt->path);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a90af30363..18d400101c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1460,7 +1460,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	int do_update_worktree = 0;
 	struct worktree **worktrees = get_worktrees();
 	const struct worktree *worktree =
-		find_shared_symref(worktrees, "HEAD", name);
+		find_shared_symref(worktrees, "HEAD", name, 0);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d5a8ee39fc..874cfff8fe 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -407,6 +407,9 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '
 	git checkout main &&
 	git worktree add wt &&
 	test_must_fail git -C wt rebase main main 2>err &&
+	test_i18ngrep "already checked out" err &&
+	git worktree add --force wt2 main &&
+	test_must_fail git rebase main main &&
 	test_i18ngrep "already checked out" err
 '
 
diff --git a/worktree.c b/worktree.c
index aa43c64119..3d686137ef 100644
--- a/worktree.c
+++ b/worktree.c
@@ -405,7 +405,8 @@ int is_worktree_being_bisected(const struct worktree *wt,
  */
 const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
-					  const char *target)
+					  const char *target,
+					  int ignore_current_worktree)
 {
 	const struct worktree *existing = NULL;
 	int i = 0;
@@ -416,6 +417,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 		struct ref_store *refs;
 		int flags;
 
+		if (wt->is_current && ignore_current_worktree)
+			continue;
+
 		if (wt->is_bare)
 			continue;
 
diff --git a/worktree.h b/worktree.h
index 9dcea6fc8c..a9f35ee990 100644
--- a/worktree.h
+++ b/worktree.h
@@ -147,7 +147,8 @@ void free_worktrees(struct worktree **);
  */
 const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
-					  const char *target);
+					  const char *target,
+					  int ignore_current_worktree);
 
 /*
  * Similar to head_ref() for all HEADs _except_ one from the current
-- 
2.39.0

Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> We prevent some operations from being executed on a branch checked out
> in a worktree other than the current one.  An example of this was
> introduced in b5cabb4 (rebase: refuse to switch to branch already
> checked out elsewhere, 2020-02-23).
>
> "find_shared_symref()" is sometimes used to find the worktree in which a
> branch is checked out.  It performs its search starting with the current
> worktree.

"starting with the current" may be a correct statement of the fact,
but it is totally unclear what the relevance it has to the problem
being solved. Rather, it is unclear what problem you are solving.

Is it 

 - We search through the worktrees, starting with the current one,
   and stop at the first one found.

 - If the current branch the the current worktree is checked out in
   a different worktree, we get the current worktree back.

 - There are callers that want to know ONLY about other worktrees;
   they check the returned value and when they see it is the current
   one, they happily ignores the fact that it might be checked out
   elsewhere as well.

> As we allow to have the same branch checked out in multiple worktrees
> simultaneously...
>
> 	$ git worktree add foo
> 	$ git worktree add -f bar foo
> 	$ git checkout --ignore-other-worktrees foo
>
> ... if the branch checked out in the current worktree is also checked
> out in another worktree, with "find_shared_symref()" we will not notice
> this "other" working tree.

It is somewhat disturbing that your solution only needs to "ignore"
the current one.  Whatever problem you are seeing by the current
code not ignoring the current worktree, wouldn't we have a similar
problem if two non-current worktrees checked out the same branch?
Would it not be a problem because any non-current worktree returned
by the function triggers the "already checked-out" safety mechanism?

Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree

[Rubén Justo]

On 17-ene-2023 15:27:29, Junio C Hamano wrote:

> being solved. Rather, it is unclear what problem you are solving.

Sorry, I didn't explain the motivation well in the message.

First, I'm not sure if "ignore_current_worktree" is correct, maybe needs
to be "prefer_current_worktree".  Having said that, let me use an example.

With...

	$ git worktree add wt1 -b main
	$ git worktree add wt2 -f main

... we get confuse results:

	$ git -C wt1 rebase main main
	Current branch main is up to date.
	$ git -C wt2 rebase main main
	fatal: 'main' is already checked out...

The problem I'm trying to solve is that find_shared_symref() returns the
first matching worktree, and a possible matching "current worktree"
might not be the first matching one.  That's why die_if_checked_out()
dies with "git -C wt2".

find_shared_symref() searches through the list of worktrees that
get_worktrees() composes: first the main worktree and then, as getdir()
returns them, those in .git/worktrees/*.  The search is sequential and
once a match is found, it is returned.  And so die_if_checked_out(),
when asked to "ignore_current_worktree", is going to consider for
"is_current" the worktree which may or may not be the "current" one,
depending on the sequence from get_worktree(), and getdir() ultimately.

If we want to disallow operations on a worktree with a checked out
branch also on another worktree, we need the "ignore_current_worktree".
But, and now I'm more in favor of this, if we prefer to allow the
operation, we need a "prefer_current_worktree", to induce
find_shared_symref() to return the "current" one if it matches, or any
other one that matches.

Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree

[Phillip Wood]

Hi Rubén

On 18/01/2023 23:50, Rubén Justo wrote:
> On 17-ene-2023 15:27:29, Junio C Hamano wrote:
> 
>> being solved. Rather, it is unclear what problem you are solving.
> 
> Sorry, I didn't explain the motivation well in the message.

I'm afraid I'm still not totally clear from this message exactly what 
the problem is. Having looked at die_if_checked_out() I think it maybe 
the second of Junio's options

  - If the current branch the the current worktree is checked out in
    a different worktree, we get the current worktree back.

Here is the function:

void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
	struct worktree **worktrees = get_worktrees();
	const struct worktree *wt;

	wt = find_shared_symref(worktrees, "HEAD", branch);
	if (wt && (!ignore_current_worktree || !wt->is_current)) {
		skip_prefix(branch, "refs/heads/", &branch);
		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
	}

	free_worktrees(worktrees);
}


It takes a flag to ignore the current worktree but uses 
find_shared_symref() which does not have such a flag to see if the 
branch is checked out. This means that if a branch is checkout out twice 
find_shared_symref() may return the current worktree rather than the one 
we're interested in.

If that is the problem you're trying to solve I think it would be better 
to keep the signature of find_shared_symref() the same and add a helper 
function that is called by die_if_checked_out() and find_shared_symref() 
which can optionally ignore the current worktree. The commit message for 
such a patch should explain you're fixing a bug rather than trying to 
change the existing behavior.

Best Wishes

Phillip


> First, I'm not sure if "ignore_current_worktree" is correct, maybe needs
> to be "prefer_current_worktree".  Having said that, let me use an example.
> 
> With...
> 
> 	$ git worktree add wt1 -b main
> 	$ git worktree add wt2 -f main
> 
> ... we get confuse results:
> 
> 	$ git -C wt1 rebase main main
> 	Current branch main is up to date.
> 	$ git -C wt2 rebase main main
> 	fatal: 'main' is already checked out...
> 
> The problem I'm trying to solve is that find_shared_symref() returns the
> first matching worktree, and a possible matching "current worktree"
> might not be the first matching one.  That's why die_if_checked_out()
> dies with "git -C wt2".
> 
> find_shared_symref() searches through the list of worktrees that
> get_worktrees() composes: first the main worktree and then, as getdir()
> returns them, those in .git/worktrees/*.  The search is sequential and
> once a match is found, it is returned.  And so die_if_checked_out(),
> when asked to "ignore_current_worktree", is going to consider for
> "is_current" the worktree which may or may not be the "current" one,
> depending on the sequence from get_worktree(), and getdir() ultimately.
> 
> If we want to disallow operations on a worktree with a checked out
> branch also on another worktree, we need the "ignore_current_worktree".
> But, and now I'm more in favor of this, if we prefer to allow the
> operation, we need a "prefer_current_worktree", to induce
> find_shared_symref() to return the "current" one if it matches, or any
> other one that matches.

Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree

[Rubén Justo]

On 19-ene-2023 10:48:27, Phillip Wood wrote:

> I'm afraid I'm still not totally clear from this message exactly what the
> problem is. Having looked at die_if_checked_out() I think it maybe the

I'm going to reroll, I hope it makes sense then.

> If that is the problem you're trying to solve I think it would be better to
> keep the signature of find_shared_symref() the same and add a helper
> function that is called by die_if_checked_out() and find_shared_symref()

OK.

> which can optionally ignore the current worktree. The commit message for
> such a patch should explain you're fixing a bug rather than trying to change
> the existing behavior.
> 

Yes, the message is not clear that the commit is fixing a bug.  I'll add
that.

Thank you. 

[PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

Changes:
- The message explains the change as a bug fix.
- Introduced a new helper is_shared_symref() to be used from
  find_shared_symref() and die_if_checked_out().
- Tests for rebase and switch.


Rubén Justo (3):
  branch: fix die_if_checked_out() when ignore_current_worktree
  rebase: refuse to switch to a branch already checked out elsewhere (test)
  switch: reject if the branch is already checked out elsewhere (test)

 branch.c          | 16 +++++++++-----
 t/t2060-switch.sh | 29 +++++++++++++++++++++++++
 t/t3400-rebase.sh | 14 ++++++++++++
 worktree.c        | 54 +++++++++++++++++++++++++----------------------
 worktree.h        |  6 ++++++
 5 files changed, 89 insertions(+), 30 deletions(-)

-- 
2.39.0

[PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

In 8d9fdd7 (worktree.c: check whether branch is rebased in another
worktree, 2016-04-22) die_if_checked_out() learned a new
option ignore_current_worktree, to modify the operation from "die() if
the branch is checked out in any worktree" to "die() if the branch
is checked out in any worktree other than the current one".

Unfortunately we implemented it by checking the flag is_current in the
worktree that find_shared_symref() returns.

When the same branch is checked out in several worktrees simultaneously,
find_shared_symref() will return the first matching worktree in the list
composed by get_worktrees().  If one of the worktrees with the checked
out branch is the current worktree, find_shared_symref() may or may not
return it, depending on the order of the list.

Let's stop using find_shared_symref() in die_if_checked_out(), to handle
correctly ignore_current_worktree.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c   | 16 +++++++++++-----
 worktree.c | 54 +++++++++++++++++++++++++++++-------------------------
 worktree.h |  6 ++++++
 3 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/branch.c b/branch.c
index d182756827..2378368415 100644
--- a/branch.c
+++ b/branch.c
@@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
 {
 	struct worktree **worktrees = get_worktrees();
-	const struct worktree *wt;
+	int i;
+
+	for (i = 0; worktrees[i]; i++)
+	{
+		if (worktrees[i]->is_current && ignore_current_worktree)
+			continue;
 
-	wt = find_shared_symref(worktrees, "HEAD", branch);
-	if (wt && (!ignore_current_worktree || !wt->is_current)) {
-		skip_prefix(branch, "refs/heads/", &branch);
-		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+		if (is_shared_symref(worktrees[i], "HEAD", branch)) {
+			skip_prefix(branch, "refs/heads/", &branch);
+			die(_("'%s' is already checked out at '%s'"),
+				branch, worktrees[i]->path);
+		}
 	}
 
 	free_worktrees(worktrees);
diff --git a/worktree.c b/worktree.c
index aa43c64119..d500d69e4c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
  * bisect). New commands that do similar things should update this
  * function as well.
  */
+int is_shared_symref(const struct worktree *wt, const char *symref,
+		     const char *target)
+{
+	const char *symref_target;
+	struct ref_store *refs;
+	int flags;
+
+	if (wt->is_bare)
+		return 0;
+
+	if (wt->is_detached && !strcmp(symref, "HEAD")) {
+		if (is_worktree_being_rebased(wt, target))
+			return 1;
+		if (is_worktree_being_bisected(wt, target))
+			return 1;
+	}
+
+	refs = get_worktree_ref_store(wt);
+	symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
+						NULL, &flags);
+	if ((flags & REF_ISSYMREF) &&
+	    symref_target && !strcmp(symref_target, target))
+		return 1;
+
+	return 0;
+}
+
 const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
 					  const char *target)
@@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 	int i = 0;
 
 	for (i = 0; worktrees[i]; i++) {
-		struct worktree *wt = worktrees[i];
-		const char *symref_target;
-		struct ref_store *refs;
-		int flags;
-
-		if (wt->is_bare)
-			continue;
-
-		if (wt->is_detached && !strcmp(symref, "HEAD")) {
-			if (is_worktree_being_rebased(wt, target)) {
-				existing = wt;
-				break;
-			}
-			if (is_worktree_being_bisected(wt, target)) {
-				existing = wt;
-				break;
-			}
-		}
-
-		refs = get_worktree_ref_store(wt);
-		symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
-							NULL, &flags);
-		if ((flags & REF_ISSYMREF) &&
-		    symref_target && !strcmp(symref_target, target)) {
-			existing = wt;
+		if (is_shared_symref(worktrees[i], symref, target)) {
+			existing = worktrees[i];
 			break;
 		}
 	}
diff --git a/worktree.h b/worktree.h
index 9dcea6fc8c..7889c4761d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
 					  const char *target);
 
+/*
+ * Returns true if a symref points to a ref in a worktree.
+ */
+int is_shared_symref(const struct worktree *wt,
+		     const char *symref, const char *target);
+
 /*
  * Similar to head_ref() for all HEADs _except_ one from the current
  * worktree, which is covered by head_ref().
-- 
2.39.0

Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Let's stop using find_shared_symref() in die_if_checked_out(), to handle
> correctly ignore_current_worktree.

This says what the code stops using, but does not say what it uses
instead.

Factoring is_shared_symref() out of find_shared_symref() is probably
a good idea that can stand alone without any other change in this
patch as a single step, and then a second step can update
die_if_checked_out() using the new function.

As the proposed log message explained, updating die_if_checked_out()
with this patch would fix a bug---can we demonstrate the existing
breakage and protect the fix from future breakages by adding a test
or two?

Other than that, it looks like it is going in the right direction.
Thanks for working on the topic.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  branch.c   | 16 +++++++++++-----
>  worktree.c | 54 +++++++++++++++++++++++++++++-------------------------
>  worktree.h |  6 ++++++
>  3 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index d182756827..2378368415 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)
>  void die_if_checked_out(const char *branch, int ignore_current_worktree)
>  {
>  	struct worktree **worktrees = get_worktrees();
> -	const struct worktree *wt;
> +	int i;
> +
> +	for (i = 0; worktrees[i]; i++)
> +	{

Style.  WRite the above on a single line, i.e.

	for (i = 0; worktrees[i]; i++) {

Optionally, we can lose the separate declaration of "i" by using C99
variable declaration, i.e.

	for (int i = 0; worktrees[i]; i++) {

> diff --git a/worktree.c b/worktree.c
> index aa43c64119..d500d69e4c 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
>   * bisect). New commands that do similar things should update this
>   * function as well.
>   */

The above comment is about find_shared_symref() which iterates over
worktrees and find the one that uses the named symref.  Now the
comment appears to apply to is_shared_symref() which does not
iterate but takes one specific worktree instance.  Do their
differences necessitate some updates to the comment?

> +int is_shared_symref(const struct worktree *wt, const char *symref,
> +		     const char *target)
> +{

What this function does sound more like "is target in use in this
particular worktree by being pointed at by the symref?"  IOW, I do
not see where "shared" comes into its name from.

"HEAD" that is tentatively detached while bisecting or rebasing the
"target" branch is still considered to point at the "target", so
perhaps symref_points_at_target() or something?

>  const struct worktree *find_shared_symref(struct worktree **worktrees,
>  					  const char *symref,
>  					  const char *target)
> @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>  	int i = 0;
>  
>  	for (i = 0; worktrees[i]; i++) {

Not a new problem, but the initialization on the declaration of "i"
is redundant and unnecessary.  Again, we can use the C99 style, i.e.

	const struct worktree *existing = NULL;
-	int i = 0;
-
-	for (i = 0; worktrees[i]; i++) {
+	for (int i = 0; worktrees[i]; i++) {

> +		if (is_shared_symref(worktrees[i], symref, target)) {
> +			existing = worktrees[i];
>  			break;
>  		}
>  	}
> diff --git a/worktree.h b/worktree.h
> index 9dcea6fc8c..7889c4761d 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>  					  const char *symref,
>  					  const char *target);
>  
> +/*
> + * Returns true if a symref points to a ref in a worktree.
> + */

Make it clear that what you called "a ref" in the above is what is
called "target" below.

> +int is_shared_symref(const struct worktree *wt,
> +		     const char *symref, const char *target);
> +
>  /*
>   * Similar to head_ref() for all HEADs _except_ one from the current
>   * worktree, which is covered by head_ref().

Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

On 21-ene-2023 17:50:55, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Let's stop using find_shared_symref() in die_if_checked_out(), to handle
> > correctly ignore_current_worktree.
> 
> This says what the code stops using, but does not say what it uses
> instead.

I thought the code for that was a better and clearer description.  I'll add
some details to the message.

> Factoring is_shared_symref() out of find_shared_symref() is probably
> a good idea that can stand alone without any other change in this
> patch as a single step, and then a second step can update
> die_if_checked_out() using the new function.

OK.  I'll split that in two.

> As the proposed log message explained, updating die_if_checked_out()
> with this patch would fix a bug---can we demonstrate the existing
> breakage and protect the fix from future breakages by adding a test
> or two?

2/3 and 3/3, I think makes more sense on its own commit.

> > -	const struct worktree *wt;
> > +	int i;
> > +
> > +	for (i = 0; worktrees[i]; i++)
> > +	{
> 
> Style.  WRite the above on a single line, i.e.
> 
> 	for (i = 0; worktrees[i]; i++) {

Sorry.  I'll fix that.

> 
> Optionally, we can lose the separate declaration of "i" by using C99
> variable declaration, i.e.
> 
> 	for (int i = 0; worktrees[i]; i++) {

OK.  Yes, I was playing with this, changed my mind and ended up with this and
the other style below, mess.

> 
> > diff --git a/worktree.c b/worktree.c
> > index aa43c64119..d500d69e4c 100644
> > --- a/worktree.c
> > +++ b/worktree.c
> > @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
> >   * bisect). New commands that do similar things should update this
> >   * function as well.
> >   */
> 
> The above comment is about find_shared_symref() which iterates over
> worktrees and find the one that uses the named symref.  Now the
> comment appears to apply to is_shared_symref() which does not
> iterate but takes one specific worktree instance.  Do their
> differences necessitate some updates to the comment?

I think the comment still makes sense as is for the new function, both the
description and the recommendation.  I will review it again.

> 
> > +int is_shared_symref(const struct worktree *wt, const char *symref,
> > +		     const char *target)
> > +{
> 
> What this function does sound more like "is target in use in this
> particular worktree by being pointed at by the symref?"  IOW, I do
> not see where "shared" comes into its name from.
> 
> "HEAD" that is tentatively detached while bisecting or rebasing the
> "target" branch is still considered to point at the "target", so
> perhaps symref_points_at_target() or something?
> 

I tried to maintain the terms as much as possible.  I'll think about the name
you suggest.

> >  const struct worktree *find_shared_symref(struct worktree **worktrees,
> >  					  const char *symref,
> >  					  const char *target)
> > @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> >  	int i = 0;
> >  
> >  	for (i = 0; worktrees[i]; i++) {
> 
> Not a new problem, but the initialization on the declaration of "i"
> is redundant and unnecessary.  Again, we can use the C99 style, i.e.
> 
> 	const struct worktree *existing = NULL;
> -	int i = 0;
> -
> -	for (i = 0; worktrees[i]; i++) {
> +	for (int i = 0; worktrees[i]; i++) {

I'll fix this.

> 
> > +		if (is_shared_symref(worktrees[i], symref, target)) {
> > +			existing = worktrees[i];
> >  			break;
> >  		}
> >  	}
> > diff --git a/worktree.h b/worktree.h
> > index 9dcea6fc8c..7889c4761d 100644
> > --- a/worktree.h
> > +++ b/worktree.h
> > @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> >  					  const char *symref,
> >  					  const char *target);
> >  
> > +/*
> > + * Returns true if a symref points to a ref in a worktree.
> > + */
> 
> Make it clear that what you called "a ref" in the above is what is
> called "target" below.
> 

Again, that was an attempt to maintain the terms from find_shared_symref().


Thank you for your review.

Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> As the proposed log message explained, updating die_if_checked_out()
>> with this patch would fix a bug---can we demonstrate the existing
>> breakage and protect the fix from future breakages by adding a test
>> or two?
>
> 2/3 and 3/3, I think makes more sense on its own commit.

Hmph, how so?  Especially once you split 1/3 into the preliminary
refactoring and the real fix, the fix becomes fairly small and
clear.  And the tests to protect the fix would go best in the same
commit.

>> The above comment is about find_shared_symref() which iterates over
>> worktrees and find the one that uses the named symref.  Now the
>> comment appears to apply to is_shared_symref() which does not
>> iterate but takes one specific worktree instance.  Do their
>> differences necessitate some updates to the comment?
>
> I think the comment still makes sense as is for the new function, both the
> description and the recommendation.  I will review it again.

OK.  Thanks.

>> > +int is_shared_symref(const struct worktree *wt, const char *symref,
>> > +		     const char *target)
>> > +{
>> 
>> What this function does sound more like "is target in use in this
>> particular worktree by being pointed at by the symref?"  IOW, I do
>> not see where "shared" comes into its name from.
>> 
>> "HEAD" that is tentatively detached while bisecting or rebasing the
>> "target" branch is still considered to point at the "target", so
>> perhaps symref_points_at_target() or something?
>
> I tried to maintain the terms as much as possible.  I'll think about the name
> you suggest.

When you did not change a thing in such a way that it does not
change the relationship between that thing and other things, it
makes perfect sense to keep the same term to refer to the thing.
Otherwise, once the thing starts playing different roles in the
world, there may be a better word to refer to the updated and
improved thing.

Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

On 22-ene-2023 11:58:05, Junio C Hamano wrote:

> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> As the proposed log message explained, updating die_if_checked_out()
> >> with this patch would fix a bug---can we demonstrate the existing
> >> breakage and protect the fix from future breakages by adding a test
> >> or two?
> >
> > 2/3 and 3/3, I think makes more sense on its own commit.
> 
> Hmph, how so?  Especially once you split 1/3 into the preliminary
> refactoring and the real fix, the fix becomes fairly small and
> clear.  And the tests to protect the fix would go best in the same
> commit.

My intention is to protect rebase (2/3) and switch (3/3).  If any of
those tests break, even if die_if_checked_out() is no longer used by
them, I try to make the original intent clear with that in there.

die_if_checked_out() was initially fine, the ignore_current_worktree was
unfortunately introduced.  I haven't checked, but other callers not
affected by the change, i.e. ignore_current_worktree = 0, his tests
should have protected them by the change.

You are right, in a future reroll, split 1/3 could leave a fairly small
commit, maybe not a bad thing.  Definitely this need a reroll, because
of the style issues, but I will wait some time for other reviewers. 

> >> The above comment is about find_shared_symref() which iterates over
> >> worktrees and find the one that uses the named symref.  Now the
> >> comment appears to apply to is_shared_symref() which does not
> >> iterate but takes one specific worktree instance.  Do their
> >> differences necessitate some updates to the comment?
> >
> > I think the comment still makes sense as is for the new function, both the
> > description and the recommendation.  I will review it again.
> 
> OK.  Thanks.
> 
> >> > +int is_shared_symref(const struct worktree *wt, const char *symref,
> >> > +		     const char *target)
> >> > +{
> >> 
> >> What this function does sound more like "is target in use in this
> >> particular worktree by being pointed at by the symref?"  IOW, I do
> >> not see where "shared" comes into its name from.
> >> 
> >> "HEAD" that is tentatively detached while bisecting or rebasing the
> >> "target" branch is still considered to point at the "target", so
> >> perhaps symref_points_at_target() or something?
> >
> > I tried to maintain the terms as much as possible.  I'll think about the name
> > you suggest.
> 
> When you did not change a thing in such a way that it does not
> change the relationship between that thing and other things, it
> makes perfect sense to keep the same term to refer to the thing.
> Otherwise, once the thing starts playing different roles in the
> world, there may be a better word to refer to the updated and
> improved thing.

I tried to maintain the relationship and the role, too.  Just introduce
the helper, as Phillip suggested and I think it is a good idea. 

Thank you.

Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Phillip Wood]

Hi Rubén

On 22/01/2023 23:21, Rubén Justo wrote:
> I tried to maintain the relationship and the role, too.  Just introduce
> the helper, as Phillip suggested and I think it is a good idea.

When I suggested adding a helper I was thinking of something like

static const struct worktree *do_find_shared_symref(struct worktree 
**worktrees,
  					  const char *symref,
  					  const char *target,
					  int ignore_current)
{
	/*
	 * Body moved from find_share_symref() with a couple
	 * of lines added to support ignore_current
	 /*
}

const struct worktree *find_shared_symref(struct worktree **worktrees,
  					  const char *symref,
  					  const char *target)
{
	return do_find_shared_symref(worktrees, symref, target, 0)
}

void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
	struct worktree **worktrees = get_worktrees();
	const struct worktree *wt;

	wt = do_find_shared_symref(worktrees, "HEAD", branch,
				   ignore_current_worktree);
	/* rest unchanged */
}

The aim was to avoid changing the public api

Best Wishes

Phillip

Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

On 24-ene-2023 10:35:26, Phillip Wood wrote:

> On 22/01/2023 23:21, Rubén Justo wrote:
> > I tried to maintain the relationship and the role, too.  Just introduce
> > the helper, as Phillip suggested and I think it is a good idea.
> 
> When I suggested adding a helper I was thinking of something like
> 
> static const struct worktree *do_find_shared_symref(struct worktree
> **worktrees,
>  					  const char *symref,
>  					  const char *target,
> 					  int ignore_current)
> {
> 	/*
> 	 * Body moved from find_share_symref() with a couple
> 	 * of lines added to support ignore_current
> 	 /*
> }
> 
> const struct worktree *find_shared_symref(struct worktree **worktrees,
>  					  const char *symref,
>  					  const char *target)
> {
> 	return do_find_shared_symref(worktrees, symref, target, 0)
> }
> 
> void die_if_checked_out(const char *branch, int ignore_current_worktree)
> {
> 	struct worktree **worktrees = get_worktrees();
> 	const struct worktree *wt;
> 
> 	wt = do_find_shared_symref(worktrees, "HEAD", branch,
> 				   ignore_current_worktree);
> 	/* rest unchanged */
> }
> 
> The aim was to avoid changing the public api

I thought about a solution like the one you suggest.  Also another one based on
iterations, something like wt_head_refs()....  I ended up with
is_shared_symref(), it adds some value, I think.

The public API remains unchanged, and I like that the comment for
find_shared_ref(), which is an important note, is moved to is_shared_symref(),
which contains the essential work related to the comment.

die_if_checked_out() needs to iterate (here was the wt_heads_refs()), but my
intuition makes me think it's a good step since we might need another level,
I'm not sure yet but, like "die_if_if_checked_out(allow_if_current)".

I'm going to send a v3 addressing the issues Junio commented, still with the
is_shared_symref().  If the above reasoning doesn't work for you or if the
change as-is introduces any problem, I have no objection to
"do_find_shared_symref()".

Thank you.

[PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Rubén Justo]

In b5cabb4a9 (rebase: refuse to switch to branch already checked out
elsewhere, 2020-02-23) we add a condition to prevent a rebase operation
involving a switch to a branch that is already checked out in another
worktree.

A bug has recently been fixed that caused this to not work as expected.

Let's add a test to notice if this changes in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3400-rebase.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d5a8ee39fc..3ce918fdb8 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '
 	git rebase main main
 '
 
+test_expect_success 'switch to branch checked out elsewhere fails' '
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+	" &&
+	git worktree add wt1 -b shared &&
+	git worktree add wt2 -f shared &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_must_fail git -C wt1 rebase shared shared &&
+	test_must_fail git -C wt2 rebase shared shared
+'
+
 test_expect_success 'switch to branch not checked out' '
 	git checkout main &&
 	git branch other &&
-- 
2.39.0

[PATCH v2 3/3] switch: reject if the branch is already checked out elsewhere (test)

[Rubén Justo]

Since 5883034 (checkout: reject if the branch is already checked out
elsewhere) in normal use, we do not allow multiple worktrees having the
same checked out branch.

A bug has recently been fixed that caused this to not work as expected.

Let's add a test to notice if this changes in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 5a7caf958c..7bea95dba2 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
 	test_cmp_config "" --default "" branch.main2.merge
 '
 
+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+		git checkout -
+	" &&
+	git checkout -b shared &&
+	test_commit shared-first &&
+	HASH1=$(git rev-parse --verify HEAD) &&
+	test_commit shared-second &&
+	test_commit shared-third &&
+	HASH2=$(git rev-parse --verify HEAD) &&
+	git worktree add wt1 -f shared &&
+	git -C wt1 bisect start &&
+	git -C wt1 bisect good $HASH1 &&
+	git -C wt1 bisect bad $HASH2 &&
+	git worktree add wt2 -f shared &&
+	git -C wt2 bisect start &&
+	git -C wt2 bisect good $HASH1 &&
+	git -C wt2 bisect bad $HASH2 &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_must_fail git -C wt1 switch shared &&
+	git -C wt1 switch --ignore-other-worktrees shared &&
+	test_must_fail git -C wt2 switch shared &&
+	git -C wt2 switch --ignore-other-worktrees shared
+'
+
 test_done
-- 
2.39.0

[PATCH v3 0/4] fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

Changes from v2:

 - is_shared_symref() is introduced in its own commit
 - code style fixes

Rubén Justo (4):
  worktree: introduce is_shared_symref()
  branch: fix die_if_checked_out() when ignore_current_worktree
  rebase: refuse to switch to a branch already checked out elsewhere (test)
  switch: reject if the branch is already checked out elsewhere (test)

 branch.c          | 14 +++++++----
 t/t2060-switch.sh | 29 ++++++++++++++++++++++
 t/t3400-rebase.sh | 14 +++++++++++
 worktree.c        | 63 +++++++++++++++++++++++------------------------
 worktree.h        |  6 +++++
 5 files changed, 89 insertions(+), 37 deletions(-)

Range-diff against v2:
1:  cacdc022f8 < -:  ---------- branch: fix die_if_checked_out() when ignore_current_worktree
-:  ---------- > 1:  6bbe05f452 worktree: introduce is_shared_symref()
-:  ---------- > 2:  d787afe77f branch: fix die_if_checked_out() when ignore_current_worktree
2:  6e9ed45f4e = 3:  4c418d37f8 rebase: refuse to switch to a branch already checked out elsewhere (test)
3:  a66e58e7b2 = 4:  00b075af6a switch: reject if the branch is already checked out elsewhere (test)

Diff against v2:
diff --git a/branch.c b/branch.c
index aa854fa65f..64b7dbfd17 100644
--- a/branch.c
+++ b/branch.c
@@ -820,10 +820,8 @@ void remove_branch_state(struct repository *r, int verbose)
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
 {
 	struct worktree **worktrees = get_worktrees();
-	int i;
 
-	for (i = 0; worktrees[i]; i++)
-	{
+	for (int i = 0; worktrees[i]; i++) {
 		if (worktrees[i]->is_current && ignore_current_worktree)
 			continue;
 
diff --git a/worktree.c b/worktree.c
index d500d69e4c..34043d8fe0 100644
--- a/worktree.c
+++ b/worktree.c
@@ -434,17 +434,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
 					  const char *target)
 {
-	const struct worktree *existing = NULL;
-	int i = 0;
 
-	for (i = 0; worktrees[i]; i++) {
-		if (is_shared_symref(worktrees[i], symref, target)) {
-			existing = worktrees[i];
-			break;
-		}
-	}
+	for (int i = 0; worktrees[i]; i++)
+		if (is_shared_symref(worktrees[i], symref, target))
+			return worktrees[i];
 
-	return existing;
+	return NULL;
 }
 
 int submodule_uses_worktrees(const char *path)
-- 
2.39.0

[PATCH v3 1/4] worktree: introduce is_shared_symref()

[Rubén Justo]

Add a new function, is_shared_symref(), which contains the heart of
find_shared_symref().  Refactor find_shared_symref() to use the new
function is_shared_symref().

Soon, we will use is_shared_symref() to search for symref beyond
the first worktree that matches.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 worktree.c | 62 +++++++++++++++++++++++++++---------------------------
 worktree.h |  6 ++++++
 2 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/worktree.c b/worktree.c
index aa43c64119..40cb9874b7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -403,44 +403,44 @@ int is_worktree_being_bisected(const struct worktree *wt,
  * bisect). New commands that do similar things should update this
  * function as well.
  */
-const struct worktree *find_shared_symref(struct worktree **worktrees,
-					  const char *symref,
-					  const char *target)
+int is_shared_symref(const struct worktree *wt, const char *symref,
+		     const char *target)
 {
-	const struct worktree *existing = NULL;
-	int i = 0;
+	const char *symref_target;
+	struct ref_store *refs;
+	int flags;
 
-	for (i = 0; worktrees[i]; i++) {
-		struct worktree *wt = worktrees[i];
-		const char *symref_target;
-		struct ref_store *refs;
-		int flags;
+	if (wt->is_bare)
+		return 0;
 
-		if (wt->is_bare)
-			continue;
+	if (wt->is_detached && !strcmp(symref, "HEAD")) {
+		if (is_worktree_being_rebased(wt, target))
+			return 1;
+		if (is_worktree_being_bisected(wt, target))
+			return 1;
+	}
 
-		if (wt->is_detached && !strcmp(symref, "HEAD")) {
-			if (is_worktree_being_rebased(wt, target)) {
-				existing = wt;
-				break;
-			}
-			if (is_worktree_being_bisected(wt, target)) {
-				existing = wt;
-				break;
-			}
-		}
+	refs = get_worktree_ref_store(wt);
+	symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
+						NULL, &flags);
+	if ((flags & REF_ISSYMREF) &&
+	    symref_target && !strcmp(symref_target, target))
+		return 1;
 
-		refs = get_worktree_ref_store(wt);
-		symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
-							NULL, &flags);
-		if ((flags & REF_ISSYMREF) &&
-		    symref_target && !strcmp(symref_target, target)) {
-			existing = wt;
-			break;
-		}
+	return 0;
+}
+
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+					  const char *symref,
+					  const char *target)
+{
+
+	for (int i = 0; worktrees[i]; i++) {
+		if (is_shared_symref(worktrees[i], symref, target))
+			return worktrees[i];
 	}
 
-	return existing;
+	return NULL;
 }
 
 int submodule_uses_worktrees(const char *path)
diff --git a/worktree.h b/worktree.h
index 9dcea6fc8c..7889c4761d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
 					  const char *target);
 
+/*
+ * Returns true if a symref points to a ref in a worktree.
+ */
+int is_shared_symref(const struct worktree *wt,
+		     const char *symref, const char *target);
+
 /*
  * Similar to head_ref() for all HEADs _except_ one from the current
  * worktree, which is covered by head_ref().
-- 
2.39.0

Re: [PATCH v3 1/4] worktree: introduce is_shared_symref()

[Phillip Wood]

Hi Rubén

I'm still not really sure why we want to make this new function public 
but the conversion looks fine. It does mean that the next patch has to 
add a loop to die_if_checked_out() though which we wouldn't need to do 
if we just changed find_shared_symref() to take an extra parameter to 
ignore the current worktree.

Best Wishes

Phillip

On 04/02/2023 23:25, Rubén Justo wrote:
> Add a new function, is_shared_symref(), which contains the heart of
> find_shared_symref().  Refactor find_shared_symref() to use the new
> function is_shared_symref().
> 
> Soon, we will use is_shared_symref() to search for symref beyond
> the first worktree that matches.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   worktree.c | 62 +++++++++++++++++++++++++++---------------------------
>   worktree.h |  6 ++++++
>   2 files changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/worktree.c b/worktree.c
> index aa43c64119..40cb9874b7 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -403,44 +403,44 @@ int is_worktree_being_bisected(const struct worktree *wt,
>    * bisect). New commands that do similar things should update this
>    * function as well.
>    */
> -const struct worktree *find_shared_symref(struct worktree **worktrees,
> -					  const char *symref,
> -					  const char *target)
> +int is_shared_symref(const struct worktree *wt, const char *symref,
> +		     const char *target)
>   {
> -	const struct worktree *existing = NULL;
> -	int i = 0;
> +	const char *symref_target;
> +	struct ref_store *refs;
> +	int flags;
>   
> -	for (i = 0; worktrees[i]; i++) {
> -		struct worktree *wt = worktrees[i];
> -		const char *symref_target;
> -		struct ref_store *refs;
> -		int flags;
> +	if (wt->is_bare)
> +		return 0;
>   
> -		if (wt->is_bare)
> -			continue;
> +	if (wt->is_detached && !strcmp(symref, "HEAD")) {
> +		if (is_worktree_being_rebased(wt, target))
> +			return 1;
> +		if (is_worktree_being_bisected(wt, target))
> +			return 1;
> +	}
>   
> -		if (wt->is_detached && !strcmp(symref, "HEAD")) {
> -			if (is_worktree_being_rebased(wt, target)) {
> -				existing = wt;
> -				break;
> -			}
> -			if (is_worktree_being_bisected(wt, target)) {
> -				existing = wt;
> -				break;
> -			}
> -		}
> +	refs = get_worktree_ref_store(wt);
> +	symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
> +						NULL, &flags);
> +	if ((flags & REF_ISSYMREF) &&
> +	    symref_target && !strcmp(symref_target, target))
> +		return 1;
>   
> -		refs = get_worktree_ref_store(wt);
> -		symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
> -							NULL, &flags);
> -		if ((flags & REF_ISSYMREF) &&
> -		    symref_target && !strcmp(symref_target, target)) {
> -			existing = wt;
> -			break;
> -		}
> +	return 0;
> +}
> +
> +const struct worktree *find_shared_symref(struct worktree **worktrees,
> +					  const char *symref,
> +					  const char *target)
> +{
> +
> +	for (int i = 0; worktrees[i]; i++) {
> +		if (is_shared_symref(worktrees[i], symref, target))
> +			return worktrees[i];
>   	}
>   
> -	return existing;
> +	return NULL;
>   }
>   
>   int submodule_uses_worktrees(const char *path)
> diff --git a/worktree.h b/worktree.h
> index 9dcea6fc8c..7889c4761d 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>   					  const char *symref,
>   					  const char *target);
>   
> +/*
> + * Returns true if a symref points to a ref in a worktree.
> + */
> +int is_shared_symref(const struct worktree *wt,
> +		     const char *symref, const char *target);
> +
>   /*
>    * Similar to head_ref() for all HEADs _except_ one from the current
>    * worktree, which is covered by head_ref().

[PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

In 8d9fdd7 (worktree.c: check whether branch is rebased in another
worktree, 2016-04-22) die_if_checked_out() learned a new option
ignore_current_worktree, to modify the operation from "die() if the
branch is checked out in any worktree" to "die() if the branch is
checked out in any worktree other than the current one".

Unfortunately we implemented it by checking the flag is_current in the
worktree that find_shared_symref() returns.

When the same branch is checked out in several worktrees simultaneously,
find_shared_symref() will return the first matching worktree in the list
composed by get_worktrees().  If one of the worktrees with the checked
out branch is the current worktree, find_shared_symref() may or may not
return it, depending on the order in the list.

Instead of find_shared_symref(), let's do the search using use the
recently introduced API is_shared_symref(), and consider
ignore_current_worktree when necessary.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c   | 14 +++++++++-----
 worktree.c |  3 +--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index e5614b53b3..64b7dbfd17 100644
--- a/branch.c
+++ b/branch.c
@@ -820,12 +820,16 @@ void remove_branch_state(struct repository *r, int verbose)
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
 {
 	struct worktree **worktrees = get_worktrees();
-	const struct worktree *wt;
 
-	wt = find_shared_symref(worktrees, "HEAD", branch);
-	if (wt && (!ignore_current_worktree || !wt->is_current)) {
-		skip_prefix(branch, "refs/heads/", &branch);
-		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+	for (int i = 0; worktrees[i]; i++) {
+		if (worktrees[i]->is_current && ignore_current_worktree)
+			continue;
+
+		if (is_shared_symref(worktrees[i], "HEAD", branch)) {
+			skip_prefix(branch, "refs/heads/", &branch);
+			die(_("'%s' is already checked out at '%s'"),
+				branch, worktrees[i]->path);
+		}
 	}
 
 	free_worktrees(worktrees);
diff --git a/worktree.c b/worktree.c
index 40cb9874b7..34043d8fe0 100644
--- a/worktree.c
+++ b/worktree.c
@@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *target)
 {
 
-	for (int i = 0; worktrees[i]; i++) {
+	for (int i = 0; worktrees[i]; i++)
 		if (is_shared_symref(worktrees[i], symref, target))
 			return worktrees[i];
-	}
 
 	return NULL;
 }
-- 
2.39.0

Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

[Ævar Arnfjörð Bjarmason]

On Sun, Feb 05 2023, Rubén Justo wrote:

> -	wt = find_shared_symref(worktrees, "HEAD", branch);
> -	if (wt && (!ignore_current_worktree || !wt->is_current)) {
> -		skip_prefix(branch, "refs/heads/", &branch);
> -		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
> +	for (int i = 0; worktrees[i]; i++) {

I see that there are existing "int i" for counting worktrees in
worktree.c, FWIW for new code I wouldn't mind if it's "size_t i"
instead, to make it future proof (and to eventually get rid of cast
warnings as we move more things from "int" to "size_t").
> @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>  					  const char *target)
>  {
>  
> -	for (int i = 0; worktrees[i]; i++) {
> +	for (int i = 0; worktrees[i]; i++)
>  		if (is_shared_symref(worktrees[i], symref, target))
>  			return worktrees[i];
> -	}

You added this function in the last commit, let's just skip adding the
braces to begin with, rather than this style-fix after the fact.

Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

On 06-feb-2023 17:56:55, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Feb 05 2023, Rubén Justo wrote:
> 
> > -	wt = find_shared_symref(worktrees, "HEAD", branch);
> > -	if (wt && (!ignore_current_worktree || !wt->is_current)) {
> > -		skip_prefix(branch, "refs/heads/", &branch);
> > -		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
> > +	for (int i = 0; worktrees[i]; i++) {
> 
> I see that there are existing "int i" for counting worktrees in
> worktree.c, FWIW for new code I wouldn't mind if it's "size_t i"
> instead, to make it future proof (and to eventually get rid of cast
> warnings as we move more things from "int" to "size_t").

OK.

> > @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> >  					  const char *target)
> >  {
> >  
> > -	for (int i = 0; worktrees[i]; i++) {
> > +	for (int i = 0; worktrees[i]; i++)
> >  		if (is_shared_symref(worktrees[i], symref, target))
> >  			return worktrees[i];
> > -	}
> 
> You added this function in the last commit, let's just skip adding the
> braces to begin with, rather than this style-fix after the fact.

OK. Thanks.

Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

[Phillip Wood]

On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Feb 05 2023, Rubén Justo wrote:
> 
>> -	wt = find_shared_symref(worktrees, "HEAD", branch);
>> -	if (wt && (!ignore_current_worktree || !wt->is_current)) {
>> -		skip_prefix(branch, "refs/heads/", &branch);
>> -		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
>> +	for (int i = 0; worktrees[i]; i++) {
> 
> I see that there are existing "int i" for counting worktrees in
> worktree.c, FWIW for new code I wouldn't mind if it's "size_t i"
> instead, to make it future proof (and to eventually get rid of cast
> warnings as we move more things from "int" to "size_t").

This seems to be different from the usual worries about int/size_t 
comparisons/truncation. worktrees is NULL terminated so there is no 
signed/unsigned comparison here as we're not comparing it to anything. 
The only concern would be that there are more than INT_MAX which seems 
unlikely.

Best Wishes

Phillip

>> @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>>   					  const char *target)
>>   {
>>   
>> -	for (int i = 0; worktrees[i]; i++) {
>> +	for (int i = 0; worktrees[i]; i++)
>>   		if (is_shared_symref(worktrees[i], symref, target))
>>   			return worktrees[i];
>> -	}
> 
> You added this function in the last commit, let's just skip adding the
> braces to begin with, rather than this style-fix after the fact.

Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 07 2023, Phillip Wood wrote:

> On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote:
>> On Sun, Feb 05 2023, Rubén Justo wrote:
>> 
>>> -	wt = find_shared_symref(worktrees, "HEAD", branch);
>>> -	if (wt && (!ignore_current_worktree || !wt->is_current)) {
>>> -		skip_prefix(branch, "refs/heads/", &branch);
>>> -		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
>>> +	for (int i = 0; worktrees[i]; i++) {
>> I see that there are existing "int i" for counting worktrees in
>> worktree.c, FWIW for new code I wouldn't mind if it's "size_t i"
>> instead, to make it future proof (and to eventually get rid of cast
>> warnings as we move more things from "int" to "size_t").
>
> This seems to be different from the usual worries about int/size_t
> comparisons/truncation. worktrees is NULL terminated so there is no
> signed/unsigned comparison here as we're not comparing it to
> anything.

Having looked at this again I think using "int" for now is the right
thing, sorry about the noise.

> The only concern would be that there are more than INT_MAX
> which seems unlikely.

My general concern isn't just with the code that we can prove doesn't
overflow in such cases, but that by having different types for such
things (which isn't the case here, I thought our "struct worktrees **"
would be alloc'd with a "size_t") we end up with coercion warnings.

Those warnings are so prevalent in this codebase that we've had to
suppress them, and consequently we make it harder to spot the real
issues.

[PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Rubén Justo]

In b5cabb4a9 (rebase: refuse to switch to branch already checked out
elsewhere, 2020-02-23) we add a condition to prevent a rebase operation
involving a switch to a branch that is already checked out in another
worktree.

A bug has recently been fixed that caused this to not work as expected.

Let's add a test to notice if this changes in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3400-rebase.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d5a8ee39fc..3ce918fdb8 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '
 	git rebase main main
 '
 
+test_expect_success 'switch to branch checked out elsewhere fails' '
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+	" &&
+	git worktree add wt1 -b shared &&
+	git worktree add wt2 -f shared &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_must_fail git -C wt1 rebase shared shared &&
+	test_must_fail git -C wt2 rebase shared shared
+'
+
 test_expect_success 'switch to branch not checked out' '
 	git checkout main &&
 	git branch other &&
-- 
2.39.0

Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Ævar Arnfjörð Bjarmason]

On Sun, Feb 05 2023, Rubén Justo wrote:

> In b5cabb4a9 (rebase: refuse to switch to branch already checked out
> elsewhere, 2020-02-23) we add a condition to prevent a rebase operation
> involving a switch to a branch that is already checked out in another
> worktree.
>
> A bug has recently been fixed that caused this to not work as expected.

Presumably in 1/4 and 2/4, if so saying "in the preceding commit(s)"
would be better.

> Let's add a test to notice if this changes in the future.

I for one would find this series much easier to follow if you started
with this test, possibly with a test_expect_failure, and as we fix the
relevant code flip them (both this and the subsequent one) to run
successfully, and include them as part of the commit that fixes the
bug).

Maybe there's reasons for why that's tricky to do in this case, so
please ignore this if so.

Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Rubén Justo]

On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:

> > Let's add a test to notice if this changes in the future.
> 
> I for one would find this series much easier to follow if you started
> with this test, possibly with a test_expect_failure, and as we fix the
> relevant code flip them (both this and the subsequent one) to run
> successfully, and include them as part of the commit that fixes the
> bug).
> 
> Maybe there's reasons for why that's tricky to do in this case, so
> please ignore this if so.

I'll give it try, I like the idea.  Thanks.

Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Phillip Wood]

Hi Rubén

On 06/02/2023 23:16, Rubén Justo wrote:
> On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:
> 
>>> Let's add a test to notice if this changes in the future.
>>
>> I for one would find this series much easier to follow if you started
>> with this test, possibly with a test_expect_failure, and as we fix the
>> relevant code flip them (both this and the subsequent one) to run
>> successfully, and include them as part of the commit that fixes the
>> bug).
>>
>> Maybe there's reasons for why that's tricky to do in this case, so
>> please ignore this if so.
> 
> I'll give it try, I like the idea.  Thanks.

Squashing the last three commits together so that the tests are 
introduced in the same commit as the fix as Junio suggested in his 
comments on the previous round would be very welcome.

Best Wishes

Phillip

Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Rubén Justo]

On 07-feb-2023 10:52:39, Phillip Wood wrote:
> Hi Rubén
> 
> On 06/02/2023 23:16, Rubén Justo wrote:
> > On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > > Let's add a test to notice if this changes in the future.
> > > 
> > > I for one would find this series much easier to follow if you started
> > > with this test, possibly with a test_expect_failure, and as we fix the
> > > relevant code flip them (both this and the subsequent one) to run
> > > successfully, and include them as part of the commit that fixes the
> > > bug).
> > > 
> > > Maybe there's reasons for why that's tricky to do in this case, so
> > > please ignore this if so.
> > 
> > I'll give it try, I like the idea.  Thanks.
> 
> Squashing the last three commits together so that the tests are introduced
> in the same commit as the fix as Junio suggested in his comments on the
> previous round would be very welcome.

Yes, I considered that, but I think that keeping the tests in their own
commit is reasonable.  The tests protect against a malfunction that we
did not notice when the implementation was done.  The commits reference
(and use similar subjects) to the original commit where we should have
introduced the tests.  If the tests fail, I think it will be easier and
less confusing to reach the original commit where the implementation was
done if we keep them separated, rather than combining all three commits.

I'm going to reorder the commits and change to use test_expect_failure().
This way the commit with the fix will also be linked.

Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> ...  If the tests fail, I think it will be easier and
> less confusing to reach the original commit where the implementation was
> done if we keep them separated, rather than combining all three commits.

No, if the test were in the same commit as the implementation, then
upon a future test breakage it is easier to see what code the test
was meant to protect, exactly because it is part of the same commit.

> I'm going to reorder the commits and change to use test_expect_failure().

Do not do that.  Adding a failing test and flipping the
expect_failure to expect_success is a sure way to make the series
harder to review.

Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Rubén Justo]

On 07-feb-2023 21:19:33, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > ...  If the tests fail, I think it will be easier and
> > less confusing to reach the original commit where the implementation was
> > done if we keep them separated, rather than combining all three commits.
> 
> No, if the test were in the same commit as the implementation, then
> upon a future test breakage it is easier to see what code the test
> was meant to protect, exactly because it is part of the same commit.

Yes, but my reasoning was that those tests protect what we did in 8d9fdd7
(worktree.c: check whether branch is rebased in another worktree, 2016-04-22)
and in b5cabb4a9 (rebase: refuse to switch to branch already checked out
elsewhere, 2020-02-23), not die_if_checked_out().

> > I'm going to reorder the commits and change to use test_expect_failure().
> 
> Do not do that.  Adding a failing test and flipping the
> expect_failure to expect_success is a sure way to make the series
> harder to review.

Hmm

[PATCH v3 4/4] switch: reject if the branch is already checked out elsewhere (test)

[Rubén Justo]

Since 5883034 (checkout: reject if the branch is already checked out
elsewhere) in normal use, we do not allow multiple worktrees having the
same checked out branch.

A bug has recently been fixed that caused this to not work as expected.

Let's add a test to notice if this changes in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 5a7caf958c..7bea95dba2 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
 	test_cmp_config "" --default "" branch.main2.merge
 '
 
+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+		git checkout -
+	" &&
+	git checkout -b shared &&
+	test_commit shared-first &&
+	HASH1=$(git rev-parse --verify HEAD) &&
+	test_commit shared-second &&
+	test_commit shared-third &&
+	HASH2=$(git rev-parse --verify HEAD) &&
+	git worktree add wt1 -f shared &&
+	git -C wt1 bisect start &&
+	git -C wt1 bisect good $HASH1 &&
+	git -C wt1 bisect bad $HASH2 &&
+	git worktree add wt2 -f shared &&
+	git -C wt2 bisect start &&
+	git -C wt2 bisect good $HASH1 &&
+	git -C wt2 bisect bad $HASH2 &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_must_fail git -C wt1 switch shared &&
+	git -C wt1 switch --ignore-other-worktrees shared &&
+	test_must_fail git -C wt2 switch shared &&
+	git -C wt2 switch --ignore-other-worktrees shared
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v3 4/4] switch: reject if the branch is already checked out elsewhere (test)

[Eric Sunshine]

On Sat, Feb 4, 2023 at 6:33 PM Rubén Justo <rjusto@gmail.com> wrote:
> Since 5883034 (checkout: reject if the branch is already checked out
> elsewhere) in normal use, we do not allow multiple worktrees having the
> same checked out branch.
>
> A bug has recently been fixed that caused this to not work as expected.
>
> Let's add a test to notice if this changes in the future.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
> @@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
> +test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
> +       test_when_finished "
> +               git worktree remove wt1 &&
> +               git worktree remove wt2 &&
> +               git branch -d shared
> +               git checkout -
> +       " &&
> +       git checkout -b shared &&

A few comments...

The test_when_finished() call has an odd mix of &&-chain and missing &&-chain.

If you do use &&-chaining inside test_when_finished(), you have to be
careful that _none_ of the cleanup commands fail, otherwise
test_when_finished() itself will fail, making the entire test fail,
_even if_ the test body succeeded. So, &&-chaining the commands in
this test_when_finished() is undesirable since any of the `git
worktree remove` and `git branch -d` commands could potentially fail.
Better would be to drop the &&-chain and ensure that the final command
succeeds.

It may be a good idea to use `||:` at the end of each command as
documentation for readers that any of the cleanup commands might fail,
which could happen if something in the body of the test fails.

The `git branch -d shared` cleanup command fails unconditionally
because it's still the checked-out branch in the directory. You need
to swap the `git branch -d shared` and `git checkout -` commands.

Taking all the above points into account, a possible rewrite would be:

  test_when_finished "
    git worktree remove wt1 ||:
    git worktree remove wt2 ||:
    git checkout - ||:
    git branch -d shared ||:
  " &&
  git checkout -b shared &&

> +       test_commit shared-first &&
> +       HASH1=$(git rev-parse --verify HEAD) &&
> +       test_commit shared-second &&
> +       test_commit shared-third &&
> +       HASH2=$(git rev-parse --verify HEAD) &&
> +       git worktree add wt1 -f shared &&
> +       git -C wt1 bisect start &&
> +       git -C wt1 bisect good $HASH1 &&
> +       git -C wt1 bisect bad $HASH2 &&
> +       git worktree add wt2 -f shared &&
> +       git -C wt2 bisect start &&
> +       git -C wt2 bisect good $HASH1 &&
> +       git -C wt2 bisect bad $HASH2 &&
> +       # we test in both worktrees to ensure that works
> +       # as expected with "first" and "next" worktrees
> +       test_must_fail git -C wt1 switch shared &&
> +       git -C wt1 switch --ignore-other-worktrees shared &&
> +       test_must_fail git -C wt2 switch shared &&
> +       git -C wt2 switch --ignore-other-worktrees shared
> +'

Re: [PATCH v3 4/4] switch: reject if the branch is already checked out elsewhere (test)

[Rubén Justo]

On 14-feb-2023 23:17:42, Eric Sunshine wrote:

> > +test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
> > +       test_when_finished "
> > +               git worktree remove wt1 &&
> > +               git worktree remove wt2 &&
> > +               git branch -d shared
> > +               git checkout -
> > +       " &&
> > +       git checkout -b shared &&
> 
> The test_when_finished() call has an odd mix of &&-chain and missing &&-chain.

Oops, thanks, that was unintentional and hides an error ...

> 
> If you do use &&-chaining inside test_when_finished(), you have to be
> careful that _none_ of the cleanup commands fail, otherwise
> test_when_finished() itself will fail, making the entire test fail,
> _even if_ the test body succeeded. So, &&-chaining the commands in
> this test_when_finished() is undesirable since any of the `git
> worktree remove` and `git branch -d` commands could potentially fail.
> Better would be to drop the &&-chain and ensure that the final command
> succeeds.
> 
> It may be a good idea to use `||:` at the end of each command as
> documentation for readers that any of the cleanup commands might fail,
> which could happen if something in the body of the test fails.
> 
> The `git branch -d shared` cleanup command fails unconditionally
> because it's still the checked-out branch in the directory. You need
> to swap the `git branch -d shared` and `git checkout -` commands.

...  in fact, that needs to be "git branch -D shared" to successfully
delete that unmerged branch.  Considering also, what you pointed out,
swapping the two commands.

About the `||:`, maybe it's a good idea to keep the '&&' and let the
test fail if any command fails in the cleanup.  However, if that is also
part of the test, maybe must not be in the cleanup... so I don't have a
strong opinion on that.

I'll wait some time, if there is no argument against the change, I'll
reroll with the fix and using '||:' as you suggested.

Thank you for your review and comments.

[PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

Since v3, two errors in 4/4 have been fixed:
  - "checkout -" must be done before "branch -d"
  - "branch -D" instead of "branch -d" because "shared" is a non-merged
    branch.

Also, '||:' has been used to chain commands in the test_when_finished()
block.

Rubén Justo (4):
  worktree: introduce is_shared_symref()
  branch: fix die_if_checked_out() when ignore_current_worktree
  rebase: refuse to switch to a branch already checked out elsewhere (test)
  switch: reject if the branch is already checked out elsewhere (test)

 branch.c          | 14 +++++++----
 t/t2060-switch.sh | 29 ++++++++++++++++++++++
 t/t3400-rebase.sh | 14 +++++++++++
 worktree.c        | 63 +++++++++++++++++++++++------------------------
 worktree.h        |  6 +++++
 5 files changed, 89 insertions(+), 37 deletions(-)

Range-diff against v3:
1:  40b5ea54d3 = 1:  22d0944aad worktree: introduce is_shared_symref()
2:  2f1479b354 = 2:  e7ba7b6fdd branch: fix die_if_checked_out() when ignore_current_worktree
3:  81a5b619c3 = 3:  8de8319d0e rebase: refuse to switch to a branch already checked out elsewhere (test)
4:  6559e58344 ! 4:  27d6ae2755 switch: reject if the branch is already checked out elsewhere (test)
    @@ t/t2060-switch.sh: test_expect_success 'tracking info copied with autoSetupMerge
      
     +test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
     +	test_when_finished "
    -+		git worktree remove wt1 &&
    -+		git worktree remove wt2 &&
    -+		git branch -d shared
    -+		git checkout -
    ++		git worktree remove wt1 ||:
    ++		git worktree remove wt2 ||:
    ++		git checkout - ||:
    ++		git branch -D shared ||:
     +	" &&
     +	git checkout -b shared &&
     +	test_commit shared-first &&
-- 
2.39.2

[PATCH v4 1/4] worktree: introduce is_shared_symref()

[Rubén Justo]

Add a new function, is_shared_symref(), which contains the heart of
find_shared_symref().  Refactor find_shared_symref() to use the new
function is_shared_symref().

Soon, we will use is_shared_symref() to search for symref beyond
the first worktree that matches.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 worktree.c | 62 +++++++++++++++++++++++++++---------------------------
 worktree.h |  6 ++++++
 2 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/worktree.c b/worktree.c
index aa43c64119..40cb9874b7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -403,44 +403,44 @@ int is_worktree_being_bisected(const struct worktree *wt,
  * bisect). New commands that do similar things should update this
  * function as well.
  */
-const struct worktree *find_shared_symref(struct worktree **worktrees,
-					  const char *symref,
-					  const char *target)
+int is_shared_symref(const struct worktree *wt, const char *symref,
+		     const char *target)
 {
-	const struct worktree *existing = NULL;
-	int i = 0;
+	const char *symref_target;
+	struct ref_store *refs;
+	int flags;
 
-	for (i = 0; worktrees[i]; i++) {
-		struct worktree *wt = worktrees[i];
-		const char *symref_target;
-		struct ref_store *refs;
-		int flags;
+	if (wt->is_bare)
+		return 0;
 
-		if (wt->is_bare)
-			continue;
+	if (wt->is_detached && !strcmp(symref, "HEAD")) {
+		if (is_worktree_being_rebased(wt, target))
+			return 1;
+		if (is_worktree_being_bisected(wt, target))
+			return 1;
+	}
 
-		if (wt->is_detached && !strcmp(symref, "HEAD")) {
-			if (is_worktree_being_rebased(wt, target)) {
-				existing = wt;
-				break;
-			}
-			if (is_worktree_being_bisected(wt, target)) {
-				existing = wt;
-				break;
-			}
-		}
+	refs = get_worktree_ref_store(wt);
+	symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
+						NULL, &flags);
+	if ((flags & REF_ISSYMREF) &&
+	    symref_target && !strcmp(symref_target, target))
+		return 1;
 
-		refs = get_worktree_ref_store(wt);
-		symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
-							NULL, &flags);
-		if ((flags & REF_ISSYMREF) &&
-		    symref_target && !strcmp(symref_target, target)) {
-			existing = wt;
-			break;
-		}
+	return 0;
+}
+
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+					  const char *symref,
+					  const char *target)
+{
+
+	for (int i = 0; worktrees[i]; i++) {
+		if (is_shared_symref(worktrees[i], symref, target))
+			return worktrees[i];
 	}
 
-	return existing;
+	return NULL;
 }
 
 int submodule_uses_worktrees(const char *path)
diff --git a/worktree.h b/worktree.h
index 9dcea6fc8c..7889c4761d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *symref,
 					  const char *target);
 
+/*
+ * Returns true if a symref points to a ref in a worktree.
+ */
+int is_shared_symref(const struct worktree *wt,
+		     const char *symref, const char *target);
+
 /*
  * Similar to head_ref() for all HEADs _except_ one from the current
  * worktree, which is covered by head_ref().
-- 
2.39.2

[PATCH v4 2/4] branch: fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

In 8d9fdd7 (worktree.c: check whether branch is rebased in another
worktree, 2016-04-22) die_if_checked_out() learned a new option
ignore_current_worktree, to modify the operation from "die() if the
branch is checked out in any worktree" to "die() if the branch is
checked out in any worktree other than the current one".

Unfortunately we implemented it by checking the flag is_current in the
worktree that find_shared_symref() returns.

When the same branch is checked out in several worktrees simultaneously,
find_shared_symref() will return the first matching worktree in the list
composed by get_worktrees().  If one of the worktrees with the checked
out branch is the current worktree, find_shared_symref() may or may not
return it, depending on the order in the list.

Instead of find_shared_symref(), let's do the search using use the
recently introduced API is_shared_symref(), and consider
ignore_current_worktree when necessary.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c   | 14 +++++++++-----
 worktree.c |  3 +--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index e5614b53b3..64b7dbfd17 100644
--- a/branch.c
+++ b/branch.c
@@ -820,12 +820,16 @@ void remove_branch_state(struct repository *r, int verbose)
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
 {
 	struct worktree **worktrees = get_worktrees();
-	const struct worktree *wt;
 
-	wt = find_shared_symref(worktrees, "HEAD", branch);
-	if (wt && (!ignore_current_worktree || !wt->is_current)) {
-		skip_prefix(branch, "refs/heads/", &branch);
-		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+	for (int i = 0; worktrees[i]; i++) {
+		if (worktrees[i]->is_current && ignore_current_worktree)
+			continue;
+
+		if (is_shared_symref(worktrees[i], "HEAD", branch)) {
+			skip_prefix(branch, "refs/heads/", &branch);
+			die(_("'%s' is already checked out at '%s'"),
+				branch, worktrees[i]->path);
+		}
 	}
 
 	free_worktrees(worktrees);
diff --git a/worktree.c b/worktree.c
index 40cb9874b7..34043d8fe0 100644
--- a/worktree.c
+++ b/worktree.c
@@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
 					  const char *target)
 {
 
-	for (int i = 0; worktrees[i]; i++) {
+	for (int i = 0; worktrees[i]; i++)
 		if (is_shared_symref(worktrees[i], symref, target))
 			return worktrees[i];
-	}
 
 	return NULL;
 }
-- 
2.39.2

[PATCH v4 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

[Rubén Justo]

In b5cabb4a9 (rebase: refuse to switch to branch already checked out
elsewhere, 2020-02-23) we add a condition to prevent a rebase operation
involving a switch to a branch that is already checked out in another
worktree.

A bug has recently been fixed that caused this to not work as expected.

Let's add a test to notice if this changes in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3400-rebase.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d5a8ee39fc..3ce918fdb8 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '
 	git rebase main main
 '
 
+test_expect_success 'switch to branch checked out elsewhere fails' '
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+	" &&
+	git worktree add wt1 -b shared &&
+	git worktree add wt2 -f shared &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_must_fail git -C wt1 rebase shared shared &&
+	test_must_fail git -C wt2 rebase shared shared
+'
+
 test_expect_success 'switch to branch not checked out' '
 	git checkout main &&
 	git branch other &&
-- 
2.39.2

[PATCH v4 4/4] switch: reject if the branch is already checked out elsewhere (test)

[Rubén Justo]

Since 5883034 (checkout: reject if the branch is already checked out
elsewhere) in normal use, we do not allow multiple worktrees having the
same checked out branch.

A bug has recently been fixed that caused this to not work as expected.

Let's add a test to notice if this changes in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 5a7caf958c..e247a4735b 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
 	test_cmp_config "" --default "" branch.main2.merge
 '
 
+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
+	test_when_finished "
+		git worktree remove wt1 ||:
+		git worktree remove wt2 ||:
+		git checkout - ||:
+		git branch -D shared ||:
+	" &&
+	git checkout -b shared &&
+	test_commit shared-first &&
+	HASH1=$(git rev-parse --verify HEAD) &&
+	test_commit shared-second &&
+	test_commit shared-third &&
+	HASH2=$(git rev-parse --verify HEAD) &&
+	git worktree add wt1 -f shared &&
+	git -C wt1 bisect start &&
+	git -C wt1 bisect good $HASH1 &&
+	git -C wt1 bisect bad $HASH2 &&
+	git worktree add wt2 -f shared &&
+	git -C wt2 bisect start &&
+	git -C wt2 bisect good $HASH1 &&
+	git -C wt2 bisect bad $HASH2 &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_must_fail git -C wt1 switch shared &&
+	git -C wt1 switch --ignore-other-worktrees shared &&
+	test_must_fail git -C wt2 switch shared &&
+	git -C wt2 switch --ignore-other-worktrees shared
+'
+
 test_done
-- 
2.39.2

Re: [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>      +	test_when_finished "
>     -+		git worktree remove wt1 &&
>     -+		git worktree remove wt2 &&
>     -+		git branch -d shared
>     -+		git checkout -
>     ++		git worktree remove wt1 ||:
>     ++		git worktree remove wt2 ||:
>     ++		git checkout - ||:
>     ++		git branch -D shared ||:
>      +	" &&

Sorry, but I do not get the point of this construct.  The
test_cleanup variable that accumulates test_when_finished scripts is
evaled without -e shopt set, so you can just remove all these ||:
and add a single "true" at the end, like

	...
	git checkout -
	git branch -D shared
	:
    " &&

for exactly the same effect, no?

Re: [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree

[Rubén Justo]

On 25-feb-2023 14:50:07, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >      +	test_when_finished "
> >     -+		git worktree remove wt1 &&
> >     -+		git worktree remove wt2 &&
> >     -+		git branch -d shared
> >     -+		git checkout -
> >     ++		git worktree remove wt1 ||:
> >     ++		git worktree remove wt2 ||:
> >     ++		git checkout - ||:
> >     ++		git branch -D shared ||:
> >      +	" &&
> 
> Sorry, but I do not get the point of this construct.  The
> test_cleanup variable that accumulates test_when_finished scripts is
> evaled without -e shopt set, so you can just remove all these ||:
> and add a single "true" at the end, like
> 
> 	...
> 	git checkout -
> 	git branch -D shared
> 	:
>     " &&
> 
> for exactly the same effect, no?

Yes, of course.

I agree that all of that '||:' can be confusing, but I'm not sure if a
final ':' is much better.

As I said in my response to Eric, I don't have a clear opinion here.

test_when_finished() was introduced in 2bf78867 (test-lib: Let tests
specify commands to be run at end of test, 2010-05-02) to allow
indicating some actions to be executed after the test block finishes,
even if the test fails, to leave a repository or overall situation
healthy for the next tests.

With that in mind, I ask myself, is it worth removing the worktrees
here?  Is it worth reporting a possible error in that removal?  If, for
example, in the future we deny the removal of a worktree under bisect,
is it worth reporting the failure here?  What about the "shared"
branch? ...

I Dunno.

Thank you for reviewing this patch.