Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

From: "Rubén Justo" <rjusto@gmail.com>
To: phillip.wood@dunelm.org.uk, Carlo Arenas <carenas@gmail.com>
Cc: git@vger.kernel.org, pclouds@gmail.com, gitster@pobox.com,
	Jinwook Jeong <vustthat@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
Date: Sun, 14 May 2023 22:21:21 +0200	[thread overview]
Message-ID: <31ab5d7c-d310-1659-5d56-2fd341c44275@gmail.com> (raw)
In-Reply-To: <a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk>

On 27-ene-2023 14:46:45, Phillip Wood wrote:
> Hi Carlo
> 
> On 20/01/2023 22:12, Carlo Arenas wrote:
> > On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > > 
> > > On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:
> > > > Commands `git switch -C` and `git checkout -B` neglect to check whether
> > > > the provided branch is already checked out in some other worktree, as
> > > > shown by the following:
> > > > 
> > > >     $ git worktree list
> > > >     .../foo    beefb00f [main]
> > > >     $ git worktree add ../other
> > > >     Preparing worktree (new branch 'other')
> > > >     HEAD is now at beefb00f first
> > > >     $ cd ../other
> > > >     $ git switch -C main
> > > >     Switched to and reset branch 'main'
> > > >     $ git worktree list
> > > >     .../foo    beefb00f [main]
> > > >     .../other  beefb00f [main]
> > > > 
> > > > Fix this problem by teaching `git switch -C` and `git checkout -B` to
> > > > check whether the branch in question is already checked out elsewhere.
> > > > 
> > > > Unlike what it is done for `git switch` and `git checkout`, that have
> > > > an historical exception to ignore other worktrees if the branch to
> > > > check is the current one (as required when called as part of other
> > > > tools), the logic implemented is more strict and will require the user
> > > > to invoke the command with `--ignore-other-worktrees` to explicitly
> > > > indicate they want the risky behaviour.
> > > > 
> > > > This matches the current behaviour of `git branch -f` and is safer; for
> > > > more details see the tests in t2400.
> > > 
> > > As I said before, it would be much easier for everyone else to
> > > understand the changes if you wrote out what they were rather than
> > > saying "look at the tests". I do appreciate the improved test
> > > descriptions though - thanks for that.
> > 
> > Apologies on that, I tried to come up with something that would
> > describe the change of behaviour other than the paragraph above and
> > couldn't come out with a better explanation except reading the tests
> > (which I know is complicated by the fact they are interlinked).
> > 
> > The behaviour I am changing is also not documented (other than by the
> > test) and might have been added as a quirk to keep the scripted rebase
> > and bisect going when confronted with branches that were checked out
> > in multiple worktrees, and as such might had not be intended for
> > `switch`, and might not be needed anymore either.
> > 
> > Using`checkout` for simplicity, but also applies to `switch`,
> > 
> >    % git worktree list
> >    .../base  6a45aba [main]
> >    % git worktree add -f ../other main
> >    Preparing worktree (checking out 'main')
> >    HEAD is now at 6a45aba init
> >    % cd ../other
> >    % git checkout main
> >    Already on 'main'
> >    % git checkout -B main
> >    fatal: 'main' is already checked out at '.../base'
> 
> Thanks for explaining that. If there is no <start-point> given we don't
> reset the branch so it seems a bit harsh to error out here.

We say in the documentation:

   +
   If `-B` is given, `<new-branch>` is created if it doesn't exist; otherwise, it
   is reset. This is the transactional equivalent of
   +
   ------------
   $ git branch -f <branch> [<start-point>]
   $ git checkout <branch>
   ------------
   +

and, since 55c4a67307 (Prevent force-updating of the current branch,
2011-08-20), we return with error on "git branch -f <current-branch> [...]".

Therefore, when the current branch is checked out in multiple worktrees, it
seems consequent to error on "git checkout -B <current_branch> [...]".

But we want to allow "git checkout -B <current_branch>", without <start-point>,
as a way to say "git reset --keep", see: 39bd6f7261 (Allow checkout -B
<current-branch> to update the current branch, 2011-11-26).

Your suggestion not to error is not only reasonable, but also seems desirable.

Thanks.