Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
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.
> @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
> static int error_dirty_index()
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)
...
}
--
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