libgit2 / libgit2sharp

Git + .NET = ❤
http://libgit2.github.com
MIT License
3.21k stars 889 forks source link

Worktree in libgit2sharp #1471

Closed schadin closed 5 years ago

schadin commented 7 years ago

Hello! You planning support worktree feature in lib?

limaolm commented 7 years ago

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

pks-t commented 7 years ago

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 commented 6 years ago

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

mminns commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Thanks, made total sense.

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

mminns commented 6 years ago

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

ethomson commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

See https://github.com/libgit2/libgit2/pull/4524 for the new option of using an already existing reference

mminns commented 6 years ago

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 https://github.com/libgit2/libgit2/pull/4524

Any thoughts?

mminns commented 6 years ago

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

ttrunck commented 6 years ago

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

ethomson commented 6 years ago

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 commented 6 years ago

(oops meant to post this a few days ago)

AFAIK a version of libgit2 supporting https://github.com/libgit2/libgit2/pull/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 commented 5 years ago

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

ethomson commented 5 years ago

Thanks @herebebeasties