Add support for worktrees #1607

mminns · 2018-09-11T19:30:30Z

See #1471 for more details.

Adds wrappers and tests for the current worktree implementation in libgit2.

This is my first PR for libgit2, please pull apart and point out anything that should be done differently 👍

mminns · 2018-09-11T21:23:25Z

Damn, did I forget to commit some test resources? It all runs fine locally :) I will check.

ethomson · 2018-09-13T15:02:37Z

Thanks @mminns! Looks like that might be the case, but I'm eager to have worktree support. :D

mminns · 2018-09-17T11:01:41Z

There were some files making up part of the test repository that were being gitignored, thought I'd added them all in now, but it looks like not quite.... back to the drawing board...

mminns · 2018-09-18T19:51:36Z

Ah despite all the red, some progress. AFAICS the tests

Pass on Windows/AppVeyor. <- But I need to look at the leak stuff
Fail on travis <- But I think this might be a path \ vs / issue in the worktree gitdirs 🤞
Fail on azure <- not sure why but I'm hoping its the same as travis 🤞

Although maybe I should have checked the LEAKING before opening the PR :)

bcmedeiros · 2018-09-27T04:00:24Z

Hey, guys!

I'm currently working on a solution that would highly benefit from this feature, if you need some help, please, let me know!

mminns · 2018-09-27T21:30:20Z

I think I finally sorted out the LEAK stuff.... fingers crossed 🤞

mminns · 2018-09-28T06:32:37Z

1 step forward

Pass on Windows/AppVeyor Leaks fixed <- \need to check the publish stage
Fail on travis <- But I think this might be a path \ vs / issue in the worktree gitdirs 🤞
Fail on azure <- not sure why but I'm hoping its the same as travis 🤞

mminns · 2018-09-28T07:28:40Z

While I fix up the remaining build, if anyone has any comments about the way I've exposed the functionality let me know. I took the submodules work as a starting point and it seems to work OK, but I'm open to any suggestions.

mminns · 2018-09-28T09:05:25Z

Ah I left the LEAK constants defined in the csproj, rookie error.

mminns · 2018-09-28T10:14:23Z

Yes! 1 green, 2 to go. 👍

mminns · 2018-09-28T10:43:51Z

Hi

Ok, I seem to have fixed up the tests and the leaks, but I am at a loss about the Azure Build errors

2018-09-28T10:36:32.3866704Z ##[error]TF400813: The user '4180bb58-415f-47d4-955f-53f0887f795e' is not authorized to access this resource.

The error seems to be re-produced in other PRs

bording · 2018-09-28T14:34:13Z

@mminns Don't worry about the Azure failures. I don't believe @ethomson has that working correctly yet.

mminns · 2018-09-28T17:25:13Z

In which case please let let rip on the code review itself ... 😰

👍

dusan-tkac · 2018-10-01T06:16:12Z

Hi @mminns! Thank you for working on this feature!
I have a use case where I find the current interface lacking:
If working folder of not-locked worktree is deleted, there is no way to prune that worktree.
The Prune method only accepts Worktree instance as a parameter, but you cannot get that instance, because repo.Worktrees["worktreeName"] throws RepositoryNotFoundException.
Maybe there should be an overload for the Prune method that takes a worktree name?

Alternatively, when getting a worktree reference and the worktree repository is not valid, it should not throw an exception. The worktree repository should simply be invalid.

dusan-tkac · 2018-10-01T07:26:43Z

@mminns ; there is also a problem I see in the last version (after the leaks fix) - I'm getting an AccessViolationException

at LibGit2Sharp.Core.NativeMethods.git_worktree_lookup(git_worktree*& reference, git_repository* repo, String name)
at LibGit2Sharp.Core.Proxy.git_worktree_lookup(RepositoryHandle repo, String name)
at LibGit2Sharp.Worktree.GetWorktreeHandle()
at LibGit2Sharp.Worktree.get_WorktreeRepository()

I have compiled your version for .Net Framework 4.7.2 and I'm debugging this in Visual Studio 2015 so it might be just something specific to this combination.
Previous version ([26db5b9]) worked fine.

