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

Wrap all the various way of doing checkouts in GTCheckoutOptions #459

Merged
merged 15 commits into from
Feb 27, 2017

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Apr 10, 2015

Fixes #457

@tiennou
Copy link
Contributor Author

tiennou commented Apr 10, 2015

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 ;-).

@joshaber
Copy link
Member

I don't feel strongly about this one way or the other. Anyone else?

@mdiep
Copy link
Contributor

mdiep commented Apr 14, 2015

I'd rather not wrap these in a class, personally.

@jspahrsummers
Copy link
Contributor

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.

@tiennou
Copy link
Contributor Author

tiennou commented Apr 15, 2015

Here's a list : cherrypick, clone (we're handling that already), merge, rebase, reset, reset, revert, submodule.

Note that some of them have those options as a parameter, others have them included in another git_*_options.

@tiennou
Copy link
Contributor Author

tiennou commented Sep 8, 2015

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.

@joshaber joshaber self-assigned this Sep 9, 2015
@joshaber
Copy link
Member

joshaber commented Sep 9, 2015

🤘

Looks like some tests need to be updated still.

@tiennou
Copy link
Contributor Author

tiennou commented Sep 9, 2015

My bad, forgot to run the tests :-S. It should pass now.

@joshaber
Copy link
Member

Looks like there are still some legit build failures 😞

@tiennou
Copy link
Contributor Author

tiennou commented Sep 10, 2015

Really, that thing was built, and tested here...

http://i.imgur.com/JESUH96.jpg

@joshaber
Copy link
Member

👏

@tiennou
Copy link
Contributor Author

tiennou commented Sep 10, 2015

Should be good now. Maybe. Who Knows.

stash_options.checkout_options = *options.git_checkoutOptions;
}

if (pop == NO) {
Copy link
Member

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.

Copy link
Contributor Author

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.

@joshaber
Copy link
Member

🤘

Some 📝 above.

@@ -216,7 +191,7 @@ extern NSString * const GTRepositoryInitOptionsOriginURLString;
/// options - A dictionary consisting of the options:
/// `GTRepositoryCloneOptionsTransportFlags`,
/// `GTRepositoryCloneOptionsBare`,
/// `GTRepositoryCloneOptionsCheckout`,
/// `GTRepositoryCloneCheckoutOptions`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bikeshedding welcome ;-).

@pietbrauer
Copy link
Member

@joshaber @tiennou Whats the state of this one? Seems, comments were applied, tests run through and everything seems fine 🤔

@tiennou
Copy link
Contributor Author

tiennou commented Jan 11, 2016

Well, apart from my own questions on naming things and copying blocks, it still looks fine ;-).

@joshaber
Copy link
Member

Ah, I didn't realize it was ready for re-review 😄

@joshaber
Copy link
Member

Just 1️⃣ 📔 about deprecating the old methods.

@tiennou
Copy link
Contributor Author

tiennou commented Jun 28, 2016

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 Dec 18, 2016
For a finer control over the unstash process, propogate checkout strategy to Objective-Git from the underlying libgit2 stash methods.
@tiennou
Copy link
Contributor Author

tiennou commented Dec 18, 2016

@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.

@pietbrauer
Copy link
Member

@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.

@tiennou
Copy link
Contributor Author

tiennou commented Dec 22, 2016

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.

@slavikus
Copy link
Contributor

@tiennou the new year is here, what should we do? :)

@tiennou
Copy link
Contributor Author

tiennou commented Jan 15, 2017

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...

@pietbrauer pietbrauer merged commit c0e98a2 into master Feb 27, 2017
@pietbrauer pietbrauer deleted the checkout-options branch February 27, 2017 09:35
@pietbrauer
Copy link
Member

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!

@slavikus
Copy link
Contributor

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 Feb 27, 2017
# Conflicts:
#	ObjectiveGit/GTRepository+Stashing.m
slavikus added a commit to slavikus/objective-git that referenced this pull request Feb 27, 2017
# Conflicts:
#	ObjectiveGit/GTRepository.h
#	ObjectiveGit/GTRepository.m
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.

Extending checkout support
6 participants