Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c

Miklos Vajna <vmiklos@frugalware.org> writes:

In what sense is this "like show-branch --merge-base"?

The only similarlity I can spot is that it can take more than two heads, but what it computes and the way it computes it seem to be different. It certainly looks much less efficient as it does not walk the ancestry chain in one-go like show-branch does.

I haven't looked at [10/10] so I do not know yet how you intend to use this value, but I am not sure if it makes sense to use the computed value as the merge base when running an octopus.

When you want to merge A, B and C, because internally we always do pairwise, you would first compute:

	T(A,B) == merge_3way(mb(A,B), A, B)

and then you would want to pick some merge base to merge that result with C:

	T(A,B,C) == merge_3way(???, T(A,B), C)

I do not think using mb(mb(A,B),C) as ??? is necessary nor optimal.

          o---o---A
         /         .
    ----1---2---B...T(A,B)
             \       .
              o---C...T(A,B,C)

"1" above is mb(A,B) (i.e. the merge base between A and B) that you would use when you merge A and B to compute an internal merge result T(A,B). And "1" also is mb(mb(A,B),C).

But if you are doing 3-way merge to arrive at T(A,B,C) by merging T(A,B) and C, don't you want to be using "2" as the merge base, not "1"?

The big comment on $MRC at the end of git-merge-octopus is about this issue. In earlier implementation, we used to use "1" to create T(A,B) and then also used it to update the variable $MRC, which is used to compute the merge base to use when the tentative tree T(A,B) is merged with the other remote in the next round. It was a mistake.

Ideally we should pick a better base between mb(A,C) and mb(B,C) (and if we used mb(B,C) that's "2" and is a much better base than "1" when merging T(A,B) and C). Using mb(mb(A,B),C) means we are guaranteed to pick the worst base for that last merge.