mminns · 2018-10-01T12:52:34Z

@dusan-tkac thanks for the feedback, I've not seen the AccessViolation but let me know if you find anything more, and yes that is a good usecase catch when deleting the worktree outside of libgit, I'll have a think

dusan-tkac · 2018-10-02T02:32:48Z

I've not seen the AccessViolation but let me know if you find anything more

@mminns , the problem was on my side. I was accessing worktree.WorktreeRepository outside of the "using" block of the worktree parent repository.

mminns · 2018-10-08T20:52:01Z

Not forgotten about this, planning to look into the pruning a deleted worktree issue asap, once the day job calms down.

mminns · 2018-10-23T19:43:14Z

@dusan-tkac I've corrected the pruning, just need to rebase again....

…t in libgit2

…y fragile. In real life they will use absolute paths, but luckily they do work with relative paths, but they have a habit of getting re-written as soon as you use Git to inspect the repository

…cross-platform compatible

…ixed test data to correctly reference the worktree's working directories

mminns · 2018-10-23T21:31:15Z

so that rebase broke all the things....

mminns · 2018-11-03T20:04:12Z

OK, some progress again, tests passing again, 1 memory leak to fix.

mminns · 2018-11-06T20:33:41Z

OK, I think that is as green as the builds will get. 👍

FWIW this PR does not include the ability to specify the target branch etc when opening a Worktree as that option, libgit2/libgit2@a22f19e, wasn't available in the native libgit2 when I started writing this. Personally I do need that functionality so I intend to add it in a follow up PR, but I'd rather try and get this merged first if possible. Although I am open to suggestions on that order of event. 😄

bcmedeiros · 2018-11-07T08:27:21Z

Although I am open to suggestions on that order of event.

I'd say we should release this PR as it is if everyone is happy with that, then we put another one for the improvement.

mminns · 2018-11-17T14:39:38Z

Hi @ethomson what happens next, if anything, with this?

danielang · 2018-11-26T13:36:03Z

Hi @ all,
first of all: Thank you @mminns for your great work! This PR works nicely in our current development.

When this PR will be merged and released in the nuget package?

ethomson · 2018-11-28T20:39:35Z

Sorry for the delay in reviewing this @mminns - this is a great PR. Thanks so much for tackling this! I have just a few questions. 😀

ethomson

My only questions are around the csproj changes - I'm not sure if they're intentional or if Visual Studio just added them itself (it does that sometimes). If they are intentional, could you help me understand what they do? And if they're not can we roll them back? Otherwise, this is an amazing pull request.

ethomson · 2018-11-28T20:40:05Z

LibGit2Sharp/LibGit2Sharp.csproj

@@ -34,6 +41,10 @@
    <PackageReference Include="Nerdbank.GitVersioning" Version="2.2.13" PrivateAssets="all" />
  </ItemGroup>

+  <ItemGroup>
+    <Service Include="{508349b6-6b84-4df5-91f0-309beebad82d}" />
+  </ItemGroup>


I'm not sure that I understand what this is...?

ethomson · 2018-11-28T20:40:38Z

LibGit2Sharp/LibGit2Sharp.csproj

+    <Compile Update="Core\Handles\Objects.cs" DependentUpon="Objects.tt">
+      <DesignTime>True</DesignTime>
+      <AutoGen>True</AutoGen>
+    </Compile>


I'm not sure why this changed - is it necessary?

LibGit2Sharp/LibGit2Sharp.csproj

mminns · 2018-11-30T21:21:20Z

Thanks for getting back to me, and good questions. I fairly sure they can be reverted but I'll clarify in the next few days.

LibGit2Sharp/LibGit2Sharp.csproj

mminns · 2018-12-20T11:34:38Z

Hi, I know everyone is busy, but just checking in to see what the state of play is with this @ethomson ?

Enjoy the holidays everyone.

ethomson · 2018-12-25T05:09:55Z

Cool, thanks for adding this @mminns!

