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

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

Hi,

On Sun, 27 Jul 2008, Christian Couder wrote:

quoted

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.

quoted

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

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