-
Notifications
You must be signed in to change notification settings - Fork 280
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
Add Push #419
Conversation
/// 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thank you! ✨ Would you mind adding some unit tests that push to a local fixture repository? 🙏 |
Sure, I'll work on some tests. Question on the Tests target, I had to add |
@phatblat Hmm, no, that definitely shouldn't be required. Did you run |
Ah, no. Sorry. bootstrap fixed me up. |
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.
Any news on whether this will be merged? As aside, is there any basic documentation on how to use objective-git? |
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.
@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. |
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 |
I think doing that later is entirely reasonable. 👍 |
@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. |
OK, parameters asserted and branch name assumption removed. This is ready for review now. 😌 |
Thank you! Sorry about the delay, I was out for FunSwiftConf. 😄 |
There are two new public methods:
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.