From: Eric Sunshine <sunshine@sunshineco.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, vustthat@gmail.com, pclouds@gmail.com
Subject: Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees
Date: Mon, 16 Jan 2023 17:18:04 -0500 [thread overview]
Message-ID: <CAPig+cR3=wDFJPr8ViUTVFDx-AvaJUGWNUnqndJ2edQPL5smVw@mail.gmail.com> (raw)
In-Reply-To: <20230116172824.93218-1-carenas@gmail.com>
On Mon, Jan 16, 2023 at 12:30 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> builtin/checkout: check the branch used in -B with worktrees
Thanks for working on this and coming up with a fix. As mentioned
earlier, I had started looking into it[1], but lacked the time to
disentangle the logic, so I'm glad to see a patch arrive so quickly.
> When multiple worktrees are being used, checkout/switch check
> that the target branch is not already checked out with code that
> evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased
> in another worktree, 2016-04-22), but that logic wasn't valid for
> -B/-C
>
> Avoid reusing the same `branch_info` structure for the checks and
> assumming that it will be rejected later if is a new branch that
> already exists as that doesn't apply to -B/-C.
Even though I'm familiar with the bug report[2] which sparked this
patch, I find the above description somewhat hard to digest; the
high-level problem it is addressing doesn't jump off the page at me.
Perhaps it can be rewritten something like this:
checkout/switch: disallow checking out same branch in multiple worktrees
Commands `git switch -C` and `git checkout -B` neglect to check
whether the branch being forcibly created is already checked out
in some other worktree, which can result in the undesirable
situation of the same branch being checked out in multiple
worktrees. For instance:
$ 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.
after which you might include some details which you wrote about initially.
> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
> - 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);
This variable name change (`new_branch_info` => `check_branch_info`)
helps make the code clearer. Good. (I had found it more than a little
confusing to have similar named variables `new_branch_info` and
`opts->new_branch` even though they are unrelated and have very
different purposes.)
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '
> +test_expect_success 'die the same branch is already checked out (checkout -B)' '
> + (
> + cd here &&
> + test_must_fail git checkout -B newmain
> + )
> +'
Although `git switch` and `git checkout` currently share
implementation, that might not always be the case going forward. As
such, this test could be made a bit more robust by testing both
commands rather than just `git-checkout`. So, perhaps:
test_expect_success 'die the same branch is already checked out' '
(
cd here &&
test_must_fail git checkout -B newmain &&
test_must_fail git switch -C newmain
)
'
> +test_expect_success 'not die on re-checking out current branch (checkout -B)' '
> + (
> + cd there &&
> + git checkout -B newmain
> + )
> +'
Good to see you considered this case too. (I had tested it myself
manually when trying out your patch.)
[1]: https://lore.kernel.org/git/CAPig+cQc1+D9gH7BAC-r03bGKWx3a9jpPyLuP-ehH-X2P+fV6Q@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAA3Q-aaO=vcZd9VLFr8UP-g06be80eUWN_GjygfyGkYmrLx9yQ@mail.gmail.com/
next prev parent reply other threads:[~2023-01-16 22:18 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 [this message]
2023-01-17 0:53 ` Rubén Justo
2023-01-18 5:44 ` Carlo Arenas
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cR3=wDFJPr8ViUTVFDx-AvaJUGWNUnqndJ2edQPL5smVw@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=carenas@gmail.com \
--cc=git@vger.kernel.org \
--cc=pclouds@gmail.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.
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).