Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> 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.  Hence, introduce a structure to group these variables, so that
>> the API can take them as a single replay_options parameter.
>>
>> The variable "me" is left as a file-scope static variable because it
>> is not an independent option.  "me" is simply a string that needs to
>> be inferred from the "action" option, and is kept global to save each
>> function the trouble of determining it independently.
>
> Hm, would it make sense for there to be a "private" section at the
> end of the replay_opts struct for variables like this?

No.  My justification: in later steps, we'd want to be able to mix
"pick" and "revert" instructions in the same instruction sheet.  This
will essentially require the parser to return a commit + a replay_opts
struct (which will contain the action information).  There's little
point in storing 100 "revert" strings for the 100 commits we want to
pick when that can easily be inferred from the action.

>> Unfortunately, this patch introduces a minor regression.  Parsing
>> strategy-option violates a C89 rule: Initializers cannot refer to
>> variables whose address is not known at compile time.  Currently, this
>> rule is violated by some other parts of Git as well, and it is
>> possible to get GCC to report these instances using the "-std=c89
>> -pedantic" option.
>
> I would be interested in fixing that (as a patch on top, maybe).
> What do you suggest:
>
>  A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
>    manipulate a static "struct replay_opts" while pick_commits et al
>    pass around a pointer to it
>
>  B. Make parse_args work like this:
>
>        copy from argument to private static struct replay_opts
>        call parse_options()
>        copy private static struct replay_opts to argument

This is something you suggested earlier, but I find it extremely inelegant.

>  C. Use new option types:
>
>        OPT_BOOL_MEMBER('n', "no-commit",
>                offsetof(struct replay_opts, no_commit),
>                "don't automatically commit"),
>
>    and teach parse_options to take an additional parameter like it
>    takes "prefix" now, to be used as a base address for options that
>    write to an offset instead of a pointer
>
> I'm leaning towards A but not sure if that would be wasted work in
> light of your plans for these APIs in the long run (i.e., is
> parse_args() going to be exposed and want to act on a caller-supplied
> struct)?

Yes, I'm definitely considering exposing parse_args in the future,
especially since I want to support command-line options in my
instruction sheet.  Implementing (C) correctly will probably have
several long-term benefits as well -- what do you feel about it?

-- Ram