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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Jul 30, 2008, 04:52 UTC
Message-ID: <200807300652.41404.chriscool@tuxfamily.org>
In-Reply-To: <alpine.DEB.1.00.0807281328520.2725@eeepc-johanness>

Hi,

Le lundi 28 juillet 2008, Johannes Schindelin a écrit :

Show 29 quoted lines

> Hi,
>
> On Mon, 28 Jul 2008, Christian Couder wrote:
> > +	rev = xmalloc((argc - 1) * sizeof(*rev));
> > +
> > +	do {
> > +		struct commit *r = get_commit_reference(argv[1]);
> > +		if (!r)
> > +			return 1;
> > +		rev[rev_nr++] = r;
> > +		argc--; argv++;
> > +	} while (argc > 1);
> > +
> > +	return show_merge_base(rev, rev_nr, show_all);
>
> 	rev = xmalloc((argc - 1) * sizeof(*rev));
>
> 	for (rev_nr = 0; rev_nr + 1 < argc; rev_nr++) {
> 		rev[rev_nr] = get_commit_reference(argv[rev_nr + 1]);
> 		if (!rev[rev_nr])
> 			return !!error("Does not refer to a commit: '%s'",
> 				argv[rev_nr + 1]);
> 	}
>
> 	return show_merge_base(rev, rev_nr, show_all);
>
> I do not know about you, but I think this is not only shorter (in spite
> of adding a helpful error message), but also simpler to understand (not
> using convoluted do { } while logic), and therefore superior.

In my last version the loop is reduced to:

+ do { + rev[rev_nr++] = get_commit_reference(argv[1]); + argc--; argv++; + } while (argc > 1);

so it's very simple.

And the stop condition is simpler in my version.

> Your performance argument is weak IMHO, as this is not a big performance
> hit, and command line parameter parsing is definitely not performance
> critical.

It feels a bit sloppy though.

Regards, Christian.