parse-options: prefer opt->value to globals in callbacks

We have several parse-options callbacks that ignore their "opt"
parameters entirely. This is a little unusual, as we'd normally put the
result of the parsing into opt->value. In the case of these callbacks,
though, they directly manipulate global variables instead (and in
most cases the caller sets opt->value to NULL in the OPT_CALLBACK
declaration).

The immediate symptom we'd like to deal with is that the unused "opt"
variables trigger -Wunused-parameter. But how to fix that is debatable.
One option is to annotate them with UNUSED. But another is to have the
caller pass in the appropriate variable via opt->value, and use it. That
has the benefit of making the callbacks reusable (in theory at least),
and makes it clear from the OPT_CALLBACK declaration which variables
will be affected (doubly so for the cases in builtin/fast-export.c,
where we do set opt->value, but it is completely ignored!).

The slight downside is that we lose type safety, since they're now
passing through void pointers.

I went with the "just use them" approach here. The loss of type safety
is unfortunate, but that is already an issue with most of the other
callbacks. If we want to try to address that, we should do so more
consistently (and this patch would prepare these callbacks for whatever
we choose to do there).

Note that in the cases in builtin/fast-export.c, we are passing
anonymous enums. We'll have to give them names so that we can declare
the appropriate pointer type within the callbacks.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 526c210bcb1d271b9e2a5c35b4f6c1c78f0a18bd..3b68b47615302473cf3e2c5c70f9ebf1c707c3af 100644 (file)
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {
 static int option_parse_stage(const struct option *opt,
                              const char *arg, int unset)
 {
+       int *stage = opt->value;
+
        BUG_ON_OPT_NEG(unset);
 
        if (!strcmp(arg, "all")) {
-               checkout_stage = CHECKOUT_ALL;
+               *stage = CHECKOUT_ALL;
        } else {
                int ch = arg[0];
                if ('1' <= ch && ch <= '3')
-                       checkout_stage = arg[0] - '0';
+                       *stage = arg[0] - '0';
                else
                        die(_("stage should be between 1 and 3 or all"));
        }
@@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
                        N_("write the content to temporary files")),
                OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
                        N_("when creating files, prepend <string>")),
