Fetch #221

jamill · 2012-10-09T21:55:30Z

This is a continuation of the PR #213, but based off of vNext branch. I have included the original commit to show the changes between the original PR and this updated PR. I can squash the commits so only the last final commit will show up if that would be easier.

This includes reaction for the updates LibGit2Sharp code base, using delegates instead of events, and includes tests for retrieving tags.

jamill · 2012-10-09T22:02:11Z

@nulltoken @carlosmn : One thing I noticed is that it does not appear that all of the expected tags are being brought down as part of the fetch - In particular, the tag "annotated_tag" in @nulltoken's test repository is not being brought down as part of the fetch tests - if I do fetch from the PowerShell command line, it retrieves this particular tag.

As of now, the tests included with this commit do not include this tag.

dahlbyk · 2012-10-10T04:42:09Z

I like the direction we're headed, but calling Fetch() still seems more complex than it needs to be. If RemoteCallbacks is made immutable and not for public consumption, I think things get simpler:

dahlbyk/libgit2sharp@jamill:Fetch...dahlbyk:Fetch

Once the API is nailed down, IRepository needs Fetch() and there should probably be an extension method that accepts the remote name instead of the Remote.

Finally, within the existing test I've not been able to get a completion handler to actually fire. Even for a small test repository, that surprises me...

nulltoken · 2012-10-10T13:37:31Z

LibGit2Sharp.Tests/BranchFixture.cs

@@ -221,6 +221,28 @@ public void CanListAllBranchesIncludingRemoteRefs()
        }

        [Fact]
