On Mon, Jan 16, 2023 at 4:53 PM Rubén Justo <rjusto@xxxxxxxxx> wrote:
> 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.
> 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.
