Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Worktree in libgit2sharp #1471

Closed
schadin opened this issue Jul 14, 2017 · 25 comments
Closed

Worktree in libgit2sharp #1471

schadin opened this issue Jul 14, 2017 · 25 comments

Comments

@schadin
Copy link

schadin commented Jul 14, 2017

Hello!
You planning support worktree feature in lib?

@limaolm
Copy link

limaolm commented Jul 20, 2017

Git-Worktree is a very convenient feature. If libgit2sharp would support it, it would be great!

@pks-t
Copy link
Member

pks-t commented Jul 21, 2017

Opening and working with worktrees like with normal repositories has been implemented with v0.25.0 of libgit2. So with the upcoming release of libgit2sharp v0.25, this will be included. What has not yet been implemented are bindings around the functions to implement creation and modification of the worktrees themselves, but this should not be hard to do.

@mminns
Copy link
Contributor

mminns commented Dec 19, 2017

Famous last words... but since I'd like to use this feature I will have a go at implementing it.

@mminns
Copy link
Contributor

mminns commented Jan 8, 2018

Hi
I've started working on this, see https://github.com/itofinity/libgit2sharp/tree/mminns/issue-1471-worktrees still plenty to do, but so far I have I main question.

I'm basing my work on the submodule support. For submodules libgit2 provides a selection of helper methods such as git_submodule_path(SubmoduleHandle) which take the opaque object as a parameter and return simple objects, e.g. strings.

There aren't any such similar methods for worktrees, so for now I've created an explicit struct definition for git_worktree https://github.com/itofinity/libgit2sharp/blob/mminns/issue-1471-worktrees/LibGit2Sharp/Core/GitWorktree.cs in my libgit2sharp branch to allow me to access its properties directly, e.g. https://github.com/itofinity/libgit2sharp/blob/mminns/issue-1471-worktrees/LibGit2Sharp/Core/Proxy.cs#L3454.

This seems to work OK, but I wondered if anyone knew the reason for having these helper methods or not in libgit2 and whether I should be looking at getting a change into libgit2 to add them?

@ethomson
Copy link
Member

ethomson commented Jan 8, 2018

Do not add a git_worktree struct. libgit2 uses opaque structures for objects (like git_worktree) and does not expose the members. This is intentional, because it allows the objects to change without breaking the API. If you rely on the layout of a structure in libgit2, it will break in future versions of libgit2.

You should use libgit2's accessor functions (eg git_worktree_is_locked).

If you think you need additional functions for working with working trees, then you should make sure that those functions are added in libgit2 first. For questions about using libgit2's working trees, either ask on StackOverflow or in slack.

@mminns
Copy link
Contributor

mminns commented Jan 9, 2018

Thanks for clarifying

Actually I think I found the method I missed previously that would allow me to work with the opaque git_worktree

GIT_EXTERN(int) git_repository_open_from_worktree(git_repository **out, git_worktree *wt);

@mminns
Copy link
Contributor

mminns commented Jan 11, 2018

Can I ask where should I ask conceptual/architectural questions?

For example libgit2 provides mechanisms to get back a list of worktree names, which can then be used to find worktree structs which can then be used to open a repository instance of the worktree.

Should an implementation in libgit2sharp simply expose the relevant functionality and, for example, have a Repository provide a list of worktree names and then require users to manually open a Repository view of a worktree by using a name. Or should the parent Repository hide this complexity and provide a list of worktree Repository instances rather than just names?

Hope that makes sense :)

@ethomson
Copy link
Member

Can I ask where should I ask conceptual/architectural questions?

Here or slack probably make the most sense.

Or should the parent Repository hide this complexity and provide a list of worktree Repository instances rather than just names?

Good question. This is a balance between performance and usability. I think that in a perfect world, we would return an enumerable of Repository objects. However, that's probably going to be unusably slow. So I think that we could either return names (and have a Repository method that can instantiate a Repository from a worktree name) or return an enumerable of Worktree objects that contain the name and a Repository property that is lazily instantiated. That way users can use something typed and easily get the Repository. This might be the best of both worlds or the worst, I'm not sure. 😀

(Disclaimer: I'm not super familiar with worktrees, so if you're reading this thinking that you don't understand what I'm saying then I may be lacking some necessary domain knowledge.)

@mminns
Copy link
Contributor

mminns commented Jan 11, 2018

Thanks, made total sense.

I'll play with the alternatives and see which seems to work the best.

@mminns
Copy link
Contributor

mminns commented Feb 7, 2018

Quick hello just say I am continuing to work on this, just delayed due to the UKs flu season.... urgggghhhh

@ethomson
Copy link
Member

ethomson commented Feb 7, 2018

Quick hello just say I am continuing to work on this, just delayed due to the UKs flue season.... urgggghhhh

Oh god, I hear ya. 🤧🤒😷 Hope you're healthy soon. And always feel free to drop by our slack if you want to chat about worktrees.

@mminns
Copy link
Contributor

mminns commented Feb 7, 2018

OK, I think I have a working solution, whether it is right or not remains to be seen.

It definitely needs some polish, probably some extra tests and the API is definitely open to discussion but I'd like to try and get some feedback before trying to polish. As such I have not created a PR, though I can if that is the preferred approach.

https://github.com/itofinity/libgit2sharp/tree/mminns/issue-1471-worktrees

Essentially the implementation looks like.....

To access existing worktrees

