Skip to content

Exposed git_merge_base as CommitCollection.GetMergeBase(...) #152

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

Closed
wants to merge 1 commit into from
Closed

Conversation

redoz
Copy link
Contributor

@redoz redoz commented May 16, 2012

Take two, now that @nulltoken updated the libgit2 version.

Contains three overloads:

Commit GetMergeBase(Commit first, Commit second);
Commit GetMergeBase(IEnumerable<Commit> commits);
Commit GetMergeBase(Commit first, Commit second, params Commit[] rest);

/// <param name="first">The first <see cref="Commit"/> for which to find the common ancestor.</param>
/// <param name="second">The second <see cref="Commit"/> for which to find the common ancestor.</param>
/// <returns>The common ancestor or null if none found.</returns>
public Commit GetMergeBase(Commit first, Commit second)
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to CommonAncestorOf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you compromise on FindCommonAncestor?

@nulltoken
Copy link
Member

@redoz Very neat job!

In order to properly cover the whole functionality, could you add some tests demonstrating the following

  • non common ancestor can be found
  • passing wrong parameters throws

One last point, could you please squash the commits altogether?

@redoz
Copy link
Contributor Author

redoz commented May 18, 2012

@nulltoken

@redoz Very neat job!

Thanks, I'm glad you approve.

One last point, could you please squash the commits altogether?

I might need some guidance on this one but I'll try to figure it out once I've fixed the issues

@nulltoken
Copy link
Member

I might need some guidance on this one but I'll try to figure it out once I've fixed the issues

There's a very nice post from @schacon "Reset Demystified". The whole is a must-read, but if you want to cut corners, take a look at the "A fun example" section which describes this in detail.

@redoz
Copy link
Contributor Author

redoz commented May 18, 2012

@nulltoken
I've attempted to squash the previous three commits into one, hopefully it all went ok.

I will try to get the code changes in as soon as possible (probably sunday or monday).

@redoz
Copy link
Contributor Author

redoz commented May 20, 2012

@nulltoken I think I've addressed all issues, I took the liberty to rename it from "GetMergeBase" to "FindCommonAncestor" instead of "CommonAncestorOf" (I can rename it again if you prefer).

I also improved the error handling and added a few more tests to test all the error conditions, you might want to check the naming on those tests to ensure they adhere to the naming convention.

@nulltoken
Copy link
Member

@redoz I manually merged this in vNext (cf. bf88875) and fixed some minor style related issues.

Thanks a lot for this!

@nulltoken nulltoken closed this May 20, 2012
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.

2 participants