From: Christian Couder <chriscool@tuxfamily.org>
Date: Wed, 30 Jul 2008 04:52:41 GMT
Subject: Re: [RFC/PATCH v3] merge-base: teach "git merge-base" to accept more than 2 arguments
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 :
> 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.