From: Phillip Wood <phillip.wood123@gmail.com>
To: 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>,
"Rubén Justo" <rjusto@gmail.com>,
"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
Date: Fri, 27 Jan 2023 14:46:45 +0000 [thread overview]
Message-ID: <a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk> (raw)
In-Reply-To: <CAPUEspjuXSncRxo5DMj1pA5zcYvn4Y6epdijYL6HJRGhk_6q7g@mail.gmail.com>
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. For "git
checkout -B <branch> <start-point>" when <branch> is checked out in
another worktree requiring --ignore-other-worktrees makes sense.
> % git checkout --ignore-other-worktrees -B main
> Already on 'main'
>
> The change of behaviour only applies to -B and it actually matches the
> documentation better.
>
>>> @@ -1533,13 +1534,13 @@ 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 (!opts->ignore_other_worktrees && !opts->force_detach &&
>>> + check_branch_path && ref_exists(check_branch_path)) {
>>
>> I think check_branch_path is NULL if opts->ignore_other_worktrees is set
>> so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly
>> below where you set check_branch_path).
>
> opts->ignore_other_worktrees was kept from the original expression;
> you are correct that is not needed anymore, but I thought it didn't
> hurt and made the code intention clearer (meaning it is obvious to
> anyone new to the code that this code will be skipped if that flag is
> set), would using an assert or a comment be a better option?
It's a good point that it makes the intention clearer, maybe we should
just leave it as it is.
>>> /*
>>> * Extract branch name from command line arguments, so
>>> * all that is left is pathspecs.
>>> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>>> new_branch_info, opts, &rev);
>>> argv += n;
>>> argc -= n;
>>> +
>>> + if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
>>> + check_branch_path = xstrdup(new_branch_info->path);
>>
>> I'm a bit confused what this is doing.
>
> The branch we are interested in might come from 2 places, either it is
> a parameter to -b, which was picked up before, or it is the argument
> to the command itself, which was detected above.
Oh, of course. I was so focused on the -b that I'd forgotten we need the
same check when we're checking out an existing branch - thanks for
putting me right.
> If both are provided, we want to make sure to use the one from -b, or
> will have the bug you sharply spotted before, which was frankly
> embarrassing.
>
>>> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
>>> index d587e0b20d..7ab7e87440 100755
>>> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
>>> test_must_fail git -C here checkout newmain
>>> '
>>>
>>> -test_expect_success 'not die the same branch is already checked out' '
>>> +test_expect_success 'allow creating multiple worktrees for same branch with force' '
>>> + git worktree add --force anothernewmain newmain
>>> +'
>>> +
>>> +test_expect_success 'allow checkout/reset from the conflicted branch' '
>>
>> I'm not sure what "the conflicted branch" means (it reminds we of merge
>> conflicts).
>
> by "conflicted" I meant one that is checked out in more than one worktree
I think it would be clearer so say that rather than "conflicted" which
has a strong association with merge conflicts.
Best Wishes
Phillip
next prev parent reply other threads:[~2023-01-27 14:46 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
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 [this message]
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=a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk \
--to=phillip.wood123@gmail.com \
--cc=carenas@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
--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.
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).