Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Jul 11, 2015 at 11:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jul 11, 2015 at 11:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Jul 11, 2015 at 10:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> The "shouldn't affect" is potentially a problem.If the current
>>> 'worktree add' process caches something (in ref handling, for example)
>>> that the 'git branch' process changes, then we may need to invalidate
>>> cache in 'worktree add' process after run_command(). I guess it's ok
>>> in this case since all we do is run_command(), we do not lookup refs
>>> or anything else afterwards.
>>
>> With this patch series applied, the code effectively does this:
>>
>>     branch = ...
>>     if (create_new_branch) {
>>         exec "git branch newbranch branch"
>>         branch = newbranch;
>>     }
>>     if (ref_exists(branch) && !detach)
>>         exec "git symbolic-ref HEAD branch"
>>     else
>>         exec "git update-ref HEAD $(git rev-parse branch)"
>>     exec "git reset --hard"
>>
>> So, if I understand your concern correctly, then you are worried that,
>> following the git-branch invocation, ref_exists() could return the
>> wrong answer with a pluggable ref-backend since it might be answering
>> based upon stale information. Is that what you mean? If so, I can see
>> how that it could be an issue. (As far as I can tell, the current
>> file-based backend doesn't have a problem with this since it's hitting
>> the filesystem directly to answer the ref_exists() question.)
>
> I meant for this final sentence to end like this:
>
>      ...to answer the ref_exists() question, but it still seems
>      fragile since some future change could introduce caching.

In this case, it's easy enough to side-step the issue since there's no
need to call ref_exists() if the new branch was created successfully
(since we know it exists). The logic would effectively become this:

    branch = ...
    if (create_new_branch) {
        exec "git branch newbranch branch"
        exec "git symbolic-ref HEAD newbranch"
    } else if (ref_exists(branch) && !detach)
        exec "git symbolic-ref HEAD branch"
    else
        exec "git update-ref HEAD $(git rev-parse branch)"
    exec "git reset --hard"