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

Add Push #419

Merged
merged 18 commits into from
Dec 8, 2014
Merged

Add Push #419

merged 18 commits into from
Dec 8, 2014

Conversation

phatblat
Copy link
Member

Since #329 has been stalled, I thought I'd take a stab at finishing this up. Pull needs to be reworked to take advantage of fetch now that #224 is in master. I'll submit that as a separate PR once I've got it working.

There are two new public methods:

  • -pushBranch:toRemote:withOptions:error:progress:
  • -pushBranches:toRemote:withOptions:error:progress:

Since libgit2 provides for pushing any number of branches I figured it would be convenient to expose that. However, the refspecs that this implementation builds assumes the remote branch has the same name as the local tracking branch.

I've tested these changes from a consuming application, but didn't add/update tests in the project because I haven't been able to get the ObjectiveGit-MacTests target to build yet.

@jspahrsummers jspahrsummers self-assigned this Nov 13, 2014
/// A `GTCredentialProvider`, that will be used to authenticate against the remote.
extern NSString *const GTRepositoryRemoteOptionsCredentialProvider;

@class GTFetchHeadEntry;
typedef void (^GTRemoteFetchTransferProgressBlock)(const git_transfer_progress *stats, BOOL *stop);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we prefer to use inline block definitions in headers, because it makes things easier on consumers. Can you please move this back to the implementation file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing. I'll inline GTRemotePushTransferProgressBlock as well.

@jspahrsummers
Copy link
Contributor

Thank you! ✨

Would you mind adding some unit tests that push to a local fixture repository? 🙏

@phatblat
Copy link
Member Author

Sure, I'll work on some tests.

Question on the Tests target, I had to add #import <Nimble/Nimble-Swift.h> to all GT*Spec.m files in order to get them to build. Is there a better way to get them to cooperate? I'm using Xcode 6.1 (6A1052d).

@jspahrsummers
Copy link
Contributor

@phatblat Hmm, no, that definitely shouldn't be required. Did you run script/bootstrap to set up the project?

@phatblat
Copy link
Member Author

Ah, no. Sorry. bootstrap fixed me up.

Ben Chatelain added 8 commits November 14, 2014 22:07
Local transport doesn't currently call the progress callbacks
Created convenience localBranchWithName block. Make branch references local to examples.

Also fixed case where afterEach wasn't deleting localRepo, which allowed removal of redundant removeItemAtURL call.

Added pending dual branch example.
@ghost
Copy link

ghost commented Dec 1, 2014

Any news on whether this will be merged?

As aside, is there any basic documentation on how to use objective-git?

@jspahrsummers
Copy link
Contributor

Sorry about that—is this ready for re-review @phatblat? We don't get notifications about new commits, only comments, so we usually comment to indicate when something is ready to go.

is there any basic documentation on how to use objective-git?

@thelucid At a high level? No, because the library is still under fairly heavy churn. We'd like to add something like that once it settles down.

In the meantime, the header docs or even libgit2 documentation is best.

@phatblat
Copy link
Member Author

phatblat commented Dec 1, 2014

Sorry for the delay. This is close but not quite ready. I've been thinking that the branch(es) parameters should be required and am planning on adding NSParameterAssert() on those. Also, I want to remove the assumption that the local and remote branches have the same name and use the tracking info for the remote branch name, when available.

Things not covered in this PR: force push & pushing tags. I'm thinking these can come later, unless someone feels that this API won't suffice. Force could be specified using options. Tags could be pushed by exposing new methods just for tags. In order to push both branches and tags, either multiple methods would need to be called (resulting in multiple network operations) or a method taking an array of GTReference could be exposed.

@jspahrsummers
Copy link
Contributor

Things not covered in this PR: force push & pushing tags. I'm thinking these can come later, unless someone feels that this API won't suffice

I think doing that later is entirely reasonable. 👍

@ghost
Copy link

ghost commented Dec 1, 2014

@phatblat Thanks for the hard work, I'm working on an app that needs push functionality so this would be perfect.

@jspahrsummers Thanks, I've found the specs to be pretty useful as documentation for the time being.

@phatblat
Copy link
Member Author

phatblat commented Dec 4, 2014

OK, parameters asserted and branch name assumption removed. This is ready for review now. 😌

@jspahrsummers
Copy link
Contributor

Thank you! Sorry about the delay, I was out for FunSwiftConf. 😄

jspahrsummers added a commit that referenced this pull request Dec 8, 2014
@jspahrsummers jspahrsummers merged commit 8828d01 into libgit2:master Dec 8, 2014
@phatblat phatblat deleted the ben/pr/push branch December 8, 2014 22:49
@jspahrsummers jspahrsummers removed their assignment May 22, 2015
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.

2 participants