Skip to content

Expose merge analysis#1305

Open
carlosmn wants to merge 4 commits intomasterfrom
cmn/merge-analysis
Open

Expose merge analysis#1305
carlosmn wants to merge 4 commits intomasterfrom
cmn/merge-analysis

Conversation

@carlosmn
Copy link
Copy Markdown
Member

This allows us to ask the library about what kind of actions
are available for merging into HEAD.


I'm still not quite sure what the overloads should be. Maybe leave as-is and think about exposing annotated commits as a type.

@ethomson
Copy link
Copy Markdown
Member

Yeah, I worry about having these two overloads. Because what happens when you want to octopus merge two commits and a ref? (Well, this is entirely theoretical, since libgit2 doesn't even do octopus merging, but you know.)

I wonder if Commit and Reference should implement IMergeable or something?

@carlosmn
Copy link
Copy Markdown
Member Author

You can peel the Reference to a commit, that shouldn't be much of an issue. They could implement IAnnotatable or something since what we actually want is an annotated commit and these are just convenience methods for that.

@ethomson
Copy link
Copy Markdown
Member

You can peel the Reference to a commit, that shouldn't be much of an issue.

Sure - I guess my thinking here is twofold: first, it would be nice if we didn't make users do the peeling. :) And second, Merge will behave differently when handed a commit versus an actual branch. Merge analysis, of course, will not but I'm wondering if we can make sure these APIs are nice and similar. At the moment we have:

    MergeResult Merge(Commit commit, Signature merger, MergeOptions options);
    MergeResult Merge(Branch branch, Signature merger, MergeOptions options);
    MergeResult Merge(string committish, Signature merger, MergeOptions options);

And it would definitely be nice to get rid of these overloads and have a single Merge that takes the same type of commits/references/etc that the analysis method takes...

@carlosmn
Copy link
Copy Markdown
Member Author

Sure, we can accept a IAnnotateCommit that knows how to get the actual handle we want from a branch/ref/revparse. It's a shame you can't implement it for string but we can have two overloads, one for rev-parse strings and one for objects.

@carlosmn
Copy link
Copy Markdown
Member Author

Updated with more interfaces, yay. The CI error seems to come from us assuming interfaces are only ever there for mocking, which seems like a bad idea.

This allows us to ask the library about what kind of actions
are available for merging into HEAD.
These are what we really use for merges and rebases, so expose them so
we can accept these instead of having multiple variations for the user
to guess.
Instead of the multiple overloads, accept IAnnotatedCommit so the user
can provide commits or references.
@carlosmn carlosmn force-pushed the cmn/merge-analysis branch from 4e99f6d to 0b61819 Compare April 26, 2016 08:42
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