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

A first stab at a pull example. #427

Merged
merged 12 commits into from
Mar 22, 2020
Merged

Conversation

zaphar
Copy link
Contributor

@zaphar zaphar commented Jun 3, 2019

I'm sure I'm missing edge cases and stuff in here since I had to basically piece this together over a couple weeks but since I've done it I figured other people could learn from my now hard won experience.

@zaphar
Copy link
Contributor Author

zaphar commented Jun 3, 2019

I was going to run cargo fmt but it reformatted way too many files to pollute this diff with so I left it as is.

examples/pull.rs Outdated Show resolved Hide resolved
@zaphar
Copy link
Contributor Author

zaphar commented Jun 6, 2019

I think this is ready for review if anyone want's to tackle it :-D It builds and runs for the limited set of cases I think I'm handling correctly.

@ehuss
Copy link
Contributor

ehuss commented Jun 6, 2019

Thanks @zaphar, I'll try to get to it sometime soon.

examples/pull.rs Outdated Show resolved Hide resolved
examples/pull.rs Outdated Show resolved Hide resolved
examples/pull.rs Outdated Show resolved Hide resolved
examples/pull.rs Outdated Show resolved Hide resolved
examples/pull.rs Outdated Show resolved Hide resolved
examples/pull.rs Outdated
extern crate docopt;
extern crate git2;
#[macro_use]
extern crate serde_derive;
Copy link
Contributor

Choose a reason for hiding this comment

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

When you rebase on master, the project has been updated to 2018 edition, so you can drop these extern crate lines. Will still need a use serde_derive::Deserialize;, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that at the end so I only have to take the hit once. :-D

@zaphar
Copy link
Contributor Author

zaphar commented Jun 18, 2019

I think this is ready for another pass

@zaphar zaphar force-pushed the add_pull_example branch 3 times, most recently from 8596ad8 to 32b0391 Compare June 19, 2019 22:19
@zaphar
Copy link
Contributor Author

zaphar commented Jun 28, 2019

@ehuss Ping

@DarrienG
Copy link

I used this as an example and it was super helpful. Thanks :)

@kallisti5
Copy link

super helpful. Why isn't this in the docs?

1 similar comment
@kallisti5
Copy link

super helpful. Why isn't this in the docs?

examples/pull.rs Outdated
// For some reason the force is required to make the working directory actually get updated
// I suspect we should be adding some logic to handle dirty working directory states
// but this is just an example so maybe not.
.force()))?;
Copy link

Choose a reason for hiding this comment

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

👋Thanks for putting this PR together; it was really helpful.

I ran into this safe vs force problem as well and posted an answer here about what I think is going on. The summary is: safe mode will work if you checkout_branch before setting the head.

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 that analysis sounds about right.

@ehuss
Copy link
Contributor

ehuss commented Mar 17, 2020

@zaphar I apologize for letting this fall through the cracks. Would you be able to rebase on master? Afterwards I'll go ahead and merge it.

@zaphar
Copy link
Contributor Author

zaphar commented Mar 20, 2020

@ehuss, I'll try to get to that this week. Work and such has been a bit crazy with CoVID-19 preparedness and response stuff.

@zaphar
Copy link
Contributor Author

zaphar commented Mar 21, 2020

I successfully rebased and have updated to StructOpt which looks like it is what got chosen instead of Docopt. Some comletely unrelated tests failed on my local so I'm not sure if the above tests are going pass or not.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss merged commit f3b87ba into rust-lang:master Mar 22, 2020
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.

None yet

5 participants