Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 23, 2023, 00:06 UTC
- Message-ID
- <xmqq355wbaar.fsf@gitster.g>
- In-Reply-To
- <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 37 quoted lines
> 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 thisThis topic has been dormant for a full two months. Aside from comments by Phillip (quoted above), are there remaining things to be done and issues to be resolved before we can see v5?
Thanks.