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

Hi again,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

 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"). :)

Ah, thanks :)

quoted

--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {

[...]

quoted

+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).

Great suggestion: one more patch changing "no_replay" to
"record_origin" it is.

quoted

+	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;
+};

[...]

quoted

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

In my defense, I thought whitespace (indentation, style) changes were
permitted as long as I'm making a functional change.  If this isn't
the case, when can I correct the style/ indentation?

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

quoted

+/* 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.

Got it.

[...]

quoted

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

Whitespace change snuck in.

Intended.  It changes indentation style to linux-tabs-only, which is
the style my editor currently works with.

[...]

quoted

@@ -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.

Ok, I'll lose the paranthesis.

quoted

@@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)

[...]

quoted

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

Ouch! That's much too painful :|
I think I'll break the rule for the moment.

quoted

 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.

Intended again, since I'm adding an argument to the list.

quoted

@@ -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).

quoted

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

Right, thanks.

-- Ram