Re: [PATCH 05/11] revert: Catch incompatible command-line options early

Hi Junio,

Junio C Hamano writes:

Ramkumar Ramachandra [off-list ref] writes:

quoted

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