worktree: mimic 'git worktree add' behavior. #5319

herrerog · 2019-12-04T15:40:28Z

When adding a worktree using git worktree add <path>, if a reference
named refs/heads/$(basename <path>) already exist, it is checkouted in
the worktree.
Mimic this behavior in libgit2.

tiennou

Hmm, isn't this supposed to be passed as git_worktree_add_options's ref field ? As far as I can see this changes the behavior, as the old code would have never reused a reference, instead returning an error. Now you can never now if this actually happened, and you can end-up reusing by accident another unrelated reference.

tiennou · 2019-12-04T17:23:28Z

src/worktree.c

-		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;


This must be git_buf_disposed of — CI reports a leak.

I fixed that, thanks.

herrerog · 2019-12-04T18:34:25Z

Hmm, isn't this supposed to be passed as git_worktree_add_options's ref field ? As far as I can see this changes the behavior, as the old code would have never reused a reference, instead returning an error. Now you can never now if this actually happened, and you can end-up reusing by accident another unrelated reference.

ref field looks a bit different to me, it's when you want to checkout ref in the worktree.
libgit2 will checkout this ref only if it already exists and is not already checked out.

When user doesn't pass ref field, current version of git_worktree_add creates a new branch with the name of the worktree in the main working tree and checkout HEAD in the new worktree.

The only thing this commit is changing is when ref field is not passed as an option and branch refs/heads/<name> already exists in the main working tree.
It would have error out previously. But with this commit, it will now checkout the commit pointed by refs/heads/<name> in the new worktree without any error.

It's the same behavior when you do the following:
git worktree add ../libgit2-wt
Git create the new worktree in ../libgit2-wt and create a branch libgit2-wt in the main repositoriy.
rm -Rf ../libgit2-wt
git worktree prune
Here we deleted the libgit2-wt and pruned it but Git kept the libgit2-wt branch in the main repository.

git worktree add ../libgit2-wt
Adding the worktree again works whereas libgit2-wt already exists.

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.
Thanks for the review.

herrerog · 2019-12-05T20:48:09Z

I have added a test test_worktree_worktree__add_remove_add which add a worktree, delete it and add it back. This may help understanding what I'm trying to achieve.

pks-t · 2019-12-13T09:48:07Z

src/worktree.c

+				goto out;
+			if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0)
+				goto out;
+		}
 	}


Hm. I feel like you've got a point with the git_branch_is_checked_out call, we're not doing that e.g. if wtopts.ref is set. Let me propose 8000 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 to git_worktree_add_options?

I think we are already checking if wopts.refs is checkouted at the beginning of the git_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 a force option as in git worktree command ?

Ah, fair enough. Didn't read into the code and only saw this single instance of if (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 this force, 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!

After thinking a bit more about it, the force flag would make sense only if the branch passed as wtopts.ref or inferred from name 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.

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.

pks-t

Sorry for taking so long to review this and thanks for your changes!

pks-t · 2020-01-24T10:34:46Z

src/worktree.c

+				goto out;
+			if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0)
+				goto out;
+		}
 	}


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.

pks-t · 2020-01-24T10:38:58Z

Rebased branch to re-trigger CI.

pks-t · 2020-01-24T11:52:07Z

I've added another commit that hides this new feature behind an option to address @tiennou's remark.

herrerog · 2020-01-24T14:49:21Z

I've added another commit that hides this new feature behind an option to address @tiennou's remark.

Thanks, this looks good to me.

pks-t · 2020-01-31T14:23:39Z

/rebuild

libgit2-azure-pipelines · 2020-01-31T14:23:42Z

Sorry @pks-t, an error occurred while trying to requeue the build.

When adding a worktree using 'git worktree add <path>', if a reference named 'refs/heads/$(basename <path>)' already exist, it is checkouted in the 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 will automatically re-use of existing branches if the new worktree name matches the branch name. While this is a handy feature, it changes behaviour in a backwards-incompatible way and might thus surprise users. Furthermore, it's impossible to tell whether we have created the worktree with a new or an existing reference. To fix this, introduce a new option `checkout_existing` that toggles this behaviour. Only if the flag is set will we now allow re-use of existing branches, while it's set to "off" by default.

ethomson · 2024-03-05T09:49:18Z

I manually merged this and fixed up the conflicts. Thanks!

tiennou reviewed Dec 4, 2019

View reviewed changes

herrerog force-pushed the worktree branch from b41f376 to 3db9ad2 Compare December 4, 2019 18:15

herrerog force-pushed the worktree branch from 3db9ad2 to 23eee13 Compare December 5, 2019 20:46

pks-t reviewed Dec 13, 2019

View reviewed changes

pks-t previously approved these changes Jan 24, 2020

View reviewed changes

pks-t force-pushed the worktree branch from 23eee13 to a453f29 Compare January 24, 2020 10:38

pks-t force-pushed the worktree branch from a453f29 to 751af7a Compare January 24, 2020 11:23

pks-t force-pushed the worktree branch from 97ff8b9 to a381078 Compare January 31, 2020 09:25

pks-t force-pushed the worktree branch from a381078 to ea5028a Compare January 31, 2020 14:25

herrerog and others added 2 commits January 31, 2020 15:25

Base automatically changed from master to main January 7, 2021 10:09

ethomson dismissed pks-t’s stale review via ea5028a July 26, 2021 20:28

ethomson added the v1.8 label Feb 22, 2024

ethomson merged commit ea5028a into libgit2:main Mar 5, 2024

ethomson added feature git compatibility labels Mar 5, 2024

branchvincent mentioned this pull request Mar 21, 2024

libgit2 1.8.1 libgit2@1.7 1.7.2 (new formula) Homebrew/homebrew-core#166767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worktree: mimic 'git worktree add' behavior. #5319

worktree: mimic 'git worktree add' behavior. #5319

worktree: mimic 'git worktree add' behavior. #5319

worktree: mimic 'git worktree add' behavior. #5319

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment