Hi again,
On Sat, Jul 2, 2011 at 3:34 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
Thanks! I wish I could write commit messages like this. I hope I've
got it right this time:
revert: Don't create invalid replay_opts in parse_args
The "--ff" command-line option cannot be used with four other
command-line options. However, when these options are specified with
"--ff" on the command-line, parse_args will still parse these
incompatible options into a replay_opts structure for use by the rest
of the program. Although pick_commits checks the validity of the
replay_opts strucutre before before starting its operation, this is
fragile design because the validity of the replay_opts structure
depends on pick_commits being called before anything else. Change
this so that an invalid replay_opts structure is not created by
parse_args in the first place, and make sure regressions in
maintaining this invariant are caught in the future by adding an
assertion in pick_commits.
And yes, adding an "assert" statement in pick_commits does seem like a
great reminder for the future :)
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html