Introduce Repository.CherryPick. #756
Conversation
|
I've only ported 3 of the tests to be sure this is the proper approach. I'd rather have this merged as-is and tests be improved on the way, since I'm totally out of time for emulating a new repo and adding tests. |
| public CheckoutNotifyHandler OnCheckoutNotify { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Commit the revert if the revert is successful. |
There was a problem hiding this comment.
Update comment for cherry-pick (instead of revert)?
|
@nulltoken fixed the passing of null. I'd rather let libgit2 handle FastForward first, then implement cherry-pick fast forward. |
| @@ -148,4 +149,4 @@ | |||
| <Target Name="AfterBuild"> | |||
| </Target> | |||
| --> | |||
| </Project> | |||
| </Project> | |||
There was a problem hiding this comment.
Could you please revert the removal of this trailing newline?
There was a problem hiding this comment.
That was automagically added by VS. I can remove it.
|
@jamill Any additional remarks? |
|
Thanks! Could you please rebase on top of latest vNext? |
| /// <returns>The result of the cherry pick.</returns> | ||
| public CherryPickResult CherryPick(Commit commit, Signature committer, CherryPickOptions options = null) | ||
| { | ||
| Ensure.ArgumentNotNull(commit, "commit"); |
There was a problem hiding this comment.
I might Ensure that committer is not null here. We also appear to be missing this check in Commit, but I think it would eventually get caught in ObjectDatabase.CreateCommit
|
I think this looks pretty good - thanks! My only concern are the tests - which I see is being tracked by #757. |
|
💥 |
Okay so, this is pretty weird, but from the looks of it, CherryPick has lots of copied stuff from RevertOptions and GitRevertOpts (basically a 1:1 mirror.)
Also, the unit test are not yet complete. Imo, they should include almost the same tests as RevertFixture. But using MergeRepo as the test repo since it is already branched.