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

Upgrade to libgit2 ff8d635 #1049

Merged
merged 2 commits into from May 29, 2015
Merged

Upgrade to libgit2 ff8d635 #1049

merged 2 commits into from May 29, 2015

Conversation

nulltoken
Copy link
Member

This is an attempt to update to the latest libgit2 while dogfooding the new libgit2 binaries NuGet package.

I've downloaded the Appveyor artifact from libgit2/libgit2sharp.nativebinaries#4 in a local folder. I then reconfigured VS to use this folder as a local NuGet package source.

While going through this process I'm taking notes in order to further update libgit2/libgit2sharp.nativebinaries#5 and fix libgit2/libgit2sharp.nativebinaries#2.

/cc @bording @jamill

@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="LibGit2Sharp.NativeBinaries" version="1.0.27-pre20150515092631" targetFramework="net40" allowedVersions="[1.0.27-pre20150515092631]" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you update this to the 1.0.27 version? Manually? If you do that, then the corresponding targets stuff in the .csproj file doesn't get updated to point to the new package.

To make sure everything gets updated properly, you'll want to update the allowedVersions property first, then run a package update via nuget.

The allowedVersions has to be updated first, otherwise the package update won't be allowed to update to the newer version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allowedVersions has to be updated first, otherwise the package update won't be allowed to update to the newer version.

I've discovered this the hard way 😉 This is part of the things we should document as part of libgit2/libgit2sharp.nativebinaries#2

Thanks a lot for watching my back! 😍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, it took an embarrassingly long time to troubleshoot this. I kept on getting AccessViolationException on tests involving the IndexEntry. The native changes were quite straightforward. I investigated the memory, thinking I was getting padding issues. Which wasn't making any sense as everything was correctly aligned.

...Until I took a look at the libgit2 version being loaded...

And everything unfolded to me being an idiot 😜

@Therzok
Copy link
Member

Therzok commented May 15, 2015

@nulltoken
Copy link
Member Author

Current status:

1193 passed, 19 failed, 5 skipped (see 'Task List'), took 148,18 seconds (xUnit.net 1.9.2 build 1705).

@nulltoken
Copy link
Member Author

🎉

1196 passed, 16 failed, 5 skipped (see 'Task List'), took 142,42 seconds (xUnit.net 1.9.2 build 1705)

All the remaining failing tests are because of the thrown System.NotImplementedException in the RemoteUpdater.

I'm done for today with this. I'll work on tackling the remaining issues tomorrow.

@nulltoken
Copy link
Member Author

 1205 passed, 7 failed, 5 skipped (see 'Task List'), took 147,73 seconds (xUnit.net 1.9.2 build 1705).

The only remaining failing tests require a change to libgit2 so that git_remote_add_fetch() and git_remote_add_push() return GIT_EINVALIDSPEC when being passed a bogus refspec.

@carlosmn nicely agreed to make those changes. Stay tuned!

namespace LibGit2Sharp.Core
{
[StructLayout(LayoutKind.Sequential)]
internal class GitPushUpdate
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

@nulltoken
Copy link
Member Author

Depends on libgit2/libgit2#3127 and libgit2/libgit2#3134

@nulltoken
Copy link
Member Author

Current status:

Test 'LibGit2Sharp.Tests.FetchFixture.CanFetchCustomRefSpecsIntoAnEmptyRepository(url: "https://github.com/libgit2/TestGitRepository", localBranchName: "master", remoteBranchName: "master")' failed:
    Assert.DoesNotContain() failure: Found: refs/remotes/testRemote/master
    TestHelpers\ExpectedFetchState.cs(85,0): at LibGit2Sharp.Tests.TestHelpers.ExpectedFetchState.RemoteUpdateTipsHandler(String referenceName, ObjectId oldId, ObjectId newId)
    RemoteCallbacks.cs(173,0): at LibGit2Sharp.RemoteCallbacks.GitUpdateTipsHandler(IntPtr str, GitOid& oldId, GitOid& newId, IntPtr data)
    at LibGit2Sharp.Core.NativeMethods.git_remote_fetch(RemoteSafeHandle remote, GitStrArray& refspecs, GitFetchOptions fetch_opts, String log_message)
    Core\Proxy.cs(2177,0): at LibGit2Sharp.Core.Proxy.git_remote_fetch(RemoteSafeHandle remote, IEnumerable`1 refSpecs, GitFetchOptions fetchOptions, String logMessage)
    Network.cs(162,0): at LibGit2Sharp.Network.DoFetch(RepositorySafeHandle repoHandle, Remote remote, String url, FetchOptions options, String logMessage, IEnumerable`1 refspecs)
    Network.cs(252,0): at LibGit2Sharp.Network.Fetch(Remote remote, IEnumerable`1 refspecs, FetchOptions options, String logMessage)
    Network.cs(226,0): at LibGit2Sharp.Network.Fetch(Remote remote, IEnumerable`1 refspecs, FetchOptions options)
    FetchFixture.cs(149,0): at LibGit2Sharp.Tests.FetchFixture.CanFetchCustomRefSpecsIntoAnEmptyRepository(String url, String localBranchName, String remoteBranchName)

1211 passed, 1 failed, 5 skipped (see 'Task List'), took 580,41 seconds (xUnit.net 1.9.2 build 1705).

The failing test is related to opportunistic updates.

The remotetips handler is called twice

  • once with (refs/heads/master:refs/remotes/testRemote/master, {0000000}, {49322bb})
  • then with (refs/heads/master:refs/remotes/testRemote/master, {49322bb}, {49322bb})

@carlosmn Do we actually need the second call?

@carlosmn
Copy link
Member

The second call is unnecessary, I've updated the branch to skip it.

@nulltoken
Copy link
Member Author

1212 passed, 0 failed, 5 skipped (see 'Task List'), took 141,71 seconds (xUnit.net 1.9.2 build 1705).

@carlosmn 😍

@nulltoken
Copy link
Member Author

And the build is passing passing!!!!!


public GitFetchOptions()
{
download_tags = TagFetchMode.Auto;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to do this in the constructor, vs as a field initializer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be TagFetchMode.Fallback?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Without it, the tests pass, but something seems weird to me (although I cannot identify what bugs me right now). Dropped it

@jamill
Copy link
Member

jamill commented May 28, 2015

👍 awesome!

@nulltoken
Copy link
Member Author

@jamill Thanks for the review! Could you please double check the fixup proposal and see if that looks better?

@jamill
Copy link
Member

jamill commented May 29, 2015

@nulltoken looks good, thanks!

@nulltoken nulltoken changed the title Upgrade to latest libgit2 Upgrade to libgit2 ff8d635 May 29, 2015
nulltoken added a commit that referenced this pull request May 29, 2015
@nulltoken nulltoken merged commit 27abed1 into vNext May 29, 2015
@nulltoken nulltoken deleted the ntk/upgrade_nuget branch May 29, 2015 05:59
@nulltoken
Copy link
Member Author

That was a little of a bumpy road, but 🎉 ‼️ It's in!

@whoisj
Copy link

whoisj commented May 29, 2015

and I'm already making use of it! 🎉 🎈

@nulltoken
Copy link
Member Author

Published as NuGet pre-release package LibGit2Sharp 0.22.0-pre20150529055949

@nulltoken nulltoken added this to the v0.22 milestone Jun 6, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6c12de4 on ntk/upgrade_nuget into ** on vNext**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants