On Wed, Mar 22, 2023 at 5:06 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
>
> > Hi Carlo
> > ...
> > 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"....
> > ...
> >> + 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).
> > ...
> >> ...
> >> + 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.
> > ...
> >> +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). Is this just testing that "checkout -b/B <branch>
> > <start-point>" works?
> > ...
> >> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '
> >
> > I think 'allow re-checking out ...' would be clearer, again I'm not
> > sure what's conflicted here.
> > ...
> >> -test_expect_success 'not die on re-checking out current branch' '
> >> +test_expect_failure 'unless using force without --ignore-other-worktrees' '
> >
> > This test passes for me - what's the reason for changing from
> > test_expect_success to test_expect_failure?
> >
> > Thanks for working on this
>
> are there remaining things
> to be done and issues to be resolved before we can see v5?
it interacted with another branch
(rj/avoid-switching-to-already-used-branch) that was just recently
merged to master.
it also was probably too aggressive as pointed[1] by Phillip after my
explanation of the change of behaviour as quoted:
>> 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.
I wasn't sure on how to proceed from there, but will come with a v5 to
discuss further.
Carlo
[1] https://lore.kernel.org/git/a848b7d5-fd40-b043-7ed9-1672f65312e6@xxxxxxxxxxxxx/
