On Mon, Jul 13, 2015 at 2:36 PM, Junio C Hamano <[email protected]> wrote:
> Eric Sunshine <[email protected]> writes:
>> This series eliminates git-checkout from the picture by instead
>> employing "git reset --hard"[2] to populate the new worktree initially.
>
> 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?
The placement of the comment in the original code wasn't bad, but
after patch 3/16 moves code around, the comment does become somewhat
confusing, so moving it to the callee seems a reasonable idea.
> - 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.
Hmm, I wasn't aware of SYMLINK_HEAD (and don't know if Duy was). The
related code in resolve_ref_unsafe() is fairly involved, worrying
about race conditions and such, however, I guess
check_linked_checkout()'s implementation can perhaps be simpler, as
it's probably far less catastrophic for it to give the wrong answer
(or just die) under such a race?
> - 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.
After sending the series, I was realized that this could be done more
cleanly with -C, but that would have to be repeated for each command,
so cp.env[] might indeed be a better choice.
> 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).
I'll see if the commit messages can be reworded a bit without becoming
too wordy. ("git reset --hard" has a nice conciseness.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
