patchworktree: teach find_shared_symref to ignore current worktree

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

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

"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.

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?

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

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.

Hi Rubén

On 18/01/2023 23:50, Rubén Justo 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 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

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

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

OK.

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

Thank you. 

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

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

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

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.

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++) {

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?

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?

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++) {

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

On 21-ene-2023 17:50:55, Junio C Hamano wrote:

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

OK.  I'll split that in two.

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

Sorry.  I'll fix that.

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

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

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

I'll fix this.

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

Thank you for your review.

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

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.

OK.  Thanks.

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.

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

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.

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.

Hi Rubén

On 22/01/2023 23:21, Rubén Justo wrote:

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

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

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.

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

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

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:

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

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:

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

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

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

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.

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

OK.

OK. Thanks.

On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote:

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

On Tue, Feb 07 2023, Phillip Wood wrote:

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

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.

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

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

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

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.

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

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

Hi Rubén

On 06/02/2023 23:16, Rubén Justo wrote:

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

On 07-feb-2023 10:52:39, Phillip Wood wrote:

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.

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

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.

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.

On 07-feb-2023 21:19:33, Junio C Hamano wrote:

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

Hmm

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

On Sat, Feb 4, 2023 at 6:33 PM Rubén Justo <rjusto@gmail.com> wrote:

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 &&

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

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

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

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 &&

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

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

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

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

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

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?

On 25-feb-2023 14:50:07, Junio C Hamano wrote:

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.