- Notifications
You must be signed in to change notification settings - Fork2.5k
worktree: mimic 'git worktree add' behavior.#5319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, isn't this supposed to be passed asgit_worktree_add_options'sref field ? As far as I can see this changes the behavior, as the old code would havenever reused a reference, instead returning an error. Now you cannever now if this actually happened, and you can end-up reusing by accident another unrelated reference.
src/worktree.c Outdated
| if ((err = git_commit_lookup(&commit, repo, &head->target.oid)) < 0) | ||
| goto out; | ||
| if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0) | ||
| git_buf ref_buf = GIT_BUF_INIT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This must begit_buf_disposed of — CI reports a leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I fixed that, thanks.
When user doesn't pass The only thing this commit is changing is when It's the same behavior when you do the following:
This currently doesn't work with libgit2 and that's what I'm trying to fix. Please let me know if I'm missing something. |
I have added a test |
src/worktree.c Outdated
| goto out; | ||
| if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0) | ||
| goto out; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm. I feel like you've got a point with thegit_branch_is_checked_out call, we're not doing that e.g. ifwtopts.ref is set. Let me propose the following flow:
if (wtopts.ref) { git_reference_dup(&ref, wtopts.ref);} else if (git_branch_lookup(&ref, name) == 0) { /* Use the ref */} else { git_branche_create(&ref);}if (git_branch_is_checked_out(ref)) { error out;}I think we ought to give the user a option to turn off re-use of branches, though. So maybe we should add a flag togit_worktree_add_options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we are already checking ifwopts.refs is checkouted at the beginning of thegit_worktree_add function with below code:
if (wtopts.ref) {if (!git_reference_is_branch(wtopts.ref)) {git_error_set(GIT_ERROR_WORKTREE, "reference is not a branch");err = -1;goto out;}if (git_branch_is_checked_out(wtopts.ref)) {git_error_set(GIT_ERROR_WORKTREE, "reference is already checked out");err = -1;goto out;}}So that should be fine.
Should we add aforce option as ingit worktree command ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, fair enough. Didn't read into the code and only saw this single instance ofif (wtopts.ref) in the diff. I'd prefer to have that check whether it's checked out in a single location as proposed by me to make it easier to extend, bu that's definitely not something you'll have to fix.
I'd like to not call thisforce, as this is a frequently overloaded name and doesn't really tell what's being forced. Something like "allow_reusing_refs" or "allow_existing_branch" would be better? I'm open for better proposals though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After thinking a bit more about it, theforce flag would make sense only if the branch passed aswtopts.ref or inferred fromname of the worktree is already checked out.
This pull request currently aborts if the branch is already checked out.
If the branch is not checked out, it will simply reuse it. So I guess that's fine from an user point of view.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This pull request currently aborts if the branch is already checked out.
Let's keep it this way for now. Can still add it later if requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for taking so long to review this and thanks for your changes!
src/worktree.c Outdated
| goto out; | ||
| if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0) | ||
| goto out; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This pull request currently aborts if the branch is already checked out.
Let's keep it this way for now. Can still add it later if requested.
Rebased branch to re-trigger CI. |
I've added another commit that hides this new feature behind an option to address@tiennou's remark. |
Thanks, this looks good to me. |
/rebuild |
Sorry@pks-t, an error occurred while trying to requeue the build. |
When adding a worktree using 'git worktree add <path>', if a referencenamed 'refs/heads/$(basename <path>)' already exist, it is checkouted inthe worktree.Mimic this behavior in libgit2.Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>
In this commit's parent, we have introduced logic that willautomatically re-use of existing branches if the new worktree namematches the branch name. While this is a handy feature, it changesbehaviour in a backwards-incompatible way and might thus surprise users.Furthermore, it's impossible to tell whether we have created theworktree with a new or an existing reference.To fix this, introduce a new option `checkout_existing` that togglesthis behaviour. Only if the flag is set will we now allow re-use ofexisting branches, while it's set to "off" by default.
I manually merged this and fixed up the conflicts. Thanks! |
When adding a worktree using
git worktree add <path>, if a referencenamed
refs/heads/$(basename <path>)already exist, it is checkouted inthe worktree.
Mimic this behavior in libgit2.