Hi Junio,
Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> > @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
> > if (cmd_opts.commit_argc < 2)
> > usage_with_options(usage_str, options);
> >
> > + if (cmd_opts.allow_ff) {
> > + int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
> > + cmd_opts.no_replay, cmd_opts.edit};
> > + die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
> > + "--no-commit", "-x", "--edit");
> > + }
>
> Why not do it like this instead?
>
> struct incompatible {
> unsigned option_bit;
> const char *option_name;
> } incompatible[] = {
> { opts->signoff, "--signoff" },
> { opts->no_commit, "--no-commit" },
> ...
> };
> verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));
>
> Or if you are shooting for ease-of-use, it might make sense to do it like
> this:
>
> verify_compatible("me", "--ff",
> "--signoff", opts->signoff,
> "--no-commit", opts->no_commit,
> ...
> NULL);
>
> and make verify_compatible() a varargs function that takes two optional
> arguments at a time, i.e. const char *, followed by an int. Then there is
> no need for extra "int opt_bitarray[]" or "struct incompatible".
>
> That would justify use of varargs, I think.
Now that you point it out, my original approach was unnecessarily
cryptic and convoluted. I've followed this approach in my new series,
and kept varargs -- the code looks much prettier now. Thanks :)
I can't justify changing the name from "die_opt_incompatible" to
"verify_compatible" though; the name I've chosen seems to be more
appropriate/ descriptive. Further, I think command-line parsing
should always be the toplevel caller, and "die" is therefore more
appropriate than "return error" here.
-- Ram
--
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