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

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

>  I get the following warning from GCC: warning: useless storage class
>  specifier in empty declaration (at the line where I've declared the
>  replay_opts struct).  What is the correct way to fix this?

Remove the useless storage class specifier ("static"). :)

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {
[...]
> +static struct replay_opts {
> +	enum { REVERT, CHERRY_PICK } action;
> +
> +	/* Boolean options */
> +	int edit;
> +	int no_replay;

replay but no replay?

I think originally git-revert.sh had a "replay" variable meaning "This
is not a revert (which undoes a commit) but a cherry-pick (which
re-does it)."  Later the purpose changed to "We are not cherry-picking
and referring to the original with cherry-pick -x but replaying a
commit and treating it as new".

Now with struct replay_opts you are proposing to make the term mean
"we are using git revert machinery, or in other words replaying the
change an old commit made (forwards or backwards)", which makes sense.
In this case there should probably be a patch right before which
renames no_replay to i_really_want_to_expose_my_private_commit_object_name
(um, I mean to record_origin or something similar).

> +	int no_commit;
> +	int signoff;
> +	int allow_ff;
> +	int allow_rerere_auto;
> +
> +	int mainline;
> +	int commit_argc;
> +	const char **commit_argv;
> +
> +	/* Merge strategy */
> +	const char *strategy;
> +	const char **xopts;
> +	size_t xopts_nr, xopts_alloc;
> +};
[...]
>  
> -static const char * const *revert_or_cherry_pick_usage(void)
> +static const char *const *revert_or_cherry_pick_usage(struct replay_opts *opts)

Line is getting long.  Whitespace change snuck in?

I suppose if I ran the world the argument would be of type "enum
replay_action", so it would be used as

	usage(revert_or_cherry_pick_usage(o->action));

> +/* For option_parse_x */
> +static const char **xopts;
> +static size_t xopts_nr, xopts_alloc;
> +

Hm.  In C89, struct initializers are not allowed to include addresses
that are not known until run-time, and we used to follow that and now
violate it all over the place.  I'm not sure if it's worth it or not.
(I'm tempted to say, let it deteriorate further and people with the
ability to test on such platforms can fix it, but commits like
v1.7.2-rc0~32^2~18, Rewrite dynamic structure initializations to
runtime assignment, 2010-05-14, suggest that some people have cared in
the recent future.)

So.

If you want to use parse_options and support such compilers, it is
indeed simplest to use static variables.  You can give them scope
local to a particular function to at least avoid namespace polution.

To avoid such static variables at the expense of support for old
compilers, one can pass a pointer to a struct to option_parse_x
instead of the dummy &xopts.  Within option_parse_x, what you pass
will be accessible as opt->value.  It's all explained in
Documentation/technical/api-parse-options.txt, or one can grep around
for OPT_CALLBACK for examples.  A simple variant on this will work
with the old compilers, too.

[...]
>  static int option_parse_x(const struct option *opt,
> -			  const char *arg, int unset)
> +			const char *arg, int unset)

Whitespace change snuck in.

[...]
> @@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,
>  	return 0;
>  }
>  
> -static void parse_args(int argc, const char **argv)
> +static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  {
> -	const char * const * usage_str = revert_or_cherry_pick_usage();
> +	const char *const *usage_str = revert_or_cherry_pick_usage(opts);
>  	int noop;
>  	struct option options[] = {
> -		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
> +		OPT_BOOLEAN('n', "no-commit", &(opts->no_commit), "don't automatically commit"),

The parentheses are not needed (and not idiomatic fwiw).  The line is
getting long so I'd suggest splitting it, though that's more a matter
of taste.

> @@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)
[...]
> -	if (commit_argc < 2)
> +
> +	/* Fill in the opts struct from values set by option_parse_x */
> +	opts->xopts = xopts;
> +	opts->xopts_nr = xopts_nr;
> +	opts->xopts_alloc = xopts_alloc;

Yep, something like this is needed (for all the options) if we want to
follow C89's option-struct-initialization rules.

>  static int do_recursive_merge(struct commit *base, struct commit *next,
> -			      const char *base_label, const char *next_label,
> -			      unsigned char *head, struct strbuf *msgbuf)
> +			const char *base_label, const char *next_label,
> +			unsigned char *head, struct strbuf *msgbuf,
> +			struct replay_opts *opts)

I'm not going to point out whitespace changes that snuck in any more.

I think I prefer the options struct to go in front (as in the
merge-recursive and diff APIs), but this is only a matter of taste.

> @@ -311,15 +329,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
>  }
>  
>  {
>  	struct merge_options o;
>  	struct tree *result, *next_tree, *base_tree, *head_tree;
>  	int clean, index_fd;
>  	const char **xopt;
>  	static struct lock_file index_lock;
> -	const char *me = (action == REVERT ? "revert" : "cherry-pick");

I think this belongs in a different patch (and likewise for its
counterpart below).

> The current code uses a set of file-scope static variables to tell the
> cherry-pick/ revert machinery how to replay the changes, and
> initializes 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.  Hence, introduce a structure to group these variables, so that
> the API can take them as a single "replay_options" parameter.

Stepping back, I think this is a good idea, to make the state being
passed around a little clearer and to make it easier for callers to
specify what they want to happen without making up fictitious argc and
argv.  Most of what remains for this to be cooked are minor things
(the biggest part is getting it to build with -std=c89 -pedantic if
wanted and teaching option_parse_x to use a callback parameter).

Thanks.