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
This 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.
