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

[Christian Couder <chriscool@tuxfamily.org>]

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.
>
> >  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.