-               OPT_CALLBACK_F(0, "stage", NULL, "(1|2|3|all)",
+               OPT_CALLBACK_F(0, "stage", &checkout_stage, "(1|2|3|all)",
                        N_("copy out the files from named stage"),
                        PARSE_OPT_NONEG, option_parse_stage),
                OPT_END()

diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82d5290f4800b238224946923cf7811b..718b5c3073d415440362c063c767a821c320a276 100644 (file)
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
 static int option_parse_exact_match(const struct option *opt, const char *arg,
                                    int unset)
 {
+       int *val = opt->value;
+
        BUG_ON_OPT_ARG(arg);
 
-       max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+       *val = unset ? DEFAULT_CANDIDATES : 0;
        return 0;
 }
 
@@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
                OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
                OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
                OPT__ABBREV(&abbrev),
-               OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
+               OPT_CALLBACK_F(0, "exact-match", &max_candidates, NULL,
                               N_("only output exact matches"),
                               PARSE_OPT_NOARG, option_parse_exact_match),
                OPT_INTEGER(0, "candidates", &max_candidates,

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 56dc69fac180126a43f8a9ca3ed116ca67283737..70aff515acbe97b20db80cc8b746e28f0149b56f 100644 (file)
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
-static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
-static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
+static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
+static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
 static int use_done_feature;
 static int no_data;
@@ -53,16 +53,18 @@ static struct revision_sources revision_sources;
 static int parse_opt_signed_tag_mode(const struct option *opt,
                                     const char *arg, int unset)
 {
+       enum signed_tag_mode *val = opt->value;
+
        if (unset || !strcmp(arg, "abort"))
-               signed_tag_mode = SIGNED_TAG_ABORT;
+               *val = SIGNED_TAG_ABORT;
        else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-               signed_tag_mode = VERBATIM;
+               *val = VERBATIM;
        else if (!strcmp(arg, "warn"))
-               signed_tag_mode = WARN;
+               *val = WARN;
        else if (!strcmp(arg, "warn-strip"))
-               signed_tag_mode = WARN_STRIP;
+               *val = WARN_STRIP;
        else if (!strcmp(arg, "strip"))
-               signed_tag_mode = STRIP;
+               *val = STRIP;
        else
                return error("Unknown signed-tags mode: %s", arg);
        return 0;
@@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 static int parse_opt_tag_of_filtered_mode(const struct option *opt,
                                          const char *arg, int unset)
 {
+       enum tag_of_filtered_mode *val = opt->value;
+
        if (unset || !strcmp(arg, "abort"))
-               tag_of_filtered_mode = TAG_FILTERING_ABORT;
+               *val = TAG_FILTERING_ABORT;
        else if (!strcmp(arg, "drop"))
-               tag_of_filtered_mode = DROP;
+               *val = DROP;
        else if (!strcmp(arg, "rewrite"))
-               tag_of_filtered_mode = REWRITE;
+               *val = REWRITE;
        else
                return error("Unknown tag-of-filtered mode: %s", arg);
        return 0;
@@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
 static int parse_opt_reencode_mode(const struct option *opt,
                                   const char *arg, int unset)
 {
+       enum reencode_mode *val = opt->value;
+
        if (unset) {
-               reencode_mode = REENCODE_ABORT;
+               *val = REENCODE_ABORT;
                return 0;
        }
 
        switch (git_parse_maybe_bool(arg)) {
        case 0:
-               reencode_mode = REENCODE_NO;
+               *val = REENCODE_NO;
                break;
        case 1:
-               reencode_mode = REENCODE_YES;
+               *val = REENCODE_YES;
                break;
        default:
                if (!strcasecmp(arg, "abort"))
-                       reencode_mode = REENCODE_ABORT;
+                       *val = REENCODE_ABORT;
                else
                        return error("Unknown reencoding mode: %s", arg);
        }

diff --git a/builtin/fetch.c b/builtin/fetch.c
index eed4a7cdb6c1d672ab7324ea129d83ea312b688e..f6b42b3d5825cdae0e3d749575f0f6a18a65c014 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
         * "git fetch --refmap='' origin foo"
         * can be used to tell the command not to store anywhere
         */
-       refspec_append(&refmap, arg);
+       refspec_append(opt->value, arg);
 
        return 0;
 }
@@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
                           PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),
                OPT_BOOL(0, "update-shallow", &update_shallow,
                         N_("accept refs that update .git/shallow")),
-               OPT_CALLBACK_F(0, "refmap", NULL, N_("refmap"),
+               OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
                               N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
                OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
                OPT_IPVERSION(&family),

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e83452654de9242f01a4045fb90f3f058ea8f7..6aadce6a1ee750a425a946f681363986ef63e18f 100644 (file)
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
 static int option_parse_where(const struct option *opt,
                              const char *arg, int unset)
 {
-       return trailer_set_where(&where, arg);
+       return trailer_set_where(opt->value, arg);
 }
 
 static int option_parse_if_exists(const struct option *opt,
                                  const char *arg, int unset)
 {
-       return trailer_set_if_exists(&if_exists, arg);
+       return trailer_set_if_exists(opt->value, arg);
 }
 
 static int option_parse_if_missing(const struct option *opt,
                                   const char *arg, int unset)
 {
-       return trailer_set_if_missing(&if_missing, arg);
+       return trailer_set_if_missing(opt->value, arg);
 }
 
 static void new_trailers_clear(struct list_head *trailers)
@@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
                OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
                OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 
-               OPT_CALLBACK(0, "where", NULL, N_("action"),
+               OPT_CALLBACK(0, "where", &where, N_("action"),
                             N_("where to place the new trailer"), option_parse_where),
-               OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
+               OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
                             N_("action if trailer already exists"), option_parse_if_exists),
-               OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
+               OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
                             N_("action if trailer is missing"), option_parse_if_missing),
 
                OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d2a162d52804cf7fdf1fd1a11303f2380c24c9e3..492372ee5dea727c33c21a39aff464328ad1d161 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4120,29 +4120,32 @@ static void add_extra_kept_packs(const struct string_list *names)
 static int option_parse_quiet(const struct option *opt, const char *arg,
                              int unset)
 {
+       int *val = opt->value;
+
        BUG_ON_OPT_ARG(arg);
 
        if (!unset)
-               progress = 0;
-       else if (!progress)
-               progress = 1;
+               *val = 0;
+       else if (!*val)
+               *val = 1;
        return 0;
 }
 
 static int option_parse_index_version(const struct option *opt,
                                      const char *arg, int unset)
 {
+       struct pack_idx_option *popts = opt->value;
        char *c;
        const char *val = arg;
 
        BUG_ON_OPT_NEG(unset);
 
-       pack_idx_opts.version = strtoul(val, &c, 10);
-       if (pack_idx_opts.version > 2)
+       popts->version = strtoul(val, &c, 10);
+       if (popts->version > 2)
                die(_("unsupported index version %s"), val);
        if (*c == ',' && c[1])
-               pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
-       if (*c || pack_idx_opts.off32_limit & 0x80000000)
+               popts->off32_limit = strtoul(c+1, &c, 0);
+       if (*c || popts->off32_limit & 0x80000000)
                die(_("bad index version '%s'"), val);
        return 0;
 }
@@ -4190,7 +4193,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
                LIST_OBJECTS_FILTER_INIT;
 
        struct option pack_objects_options[] = {
-               OPT_CALLBACK_F('q', "quiet", NULL, NULL,
+               OPT_CALLBACK_F('q', "quiet", &progress, NULL,
                               N_("do not show progress meter"),
                               PARSE_OPT_NOARG, option_parse_quiet),
                OPT_SET_INT(0, "progress", &progress,
@@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
                OPT_BOOL(0, "all-progress-implied",
                         &all_progress_implied,
                         N_("similar to --all-progress when progress meter is shown")),
-               OPT_CALLBACK_F(0, "index-version", NULL, N_("<version>[,<offset>]"),
+               OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("<version>[,<offset>]"),
                  N_("write the pack index file in the specified idx format version"),
                  PARSE_OPT_NONEG, option_parse_index_version),
                OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,