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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2008, 04:47 UTC
Message-ID: <7vabg43pcp.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080727053324.b54fe48e.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 10 quoted lines

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

Show 17 quoted lines

>  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);