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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 6, 2011, 09:09 UTC
Message-ID: <20110706090915.GD15682@elie>
In-Reply-To: <1309938868-2028-8-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 10 quoted lines

> 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?

Show 6 quoted lines

> 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

 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)?