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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 27 Jul 2008, Christian Couder wrote:

> 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
> @@ -38,15 +48,22 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
>  			usage(merge_base_usage);
>  		argc--; argv++;
>  	}
> -	if (argc != 3)
> +	if (argc < 3)
>  		usage(merge_base_usage);
> -	if (get_sha1(argv[1], rev1key))
> -		die("Not a valid object name %s", argv[1]);
> -	if (get_sha1(argv[2], rev2key))
> -		die("Not a valid object name %s", argv[2]);
> -	rev1 = lookup_commit_reference(rev1key);
> -	rev2 = lookup_commit_reference(rev2key);
> -	if (!rev1 || !rev2)
> +
> +	rev1 = get_commit_reference(argv[1]);
> +	if (!rev1)
>  		return 1;

Why do you special case rev1?  Is it so special?  Just handle it together 
with all of the other arguments!

IOW have one commit array, and do not call it "prev".

> -	return show_merge_base(rev1, rev2, show_all);
> +	argc--; argv++;
> +
> +	do {
> +		struct commit *rev2 = get_commit_reference(argv[1]);
> +		if (!rev2)
> +			return 1;
> +		ALLOC_GROW(prev2, prev2_nr + 1, prev2_alloc);
> +		prev2[prev2_nr++] = rev2;
> +		argc--; argv++;
> +	} while (argc > 1);

Now, this is ugly.  You know beforehand the _exact_ number of arguments, 
and yet you dynamically grow the array?  Also, why do you use a do { } 
while(), when a for () would be much, much clearer?

BTW I seem to recall that get_merge_bases_many() was _not_ the same as 
get_merge_octopus().  Could you please remind me what _many() does?

Ciao,
Dscho