git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: git@vger.kernel.org, vustthat@gmail.com, sunshine@sunshineco.com,
	pclouds@gmail.com
Subject: Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees
Date: Tue, 17 Jan 2023 21:44:16 -0800	[thread overview]
Message-ID: <CAPUEsphrbPVZtZi_GV9=8OqOqjk+SZ1JJf1bU_HWpUnT1H6YzA@mail.gmail.com> (raw)
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:
> 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.

  reply	other threads:[~2023-01-18  5:44 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-16 17:28 [PATCH] builtin/checkout: check the branch used in -B with worktrees Carlo Marcelo Arenas Belón
2023-01-16 22:18 ` Eric Sunshine
2023-01-17  0:53 ` Rubén Justo
2023-01-18  5:44   ` Carlo Arenas [this message]
2023-01-18  6:15 ` [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees Carlo Marcelo Arenas Belón
2023-01-18  6:52   ` Junio C Hamano
2023-01-18  7:58     ` Carlo Arenas
2023-01-18 16:10       ` Junio C Hamano
2023-01-18 22:55   ` Junio C Hamano
2023-01-19  5:53   ` [PATCH v3] " Carlo Marcelo Arenas Belón
2023-01-19  7:23     ` Re* " Junio C Hamano
2023-01-19  7:41       ` Carlo Arenas
2023-01-19 14:21     ` Phillip Wood
2023-01-20  3:10     ` Junio C Hamano
2023-01-20  3:53       ` Carlo Arenas
2023-01-20  4:39         ` Carlo Arenas
2023-01-20 11:35     ` [PATCH v4] " Carlo Marcelo Arenas Belón
2023-01-20 15:08       ` Phillip Wood
2023-01-20 22:12         ` Carlo Arenas
2023-01-27 14:46           ` Phillip Wood
2023-05-14 20:21             ` Rubén Justo
2023-03-23  0:06         ` Junio C Hamano
2023-03-24  3:49           ` Carlo Arenas
2023-05-14 20:24       ` Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPUEsphrbPVZtZi_GV9=8OqOqjk+SZ1JJf1bU_HWpUnT1H6YzA@mail.gmail.com' \
    --to=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=rjusto@gmail.com \
    --cc=sunshine@sunshineco.com \
    --cc=vustthat@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).