Skip to content

Refactor Network Internal API #1087

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

Merged
merged 2 commits into from
Jun 10, 2015
Merged

Refactor Network Internal API #1087

merged 2 commits into from
Jun 10, 2015

Conversation

whoisj
Copy link

@whoisj whoisj commented Jun 9, 2015

Fixes #1084

  • Splits the DoFetch into three overloaded methods.
    • Primary which takes a RemoteSafeHandle
    • Overload which takes a Remote, acquires a RemoteSafeHandle, and calls primary
    • Overload which takes a URL, acquires a RemoteSafeHandle, and calls primary
  • Updates callers to use the correct instance methods.
  • Adds a lot of asserting to help with future debugging.
  • Minor beautification in a secondary commit.

@nulltoken
Copy link
Member

Looks pretty neat to me. @jamill ?


RemoteSafeHandle remoteHandle;

remoteHandle = Proxy.git_remote_create_anonymous(repoHandle, url);
Copy link
Member

Choose a reason for hiding this comment

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

any reason to not just initialize the remoteHandle to this value when declaring it? Not big deal either way...

@jamill
Copy link
Member

jamill commented Jun 10, 2015

Looks good to me - left just 2 small comments that are more style oriented. Thanks @whoisj!

@whoisj
Copy link
Author

whoisj commented Jun 10, 2015

Both fair. I'll update in the morning. Thanks

@whoisj
Copy link
Author

whoisj commented Jun 10, 2015

@jamill just pushed the changes you asked for.

Let me know if this is ready, thanks.

@whoisj
Copy link
Author

whoisj commented Jun 10, 2015

Looks like a random failure in AppVeyor. I'll "touch" a commit and restart it.

@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="utf-8" ?>
<configuration>
Copy link
Member

Choose a reason for hiding this comment

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

@whoisj You may have unintentionally staged too many files....

Copy link
Author

Choose a reason for hiding this comment

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

😕

Copy link
Author

Choose a reason for hiding this comment

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

@nulltoken fixed, thanks.

@nulltoken nulltoken added this to the v0.22 milestone Jun 10, 2015
nulltoken added a commit that referenced this pull request Jun 10, 2015
@nulltoken nulltoken merged commit 289f176 into libgit2:vNext Jun 10, 2015
@nulltoken
Copy link
Member

👍 I'm eager to see the new Coverity numbers!!!

@whoisj whoisj deleted the refactor-network-internal-api branch June 10, 2015 18:51
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.

3 participants