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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> 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