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

Hi,

On Wed, 30 Jul 2008, Christian Couder wrote:

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

quoted

On Mon, 28 Jul 2008, Christian Couder wrote:

quoted

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

quoted

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