git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] worktree: teach find_shared_symref to ignore current worktree
@ 2023-01-17  0:36 Rubén Justo
  2023-01-17 23:27 ` Junio C Hamano
  2023-01-22  1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
  0 siblings, 2 replies; 15+ messages in thread
From: Rubén Justo @ 2023-01-17  0:36 UTC (permalink / raw)
  To: Git List; +Cc: Eric Sunshine

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree
  2023-01-17  0:36 [PATCH] worktree: teach find_shared_symref to ignore current worktree Rubén Justo
@ 2023-01-17 23:27 ` Junio C Hamano
  2023-01-18 23:50   ` Rubén Justo
  2023-01-22  1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
  1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2023-01-17 23:27 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List, Eric Sunshine

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?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree
  2023-01-17 23:27 ` Junio C Hamano
@ 2023-01-18 23:50   ` Rubén Justo
  2023-01-19 10:48     ` Phillip Wood
  0 siblings, 1 reply; 15+ messages in thread
From: Rubén Justo @ 2023-01-18 23:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Eric Sunshine

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree
  2023-01-18 23:50   ` Rubén Justo
@ 2023-01-19 10:48     ` Phillip Wood
  2023-01-19 23:18       ` Rubén Justo
  0 siblings, 1 reply; 15+ messages in thread
From: Phillip Wood @ 2023-01-19 10:48 UTC (permalink / raw)
  To: Rubén Justo, Junio C Hamano; +Cc: Git List, Eric Sunshine

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree
  2023-01-19 10:48     ` Phillip Wood
@ 2023-01-19 23:18       ` Rubén Justo
  0 siblings, 0 replies; 15+ messages in thread
From: Rubén Justo @ 2023-01-19 23:18 UTC (permalink / raw)
  To: phillip.wood, Junio C Hamano; +Cc: Git List, Eric Sunshine

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. 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree
  2023-01-17  0:36 [PATCH] worktree: teach find_shared_symref to ignore current worktree Rubén Justo
  2023-01-17 23:27 ` Junio C Hamano
@ 2023-01-22  1:20 ` Rubén Justo
  2023-01-22  1:23   ` [PATCH v2 1/3] branch: " Rubén Justo
                     ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Rubén Justo @ 2023-01-22  1:20 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Phillip Wood

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-22  1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
@ 2023-01-22  1:23   ` Rubén Justo
  2023-01-22  1:50     ` Junio C Hamano
  2023-01-22  1:28   ` [PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test) Rubén Justo
  2023-01-22  1:28   ` [PATCH v2 3/3] switch: reject if the branch is " Rubén Justo
  2 siblings, 1 reply; 15+ messages in thread
From: Rubén Justo @ 2023-01-22  1:23 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Phillip Wood

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test)
  2023-01-22  1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
  2023-01-22  1:23   ` [PATCH v2 1/3] branch: " Rubén Justo
@ 2023-01-22  1:28   ` Rubén Justo
  2023-01-22  1:28   ` [PATCH v2 3/3] switch: reject if the branch is " Rubén Justo
  2 siblings, 0 replies; 15+ messages in thread
From: Rubén Justo @ 2023-01-22  1:28 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Phillip Wood

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] switch: reject if the branch is already checked out elsewhere (test)
  2023-01-22  1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
  2023-01-22  1:23   ` [PATCH v2 1/3] branch: " Rubén Justo
  2023-01-22  1:28   ` [PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test) Rubén Justo
@ 2023-01-22  1:28   ` Rubén Justo
  2 siblings, 0 replies; 15+ messages in thread
From: Rubén Justo @ 2023-01-22  1:28 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Phillip Wood

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-22  1:23   ` [PATCH v2 1/3] branch: " Rubén Justo
@ 2023-01-22  1:50     ` Junio C Hamano
  2023-01-22 11:51       ` Rubén Justo
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2023-01-22  1:50 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List, Phillip Wood

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().

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-22  1:50     ` Junio C Hamano
@ 2023-01-22 11:51       ` Rubén Justo
  2023-01-22 19:58         ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Rubén Justo @ 2023-01-22 11:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Phillip Wood

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-22 11:51       ` Rubén Justo
@ 2023-01-22 19:58         ` Junio C Hamano
  2023-01-22 23:21           ` Rubén Justo
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2023-01-22 19:58 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List, Phillip Wood

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-22 19:58         ` Junio C Hamano
@ 2023-01-22 23:21           ` Rubén Justo
  2023-01-24 10:35             ` Phillip Wood
  0 siblings, 1 reply; 15+ messages in thread
From: Rubén Justo @ 2023-01-22 23:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Phillip Wood

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-22 23:21           ` Rubén Justo
@ 2023-01-24 10:35             ` Phillip Wood
  2023-01-26  3:07               ` Rubén Justo
  0 siblings, 1 reply; 15+ messages in thread
From: Phillip Wood @ 2023-01-24 10:35 UTC (permalink / raw)
  To: Rubén Justo, Junio C Hamano; +Cc: Git List, 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
  2023-01-24 10:35             ` Phillip Wood
@ 2023-01-26  3:07               ` Rubén Justo
  0 siblings, 0 replies; 15+ messages in thread
From: Rubén Justo @ 2023-01-26  3:07 UTC (permalink / raw)
  To: phillip.wood, Junio C Hamano; +Cc: Git List

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-01-26  3:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-17  0:36 [PATCH] worktree: teach find_shared_symref to ignore current worktree Rubén Justo
2023-01-17 23:27 ` Junio C Hamano
2023-01-18 23:50   ` Rubén Justo
2023-01-19 10:48     ` Phillip Wood
2023-01-19 23:18       ` Rubén Justo
2023-01-22  1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
2023-01-22  1:23   ` [PATCH v2 1/3] branch: " Rubén Justo
2023-01-22  1:50     ` Junio C Hamano
2023-01-22 11:51       ` Rubén Justo
2023-01-22 19:58         ` Junio C Hamano
2023-01-22 23:21           ` Rubén Justo
2023-01-24 10:35             ` Phillip Wood
2023-01-26  3:07               ` Rubén Justo
2023-01-22  1:28   ` [PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test) Rubén Justo
2023-01-22  1:28   ` [PATCH v2 3/3] switch: reject if the branch is " Rubén Justo

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).