Junio C Hamano venit, vidit, dixit 13.07.2015 20:36: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > >> 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. > Related to that, I'm interested in "worktree list", and I'm wondering how many more worktree commands we foresee, and therefore how much refactoring should be done: Currently, the parsing of the contents of .../worktrees/ into worktree information is done right in prune-spcefic functions. They will have to be refactored. The following questions come to my mind: - Is a simple funtion refactoring enough, or should we introduce a worktree struct (and a list of such)? - Should each command do its own looping, or do we want for_each_worktree() with a callback? - Is a fixed output format for "list"[1] enough, or do we want something like for-each-ref or log formats (GSOC project...)? - Finally: Who will be stepping on whose toes doing this? Michael [1] Something like: * fooworktree (master) barworktree (HEAD detached from deadbeef) with "*" denoting the worktree you're in (if any) and (optionally?) adding the "on" info from "git branch" in parentheses after each worktree (checked out branch name, or detached info). Maybe the path, too? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html