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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 15, 2023, 04:17 UTC
Message-ID: <CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com>
In-Reply-To: <b18a5710-2791-f892-8460-dda7ea41d88a@gmail.com>

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

Show 20 quoted lines

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

Show 20 quoted lines

> +       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
> +'