Re: [PATCH 09/13] revert: Catch incompatible command-line options early

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 2, 2011, 10:04 UTC
Message-ID: <20110702100436.GB19090@elie>
In-Reply-To: <20110702095334.GA19090@elie>

Jonathan Nieder wrote:

> Ramkumar Ramachandra wrote:

>>     Some incompatible command-line options are caught in pick_commits
>>     after parse_args has parsed the options and populated the options
>>     structure.  Change this so that parse_args only parses valid
>>     command-line options

[...]

> Alas, I still don't get it.  How can I (the fearful reader) demonstrate
> the problem that this fixes?  Is it a big problem?  How does the patch
> fix it?  Does the patch have downsides, or is it mostly risk-free?

To be clearer, here's a quick example in some hypothetical alternate world:

	cherry-pick/revert: do not create a nonsense "struct opts" when given nonsense argv

	The --foo and --bar options make no sense together, but if I
	run "git cherry-pick --foo --bar" then parse_cherry_pick_option
	will return a nonsense struct with both "fooing" and "barring"
	set to true, which makes no sense.  Currently it's not a
	problem since the code that cares is protected by a check:

		if (opts.fooing && opts.barring)
			die("cannot foo and bar at the same time");

	But that is very fragile --- the frob() code-path relies on
	opts.fooing and opts.barring not both being true without such
	a check and it's only a coincidence that futz() is called
	first and makes that check, protecting it.

	We can make sure such a nonsensical options struct is never
	created by checking right away that --foo and --bar are not
	passed together at option-parsing time.  Meanwhile, make sure
	regressions in maintaining this invariant are caught in the
	future by adding an assertion "assert(!opts->fooing ||
	!opts->barring)" to both frob() and futz().

	The discussion above also applies to --bar and --qux.  Fix
	that, too.