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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 30, 2008, 13:51 UTC
Message-ID: <alpine.LSU.1.00.0807301546360.3486@wbgn129.biozentrum.uni-wuerzburg.de>
In-Reply-To: <200807300652.41404.chriscool@tuxfamily.org>

Hi,

On Wed, 30 Jul 2008, Christian Couder wrote:

Show 40 quoted lines

> Le lundi 28 juillet 2008, Johannes Schindelin a écrit :
>
> > 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.

Does not matter. The logic is convoluted, i.e. not how (most) humans think. And you cheat by putting the argc and argv statements in one line.

And you do not use argc and argv after the loop, so those statements are pointless.

And you cheat by not needing that die() anymore because you posted a (good!) patch _after_ I sent my version.

So it could e as short as this:

 	while (++rev_nr < argc)
 		rev[rev_nr - 1] = get_commit_reference(argv[rev_nr]);

I would contend that the for() version is more readable.

Show 5 quoted lines

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

Sure.

If you think that obviousness in a command line parsing is less important than performance.

Ciao, Dscho