| Message ID | b18a5710-2791-f892-8460-dda7ea41d88a@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/4] worktree: introduce is_shared_symref() | expand |
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 > +'
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.
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
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(+)