Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree
On Wed, Jul 15, 2015 at 1:48 PM, Eric Sunshine wrote: >> - 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 I'm aware of it. I just didn't remember it when I wrote this code. > 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? And if I remember correctly Mike Haggerty had a series to refactor ref parsing code and reuse in this place (it was my promise to do it, but he took over). I think the series was halted because refs.c was going through a major update at that time. I think we could leave it as is for now and completely replace it at some point in future. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree
On Mon, Jul 13, 2015 at 2:36 PM, Junio C Hamano wrote:
> Eric Sunshine 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree
On Tue, Jul 14, 2015 at 5:53 AM, Michael J Gruber wrote: >> Eric Sunshine 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. > Related to that, I'm interested in "worktree list", and I'm wondering > how many more worktree commands we foresee[...] The ones I and others came up with (beyond 'add' and 'prune') are 'list', 'remove', 'mv', and 'lock' (for locking and unlocking). I specifically added a BUGS section to the documentation[1] as a reminder. [1]: http://article.gmane.org/gmane.comp.version-control.git/273431 > , 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? for_each_worktree() might be overkill at this time, as I think only 'prune' and 'list' would benefit directly. 'remove', 'lock', and 'mv' probably just want to lookup a particular worktree (with 'mv', when renaming, also possibly looking up the destination worktree to check if it already exist). > - 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? I had considered working on some of the commands as time permits, but don't currently have concrete plans to do so. You're welcome to jump in and tackle these ideas (but perhaps let us know, so toes don't get trod upon). > [1] Something like: > > * fooworktree (master) > barworktree (HEAD detached from deadbeef) > > with "*" denoting the worktree you're in (if any) and (optionally?) Considering that "git worktree list" can be invoked from the main worktree or any linked worktree, it would be a good idea to include the main worktree in the output as well. > adding the "on" info from "git branch" in parentheses after each > worktree (checked out branch name, or detached info). Maybe the path, too? I also had something very much like this in mind, possibly extending the output either with --verbose or custom format capability (keeping the GSoC project in mind), but otherwise hadn't put much thought into it. Showing the path seems quite important, so I'd say yes to that question, probably by default. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree
On Tue, Jul 14, 2015 at 4:53 PM, Michael J Gruber wrote: > 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? fhe for_each_xxx sounds like a good pattern to reuse. > - Is a fixed output format for "list"[1] enough, or do we want something > like for-each-ref or log formats (GSOC project...)? We could stick to a fixed format for "worktree list" then introduce --format later if one output does not please everybody. > - Finally: Who will be stepping on whose toes doing this? I'm happy to step aside and let people implement it. I still have plenty of hanging topics to finish up :( -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree
Junio C Hamano venit, vidit, dixit 13.07.2015 20:36: > Eric Sunshine 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 >> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree
Eric Sunshine 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 > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
