Le dimanche 27 juillet 2008, Johannes Schindelin a écrit :
> Hi,
>
> On Sun, 27 Jul 2008, Christian Couder wrote:
> > 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.
> > - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