mminns force-pushed the mminns/issue-1471-worktrees branch 2 times, most recently from 6a24e12 to 05e0283 Compare September 17, 2018 10:54

mminns added 8 commits October 23, 2018 20:54

Issue-1471 Add worktree support, based on the current level of suppor…

c4c15a5

…t in libgit2

Issue-1471 Added functionality to apply or remove locks.

5ee02bd

Issue-1471 Added functionality to add and prune worktrees

aba7687

Issue-1471 The paths in the dot_git\worktree\repo\gitdir file are ver…

e87a9fb

…y fragile. In real life they will use absolute paths, but luckily they do work with relative paths, but they have a habit of getting re-written as soon as you use Git to inspect the repository

Issue-1471 Fix handle leaks

60d4e67

Issue-1471 Undo setting of LEAK constants

049d313

Issue-1471 ensure the directory stuctures used in worktree tests are …

e642bf9

…cross-platform compatible

Issue-1471 Added the ability to prune already deleted repositories. F…

bd98667

…ixed test data to correctly reference the worktree's working directories

mminns force-pushed the mminns/issue-1471-worktrees branch from 82e42a0 to bd98667 Compare October 23, 2018 20:06

Issue-1471 Corrected tests.

0b69d4d

mminns added 2 commits November 3, 2018 19:23

Trigger CI

8874842

Issue 1471 add .git files into the repository

016fc95

mminns added 2 commits November 6, 2018 19:59

Issue-1471 Fix memory leak in test

9d17daa

Issue-1471 Added code comments to WorktreeLock

222d4c4

ethomson reviewed Nov 28, 2018

View reviewed changes

Issue-1471 undo unneeded csproj changes

7e7e84a

bording requested changes Dec 3, 2018

View reviewed changes

LibGit2Sharp/LibGit2Sharp.csproj Outdated Show resolved Hide resolved

ssue-1471 Removed unneeded build constants

87517d9

bording approved these changes Dec 3, 2018

View reviewed changes

ethomson merged commit 0dcaefc into libgit2:master Dec 25, 2018

This was referenced Sep 4, 2019

Worktree in libgit2sharp #1471

Closed

Fails within a git 'worktree' dotnet/Nerdbank.GitVersioning#152

Closed

Add support for worktrees #1607

Add support for worktrees #1607

Uh oh!

Conversation

mminns commented Sep 11, 2018

Uh oh!

mminns commented Sep 11, 2018

Uh oh!

ethomson commented Sep 13, 2018

Uh oh!

mminns commented Sep 17, 2018

Uh oh!

mminns commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcmedeiros commented Sep 27, 2018

Uh oh!

mminns commented Sep 27, 2018

Uh oh!

mminns commented Sep 28, 2018

Uh oh!

mminns commented Sep 28, 2018

Uh oh!

mminns commented Sep 28, 2018

Uh oh!

mminns commented Sep 28, 2018

Uh oh!

mminns commented Sep 28, 2018

Uh oh!

bording commented Sep 28, 2018

Uh oh!

mminns commented Sep 28, 2018

Uh oh!

dusan-tkac commented Oct 1, 2018

Uh oh!

dusan-tkac commented Oct 1, 2018

Uh oh!

mminns commented Oct 1, 2018

Uh oh!

dusan-tkac commented Oct 2, 2018

Uh oh!

mminns commented Oct 8, 2018

Uh oh!

mminns commented Oct 23, 2018

Uh oh!

mminns commented Oct 23, 2018

Uh oh!

mminns commented Nov 3, 2018

Uh oh!

mminns commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcmedeiros commented Nov 7, 2018

Uh oh!

mminns commented Nov 17, 2018

Uh oh!

danielang commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented Nov 28, 2018

Uh oh!

ethomson left a comment

Choose a reason for hiding this comment

Uh oh!

ethomson Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

mminns Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

ethomson Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

mminns Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mminns commented Nov 30, 2018

Uh oh!

Uh oh!

mminns commented Sep 18, 2018 •

edited

Loading

mminns commented Nov 6, 2018 •

edited

Loading

danielang commented Nov 26, 2018 •

edited

Loading