From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sat, 02 Jul 2011 11:19:20 GMT
Subject: Re: [PATCH 09/13] revert: Catch incompatible command-line options early
Message-ID: <BANLkTik2t=qPfJe7JUsoeka79ZYYRmU_MA@mail.gmail.com>
In-Reply-To: <20110702100436.GB19090@elie>

Hi again,

On Sat, Jul 2, 2011 at 3:34 PM, Jonathan Nieder <jrnieder@gmail.com> 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