Re: [PATCH v3 18/23] checkout: retire --to option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 7, 2015, 07:08 UTC
Message-ID: <CAPig+cSBp-U_jC3fcPXkZQ6kEPh7TRs2bAwKYQGGTtoGR3UYeg@mail.gmail.com>
In-Reply-To: <xmqqh9physyu.fsf@gitster.dls.corp.google.com>

On Mon, Jul 6, 2015 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> Now that "git worktree add" has achieved user-facing feature-parity with
>> "git checkout --to", retire the latter.
>> [...]
>> This effectively reverts changes to checkout.c by 529fef2 (checkout:
>> support checking out into a new working directory, 2014-11-30) with the
>> exception of merge_working_tree() and switch_branches() which still
>> require specialized knowledge that a the checkout is occurring in a
>> newly-created linked worktree (signaled to them by the private
>> GIT_CHECKOUT_NEW_WORKTREE environment variable).
>
> I do not quite understand why we still need the hidden environment
> variable.  Is this a sign that the implementation is shared too much
> between unrelated codepaths (or to put it another way, perhaps API
> functions that are not best fit are being used)?

Duy had responded[1][2] to my inquiry about these two remaining bits of checkout.c code with intimate knowledge of the initial linked-worktree checkout (merge_working_tree & switch_branches), but I don't know the checkout code well enough (yet) to fully understand his responses or to answer your question satisfactorily. This is also why I was afraid to rip out those final two bits of code, even though in my own testing, having ripped them out locally, I was unable to trigger any sort of "bad" behavior, as far as I could tell. Thus, I wasn't sure if those two bits of code were needed, and was hoping someone more qualified (Duy, for instance) would be able to provide a more authoritative answer.

Having now re-read and finally digested Duy's response about switch_branches(), when I rip out the `new_worktree_mode` check locally, I find that I can trigger the misleading warning about an orphaned commit, so that bit of code still serves a practical purpose. This doesn't justify that such ugly intimacy between git-worktree and git-checkout is desirable; only that it still seems to be needed until "git reset --hard" can be swapped in to replace "git checkout".

[1]: http://article.gmane.org/gmane.comp.version-control.git/273225 [2]: http://article.gmane.org/gmane.comp.version-control.git/273226

Show 16 quoted lines

> Stepping back a bit, with or without the new "linked worktree"
> feature, when you came across a repository whose working tree does
> not have any file (i.e. somebody ran "git ls-files | xargs rm"), you
> do not know and care what is in .git/index right now, you do not
> know and care what branch its .git/HEAD points at, but you *do* know
> what branch you want to be on (or where you want its HEAD detached
> at), what would be the command you would use?
>
> The state immediately after a new worktree is constructed by
> populating /path/main/.git/worktrees/test-next/ and pointing it from
> /path/other/test-next/.git but before the index or the files under
> /path/other/test-next/ are populated is exactly that situation, no?
> Wouldn't "symbolic-ref HEAD the-branch-i-want" (or "update-ref HEAD
> the-commit-i-want" in the detached case) followed by "reset --hard"
> the more natural thing to use, instead of merge-working-tree and
> switch-branches that are implementation details of "checkout"?

It seems so to me. I didn't repeat it in the v3 cover letter, but the v2 cover letter said this (which still holds true for v3):

    v2 does not attempt either of the suggestions by Junio[*3*] or
    Duy[*4*] for eliminating git-checkout from the equation, which
    would allow us to remove the final couple bits of code in
    git-checkout which require intimate knowledge that the checkout
    is occurring in a newly created linked worktree. This series is
    already too long, and I didn't want it to grow further by
    implementing either of those ideas. Instead, this series leaves
    git-worktree at a state where one or the other of those
    suggestions can be done as follow-on patches touching only the
    underlying machinery, without affecting the user-facing
    interface.

    [*3*]: via private email which suggested using "git-reset --hard"
           rather than "git checkout" to populate the new linked
           worktree.
    [*4*]: http://thread.gmane.org/gmane.comp.version-control.git/273032/focus=273226

In order to use "git reset --hard" in place of "git checkout", git-worktree will need to handle -b/-B, --detach, --force (and possibly --track and --orphan) itself. I'm still in the process of familiarizing myself with the code needed to perform those actions, as well as whatever else is needed to make "git reset --hard" a reality, so I am not yet in a position to say now much time or work will be required to swap out "git checkout" for "git reset --hard". As such, I didn't want to hold up the series for an unknown amount of time while studying the issue; and, as the series stands, the remaining ugly intimate knowledge between git-worktree and git-checkout is behind-the-scenes and localized (not affecting the user experience).