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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Jul 28, 2008, 05:49 UTC
Message-ID: <200807280749.35278.chriscool@tuxfamily.org>
In-Reply-To: <alpine.DEB.1.00.0807271631470.5526@eeepc-johanness>

Le dimanche 27 juillet 2008, Johannes Schindelin a écrit :

Show 30 quoted lines

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

Ok, I have done that in v3.

Show 14 quoted lines

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

You are right. I guess I dealt too much with complex parsing of arguments where you can have options everywhere, and perhaps I also felt somewhat guilty for not having used ALLOC_GROW before, or something.

> Also, why do you use a do { } 
> while(), when a for () would be much, much clearer?

Well, we already know that there are at least 2 commits to parse, so it feels a little bit wastefull to check first again. Also the code to parse the [--all|-a] option before use a while loop with "argc--; argv++;" at the end, so I think it is more coherent like that.

Thanks, Christian.

Show 5 quoted lines

> 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