Worktree wt = repository.Worktrees["worktree_name"];

To check lock status

bool isLocked = wt.IsLocked;

To 'use' the worktree as a normal Repository

Repository wtRepository = wt.WorktreeRepository;

To lock/unlock a worktree

wt.Lock("reason");

wt.Unlock();

To Add a new worktree

repository.Worktree.Add("branch_name", "worktree_name", "c:\full\path\to\worktree", false /* islocked */);

To remove a worktree

repository.Worktree.Prune(wt);

It all seems to work correctly. The only functional oddity is that is seems the libgit2 git_worktree_add() function always creates a new branch using on the worktree name and based on the current branch of the parent repo. If you try and add a new worktree using the name of an existing branch it errors.

In order to do, what I would suggest, the 'normal' process of checking out a specific branch as a new worktree you have to do it kind of backwards, i,e. add a new worktree, and its associated new branch, then explicitly checkout the branch you really want.

Anyway I'll be very pleased to hear/see any feedback.

Cheers

@pks-t
Copy link
Member

pks-t commented Feb 8, 2018

I cannot comment on the C#-specific API, so I'll leave that to someone with experience in that.

I do agree that the current git_worktree_add is a bit lacking, though. Originally, I've modelled the API to be very similar to what git.git does. I realize the documentation never states that we do auto-create the branch, though, and we should definitly provide a way to change that default behaviour. I think we should be safe to just extend git_worktree_add_options by a new field git_reference *branch. In case that field is set, we will not create a new branch but re-use the given branch, provided it was not yet checked anywhere.

Any further thoughts on this?

@mminns
Copy link
Contributor

mminns commented Feb 8, 2018

Hi, thanks for the feedback.

Overall, yes I think it would be great to have the ability to add a worktree based on an existing commitish, rather than having the 2 steps.

FWIW is there a reason to use the options rather than just add a new parameter? Tell me if I'm wrong but the options seem more appropriate to the --blah style options in Git rather parameters like ther commitish.

How feasible is it just to change the behaviour of the 'name' parameter? Looking at the worktree functionality there is no name parameter as such for add, https://git-scm.com/docs/git-worktree, in practice it is just the last part of the path. In ligit2 the name appears to be used to create a branch of that name could it not check if that branch already exists and if it does and it is not already checked out as the working copy or as a worktree then checkout that branch as the new worktree?

@pks-t
Copy link
Member

pks-t commented Feb 8, 2018

Yes, there is a reason: keeping the current API intact. We cannot just add parameters to already released APIs, as that would break downstream users of libgit2. The same, or even worse, holds for changing behavior of existing parameters.

name is not only used for the branch, but it is also used as the real "name" for the worktree itself. So if you execute git_worktree_add with name="foobar", you will have a new worktree "foobar" at ".git/worktrees/foobar/". But yes, the second meaning of "name" is also to determine which branch to create/use for the new worktree.

@mminns
Copy link
Contributor

mminns commented Feb 8, 2018

I assumed that was the case, but thought it worth asking :)

Although extending the behaviour of name to support existing branch names seems, possibly, less problematic.

However if that isn't possible, then the options approach would work.

As another alternative. What about rather than specifying the name of an existing branch in the options, the options has a flag that, if set, extends the behaviour of name to support existing branches?

@pks-t
Copy link
Member

pks-t commented Feb 9, 2018

We could probably do so. I think upstream git is currently moving into the same direction with the path, but if I'm not mistaken it didn't yet release that code.

I'll come up with the reference option to support using a specific reference instead of creating a new branch. I guess I'll leave the change for name to somebody else (could be you, if you're interested enough), though.

@pks-t
Copy link
Member

pks-t commented Feb 9, 2018

See libgit2/libgit2#4524 for the new option of using an already existing reference

@mminns
Copy link
Contributor

mminns commented Mar 6, 2018

Hi I'm wonder where it is best to go with this. Complete my current implementation,

  1. relying on switching branches within libgit2sharp
  2. do nothing, just expose libgit2 functionality and rely on the 3rd party dev to mange branch switching
  3. wait for worktree: add ability to create worktree with pre-existing branch libgit2#4524

Any thoughts?

@mminns
Copy link
Contributor

mminns commented Apr 22, 2018

Hi @ethomson I see libgit2/libgit2#4524 was merged which is great. AFAICS that code hasn't made it over here yet? Very roughly speaking when is that likely?

@ttrunck
Copy link

ttrunck commented Jul 20, 2018

Is there any news here? Is there a version before libgit2/libgit2#4524 merged in libgit2sharp?
Thanks

@ethomson
Copy link
Member

The version of libgit2 that's included in LibGit2Sharp does support worktrees, but LibGit2Sharp has not added support for it. That's work that needs to be done by a volunteer.

@mminns
Copy link
Contributor

mminns commented Sep 17, 2018

(oops meant to post this a few days ago)

AFAIK a version of libgit2 supporting libgit2/libgit2#4524 is not available to libgit2sharp yet? I'd be very happy to be corrected on that.

With that in mind I've resurrected my original branch adding support for worktrees based on the currently available libgit2 implementation and opened a PR.

@herebebeasties
Copy link

@ethomson Think you missed closing this issue when you landed #1607 which I presume fixed it, back on Christmas day last year. Thanks.

@ethomson
Copy link
Member

ethomson commented Sep 4, 2019

Thanks @herebebeasties

@ethomson ethomson closed this as completed Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants