From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sun, 08 May 2011 11:47:51 GMT
Subject: Re: [PATCH 05/11] revert: Catch incompatible command-line options early
Message-ID: <20110508114749.GA3114@ramkum.desktop.amazon.com>
In-Reply-To: <7vfwpotqdx.fsf@alter.siamese.dyndns.org>

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> 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