From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 23 Mar 2023 00:06:52 GMT
Subject: Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
Message-ID: <xmqq355wbaar.fsf@gitster.g>
In-Reply-To: <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>

Phillip Wood <phillip.wood123@gmail.com> 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.