Added FindMergeBases on the ObjectDatabase class, equivalent to git merge-base --all.

[otac0n]

This should fix #1147.

[otac0n]

Oh, I had trouble getting my new test to run in NCrunch, but it ran fine in the VS Test Runner.

NCrunch wasn't copying all of the files that are specified in the test project's Additional Files to Include.

I'm not sure if this is typical, but since it worked fine in VS, I decided it was worth submitting.

[nulltoken]

/cc'ing @Zoltu as he made the NCrunch magic happened

Any hint?

[MicahZoltu]

I'll clone the repo and see if I can reproduce the nCrunch failure and/or give any hints at a fix.

[MicahZoltu]

Works for me, all tests pass except the one ignored one. Here are my steps:

1. Clone the branch otac0n:vNext.
2. Open solution in VS2013.
3. Build all.
4. Enable nCrunch v2.15.0.9

Perhaps try:

1. Close Visual Studio
2. git clean -dfx (this will reset you back to a freshly-cloned state)
3. Launch Visual Studio
4. Build
5. Enable nCrunch.

[otac0n]

I re-cloned and everything worked fine. Must have been a bug in NCrunch.

So, how soon can this go into a pre-release package? I need this feature in my projects.

[nulltoken]

Can you please change the return type into an IEnumerable<Commit>?

[otac0n]

Would IList<Commit> be acceptable? I feel like losing the Count when it is readily available is a bad idea.

[nulltoken]

As long as we're internally using a IList<> (ie. the ToList() call), returning an IEnumerable<> will not enumerate the wole collection and directly leverage the .Count property of the returned collection.

So returning IEnumerable<> shouldn't hurt on that topic.

[otac0n]

But IEnumerable<> doesn't have a Count property, and I feel like people will want it.

[MicahZoltu]

There is an extension method Count() on IEnumerable so you can count it if you need to without returning a list. If it is backed by a list, then count will just return the value of the Count property. That being said, the user should assume that counting will result in enumeration and act accordingly (possibly calling ToList() first to avoid multiple enumeration.

That being said, ICollection is probably what you really want if you just want the ability to count (but not index into the list). So really, the discussion is between ICollection and IEnumerable. Not that I have any say in anything, but I am with @nulltoken on using IEnumerable unless there is a strong belief that the end-user will need to call count in a performant way. I don't know this area of code or how big the resulting enumerable will be but is anyone really going to care about the cost of a ToList or an enumeration? If not, then returning IEnumerable makes it more clear to the user that the thing they are getting is not something they should modify, it is something they can iterate over (possibly getting the count via iteration).

[nulltoken]

Currently, IEnumerable<> is used when returning a collection of things, API wide. I'd rather stick with this principle.

[nulltoken]

Could you please revert this? ^^

[otac0n]

I made it align better; was it intentionally misaligned?

Or would you just rather not have that change in this pull request?

[nulltoken]

Was it intentionally misaligned?

I don't think it was 😉

Or would you just rather not have that change in this pull request?

We tend to prefer beautification changes to live in their own commits. If you're ok with extracting it in a different commit, it'd be glorious.

[nulltoken]

@otac0n In the original SO question you were using a command line that was both using --all and --octopus. Would you know an example where using those combined switches would return a different result than --all alone?

The rationale behind my question is that by reviewing your PR, I've started to wonder if your method shouldn't actually be favored with regards to the current FindMergeBase() implementations. Would that make sense, we would obsolete (in a different PR) the other ones and modify the signature of this one to maybe accept a FindMergeBasesOptions parameter. However, that would require some additional test coverage to ensure that valid combinations of options return sensible results.

/cc @carlosmn

[otac0n]

@nulltoken I don't know when an --octopus and an --all --octopus would behave differently. After the comment @carlosmn made on #1147, I assumed that --all --octopus was a redundant configuration.

However, looking over the docs a bit further, it seems that this is an explicitly called-out case (in the synopsis). I'll do a bit more research tonight, to see what difference (if any) exists between the two configurations.

[otac0n]

@nulltoken @carlosmn It looks like libgit2 lacks the ability to specify --all --octopus.

See:
https://github.com/libgit2/libgit2/blob/master/src/merge.c#L179
https://github.com/git/git/blob/master/builtin/merge-base.c#L71

[nulltoken]

@peff Is there a case where git merge-base --all --octopus returns more than one revision?

The doc is a bit confusing regarding this. It looks like it's valid to combine the two switches, but

I'm not sure to understand what's the expected result

**--octopus**
  Compute the best common ancestors of all supplied commits, in preparation for an n-way merge.
**--all**
  Output all merge bases for the commits, instead of just one.

Would we get only one? Should we get all the best ones? Or the best one in first position and all the others merge bases after that?

[peff]

@nulltoken You generally get multiple merge bases as a result of criss-cross merges. So I think you could have an octopus with at least one pair-wise criss-cross, and as long as those commits work as a base for the other parts of the octopus, you still get multiple bases. For example:

# setup and helpers
commit() {
    echo "$1" >"$1.t" &&
    git add "$1.t" &&
    git commit -m "$1" &&
    git tag "$1"
}
export GIT_EDITOR=:
git init repo && cd repo
commit base

# two branches, each with two commits
git checkout -b a base
commit a1
commit a2
git checkout -b b base
commit b1
commit b2

# each merges from the history of the other
git checkout a
git merge b1
git checkout b
git merge a1

# and now we have a criss-cross merge, with multiple bases; the
# first commands returns a single base (picked arbitrarily), but the
# latter shows two bases (a1 and b1).
git merge-base a b | git name-rev --stdin
git merge-base --all a b | git name-rev --stdin

# now let's add in a third branch; we have to walk all the way back
# to the base for this one, since our new branch doesn't include
# the criss-cross
git checkout -b c base
commit c1
commit c2
git merge-base --all --octopus a b c | git name-rev --stdin

# but now let's merge in our criss-cross bases; that makes them
# suitable merge bases, and therefore we're back up to having two
# merge bases
git merge a1
git merge b1
git merge-base --all --octopus a b c | git name-rev --stdin

# and if we want to make it even crazier, we can do criss-cross all of
# the branches together, and get three bases!
git checkout a
git merge c1
git checkout b
git merge c1
git merge-base --all --octopus a b c | git name-rev --stdin