Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 13, 2015, 18:36 UTC
Message-ID: <xmqqsi8rzyzo.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1436573146-3893-1-git-send-email-sunshine@sunshineco.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 13 quoted lines

> This is a follow-on series to [1], which migrated "git checkout --to"
> functionality to "git worktree add". That series continued using "git
> checkout" for the initial population of the new worktree, which required
> git-checkout to have too intimate knowledge that it was operating in a
> newly created worktree.
>
> This series eliminates git-checkout from the picture by instead
> employing "git reset --hard"[2] to populate the new worktree initially.
>
> It is built atop 1eb07d8 (worktree: add: auto-vivify new branch when
> <branch> is omitted, 2015-07-06), currently in 'next', which is
> es/worktree-add except for the final patch (which retires
> --ignore-other-worktrees) since the intention[3] was to drop that patch.

A few comments on things I noticed while reading (mostly coming from the original before this patch series):

 - What does this comment apply to?

        /*
         * $GIT_COMMON_DIR/HEAD is practically outside
         * $GIT_DIR so resolve_ref_unsafe() won't work (it
         * uses git_path). Parse the ref ourselves.
         */

   It appears in front of a call to check-linked-checkout, but I
   think the comment attempts to explain why it manually decides
   what the path should be in that function, so perhaps move it to
   the callee from the caller?

 - check_linked_checkout() when trying to decide what branch is
   checked out assumes HEAD is always a regular file, but I do not
   think we have dropped the support of SYMLINK_HEAD yet.  It needs
   to check st_mode and readlink(2), like resolve_ref_unsafe() does.

 - After a new skelton worktree is set up, the code runs a few
   commands to finish populating it, under a different pair of
   GIT_DIR/GIT_WORK_TREE, but the function does so with setenv(); it
   may be cleaner to use cp.env[] for it, as the process we care
   about using the updated environment is not "worktree add" command
   we are running ourselves, but "update-ref/symbolic-ref" and
   "reset" commands that run in the new worktree.

Other than that, looks nicely done.

I however have to wonder if the stress on "reset --hard" on log messages of various commits (and in the endgame) is somewhat misplaced.

The primary thing we wanted to see, which this series nicely brings us, is to remove "new-worktree-mode" hack from "checkout" (in other words, instead of "reset --hard", "checkout -f" would also have been a satisfactory endgame).

Thanks.