Skip to content

Better Worktree Support #404

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

Closed
wants to merge 3 commits into from
Closed

Conversation

mkeeler
Copy link
Contributor

@mkeeler mkeeler commented Feb 25, 2019

I have a need to have support for worktrees in the library so I figured I would take a crack at implementing this.

The PR pulls in the latest upstream libgit2 release (required for some of the worktree support) and implements a bunch of worktree related functionality. I have some more polish to put on the PR but figured I would go ahead and submit the draft to get feedback before spending much more time on it. Please let me know if these changes would be acceptable and if so what a path to getting this merged should look like.

TODOs:

[ ] More thorough unit tests.
[ ] An example program.

@mkeeler
Copy link
Contributor Author

mkeeler commented Mar 1, 2019

@joshtriplett Any thoughts about whether this would be acceptable before I finish up with more testing and an example program?

@joshtriplett
Copy link
Member

This looks promising to me. I'd love to have better worktree support.

Ideally, I'd love to see a way to easily expand the existing tests to run in both worktree and non-worktree scenarios, because I fully expect to have weird corner-cases of "oops, that ran on the worktree/repository and not the repository/worktree".

@mkeeler
Copy link
Contributor Author

mkeeler commented Mar 1, 2019

Thats a good point. Up until recently ligbit2 didn't support worktrees on bare repositories. I will see what I can come up with as far as integrating more tests in both a worktree and non-worktree scenarios.

@joshtriplett
Copy link
Member

joshtriplett commented Mar 1, 2019

Bare vs non-bare is a good point as well: ideally, we should test normal, worktree-with-bare, and worktree-with-non-bare. And in that last case, we should have cross-checks to make sure that the repository's working copy is entirely untouched after each test that should apply to the worktree, and vice versa.

(Note that we don't need to do tests libgit2 already does; this isn't to test the C API, it's to test that our wrappers DTRT in every case.)

@sahithyen
Copy link

Is this still in progress? This would be a good feature with which I would like to optimize one of my projects.

@mkeeler
Copy link
Contributor Author

mkeeler commented Jul 22, 2019

@sahithyen Unfortunately I haven't had any time in the last few months to overhaul the testing aspects of it (although I would like to). The branch I have generally works but it needs some more polish before it could be merged.

Also, its been a while so I assume I will need to rebase as well.

Thinking about the testing some more I am not sure all the git2-rs tests need to run on both worktrees and non-worktree repositories. Whether we request libgit2 to open the repository normally or open the repository from a worktree what you end up with is libgit2 just handling that. Once you have the repository handle all operations should be the same and that is libgit2's responsibility.

With that in mind maybe a smaller amount smoke tests would be good to determine that operations on worktrees generally work while assuming libgit2 properly tests everything on their end.

@gjnoonan
Copy link

I know it's been a while, but I am currently working on some tooling that requires workspaces, so it would be nice to have this patch.

Is there going to be any movement on this? I don't mind picking up the slack if not.

@mkeeler
Copy link
Contributor Author

mkeeler commented Dec 24, 2019

@gjnoonan I still don’t have much spare time to work on this. Feel free to take it over. Last time I was working on this the main thing to figure out was a better way to handle tests of features both within a worktree and in a normal repo clone (and probably something with bare repositories as well). The code as is was functioning properly for me.

@denisoster
Copy link

It is a pity that the work is suspended

@mkeeler mkeeler force-pushed the worktrees branch 2 times, most recently from 57c9ecc to 9699478 Compare April 26, 2020 16:23
@mkeeler
Copy link
Contributor Author

mkeeler commented Apr 26, 2020

Trying to get back to this work now. I have rebased all of it against the latest master and next will be trying to tackle the testing challenges.

I am thinking about exploring macros to see if they could generate multiple tests for one block of code where a test repo is injected of various forms:

  • Typical - one worktree with .git dir in that directory
  • Bare - no worktree at all - just git info
  • Typical + Worktree - typical setup with an additional directory for a second worktree. Git info is still in the .git directory of the typical tree.
  • Bar + Worktree - bare repo with a worktree created in some other directory.

The systest changes were necessary due to the actual field name used in the C library being “ref” and not being able to name the field the same in the Rust struct because “ref” is a reserved keyword.
mkeeler added 2 commits April 27, 2020 09:20
The type it aliases, git_indexer_progress, is still checked but this prevents spurious warnings about using deprecated fields.
@joshtriplett
Copy link
Member

Now that #603 has been merged, is there any additional work in this PR that hasn't gone in at this point?

If not, thank you to @mkeeler for starting this effort!

@mkeeler
Copy link
Contributor Author

mkeeler commented Jan 25, 2021

Thanks all for taking this over and getting it merged. This can be closed.

@mkeeler mkeeler closed this Jan 25, 2021
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

Successfully merging this pull request may close these issues.

5 participants