Hi Junio,
Junio C Hamano writes:
> 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.
Right. Thanks for the nice commit message :)
> 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:
Good idea. I've passed the opts pointer around in the new series.
> > @@ -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)
> ...
> }
The very first patch removes this function (and puts the functionality
elsewhere) in the new series, so this comment doesn't apply.
Thanks for the review!
-- 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