Wrap all the various way of doing checkouts in GTCheckoutOptions by tiennou · Pull Request #459 · libgit2/objective-git (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation44 Commits15 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
I hit some "disappointment" while transitioning the clone API. I'm not sure what to do, so I wrote a wrapper for the current BOOL option, but I'd like some feedback on that.
Tests are passing here, and right now it handles the same thing as before, but now it can be more easily extended for the other use cases. I might take a look this week-end at adding path support for the checkout case (since I think clone is pretty complete).
@jspahrsummers Feel free to hint me at some use cases you need to switch to Objectified glory ;-).
I don't feel strongly about this one way or the other. Anyone else?
I'd rather not wrap these in a class, personally.
I think this approach makes sense if there are many different operations that may need “checkout options.” I don't know if that's the case or not.
Rebased that against master, and added support for the just-landed stash API.
Note that b4d5816, fcbcf23 and cc41976 have nothing to do with proposal, so cherry-picking them MIGHT be good.
🤘
Looks like some tests need to be updated still.
My bad, forgot to run the tests :-S. It should pass now.
Looks like there are still some legit build failures 😞
Should be good now. Maybe. Who Knows.
| stash_options.checkout_options = *options.git_checkoutOptions; |
|---|
| } |
| if (pop == NO) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This combining of the methods feels off to me since they're pretty different things.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't really proud of that when I DRYed it... I'll revert that one.
| @@ -216,7 +191,7 @@ extern NSString * const GTRepositoryInitOptionsOriginURLString; |
|---|
| /// options - A dictionary consisting of the options: |
| /// `GTRepositoryCloneOptionsTransportFlags`, |
| /// `GTRepositoryCloneOptionsBare`, |
| /// `GTRepositoryCloneOptionsCheckout`, |
| /// `GTRepositoryCloneCheckoutOptions`, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding welcome ;-).
@joshaber @tiennou Whats the state of this one? Seems, comments were applied, tests run through and everything seems fine 🤔
Well, apart from my own questions on naming things and copying blocks, it still looks fine ;-).
Ah, I didn't realize it was ready for re-review 😄
Just 1️⃣ 📔 about deprecating the old methods.
Rebased because Xcode decided to eat my submodules (thanks Xcode). Sorry about that. I've removed all stuff related to my tentative merging of the stash machinery since that was unrelated and added 8449a34 to handle pathspecs.
@joshaber I can't seem to parse your emojies, what did you meant ?
tiennou referenced this pull request
For a finer control over the unstash process, propogate checkout strategy to Objective-Git from the underlying libgit2 stash methods.
@pietbrauer Can this be reviewed/merged ? It seems @slavikus has made a few commits that add checkout customization support to stash, which was the point of this PR.
@tiennou Can you and @slavikus sort that out? I don't have a huge stake in this PR and just pinged people so we can merge/close some PRs.
I just am generally cautious about abusing my own merge rights, that's all. Let's say that if there's no movement here until next year, I'll merge.
@tiennou the new year is here, what should we do? :)
Added documentation and fixed a few dangling things. @slavikus can you review that ? I just want to make sure it's 👍 by at least another set of eyes before merging...
I felt adventurous today so I thought I would merge this. Looked good to me and it's not that the world would stop moving if we merge this. Thanks for the effort @tiennou!
Dangit, missed the initial notification back in January for some reason. I've been following this merge for a while, and I think it's looking good. Thanks guys!
slavikus added a commit to slavikus/objective-git that referenced this pull request
Conflicts:
ObjectiveGit/GTRepository+Stashing.m
slavikus added a commit to slavikus/objective-git that referenced this pull request