Re: [PATCH 05/11] revert: Catch incompatible command-line options early
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:01
Ramkumar Ramachandra [off-list ref] writes:
+static void die_opt_incompatible(const char *me, const char *base_opt,
+ int nargs, int opt_bitarray[], ...)
+{This feels way overkill to use vararg. This may be _one_ way to do what you want to do, and while I don't think it is the best way, it may be Ok. "opt_bitarray[]" is a horrible name, though. It does not convey what the variable is about (its "purpose"). It doesn't even correctly describe the way that unspecified purpose happens to be implemented (claims to be bitarray but it is just an array of ints and what matters is if any bit is set in the array).
quoted hunk ↗ jump to hunk
static void parse_args(int argc, const char **argv) { const char * const * usage_str = revert_or_cherry_pick_usage();@@ -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.