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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 12, 2015, 03:10 UTC
Message-ID: <CAPig+cT-1juGLZK+F1Cy+MDE8+_bPqnX6fgfpoMnd2Qypu3oZQ@mail.gmail.com>
In-Reply-To: <CACsJy8A8CeyeYZnMpbdWW-etohp3TWzGrro1khzD=8skydm6Cg@mail.gmail.com>

On Sat, Jul 11, 2015 at 10:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 30 quoted lines

> On Sun, Jul 12, 2015 at 9:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Jul 11, 2015 at 9:20 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sat, Jul 11, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> The plan is eventually to populate the new worktree via "git reset
>>>> --hard" rather than "git checkout". Thus, rather than piggybacking on
>>>> git-checkout's -b/-B ability to create a new branch at checkout time,
>>>> git-worktree will need to do so itself.
>>>
>>> I don't know much about ref handling code (especially after the big
>>> transaction update), so i may be wrong, but do we need to invalidate
>>> some caches in refs.c after this? The same for update-ref in the other
>>> patch. We may need to re-read the index after 'reset --hard' too if we
>>> ever need to do touch the index after that (unlikely though in the
>>> case of 'worktree add')
>>
>> I'm not sure I understand. Are you talking about this patch's
>> implementation or a possible future change which uses the C API rather
>> than git-branch?
>>
>> If you're talking about this patch, then I don't think we need to do
>> anything more, as the "git branch" and "git reset --hard" invocations
>> are separate process invocations which shouldn't affect the current
>> worktree or the current "git worktree add" process.
>
> 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.)