Re: [RFC/PATCH v2] merge-base: teach "git merge-base" to accept more than 2 arguments

Christian Couder [off-list ref] writes:

 OPTIONS
 -------
 --all::
-	Output all common ancestors for the two commits instead of
-	just one.
+	Output all merge bases for the commits instead of just one. No
+	merge bases in the output should be an ancestor of another
+	merge base in the output. Each common ancestor of the first
+	commit and any of the other commits passed as arguments,
+	should be an ancestor of one of the merge bases in the output.

The point of merge_bases_many() is that it allows you to compute a merge
base between a commit and another commit that does not yet exist which is
a merge across all others.

               o---o---o---o---C
              /                 :
             /   o---o---o---B..(M)
            /   /                 :
	---o---*---o---o---o---A..(X)

Suppose you have commits A, B and C, and you would want to come up with an
Octopus merge X across these three commits.  Because our low-level merge
machinery works always on two trees with one common ancestor tree, we
would first create a tree that is a merge between B and C (which is marked
as (M) in the picture), and then merge the tree of (M) and A using common
ancestor between (M) and A.

If we did not have merge_bases_many(), we would actually create (M) as a
real commit, and compute merge base between A and (M), which is marked as
"*" in the picture.  The use of merge_bases_many() allows us to run the
same merge base computation without actually creating commit (M).  Instead
of computing merge-base between A and (M), you can ask for the merge base
between A ("the first commit") and B, C ("the other commits") to obtain
the same answer "*".

Base between A and B is that "*", and you are correct to say that it is an
ancestor of the "*" that is output from the command; base between A and C
is the parent of "*", and again you are correct to say it is ancestor of
the "*" that is output from the command.

But if we output any other commit between "*" and A from the command, it
still satisifies your condition.  "The merge base between A and each of B,
C,... should be an ancestor of what is output".  In order to satisify your
condition, in the extreme case, we could even output A.  Both the merge
base between A and B, and the merge base between A and C, would be an
ancestor of A.

So your description may not be incorrect, but I think it completely misses
the point of what is being computed.

This is just style, but if you must break lines somewhere, I'd prefer to
have prev2_nr and prev2 on the same line, like this:

	struct commit_list *result = get_merge_bases_many(rev1,
							  prev2_nr, prev2, 0);

because they logically belong to each other.  Further, I think this
84-column single-line statement is perfectly fine as well in this case:

	struct commit_list *result = get_merge_bases_many(rev1, prev2_nr, prev2, 0);

I would probably do this myself in this case, though:

	struct commit_list *result;

	result  = get_merge_bases_many(rev1, prev2_nr, prev2, 0);

Le dimanche 27 juillet 2008, Junio C Hamano a écrit :

The point of merge_bases_many() is that it allows you to compute a merge
base between a commit and another commit that does not yet exist which is
a merge across all others.

               o---o---o---o---C
              /                 :
             /   o---o---o---B..(M)
            /   /                 :
	---o---*---o---o---o---A..(X)

Suppose you have commits A, B and C, and you would want to come up with
an Octopus merge X across these three commits.  Because our low-level
merge machinery works always on two trees with one common ancestor tree,
we would first create a tree that is a merge between B and C (which is
marked as (M) in the picture), and then merge the tree of (M) and A using
common ancestor between (M) and A.

If we did not have merge_bases_many(), we would actually create (M) as a
real commit, and compute merge base between A and (M), which is marked as
"*" in the picture.  The use of merge_bases_many() allows us to run the
same merge base computation without actually creating commit (M). 
Instead of computing merge-base between A and (M), you can ask for the
merge base between A ("the first commit") and B, C ("the other commits")
to obtain the same answer "*".

Yeah, but what is quite confusing I think is that a merge base seems to be 
defined as "an as good as possible _common_ ancestor" but in this case, the 
result of "git merge-base A B C" is not an ancestor of C (even with --all 
option). So perhaps we need a better definition.

Base between A and B is that "*", and you are correct to say that it is
an ancestor of the "*" that is output from the command; base between A
and C is the parent of "*", and again you are correct to say it is
ancestor of the "*" that is output from the command.

But if we output any other commit between "*" and A from the command, it
still satisifies your condition.  "The merge base between A and each of
B, C,... should be an ancestor of what is output".  In order to satisify
your condition, in the extreme case, we could even output A.  Both the
merge base between A and B, and the merge base between A and C, would be
an ancestor of A.

So your description may not be incorrect, but I think it completely
misses the point of what is being computed.

quoted

 Author
 ------

diff --git a/builtin-merge-base.c b/builtin-merge-base.c
index 1cb2925..f2c9756 100644
--- a/builtin-merge-base.c
+++ b/builtin-merge-base.c

@@ -2,9 +2,11 @@
 #include "cache.h"
 #include "commit.h"

-static int show_merge_base(struct commit *rev1, struct commit *rev2,

int show_all) +static int show_merge_base(struct commit *rev1, int
prev2_nr, +			   struct commit **prev2, int show_all)
 {
-	struct commit_list *result = get_merge_bases(rev1, rev2, 0);
+	struct commit_list *result = get_merge_bases_many(rev1, prev2_nr,
+							  prev2, 0);

This is just style, but if you must break lines somewhere, I'd prefer to
have prev2_nr and prev2 on the same line, like this:

	struct commit_list *result = get_merge_bases_many(rev1,
							  prev2_nr, prev2, 0);

because they logically belong to each other.  Further, I think this
84-column single-line statement is perfectly fine as well in this case:

	struct commit_list *result = get_merge_bases_many(rev1, prev2_nr, prev2,
0);

I would probably do this myself in this case, though:

	struct commit_list *result;

	result  = get_merge_bases_many(rev1, prev2_nr, prev2, 0);

I used that in v3.

Thanks,
Christian.