+        public void CanResolveRemote()
+        {


Could you please split this test into three smaller ones?
This should allow you to have more explicit test method names and possibly get rid of the comments.

Sure - will split this test.

jamill · 2012-10-10T13:38:29Z

@dahlbyk Thank you for the quick feedback!

If RemoteCallbacks is made immutable and not for public consumption, I think things get simpler:

The reason I was initially hesitant to go with this approach is that the RemoteCallbacks structure appears (to me) that it might be applicable more widely than just fetch at some point (These are callbacks set on the remote - they seem applicable to at least Pull and Fetch... not sure if all of these callbacks make sense with Push).

Having said that, I am fine going with your API and will split out the callbacks.

Finally, within the existing test I've not been able to get a completion handler to actually fire. Even for a small test repository, that surprises me

Yes, this is why I did not expose this callback in my initial implementation. Unless I missed it, I actually do not see where this callback is called in libgit2. I included it so that when this functionality is added to libgit2, we should pick it up (albeit untested...) Maybe the correct thing to do is to not expose this callback until it is implemented in libgit2. (/cc @carlosmn)

I will update the PR with the API you suggested, add Fetch to IRepository, and add the extension method taking the remote name.

nulltoken · 2012-10-10T13:38:48Z

LibGit2Sharp.Tests/RepositoryFixture.cs

+        [Theory]
+        [InlineData("http://github.com/nulltoken/TestGitRepository")]
+        [InlineData("https://github.com/nulltoken/TestGitRepository")]
+        //[InlineData("git@github.com:nulltoken/TestGitRepository")]


git://github.com/libgit2/TestGitRepository.git should work

The fetch test fails for this protocol, looks like this transport is not implemented yet:

LibGit2Sharp.Tests.RepositoryFixture.CanFetchFromEmptyRepository(url: "git@github.com:libgit2/TestGitRepository") [FAIL] LibGit2Sharp.LibGit2SharpException : An error was raised by libgit2. Category = Net (Error). This transport isn't implemented. Sorry

git read-only protocol (git://github.com/libgit2/TestGitRepository.git) should work. You might be willing to give it a try :wink2:

ssh (git@github.com:libgit2/TestGitRepository) is indeed not implemented yet

You are correct - I will add the git read-only protocol to the set of tests that are run.

Strange, I updated the InlineData content to the following

[InlineData("http://github.com/libgit2/TestGitRepository")] [InlineData("https://github.com/libgit2/TestGitRepository")] [InlineData("git://github.com/libgit2/TestGitRepository.git")]

Here's the result of the test run

------ Test started: Assembly: LibGit2Sharp.Tests.dll ------ Test 'LibGit2Sharp.Tests.RepositoryFixture.CanFetchFromEmptyRepository(url: "git://github.com/libgit2/TestGitRepository.git")' failed: Could not find the reference refs/tags/annotated_tag in the list of expected reference updates. RepositoryFixture.cs(546,0): at LibGit2Sharp.Tests.RepositoryFixture.ExpectedFetchState.RemoteUpdateTipsHandler(String referenceName, ObjectId oldId, ObjectId newId) RemoteCallbacks.cs(123,0): at LibGit2Sharp.RemoteCallbacks.GitUpdateTipsHandler(IntPtr str, GitOid& oldId, GitOid& newId, IntPtr data) at LibGit2Sharp.Core.NativeMethods.git_remote_update_tips(RemoteSafeHandle remote) Core\Proxy.cs(1101,0): at LibGit2Sharp.Core.Proxy.git_remote_update_tips(RemoteSafeHandle remote) Repository.cs(630,0): at LibGit2Sharp.Repository.FetchInternal(RemoteSafeHandle remoteHandle, Int64& bytes, GitIndexerStats& gitIndexerStats, Nullable`1 callbacks) Repository.cs(450,0): at LibGit2Sharp.Repository.Fetch(Remote remote, FetchProgress progress, RemoteCallbacks callbacks) RepositoryFixture.cs(105,0): at LibGit2Sharp.Tests.RepositoryFixture.CanFetchFromEmptyRepository(String url) 2 passed, 1 failed, 0 skipped, took 7,32 seconds (xUnit.net 1.9.0 build 1566).

yes :)

It appears that the git read-only protocol correctly fetches the expected references. I will update the expected results with the correct expected results. This means that the git read-only protocol will pass while http / https will fail.

The fix for fetching from empty repos broke capability detection in HTTP (and probably cloning, come to think of it). I'm fixing this in the library now.

nulltoken · 2012-10-10T13:53:32Z

I will update the PR with the API you suggested, add Fetch to IRepository, and add the extension method taking the remote name.

@jamill @dahlbyk I'm playing with an idea: moving the Fetch() method to the Remote type and adding two extension methods to IRepository (one accepting a Remote instance, another one accepting a remote name). How do you feel about this?

nulltoken · 2012-10-10T13:54:32Z

the tag "annotated_tag" in @nulltoken's test repository is not being brought down as part of the fetch tests

I can replicate this as well. @carlosmn is going to investigate this further.

jamill · 2012-10-10T14:18:17Z

@jamill @dahlbyk I'm playing with an idea: moving the Fetch() method to the Remote type and adding two
extension methods to IRepository (one accepting a Remote instance, another one accepting a remote name). How > do you feel about this?

The reason I implemented Fetch() on the Repository type (instead of the Remote type) was to enable fetching from a URL (i.e. a non-named remote). While this is not currently implemented, it would be easy to add with the current implementation (It seems this is a valid scenario to support). If Fetch() is implemented on the Remote type, how would clients create a Remote object (with only a URL) in order to fetch from?

nulltoken · 2012-10-10T17:27:10Z

The reason I implemented Fetch() on the Repository type (instead of the Remote type) was to enable fetching from a URL (i.e. a non-named remote). [...] If Fetch() is implemented on the Remote type, how would clients create a Remote object (with only a URL) in order to fetch from?

I understand your point. However, I'm not sure that I'm willing to support temporary, in-memory nameless remotes in the near future. I haven't found a clear use case yet where a non persisted remote is a compelling requirement.

jamill · 2012-10-10T18:56:56Z

I see - thank you for the explaination :)

Yes, in that case I do not have any objections to putting the Fetch() implementation on the Remote type. Unless I hear otherwise, I will move it over.

nulltoken · 2012-10-10T21:16:30Z

LibGit2Sharp.Tests/RepositoryFixture.cs

+        [InlineData("http://github.com/nulltoken/TestGitRepository")]
+        [InlineData("https://github.com/nulltoken/TestGitRepository")]
+        //[InlineData("git@github.com:nulltoken/TestGitRepository")]
+        public void CanFetchFromEmptyRepository(string url)


BTW, I'm not sure the name of this test fully reflects its content ;) ^^

I will change this to CanFetchIntoAnEmptyRepository - (when originally naming this, my thought was "we are executing fetch from (inside of) an empty repository"

jamill · 2012-10-11T17:30:13Z

@nulltoken @dahlbyk : I have updated the Pull Request based on your feedback.

The travis build failed due to the differences in the expected vs actual tags fetched. This update includes several changes:

I have updated the Fetch API to be more in line with @dahlbyk's suggestion
- If you would like me to directly pull your commits into my branch and submit those commits as is, I could do that as well.
Fetch is now implemented on Remote class
I have added extension methods to the RepositoryExtensions class
I have added the parameter to specify tag fetching strategy to the Fetch interface
Tests have been updated to reflect expected tag fetching behavior

I have added a bit of infrastructure around the fetch tests (for specifying and verifying expected references). If you have suggestions on a different approach you would like here, please let me know.

Thanks again for taking the time to look at this PR and provide feedback.

nulltoken · 2012-10-11T17:38:50Z

The travis build failed due to the differences in the expected vs actual tags fetched.

I cannot update the binaries yet with @carlosmn's fix. A small issue prevents the latest development tip from being compiled by MSVC with GIT_THREADS being defined. This will hopefully get quickly fixed.

nulltoken · 2012-10-14T19:27:27Z

I cannot update the binaries yet with @carlosmn's fix. A small issue prevents the latest development tip from being compiled by MSVC with GIT_THREADS being defined. This will hopefully get quickly fixed.

@jamill Sorry. No news on this front yet. None of the pthread_cond_* are defined, which makes the compiler unhappy when GIT_THREADSis set.

@schu confirmed the issue ("win32/pthread* is lacking the corresponding wrappers")

/cc @phkelley

dahlbyk · 2012-10-15T02:17:22Z

If you would like me to directly pull your commits into my branch and submit those commits as is, I could do that as well.

That won't be necessary.

The reason I implemented Fetch() on the Repository type (instead of the Remote type) was to enable fetching from a URL (i.e. a non-named remote). [...] If Fetch() is implemented on the Remote type, how would clients create a Remote object (with only a URL) in order to fetch from?

I understand your point. However, I'm not sure that I'm willing to support temporary, in-memory nameless remotes in the near future. I haven't found a clear use case yet where a non persisted remote is a compelling requirement.

I actually fetch specific branches by URL quite a bit to look at PRs from individuals that haven't earned a spot in my remote list. (I'd love to see a "Fetch This Branch in GH4W" button.) git.git just leaves the reference in FETCH_HEAD. That said, I don't think building our own implementation is worth the effort; if libgit2 adds support, we can expose it from the Repository.

jamill · 2012-10-15T18:49:24Z

@dahlbyk Thanks again!

I wonder if this extension is even necessary, since the Remote passed in exposes Fetch() itself?

I agree - removed.

File name has typo

Thanks... fixed.

Until we have reason to expose publicly, I'd kill this ctor and mark the class internal.

I made this class internal. I moved the delegate definitions over to the Remote class - I also thought about not defining these inside of a class, but rather directly in the LibGit2Sharp namespace - but as these are specific to a Remote (or the RemoteCallback) I decided to keep them defined within a class.

dahlbyk · 2012-10-15T23:14:37Z

I made this class internal. I moved the delegate definitions over to the Remote class - I also thought about not defining these inside of a class, but rather directly in the LibGit2Sharp namespace - but as these are specific to a Remote (or the RemoteCallback ) I decided to keep them defined within a class.

Now that you mention it, the nested delegates are probably why I didn't mark it internal myself. Having them live on Remote makes sense for Fetch as implemented now, but less so if they're also used for Clone or Fetch(url). If we don't want to pollute the root namespace, we could add a Handlers namespace where delegates live (since they're rarely needed by name thanks to inference).

jamill · 2012-10-16T17:11:34Z

I added the delegate definitions into a Handlers namespace and updated the Pull Requst.

Thanks again!

jamill · 2012-10-16T18:51:03Z

@jamill Sorry. No news on this front yet. None of the pthread_cond_* are defined, which makes the compiler
unhappy when GIT_THREADSis set.

@nulltoken looks like this issue was resolved with libgit2/libgit2#987. Would the next step be to update the libgit2 binaries and re-run the tests?

nulltoken · 2012-10-16T18:59:19Z

I added the delegate definitions into a Handlers namespace and updated the Pull Request.

@jamill @dahlbyk I'm not really sure about the discoverability of the definition of these handlers if we put them in a different namespace. Not everyone has got ReSharper... However, we know/hope that the API is going to change (cf. this comment), so let's keep it that way for now.

nulltoken · 2012-10-16T19:01:20Z

@jamill Sorry. No news on this front yet. None of the pthread_cond_* are defined, which makes the compiler
unhappy when GIT_THREADSis set.

@nulltoken looks like this issue was resolved with libgit2/libgit2#987. Would the next step be to update the libgit2 binaries and re-run the tests?

I'm on it! ;)

nulltoken · 2012-10-16T19:55:56Z

I'm on it! ;)

@jamill ...and done! vNext has been updated. You should be good to go ;)

Just a few things, I've noticed:

vNext also bears a GitIndexerStats type. Could you please fix my mistake and update its definition so that it exposes uints instead of ints?
Unless I'm wrong RepositoryFixture.CanFetchFromRemoteByName() won't allow you to retrieve the nearly-dangling tag. How do you feel about demonstrating two successive Fetch() calls and the use of TagOption?

I was thinking about something similar to

$ git init .
Initialized empty Git repository in d:/New folder/.git/

$ git remote add origin https://github.com/libgit2/TestGitRepository

$ git fetch
remote: Counting objects: 69, done.
remote: Compressing objects: 100% (45/45), done.
remote: Total 69 (delta 3), reused 69 (delta 3)
Unpacking objects: 100% (69/69), done.
From https://github.com/libgit2/TestGitRepository
 * [new branch]      first-merge -> origin/first-merge
 * [new branch]      master     -> origin/master
 * [new branch]      no-parent  -> origin/no-parent
From https://github.com/libgit2/TestGitRepository
 * [new tag]         annotated_tag -> annotated_tag
 * [new tag]         blob       -> blob
 * [new tag]         commit_tree -> commit_tree

$ git fetch --tags
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (1/1), done.
From https://github.com/libgit2/TestGitRepository
 * [new tag]         nearly-dangling -> nearly-dangling

Now would be good time to rebase and squash.
Final nitpick: could you please get rid of the trailing whitespaces?

I'm really eager to merge this. This is a fantastic job!

jamill · 2012-10-16T20:12:59Z

@nulltoken Thanks for updating vNext so quickly :) I will rebase / squash, and update the pull request.

•Unless I'm wrong RepositoryFixture.CanFetchFromRemoteByName() won't allow you to retrieve the nearly-dangling tag. How do you feel about demonstrating two successive Fetch() calls and the use of TagOption?

Sure - I will update the test here to perform an additional fetch and verify the results.

•Final nitpick: could you please get rid of the trailing whitespaces?

Just to make sure I understand, you are referring to the whitespace at the end of RepositoryFixture? I will remove that. Are there other places that you were referring to as well?

Thanks again!

jamill · 2012-10-16T20:17:28Z

Just to make sure I understand, you are referring to the whitespace at the end of RepositoryFixture? I will remove that. Are there other places that you were referring to as well?

Nevermind, I think I see what you mean. I will take care of these.

nulltoken · 2012-10-16T20:19:10Z

Try running $ git diff --check HEAD~5 on top of 9b87b8c91ab26237753a08345641586cebc952ad

Those will be easier to cleanup once the squash is done.

jamill · 2012-10-16T22:13:07Z

Ok - I have squashed the previous commits into a single commit. I still included several commits to break out the changes based on the rebase reaction and other recent changes. I can go ahead and squash these as well if preferred.

@nulltoken commit be2be63 includes the changes for the GitIndexerStats. I am not sure if you have a preference for these objects being structs vs classes, but I updated this to be a struct... This is more of an internal implementation detail, so we can change it to be whatever we want in the end.

dahlbyk · 2012-10-17T03:09:28Z

Final nitpick: could you please get rid of the trailing whitespaces?

A man after my own heart...

dahlbyk · 2012-10-17T04:30:16Z

@jamill @dahlbyk I'm not really sure about the discoverability of the definition of these handlers if we put them in a different namespace. Not everyone has got ReSharper... However, we know/hope that the API is going to change (cf. this comment), so let's keep it that way for now.

No need for R#: VS (since 2010 at least) will show a smart tag to add a missing namespace. Though really type inference in C# 3 and up makes explicit use of delegate types pretty rare in my experience.

nulltoken · 2012-10-17T17:16:25Z

@jamill I manually merged your awesome pull request in vNext. THANKS!

✨✨✨

Enable multiplexing if the server supports it. This allows us to give feedback to the user when the server is doing work and we're waiting for the pack data to arrive. References libgit2#221, libgit2#222

nulltoken mentioned this pull request Oct 10, 2012

Win http fetch #213

Closed

nulltoken reviewed Oct 10, 2012
View reviewed changes

ben mentioned this pull request Oct 11, 2012

Clone #223

Merged

Update Proxy and NativeMethods in preperation for Fetch.

be2be63

jamill added 3 commits October 16, 2012 16:34

Add ability to fetch from remotes.

4e960f8

Include test to verify that FetchRemoteByName can download all tags.

d107dd8

Remove trailing whitespaces.

008c2b8

nulltoken closed this Oct 17, 2012

Fetch #221

Fetch #221

Uh oh!

Conversation

jamill commented Oct 9, 2012

Uh oh!

jamill commented Oct 9, 2012

Uh oh!

dahlbyk commented Oct 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented Oct 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Oct 10, 2012

Uh oh!

nulltoken commented Oct 10, 2012

Uh oh!

jamill commented Oct 10, 2012

Uh oh!

nulltoken commented Oct 10, 2012

Uh oh!

jamill commented Oct 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented Oct 11, 2012

Uh oh!

nulltoken commented Oct 11, 2012

Uh oh!

nulltoken commented Oct 14, 2012

Uh oh!

dahlbyk commented Oct 15, 2012

Uh oh!

jamill commented Oct 15, 2012

Uh oh!

dahlbyk commented Oct 15, 2012

Uh oh!

jamill commented Oct 16, 2012

Uh oh!

jamill commented Oct 16, 2012

Uh oh!

nulltoken commented Oct 16, 2012

Uh oh!

nulltoken commented Oct 16, 2012

Uh oh!

nulltoken commented Oct 16, 2012

Uh oh!

jamill commented Oct 16, 2012

Uh oh!

jamill commented Oct 16, 2012

Uh oh!

nulltoken commented Oct 16, 2012

Uh oh!

jamill commented Oct 16, 2012

Uh oh!

dahlbyk commented Oct 17, 2012

Uh oh!

dahlbyk commented Oct 17, 2012

Uh oh!