Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees

From: Carlo Arenas <carenas@gmail.com>
Date: Jan 18, 2023, 05:44 UTC
Message-ID: <CAPUEsphrbPVZtZi_GV9=8OqOqjk+SZ1JJf1bU_HWpUnT1H6YzA@mail.gmail.com>
In-Reply-To: <45cb1b38-1284-ddc9-a2d8-0a45f10abafb@gmail.com>

On Mon, Jan 16, 2023 at 4:53 PM Rubén Justo <rjusto@gmail.com> wrote:

Show 20 quoted lines

> On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:
>
> > @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
> >       if (!opts->can_switch_when_in_progress)
> >               die_if_some_operation_in_progress();
> >
> > -     if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
> > -         !opts->ignore_other_worktrees) {
> > +     if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {
> >               int flag;
> >               char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
> >               if (head_ref &&
> > -                 (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
> > -                     die_if_checked_out(new_branch_info->path, 1);
> > +                 (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> > +                     die_if_checked_out(check_branch_info->path, 1);
>
> What we are doing here, if I understand it correctly, is dying if the
> branch is _not checked out in the current worktree_ and _checked out in
> any other worktree_.  Which is OK for normal checkout/switch.

I think the exception was added to `checkout` only, where it is definitely needed, but switch probably should be more strict as it is not "plumbing" and as you pointed out there is already a UI option to override its safety.

> But IMHO with "checkout -B" we have to die if the branch is checked out
> in any other worktree, regardless of whether or not it is checked out in
> the current working tree.

I have to admit, I thought about that too, but then avoided making a change as checkout behaviour affects a lot of other places, but in retrospect I think that in this case it might be worth the change of behaviour, since it is connected with the bugfix.

Before, the operation was allowed and the logic never tried to prevent it (which is why in my first look, I thought it might have been intentional), but once Eric pointed out it was a bug, then the obvious conclusion would be to prevent it with the extended logic, as you pointed out.

Show 6 quoted lines

> Perhaps the scenario where the user has the same branch checked out in
> multiple worktrees and tries to reset in one of them, is one of those
> where we could use the "if it hurts, don't do it". But we are providing
> the "--ignore-other-worktrees" modifier, so I think we should disallow
> the "-B" if the branch is checked out in any other worktree, and let
> the user use "--ignore-other-worktrees" if he wants to go wild.

v2 includes this and AFAIK nothing is broken yet.

Carlo

PS. As explained before, tightening `switch` might be a good idea, but it is obviously punted for this change and will be addressed later.