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

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> 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).

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