Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Again, "In later steps, a new API that takes a single commit and replays it in forward (cherry-pick) or backward (revert) direction will be introduced, and will take this structure as a parameter to tell it what to do" is missing from the above description.

More importantly, the primary purpose of these variables is _not_ "to parse command line options into". It is to actively affect what happens in the code, and "parse command line" is merely a way to assign the initial values to them. So I'd rather see this patch described perhaps like this:

    cherry-pick/revert: introduce "struct replay_options"

    The current code uses a set of file-scope static variables to instruct
    the cherry-pick/revert machinery how to replay the changes, and
    initialises them by parsing the command line arguments. In later steps
    in this series, we would like to introduce an API function that calls
    into this machinery directly and have a way to tell it what to do.

    Introduce a structure to group these variables, so that the API can
    take them as a single "replay_options" parameter.

I strongly prefer to see this patch also update the callchain to pass a pointer to the options struct as parameter. I can guess without reading the rest the series that at some later step you would do that, but I think it makes more sense to do the conversion at this step, as you will be touching lines that use the global variables in this patch anyway, like this:

- return action == REVERT ? revert_usage : cherry_pick_usage; + return cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;

which should eventually become something like:

	return opts->usage_message;

at the very last step when this codepath is _fully_ libified (I don't know if the final result of your series still is a "choose only from these two canned messages" API, or gives the ability to the caller to specify the usage message---I am just showing how it should look like at the end of a full libification). So it would make sense to see:

	return opts->action == REVERT ? revert_usage : cherry_pick_usage;

in this patch.

It is probably a remnant of the earlier patches in this series, but this should start with:

	static int error_dirty_index(void)

Of course, you will actually be passing the options structure, so it would become:

	static int error_dirty_index(struct replay_options *opts)
        {
        	...
                if (opts->action == REVERT)
                	...
	}