* [PATCH 0/8] more unused parameters in parseopt callbacks
@ 2023-08-31 7:09 Jeff King
2023-08-31 7:12 ` [PATCH 1/8] merge: make xopts a strvec Jeff King
` (8 more replies)
0 siblings, 9 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:09 UTC (permalink / raw)
To: git; +Cc: René Scharfe
Here are some more patches silencing -Wunused-parameter warnings. I've
prepared them on top of the patches queued in jk/unused-post-2.42, but
they should apply equally well directly on 'master'.
These ones are a bit more interesting than usual, because there were
some actual code changes and cleanups that seemed preferable to just
annotating.
I'm adding René to the cc based on our earlier discussion of callbacks
which ignore opt->value. See patch 3 in particular. Also some of your
recent work gets a nice shout-out in patch 2. :)
[1/8]: merge: make xopts a strvec
[2/8]: merge: simplify parsing of "-n" option
[3/8]: parse-options: prefer opt->value to globals in callbacks
[4/8]: parse-options: mark unused "opt" parameter in callbacks
[5/8]: merge: do not pass unused opt->value parameter
[6/8]: parse-options: add more BUG_ON() annotations
[7/8]: interpret-trailers: mark unused "unset" parameters in option callbacks
[8/8]: parse-options: mark unused parameters in noop callback
builtin/add.c | 2 ++
builtin/checkout-index.c | 2 +-
builtin/describe.c | 6 +++--
builtin/fast-export.c | 36 +++++++++++++++++-------------
builtin/fetch.c | 3 ++-
builtin/gc.c | 2 +-
builtin/interpret-trailers.c | 18 +++++++--------
builtin/log.c | 18 ++++++++++-----
builtin/merge.c | 43 +++++++++---------------------------
builtin/pack-objects.c | 27 ++++++++++++----------
builtin/read-tree.c | 2 +-
builtin/update-index.c | 4 ++--
parse-options-cb.c | 4 +++-
13 files changed, 84 insertions(+), 83 deletions(-)
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 1/8] merge: make xopts a strvec
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
@ 2023-08-31 7:12 ` Jeff King
2023-08-31 7:22 ` Jeff King
2023-08-31 15:46 ` Junio C Hamano
2023-08-31 7:14 ` [PATCH 2/8] merge: simplify parsing of "-n" option Jeff King
` (7 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:12 UTC (permalink / raw)
To: git; +Cc: René Scharfe
The "xopts" variable uses a custom array with ALLOC_GROW(). Using a
strvec simplifies things a bit. We need fewer variables, and we can also
ditch our custom parseopt callback in favor of OPT_STRVEC().
As a bonus, this means that "--no-strategy-option", which was previously
a silent noop, now does something useful: like other list-like options,
it will clear the list of -X options seen so far.
Signed-off-by: Jeff King <peff@peff.net>
---
I guess you could argue this is a backwards-incompatible change, but the
existing behavior of --no-strategy-option is so dumb that I can't
believe somebody would prefer it (plus revert/cherry-pick already use
OPT_STRVEC for their matching "-X").
I didn't bother adding a test since we're just re-using OPT_STRVEC code
that is used elsewhere.
builtin/merge.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index de68910177..53e9fe70d9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -79,8 +79,7 @@ static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
static size_t use_strategies_nr, use_strategies_alloc;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+static struct strvec xopts = STRVEC_INIT;
static const char *branch;
static char *branch_mergeoptions;
static int verbosity;
@@ -242,17 +241,6 @@ static int option_parse_strategy(const struct option *opt,
return 0;
}
-static int option_parse_x(const struct option *opt,
- const char *arg, int unset)
-{
- if (unset)
- return 0;
-
- ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
- xopts[xopts_nr++] = xstrdup(arg);
- return 0;
-}
-
static int option_parse_n(const struct option *opt,
const char *arg, int unset)
{
@@ -287,8 +275,8 @@ static struct option builtin_merge_options[] = {
N_("verify that the named commit has a valid GPG signature")),
OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
N_("merge strategy to use"), option_parse_strategy),
- OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),
- N_("option for selected merge strategy"), option_parse_x),
+ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
OPT_CALLBACK('m', "message", &merge_msg, N_("message"),
N_("merge commit message (for a non-fast-forward merge)"),
option_parse_message),
@@ -749,9 +737,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
o.show_rename_progress =
show_progress == -1 ? isatty(2) : show_progress;
- for (x = 0; x < xopts_nr; x++)
- if (parse_merge_opt(&o, xopts[x]))
- die(_("unknown strategy option: -X%s"), xopts[x]);
+ for (x = 0; x < xopts.nr; x++)
+ if (parse_merge_opt(&o, xopts.v[x]))
+ die(_("unknown strategy option: -X%s"), xopts.v[x]);
o.branch1 = head_arg;
o.branch2 = merge_remote_util(remoteheads->item)->name;
@@ -777,7 +765,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
return clean ? 0 : 1;
} else {
return try_merge_command(the_repository,
- strategy, xopts_nr, xopts,
+ strategy, xopts.nr, xopts.v,
common, head_arg, remoteheads);
}
}
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 2/8] merge: simplify parsing of "-n" option
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
2023-08-31 7:12 ` [PATCH 1/8] merge: make xopts a strvec Jeff King
@ 2023-08-31 7:14 ` Jeff King
2023-08-31 15:56 ` Junio C Hamano
2023-08-31 7:17 ` [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks Jeff King
` (6 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:14 UTC (permalink / raw)
To: git; +Cc: René Scharfe
The "-n" option is implemented by an option callback, as it is really a
"reverse bool". If given, it sets show_diffstat to 0. In theory, when
negated, it would set the same flag to 1. But it's not possible to
trigger that, since short options cannot be negated.
So in practice this is really just a SET_INT to 0. Let's use that
instead, which shortens the code.
Note that negation here would do the wrong thing (as with any SET_INT
with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof
ourselves against somebody adding a long option name (which would make
it possible to negate). But there's not much point:
1. Nobody is going to do that, because the negated form already
exists, and is called "--stat" (which is defined separately so that
"--no-stat" works).
2. If they did, the BUG() check added by 3284b93862 (parse-options:
disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
that check is smart enough to realize that our short-only option is
OK).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 53e9fe70d9..21363b7985 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
return 0;
}
-static int option_parse_n(const struct option *opt,
- const char *arg, int unset)
-{
- BUG_ON_OPT_ARG(arg);
- show_diffstat = unset;
- return 0;
-}
-
static struct option builtin_merge_options[] = {
- OPT_CALLBACK_F('n', NULL, NULL, NULL,
- N_("do not show a diffstat at the end of the merge"),
- PARSE_OPT_NOARG, option_parse_n),
+ OPT_SET_INT('n', NULL, &show_diffstat,
+ N_("do not show a diffstat at the end of the merge"), 0),
OPT_BOOL(0, "stat", &show_diffstat,
N_("show a diffstat at the end of the merge")),
OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
2023-08-31 7:12 ` [PATCH 1/8] merge: make xopts a strvec Jeff King
2023-08-31 7:14 ` [PATCH 2/8] merge: simplify parsing of "-n" option Jeff King
@ 2023-08-31 7:17 ` Jeff King
2023-08-31 16:14 ` Junio C Hamano
2023-08-31 7:18 ` [PATCH 4/8] parse-options: mark unused "opt" parameter " Jeff King
` (5 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:17 UTC (permalink / raw)
To: git; +Cc: René Scharfe
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>
---
I was tempted to push some of these globals into the cmd_foo()
functions, to make sure we don't make the same mistake again. But there
are lots of ripple effects from other functions which want to access the
globals (and passing them makes the code longer for little benefit).
Plus it would make things inconsistent, with some options globals and
some not. So that can be a cleanup for another day if somebody is
interested (and this takes us one tiny step closer).
builtin/describe.c | 6 ++++--
builtin/fast-export.c | 36 +++++++++++++++++++++---------------
builtin/interpret-trailers.c | 12 ++++++------
builtin/pack-objects.c | 21 ++++++++++++---------
4 files changed, 43 insertions(+), 32 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..718b5c3073 100644
--- 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 56dc69fac1..70aff515ac 100644
--- 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/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e8345265..6aadce6a1e 100644
--- 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 d2a162d528..492372ee5d 100644
--- 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,
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
` (2 preceding siblings ...)
2023-08-31 7:17 ` [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks Jeff King
@ 2023-08-31 7:18 ` Jeff King
2023-08-31 16:33 ` Junio C Hamano
2023-08-31 7:18 ` [PATCH 5/8] merge: do not pass unused opt->value parameter Jeff King
` (4 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:18 UTC (permalink / raw)
To: git; +Cc: René Scharfe
The previous commit argued that parse-options callbacks should try to
use opt->value rather than touching globals directly. In some cases,
however, that's awkward to do. Some callbacks touch multiple variables,
or may even just call into an abstracted function that does so.
In some of these cases we _could_ convert them by stuffing the multiple
variables into a single struct and passing the struct pointer through
opt->value. But that may make other parts of the code less readable,
as the struct relationship has to be mentioned everywhere.
Let's just accept that these cases are special and leave them as-is. But
we do need to mark their "opt" parameters to satisfy -Wunused-parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/checkout-index.c | 2 +-
builtin/fetch.c | 3 ++-
builtin/gc.c | 2 +-
builtin/log.c | 16 ++++++++++------
builtin/merge.c | 2 +-
builtin/pack-objects.c | 6 +++---
builtin/read-tree.c | 2 +-
builtin/update-index.c | 4 ++--
8 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..95b3717dd1 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = {
NULL
};
-static int option_parse_stage(const struct option *opt,
+static int option_parse_stage(const struct option *opt UNUSED,
const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8f93529505..cd2afe33e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v,
return git_default_config(k, v, ctx, cb);
}
-static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
+static int parse_refmap_arg(const struct option *opt UNUSED,
+ const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
diff --git a/builtin/gc.c b/builtin/gc.c
index 369bd43fb2..b842349d86 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
strbuf_release(&config_name);
}
-static int task_option_parse(const struct option *opt,
+static int task_option_parse(const struct option *opt UNUSED,
const char *arg, int unset)
{
int i, num_selected = 0;
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..87e29c4171 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
-static int clear_decorations_callback(const struct option *opt,
- const char *arg, int unset)
+static int clear_decorations_callback(const struct option *opt UNUSED,
+ const char *arg, int unset)
{
string_list_clear(&decorate_refs_include, 0);
string_list_clear(&decorate_refs_exclude, 0);
use_default_decoration_filter = 0;
return 0;
}
-static int decorate_callback(const struct option *opt, const char *arg, int unset)
+static int decorate_callback(const struct option *opt UNUSED, const char *arg,
+ int unset)
{
if (unset)
decoration_style = 0;
@@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
return 0;
}
-static int header_callback(const struct option *opt, const char *arg, int unset)
+static int header_callback(const struct option *opt UNUSED, const char *arg,
+ int unset)
{
if (unset) {
string_list_clear(&extra_hdr, 0);
@@ -1567,7 +1569,8 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
return 0;
}
-static int to_callback(const struct option *opt, const char *arg, int unset)
+static int to_callback(const struct option *opt UNUSED, const char *arg,
+ int unset)
{
if (unset)
string_list_clear(&extra_to, 0);
@@ -1576,7 +1579,8 @@ static int to_callback(const struct option *opt, const char *arg, int unset)
return 0;
}
-static int cc_callback(const struct option *opt, const char *arg, int unset)
+static int cc_callback(const struct option *opt UNUSED, const char *arg,
+ int unset)
{
if (unset)
string_list_clear(&extra_cc, 0);
diff --git a/builtin/merge.c b/builtin/merge.c
index 21363b7985..0436986dab 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
use_strategies[use_strategies_nr++] = s;
}
-static int option_parse_strategy(const struct option *opt,
+static int option_parse_strategy(const struct option *opt UNUSED,
const char *name, int unset)
{
if (unset)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 492372ee5d..91b4b7c177 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
show_object(obj, name, data);
}
-static int option_parse_missing_action(const struct option *opt,
+static int option_parse_missing_action(const struct option *opt UNUSED,
const char *arg, int unset)
{
assert(arg);
@@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
return 0;
}
-static int option_parse_unpack_unreachable(const struct option *opt,
+static int option_parse_unpack_unreachable(const struct option *opt UNUSED,
const char *arg, int unset)
{
if (unset) {
@@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
return 0;
}
-static int option_parse_cruft_expiration(const struct option *opt,
+static int option_parse_cruft_expiration(const struct option *opt UNUSED,
const char *arg, int unset)
{
if (unset) {
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 1fec702a04..8196ca9dd8 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
NULL
};
-static int index_output_cb(const struct option *opt, const char *arg,
+static int index_output_cb(const struct option *opt UNUSED, const char *arg,
int unset)
{
BUG_ON_OPT_NEG(unset);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index aee3cb8cbd..59acae3336 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
return 0;
}
-static int resolve_undo_clear_callback(const struct option *opt,
+static int resolve_undo_clear_callback(const struct option *opt UNUSED,
const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
}
static enum parse_opt_result cacheinfo_callback(
- struct parse_opt_ctx_t *ctx, const struct option *opt,
+ struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
const char *arg, int unset)
{
struct object_id oid;
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 5/8] merge: do not pass unused opt->value parameter
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
` (3 preceding siblings ...)
2023-08-31 7:18 ` [PATCH 4/8] parse-options: mark unused "opt" parameter " Jeff King
@ 2023-08-31 7:18 ` Jeff King
2023-08-31 16:53 ` Junio C Hamano
2023-08-31 7:19 ` [PATCH 6/8] parse-options: add more BUG_ON() annotations Jeff King
` (3 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:18 UTC (permalink / raw)
To: git; +Cc: René Scharfe
The option_parse_strategy() callback does not look at opt->value;
instead it calls append_strategy(), which manipulates the global
use_strategies array directly. But the OPT_CALLBACK declaration assigns
"&use_strategies" to opt->value.
One could argue this is good, as it tells the reader what we generally
expect the callback to do. But it is also bad, because it can mislead
you into thinking that swapping out "&use_strategies" there might have
any effect. Let's switch it to pass NULL (which is what every other
"does not bother to look at opt->value" callback does). If you want to
know what the callback does, it's easy to read the function itself.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 0436986dab..545da0c8a1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {
OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
OPT_BOOL(0, "verify-signatures", &verify_signatures,
N_("verify that the named commit has a valid GPG signature")),
- OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
+ OPT_CALLBACK('s', "strategy", NULL, N_("strategy"),
N_("merge strategy to use"), option_parse_strategy),
OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
N_("option for selected merge strategy")),
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 6/8] parse-options: add more BUG_ON() annotations
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
` (4 preceding siblings ...)
2023-08-31 7:18 ` [PATCH 5/8] merge: do not pass unused opt->value parameter Jeff King
@ 2023-08-31 7:19 ` Jeff King
2023-08-31 16:58 ` Junio C Hamano
2023-08-31 7:19 ` [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
` (2 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:19 UTC (permalink / raw)
To: git; +Cc: René Scharfe
These callbacks are similar to the ones touched by 517fe807d6 (assert
NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were
either missed in that commit (the one in add.c) or were added later (the
one in log.c).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/add.c | 2 ++
builtin/log.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/builtin/add.c b/builtin/add.c
index 4b0dd798df..cf59108523 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -232,6 +232,8 @@ static char *chmod_arg;
static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
{
+ BUG_ON_OPT_ARG(arg);
+
/* if we are told to ignore, we are not adding removals */
*(int *)opt->value = !unset ? 0 : 1;
return 0;
diff --git a/builtin/log.c b/builtin/log.c
index 87e29c4171..80fa642858 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
static int clear_decorations_callback(const struct option *opt UNUSED,
const char *arg, int unset)
{
+ BUG_ON_OPT_NEG(unset);
+ BUG_ON_OPT_ARG(arg);
string_list_clear(&decorate_refs_include, 0);
string_list_clear(&decorate_refs_exclude, 0);
use_default_decoration_filter = 0;
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
` (5 preceding siblings ...)
2023-08-31 7:19 ` [PATCH 6/8] parse-options: add more BUG_ON() annotations Jeff King
@ 2023-08-31 7:19 ` Jeff King
2023-08-31 17:04 ` Junio C Hamano
2023-08-31 7:20 ` [PATCH 8/8] parse-options: mark unused parameters in noop callback Jeff King
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:19 UTC (permalink / raw)
To: git; +Cc: René Scharfe
There are a few parse-option callbacks that do not look at their "unset"
parameters, but also do not set PARSE_OPT_NONEG. At first glance this
seems like a bug, as we'd ignore "--no-if-exists", etc.
But they do work fine, because when "unset" is true, then "arg" is NULL.
And all three functions pass "arg" on to helper functions which do the
right thing with the NULL.
Note that this shortcut would not be correct if any callback used
PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
false). But none of these do.
So the code is fine as-is. But we'll want to mark the unused "unset"
parameters to quiet -Wunused-parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/interpret-trailers.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6aadce6a1e..a5f9adf6b9 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;
static enum trailer_if_missing if_missing;
static int option_parse_where(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
return trailer_set_where(opt->value, arg);
}
static int option_parse_if_exists(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
return trailer_set_if_exists(opt->value, arg);
}
static int option_parse_if_missing(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
return trailer_set_if_missing(opt->value, arg);
}
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 8/8] parse-options: mark unused parameters in noop callback
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
` (6 preceding siblings ...)
2023-08-31 7:19 ` [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
@ 2023-08-31 7:20 ` Jeff King
2023-08-31 17:05 ` Junio C Hamano
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
8 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:20 UTC (permalink / raw)
To: git; +Cc: René Scharfe
Unsurprisingly, the noop options callback doesn't bother to look at any
of its parameters. Let's mark them so that -Wunused-parameter does not
complain.
Another option would be to drop the callback and have parse-options
itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
real benefit.
Signed-off-by: Jeff King <peff@peff.net>
---
parse-options-cb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..bdc7fae497 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
return 0;
}
-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
+int parse_opt_noop_cb(const struct option *opt UNUSED,
+ const char *arg UNUSED,
+ int unset UNUSED)
{
return 0;
}
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 1/8] merge: make xopts a strvec
2023-08-31 7:12 ` [PATCH 1/8] merge: make xopts a strvec Jeff King
@ 2023-08-31 7:22 ` Jeff King
2023-08-31 11:18 ` Phillip Wood
2023-08-31 15:46 ` Junio C Hamano
1 sibling, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 7:22 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, René Scharfe
On Thu, Aug 31, 2023 at 03:12:30AM -0400, Jeff King wrote:
> The "xopts" variable uses a custom array with ALLOC_GROW(). Using a
> strvec simplifies things a bit. We need fewer variables, and we can also
> ditch our custom parseopt callback in favor of OPT_STRVEC().
>
> As a bonus, this means that "--no-strategy-option", which was previously
> a silent noop, now does something useful: like other list-like options,
> it will clear the list of -X options seen so far.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess you could argue this is a backwards-incompatible change, but the
> existing behavior of --no-strategy-option is so dumb that I can't
> believe somebody would prefer it (plus revert/cherry-pick already use
> OPT_STRVEC for their matching "-X").
>
> I didn't bother adding a test since we're just re-using OPT_STRVEC code
> that is used elsewhere.
I only noticed the "revert/cherry-pick" thing while sending this patch
out. But it seems that it was changed recently-ish, and Phillip noted
the same behavior change there (along with mentioning merge.c). So now I
doubly feel this is the right thing to do. :)
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/8] merge: make xopts a strvec
2023-08-31 7:22 ` Jeff King
@ 2023-08-31 11:18 ` Phillip Wood
0 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2023-08-31 11:18 UTC (permalink / raw)
To: Jeff King, git; +Cc: Phillip Wood, René Scharfe
Hi Peff
On 31/08/2023 08:22, Jeff King wrote:
> On Thu, Aug 31, 2023 at 03:12:30AM -0400, Jeff King wrote:
> I only noticed the "revert/cherry-pick" thing while sending this patch
> out. But it seems that it was changed recently-ish, and Phillip noted
> the same behavior change there (along with mentioning merge.c). So now I
> doubly feel this is the right thing to do. :)
Thanks for doing this, I had intended to follow up the
cherry-pick/revert changes by changing merge as well but didn't get
around to it. I think the current behavior is unlikely to be useful to
anyone.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/8] merge: make xopts a strvec
2023-08-31 7:12 ` [PATCH 1/8] merge: make xopts a strvec Jeff King
2023-08-31 7:22 ` Jeff King
@ 2023-08-31 15:46 ` Junio C Hamano
2023-08-31 20:55 ` Taylor Blau
1 sibling, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 15:46 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> The "xopts" variable uses a custom array with ALLOC_GROW(). Using a
> strvec simplifies things a bit. We need fewer variables, and we can also
> ditch our custom parseopt callback in favor of OPT_STRVEC().
>
> As a bonus, this means that "--no-strategy-option", which was previously
> a silent noop, now does something useful: like other list-like options,
> it will clear the list of -X options seen so far.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess you could argue this is a backwards-incompatible change, but the
> existing behavior of --no-strategy-option is so dumb that I can't
> believe somebody would prefer it (plus revert/cherry-pick already use
> OPT_STRVEC for their matching "-X").
>
> I didn't bother adding a test since we're just re-using OPT_STRVEC code
> that is used elsewhere.
I do not think of any useful way to have "--no-strategy-option" on
the command line (either as an early part of an alias or in a
script) that does nothing (it's not like the command requires at
least one -X option on the command line), either. Just like
fb60b9f3 (sequencer: use struct strvec to store merge strategy
options, 2023-04-10), which met no complaints about a possible
fallout by the behaviour change, I do not think that this change
even deserves an entry in the backward compatibility notes.
We did not have strvec, let alone OPT_STRVEC(), back when the -Xopts
was invented around the Git 1.7.0 cycle. The loss of custom xopts
code is very nice.
Thanks.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/8] merge: simplify parsing of "-n" option
2023-08-31 7:14 ` [PATCH 2/8] merge: simplify parsing of "-n" option Jeff King
@ 2023-08-31 15:56 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 15:56 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> 2. If they did, the BUG() check added by 3284b93862 (parse-options:
> disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
> that check is smart enough to realize that our short-only option is
> OK).
;-).
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/merge.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
More code deletion. Nice.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks
2023-08-31 7:17 ` [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks Jeff King
@ 2023-08-31 16:14 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 16:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> ... 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!).
Thanks for doing this. I vaguely recall in my recent reviews I said
that I prefer the style for exactly the second reason above.
> 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>
> ---
> I was tempted to push some of these globals into the cmd_foo()
> functions, to make sure we don't make the same mistake again. But there
> are lots of ripple effects from other functions which want to access the
> globals (and passing them makes the code longer for little benefit).
> Plus it would make things inconsistent, with some options globals and
> some not. So that can be a cleanup for another day if somebody is
> interested (and this takes us one tiny step closer).
Yup, as long as these globals are file-scope static that these
cmd_foo() use to decide and prepare arguments to pass to the more
library-ish part of the code, I do not mind them not being on stack.
> diff --git a/builtin/describe.c b/builtin/describe.c
> index b28a4a1f82..718b5c3073 100644
> --- 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,
If we ever choose to libify builtin/describe.c::describe_commit(),
max_candidates may want to become a parameter to it, which would
allow cmd_describe() to have it on its stack, but until that
happens, I agree that it is not a very useful churn.
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index c5e8345265..6aadce6a1e 100644
> --- 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")),
OK. These three variables should be fun (but not too difficult) to
convert to on-stack variables. option_parse_trailer() needs to
learn to take a struct that holds "trailers", in addition to these
three variables as its members to lose its dependency on the
globals, and then these three callbacks can learn to take the same
struct and fill their own members in it.
Of course that is outside the scope of this series.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks
2023-08-31 7:18 ` [PATCH 4/8] parse-options: mark unused "opt" parameter " Jeff King
@ 2023-08-31 16:33 ` Junio C Hamano
2023-08-31 17:50 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 16:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index f62f13f2b5..95b3717dd1 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = {
> NULL
> };
>
> -static int option_parse_stage(const struct option *opt,
> +static int option_parse_stage(const struct option *opt UNUSED,
> const char *arg, int unset)
> {
> BUG_ON_OPT_NEG(unset);
I suspect that the original is buggy; when given
$ git checkout-index --stage=all --stage=1 path
the first option turns --temp on, but the second one does not turn
it off. For now I think being bug-to-bug compatible and annotating
the opt as UNUSED is good, but as a follow-up, we could make the
caller:
(1) point &checkout_stage with opt->value;
(2) make to_tempfile to tristate <unspecified, false, true> by
initializing it to -1;
(3) adjust to_tempfile that is still <unspecified> after
parse_options() returns, according to the value in
checkout_stage.
and then this can follow the "opt->value points at the variable that
is affected" pattern.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8f93529505..cd2afe33e5 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v,
> return git_default_config(k, v, ctx, cb);
> }
>
> -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> +static int parse_refmap_arg(const struct option *opt UNUSED,
> + const char *arg, int unset)
> {
> BUG_ON_OPT_NEG(unset);
Can't this just point opt->value at the global &refmap? Obviously
not a huge deal, as we could have taken the "annotate as UNUSED"
approach for all the functions in [3/8].
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 369bd43fb2..b842349d86 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
> strbuf_release(&config_name);
> }
>
> -static int task_option_parse(const struct option *opt,
> +static int task_option_parse(const struct option *opt UNUSED,
> const char *arg, int unset)
> {
> int i, num_selected = 0;
The TASK__COUNT constant is very closely tied to the task[] array
and it is not worth passing the address of the array in opt->value.
The loss of clarity inside the callback funtion is a more serious
downside than the value on the caller side to document what array
is being modified. So I agree this is a good change.
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index aee3cb8cbd..59acae3336 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
> return 0;
> }
>
> -static int resolve_undo_clear_callback(const struct option *opt,
> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,
> const char *arg, int unset)
> {
> BUG_ON_OPT_NEG(unset);
> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
> }
>
> static enum parse_opt_result cacheinfo_callback(
> - struct parse_opt_ctx_t *ctx, const struct option *opt,
> + struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
> const char *arg, int unset)
> {
> struct object_id oid;
These two refuses to take "arg" and the callback mechanism is used
purely to trigger the side effect of the function, not as a part of
parsing something for its "value", so I agree that these UNUSED
annotations reflect the reality very well.
Thanks.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/8] merge: do not pass unused opt->value parameter
2023-08-31 7:18 ` [PATCH 5/8] merge: do not pass unused opt->value parameter Jeff King
@ 2023-08-31 16:53 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 16:53 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> The option_parse_strategy() callback does not look at opt->value;
> instead it calls append_strategy(), which manipulates the global
> use_strategies array directly. But the OPT_CALLBACK declaration assigns
> "&use_strategies" to opt->value.
>
> One could argue this is good, as it tells the reader what we generally
> expect the callback to do. But it is also bad, because it can mislead
> you into thinking that swapping out "&use_strategies" there might have
> any effect. Let's switch it to pass NULL (which is what every other
> "does not bother to look at opt->value" callback does). If you want to
> know what the callback does, it's easy to read the function itself.
Good. It is not like we have two options and each of them affect
its own list of strategies. There is a single use_strategies list
that is populated not just by the parse options callback but by
other callers. So I agree that the new way is less confusing.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/merge.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0436986dab..545da0c8a1 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {
> OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
> OPT_BOOL(0, "verify-signatures", &verify_signatures,
> N_("verify that the named commit has a valid GPG signature")),
> - OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
> + OPT_CALLBACK('s', "strategy", NULL, N_("strategy"),
> N_("merge strategy to use"), option_parse_strategy),
> OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> N_("option for selected merge strategy")),
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/8] parse-options: add more BUG_ON() annotations
2023-08-31 7:19 ` [PATCH 6/8] parse-options: add more BUG_ON() annotations Jeff King
@ 2023-08-31 16:58 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 16:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> These callbacks are similar to the ones touched by 517fe807d6 (assert
> NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were
> either missed in that commit (the one in add.c) or were added later (the
> one in log.c).
They obviously look good. Thanks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/add.c | 2 ++
> builtin/log.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 4b0dd798df..cf59108523 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -232,6 +232,8 @@ static char *chmod_arg;
>
> static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
> {
> + BUG_ON_OPT_ARG(arg);
> +
> /* if we are told to ignore, we are not adding removals */
> *(int *)opt->value = !unset ? 0 : 1;
> return 0;
> diff --git a/builtin/log.c b/builtin/log.c
> index 87e29c4171..80fa642858 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
> static int clear_decorations_callback(const struct option *opt UNUSED,
> const char *arg, int unset)
> {
> + BUG_ON_OPT_NEG(unset);
> + BUG_ON_OPT_ARG(arg);
> string_list_clear(&decorate_refs_include, 0);
> string_list_clear(&decorate_refs_exclude, 0);
> use_default_decoration_filter = 0;
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks
2023-08-31 7:19 ` [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
@ 2023-08-31 17:04 ` Junio C Hamano
2023-08-31 17:56 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 17:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> There are a few parse-option callbacks that do not look at their "unset"
> parameters, but also do not set PARSE_OPT_NONEG. At first glance this
> seems like a bug, as we'd ignore "--no-if-exists", etc.
>
> But they do work fine, because when "unset" is true, then "arg" is NULL.
> And all three functions pass "arg" on to helper functions which do the
> right thing with the NULL.
Yuck. That is ugly.
> Note that this shortcut would not be correct if any callback used
> PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
> false). But none of these do.
That is even uglier. Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG()
that catch discrepancies between options[] flags and the expectation
by the callback function, there is no way for us to protect against
such mistakes?
> So the code is fine as-is. But we'll want to mark the unused "unset"
> parameters to quiet -Wunused-parameter.
OK. Thanks.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/interpret-trailers.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 6aadce6a1e..a5f9adf6b9 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;
> static enum trailer_if_missing if_missing;
>
> static int option_parse_where(const struct option *opt,
> - const char *arg, int unset)
> + const char *arg, int unset UNUSED)
> {
> return trailer_set_where(opt->value, arg);
> }
>
> static int option_parse_if_exists(const struct option *opt,
> - const char *arg, int unset)
> + const char *arg, int unset UNUSED)
> {
> return trailer_set_if_exists(opt->value, arg);
> }
>
> static int option_parse_if_missing(const struct option *opt,
> - const char *arg, int unset)
> + const char *arg, int unset UNUSED)
> {
> return trailer_set_if_missing(opt->value, arg);
> }
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/8] parse-options: mark unused parameters in noop callback
2023-08-31 7:20 ` [PATCH 8/8] parse-options: mark unused parameters in noop callback Jeff King
@ 2023-08-31 17:05 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 17:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> Unsurprisingly, the noop options callback doesn't bother to look at any
> of its parameters. Let's mark them so that -Wunused-parameter does not
> complain.
;-)
Thanks.
> Another option would be to drop the callback and have parse-options
> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
> real benefit.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> parse-options-cb.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index a24521dee0..bdc7fae497 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
> +int parse_opt_noop_cb(const struct option *opt UNUSED,
> + const char *arg UNUSED,
> + int unset UNUSED)
> {
> return 0;
> }
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks
2023-08-31 16:33 ` Junio C Hamano
@ 2023-08-31 17:50 ` Jeff King
2023-08-31 20:48 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 17:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
On Thu, Aug 31, 2023 at 09:33:22AM -0700, Junio C Hamano wrote:
> > -static int option_parse_stage(const struct option *opt,
> > +static int option_parse_stage(const struct option *opt UNUSED,
> > const char *arg, int unset)
> > {
> > BUG_ON_OPT_NEG(unset);
>
> I suspect that the original is buggy; when given
>
> $ git checkout-index --stage=all --stage=1 path
>
> the first option turns --temp on, but the second one does not turn
> it off. For now I think being bug-to-bug compatible and annotating
> the opt as UNUSED is good, but as a follow-up, we could make the
> caller:
>
> (1) point &checkout_stage with opt->value;
>
> (2) make to_tempfile to tristate <unspecified, false, true> by
> initializing it to -1;
>
> (3) adjust to_tempfile that is still <unspecified> after
> parse_options() returns, according to the value in
> checkout_stage.
>
> and then this can follow the "opt->value points at the variable that
> is affected" pattern.
Yeah, I think that would work, with one extra bit:
(4) complain when (!to_tempfile && checkout_stage == CHECKOUT_ALL)
I do think it would be better to fix separately, but maybe if I'm
re-rolling I can do it as an early patch in the series (it is not much
different than the "xopts" fix in scope).
> > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> > +static int parse_refmap_arg(const struct option *opt UNUSED,
> > + const char *arg, int unset)
> > {
> > BUG_ON_OPT_NEG(unset);
>
> Can't this just point opt->value at the global &refmap? Obviously
> not a huge deal, as we could have taken the "annotate as UNUSED"
> approach for all the functions in [3/8].
Hmm, yeah. I think I looked at the abstract refspec_append() here and
assumed that it might be touching other variables. But it's not. It's
operating purely on the &refspec we pass it (and even though it uses
ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So
yeah, it really should have been converted in patch 3.
I think that is probably worth a re-roll.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks
2023-08-31 17:04 ` Junio C Hamano
@ 2023-08-31 17:56 ` Jeff King
0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 17:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
On Thu, Aug 31, 2023 at 10:04:09AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > There are a few parse-option callbacks that do not look at their "unset"
> > parameters, but also do not set PARSE_OPT_NONEG. At first glance this
> > seems like a bug, as we'd ignore "--no-if-exists", etc.
> >
> > But they do work fine, because when "unset" is true, then "arg" is NULL.
> > And all three functions pass "arg" on to helper functions which do the
> > right thing with the NULL.
>
> Yuck. That is ugly.
Yep. I wondered about adding a comment here warning about the situation,
but it felt kind of content-less. Something like:
/* if unset, arg is NULL and handled below */
trailer_set_where(opt->value, arg);
> > Note that this shortcut would not be correct if any callback used
> > PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
> > false). But none of these do.
>
> That is even uglier. Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG()
> that catch discrepancies between options[] flags and the expectation
> by the callback function, there is no way for us to protect against
> such mistakes?
I guess it would be something like:
if (!unset && !arg)
BUG("unexpected use of PARSE_OPT_OPTARG");
I think it is less important than those other ones, though, because the
mistake here is the OPT_CALLBACK declaration adding a flag that the
callback is not prepared to handle. Whereas in the other ones, the bug
is that the declaration _forgot_ to use a flag, which is a much more
likely bug.
So I dunno. If it were more than this one case (well, three, but they
are all of the same form) I'd be more worried.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks
2023-08-31 17:50 ` Jeff King
@ 2023-08-31 20:48 ` Jeff King
0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
On Thu, Aug 31, 2023 at 01:50:18PM -0400, Jeff King wrote:
> > > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> > > +static int parse_refmap_arg(const struct option *opt UNUSED,
> > > + const char *arg, int unset)
> > > {
> > > BUG_ON_OPT_NEG(unset);
> >
> > Can't this just point opt->value at the global &refmap? Obviously
> > not a huge deal, as we could have taken the "annotate as UNUSED"
> > approach for all the functions in [3/8].
>
> Hmm, yeah. I think I looked at the abstract refspec_append() here and
> assumed that it might be touching other variables. But it's not. It's
> operating purely on the &refspec we pass it (and even though it uses
> ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So
> yeah, it really should have been converted in patch 3.
Oh, btw, there are two other cases in this patch that _could_ be
converted but I left alone. In log.c, we have a to_callback() and a
cc_callback(), both of which take a single string list. But there is
also header_callback(), which resets both of those when it sees
--no-add-header. So I left them alone as a set.
However, it occurs to me that to_callback() and cc_callback() can just
be OPT_STRING_LIST these days. And that is probably a worthwhile
cleanup to do.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/8] merge: make xopts a strvec
2023-08-31 15:46 ` Junio C Hamano
@ 2023-08-31 20:55 ` Taylor Blau
0 siblings, 0 replies; 47+ messages in thread
From: Taylor Blau @ 2023-08-31 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, René Scharfe
On Thu, Aug 31, 2023 at 08:46:25AM -0700, Junio C Hamano wrote:
> > I guess you could argue this is a backwards-incompatible change, but the
> > existing behavior of --no-strategy-option is so dumb that I can't
> > believe somebody would prefer it (plus revert/cherry-pick already use
> > OPT_STRVEC for their matching "-X").
> >
> > I didn't bother adding a test since we're just re-using OPT_STRVEC code
> > that is used elsewhere.
>
> I do not think of any useful way to have "--no-strategy-option" on
> the command line (either as an early part of an alias or in a
> script) that does nothing (it's not like the command requires at
> least one -X option on the command line), either. Just like
> fb60b9f3 (sequencer: use struct strvec to store merge strategy
> options, 2023-04-10), which met no complaints about a possible
> fallout by the behaviour change, I do not think that this change
> even deserves an entry in the backward compatibility notes.
I concur with both of you. In a project like this one, we should be
rather generous with the set of things we expect users to do. But even
in a quite generous interpretation, I cannot imagine anybody relying on
this behavior, so I think skipping a mention of it in the backwards
compatibility section makes sense.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 0/10] more unused parameters in parseopt callbacks
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
` (7 preceding siblings ...)
2023-08-31 7:20 ` [PATCH 8/8] parse-options: mark unused parameters in noop callback Jeff King
@ 2023-08-31 21:16 ` Jeff King
2023-08-31 21:17 ` [PATCH v2 01/10] merge: make xopts a strvec Jeff King
` (9 more replies)
8 siblings, 10 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
On Thu, Aug 31, 2023 at 03:09:35AM -0400, Jeff King wrote:
> Here are some more patches silencing -Wunused-parameter warnings. I've
> prepared them on top of the patches queued in jk/unused-post-2.42, but
> they should apply equally well directly on 'master'.
And here's a re-roll based on feedback from Junio:
[01/10]: merge: make xopts a strvec
[02/10]: merge: simplify parsing of "-n" option
[03/10]: format-patch: use OPT_STRING_LIST for to/cc options
[04/10]: checkout-index: delay automatic setting of to_tempfile
[05/10]: parse-options: prefer opt->value to globals in callbacks
[06/10]: parse-options: mark unused "opt" parameter in callbacks
[07/10]: merge: do not pass unused opt->value parameter
[08/10]: parse-options: add more BUG_ON() annotations
[09/10]: interpret-trailers: mark unused "unset" parameters in option callbacks
[10/10]: parse-options: mark unused parameters in noop callback
Range-diff is below, but the most interesting changes are in the two new
patches (3 and 4):
- patch 3 simplifies away a few string-list callbacks (which can then
be omitted from patch 6, the "mark unused opt" one).
- patch 4 fixes some minor bugs with "checkout-index --stage". In
doing so, it's now eligible for cleanup in patch 5 ("prefer
opt->value") and its annotation dropped from patch 6
- patch 5 ("prefer opt->value") converts the missed refmap callback,
and so its annotation is gone from patch 6 now
- a few extra clarifying comments
1: 8750d92016 ! 1: 82940b6936 merge: make xopts a strvec
@@ Commit message
As a bonus, this means that "--no-strategy-option", which was previously
a silent noop, now does something useful: like other list-like options,
- it will clear the list of -X options seen so far.
+ it will clear the list of -X options seen so far. This matches the
+ behavior of revert/cherry-pick, which made the same change in fb60b9f37f
+ (sequencer: use struct strvec to store merge strategy options,
+ 2023-04-10).
Signed-off-by: Jeff King <peff@peff.net>
2: ffdae69bd6 = 2: c98b87feb9 merge: simplify parsing of "-n" option
-: ---------- > 3: 615fef8f90 format-patch: use OPT_STRING_LIST for to/cc options
-: ---------- > 4: 66cf7bcf08 checkout-index: delay automatic setting of to_tempfile
3: f00c78cc49 ! 5: a7c2bfecba parse-options: prefer opt->value to globals in callbacks
@@ Commit message
Signed-off-by: Jeff King <peff@peff.net>
+ ## builtin/checkout-index.c ##
+@@ builtin/checkout-index.c: 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"));
+ }
+@@ builtin/checkout-index.c: 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()
+
## builtin/describe.c ##
@@ builtin/describe.c: static void describe(const char *arg, int last_one)
static int option_parse_exact_match(const struct option *opt, const char *arg,
@@ builtin/fast-export.c: static int parse_opt_tag_of_filtered_mode(const struct op
return error("Unknown reencoding mode: %s", arg);
}
+ ## builtin/fetch.c ##
+@@ builtin/fetch.c: 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;
+ }
+@@ builtin/fetch.c: 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),
+
## builtin/interpret-trailers.c ##
@@ builtin/interpret-trailers.c: static enum trailer_if_missing if_missing;
static int option_parse_where(const struct option *opt,
4: 19621fc01c ! 6: bd915fe4de parse-options: mark unused "opt" parameter in callbacks
@@ Commit message
Signed-off-by: Jeff King <peff@peff.net>
- ## builtin/checkout-index.c ##
-@@ builtin/checkout-index.c: static const char * const builtin_checkout_index_usage[] = {
- NULL
- };
-
--static int option_parse_stage(const struct option *opt,
-+static int option_parse_stage(const struct option *opt UNUSED,
- const char *arg, int unset)
- {
- BUG_ON_OPT_NEG(unset);
-
- ## builtin/fetch.c ##
-@@ builtin/fetch.c: static int git_fetch_config(const char *k, const char *v,
- return git_default_config(k, v, ctx, cb);
- }
-
--static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
-+static int parse_refmap_arg(const struct option *opt UNUSED,
-+ const char *arg, int unset)
- {
- BUG_ON_OPT_NEG(unset);
-
-
## builtin/gc.c ##
@@ builtin/gc.c: static void initialize_task_config(int schedule)
strbuf_release(&config_name);
@@ builtin/log.c: static int inline_callback(const struct option *opt, const char *
{
if (unset) {
string_list_clear(&extra_hdr, 0);
-@@ builtin/log.c: static int header_callback(const struct option *opt, const char *arg, int unset)
- return 0;
- }
-
--static int to_callback(const struct option *opt, const char *arg, int unset)
-+static int to_callback(const struct option *opt UNUSED, const char *arg,
-+ int unset)
- {
- if (unset)
- string_list_clear(&extra_to, 0);
-@@ builtin/log.c: static int to_callback(const struct option *opt, const char *arg, int unset)
- return 0;
- }
-
--static int cc_callback(const struct option *opt, const char *arg, int unset)
-+static int cc_callback(const struct option *opt UNUSED, const char *arg,
-+ int unset)
- {
- if (unset)
- string_list_clear(&extra_cc, 0);
## builtin/merge.c ##
@@ builtin/merge.c: static void append_strategy(struct strategy *s)
5: 2d6a3b9e1b = 7: 1720ddebc1 merge: do not pass unused opt->value parameter
6: 7669758cc7 = 8: a93f155f21 parse-options: add more BUG_ON() annotations
7: b80020daec ! 9: 529573e281 interpret-trailers: mark unused "unset" parameters in option callbacks
@@ Commit message
false). But none of these do.
So the code is fine as-is. But we'll want to mark the unused "unset"
- parameters to quiet -Wunused-parameter.
+ parameters to quiet -Wunused-parameter. I've also added a comment to
+ make this rather subtle situation more explicit.
Signed-off-by: Jeff King <peff@peff.net>
@@ builtin/interpret-trailers.c: static enum trailer_if_exists if_exists;
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
++ /* unset implies NULL arg, which is handled in our helper */
return trailer_set_where(opt->value, arg);
}
static int option_parse_if_exists(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
++ /* unset implies NULL arg, which is handled in our helper */
return trailer_set_if_exists(opt->value, arg);
}
static int option_parse_if_missing(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
++ /* unset implies NULL arg, which is handled in our helper */
return trailer_set_if_missing(opt->value, arg);
}
+
8: f21961ed23 = 10: 943161eaf2 parse-options: mark unused parameters in noop callback
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 01/10] merge: make xopts a strvec
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
@ 2023-08-31 21:17 ` Jeff King
2023-08-31 21:17 ` [PATCH v2 02/10] merge: simplify parsing of "-n" option Jeff King
` (8 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
The "xopts" variable uses a custom array with ALLOC_GROW(). Using a
strvec simplifies things a bit. We need fewer variables, and we can also
ditch our custom parseopt callback in favor of OPT_STRVEC().
As a bonus, this means that "--no-strategy-option", which was previously
a silent noop, now does something useful: like other list-like options,
it will clear the list of -X options seen so far. This matches the
behavior of revert/cherry-pick, which made the same change in fb60b9f37f
(sequencer: use struct strvec to store merge strategy options,
2023-04-10).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index de68910177..53e9fe70d9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -79,8 +79,7 @@ static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
static size_t use_strategies_nr, use_strategies_alloc;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+static struct strvec xopts = STRVEC_INIT;
static const char *branch;
static char *branch_mergeoptions;
static int verbosity;
@@ -242,17 +241,6 @@ static int option_parse_strategy(const struct option *opt,
return 0;
}
-static int option_parse_x(const struct option *opt,
- const char *arg, int unset)
-{
- if (unset)
- return 0;
-
- ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
- xopts[xopts_nr++] = xstrdup(arg);
- return 0;
-}
-
static int option_parse_n(const struct option *opt,
const char *arg, int unset)
{
@@ -287,8 +275,8 @@ static struct option builtin_merge_options[] = {
N_("verify that the named commit has a valid GPG signature")),
OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
N_("merge strategy to use"), option_parse_strategy),
- OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),
- N_("option for selected merge strategy"), option_parse_x),
+ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
OPT_CALLBACK('m', "message", &merge_msg, N_("message"),
N_("merge commit message (for a non-fast-forward merge)"),
option_parse_message),
@@ -749,9 +737,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
o.show_rename_progress =
show_progress == -1 ? isatty(2) : show_progress;
- for (x = 0; x < xopts_nr; x++)
- if (parse_merge_opt(&o, xopts[x]))
- die(_("unknown strategy option: -X%s"), xopts[x]);
+ for (x = 0; x < xopts.nr; x++)
+ if (parse_merge_opt(&o, xopts.v[x]))
+ die(_("unknown strategy option: -X%s"), xopts.v[x]);
o.branch1 = head_arg;
o.branch2 = merge_remote_util(remoteheads->item)->name;
@@ -777,7 +765,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
return clean ? 0 : 1;
} else {
return try_merge_command(the_repository,
- strategy, xopts_nr, xopts,
+ strategy, xopts.nr, xopts.v,
common, head_arg, remoteheads);
}
}
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 02/10] merge: simplify parsing of "-n" option
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
2023-08-31 21:17 ` [PATCH v2 01/10] merge: make xopts a strvec Jeff King
@ 2023-08-31 21:17 ` Jeff King
2023-09-02 6:20 ` René Scharfe
2023-08-31 21:17 ` [PATCH v2 03/10] format-patch: use OPT_STRING_LIST for to/cc options Jeff King
` (7 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
The "-n" option is implemented by an option callback, as it is really a
"reverse bool". If given, it sets show_diffstat to 0. In theory, when
negated, it would set the same flag to 1. But it's not possible to
trigger that, since short options cannot be negated.
So in practice this is really just a SET_INT to 0. Let's use that
instead, which shortens the code.
Note that negation here would do the wrong thing (as with any SET_INT
with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof
ourselves against somebody adding a long option name (which would make
it possible to negate). But there's not much point:
1. Nobody is going to do that, because the negated form already
exists, and is called "--stat" (which is defined separately so that
"--no-stat" works).
2. If they did, the BUG() check added by 3284b93862 (parse-options:
disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
that check is smart enough to realize that our short-only option is
OK).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 53e9fe70d9..21363b7985 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
return 0;
}
-static int option_parse_n(const struct option *opt,
- const char *arg, int unset)
-{
- BUG_ON_OPT_ARG(arg);
- show_diffstat = unset;
- return 0;
-}
-
static struct option builtin_merge_options[] = {
- OPT_CALLBACK_F('n', NULL, NULL, NULL,
- N_("do not show a diffstat at the end of the merge"),
- PARSE_OPT_NOARG, option_parse_n),
+ OPT_SET_INT('n', NULL, &show_diffstat,
+ N_("do not show a diffstat at the end of the merge"), 0),
OPT_BOOL(0, "stat", &show_diffstat,
N_("show a diffstat at the end of the merge")),
OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 03/10] format-patch: use OPT_STRING_LIST for to/cc options
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
2023-08-31 21:17 ` [PATCH v2 01/10] merge: make xopts a strvec Jeff King
2023-08-31 21:17 ` [PATCH v2 02/10] merge: simplify parsing of "-n" option Jeff King
@ 2023-08-31 21:17 ` Jeff King
2023-08-31 21:20 ` [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile Jeff King
` (6 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
The to_callback() and cc_callback() functions are identical to the
generic parse_opt_string_list() function (except that they don't handle
optional arguments, but that's OK because their callers do not use the
OPTARG flag).
Let's simplify the code by using OPT_STRING_LIST.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/log.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..fb90d43717 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1567,24 +1567,6 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
return 0;
}
-static int to_callback(const struct option *opt, const char *arg, int unset)
-{
- if (unset)
- string_list_clear(&extra_to, 0);
- else
- string_list_append(&extra_to, arg);
- return 0;
-}
-
-static int cc_callback(const struct option *opt, const char *arg, int unset)
-{
- if (unset)
- string_list_clear(&extra_cc, 0);
- else
- string_list_append(&extra_cc, arg);
- return 0;
-}
-
static int from_callback(const struct option *opt, const char *arg, int unset)
{
char **from = opt->value;
@@ -1957,8 +1939,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
OPT_GROUP(N_("Messaging")),
OPT_CALLBACK(0, "add-header", NULL, N_("header"),
N_("add email header"), header_callback),
- OPT_CALLBACK(0, "to", NULL, N_("email"), N_("add To: header"), to_callback),
- OPT_CALLBACK(0, "cc", NULL, N_("email"), N_("add Cc: header"), cc_callback),
+ OPT_STRING_LIST(0, "to", &extra_to, N_("email"), N_("add To: header")),
+ OPT_STRING_LIST(0, "cc", &extra_cc, N_("email"), N_("add Cc: header")),
OPT_CALLBACK_F(0, "from", &from, N_("ident"),
N_("set From address to <ident> (or committer ident if absent)"),
PARSE_OPT_OPTARG, from_callback),
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (2 preceding siblings ...)
2023-08-31 21:17 ` [PATCH v2 03/10] format-patch: use OPT_STRING_LIST for to/cc options Jeff King
@ 2023-08-31 21:20 ` Jeff King
2023-08-31 22:12 ` Junio C Hamano
2023-09-02 6:20 ` René Scharfe
2023-08-31 21:21 ` [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks Jeff King
` (5 subsequent siblings)
9 siblings, 2 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
Using --stage=all requires writing to tempfiles, since we cannot put
multiple stages into a single file. So --stage=all implies --temp.
But we do this by setting to_tempfile in the options callback for
--stage, rather than after all options have been parsed. This leads to
two bugs:
1. If you run "checkout-index --stage=all --stage=2", this should not
imply --temp, but it currently does. The callback cannot just unset
to_tempfile when it sees the "2" value, because it no longer knows
if its value was from the earlier --stage call, or if the user
specified --temp explicitly.
2. If you run "checkout-index --stage=all --no-temp", the --no-temp
will overwrite the earlier implied --temp. But this mode of
operation cannot work, and the command will fail with "<path>
already exists" when trying to write the higher stages.
We can fix both by lazily setting to_tempfile. We'll make it a tristate,
with -1 as "not yet given", and have --stage=all enable it only after
all options are parsed. Likewise, after all options are parsed we can
detect and reject the bogus "--no-temp" case.
Note that this does technically change the behavior for "--stage=all
--no-temp" for paths which have only one stage present (which
accidentally worked before, but is now forbidden). But this behavior was
never intended, and you'd have to go out of your way to try to trigger
it.
The new tests cover both cases, as well the general "--stage=all implies
--temp", as most of the other tests explicitly say "--temp". Ironically,
the test "checkout --temp within subdir" is the only one that _doesn't_
use "--temp", and so was implicitly covering this case. But it seems
reasonable to have a more explicit test alongside the other related
ones.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
One other bug-let I noticed in this function: it only looks at the first
byte of the stage argument, so "--stage=13" is the same as "--stage=1".
We could tighten this, but I wonder if anybody does something weird like
"--stage=2ours" or something. That seems pretty unlikely, but I'd still
prefer to do it as a separate patch (if at all).
builtin/checkout-index.c | 8 ++++++--
t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..6687a495ff 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -24,7 +24,7 @@
static int nul_term_line;
static int checkout_stage; /* default to checkout stage0 */
static int ignore_skip_worktree; /* default to 0 */
-static int to_tempfile;
+static int to_tempfile = -1;
static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
static struct checkout state = CHECKOUT_INIT;
@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,
BUG_ON_OPT_NEG(unset);
if (!strcmp(arg, "all")) {
- to_tempfile = 1;
checkout_stage = CHECKOUT_ALL;
} else {
int ch = arg[0];
@@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
state.base_dir = "";
state.base_dir_len = strlen(state.base_dir);
+ if (to_tempfile < 0)
+ to_tempfile = (checkout_stage == CHECKOUT_ALL);
+ if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
+ die("--stage=all and --no-temp are incompatible");
+
/*
* when --prefix is specified we do not want to update cache.
*/
diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh
index b16d69ca4a..8eb2ef44eb 100755
--- a/t/t2004-checkout-cache-temp.sh
+++ b/t/t2004-checkout-cache-temp.sh
@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '
test $(cat $s3) = tree3path1)
'
+test_expect_success '--stage=all implies --temp' '
+ rm -f path* .merge_* actual &&
+ git checkout-index --stage=all -- path1 &&
+ test_path_is_missing path1
+'
+
+test_expect_success 'overriding --stage=all resets implied --temp' '
+ rm -f path* .merge_* actual &&
+ git checkout-index --stage=all --stage=2 -- path1 &&
+ echo tree2path1 >expect &&
+ test_cmp expect path1
+'
+
+test_expect_success '--stage=all --no-temp is rejected' '
+ rm -f path* .merge_* actual &&
+ test_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&
+ grep -v "already exists" err &&
+ grep "stage=all and --no-temp are incompatible" err
+'
+
test_expect_success 'checkout some stages/one file to temporary files' '
rm -f path* .merge_* actual &&
git checkout-index --stage=all --temp -- path2 >actual &&
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (3 preceding siblings ...)
2023-08-31 21:20 ` [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile Jeff King
@ 2023-08-31 21:21 ` Jeff King
2023-09-02 7:34 ` René Scharfe
2023-08-31 21:21 ` [PATCH v2 06/10] parse-options: mark unused "opt" parameter " Jeff King
` (4 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
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>
---
builtin/checkout-index.c | 8 +++++---
builtin/describe.c | 6 ++++--
builtin/fast-export.c | 36 +++++++++++++++++++++---------------
builtin/fetch.c | 4 ++--
builtin/interpret-trailers.c | 12 ++++++------
builtin/pack-objects.c | 21 ++++++++++++---------
6 files changed, 50 insertions(+), 37 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 6687a495ff..6ef6ac4c2e 100644
--- 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 b28a4a1f82..718b5c3073 100644
--- 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 56dc69fac1..70aff515ac 100644
--- 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 8f93529505..fd134ba74d 100644
--- 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 c5e8345265..6aadce6a1e 100644
--- 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 d2a162d528..492372ee5d 100644
--- 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,
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (4 preceding siblings ...)
2023-08-31 21:21 ` [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks Jeff King
@ 2023-08-31 21:21 ` Jeff King
2023-09-02 10:12 ` René Scharfe
2023-08-31 21:21 ` [PATCH v2 07/10] merge: do not pass unused opt->value parameter Jeff King
` (3 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
The previous commit argued that parse-options callbacks should try to
use opt->value rather than touching globals directly. In some cases,
however, that's awkward to do. Some callbacks touch multiple variables,
or may even just call into an abstracted function that does so.
In some of these cases we _could_ convert them by stuffing the multiple
variables into a single struct and passing the struct pointer through
opt->value. But that may make other parts of the code less readable,
as the struct relationship has to be mentioned everywhere.
Let's just accept that these cases are special and leave them as-is. But
we do need to mark their "opt" parameters to satisfy -Wunused-parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/gc.c | 2 +-
builtin/log.c | 10 ++++++----
builtin/merge.c | 2 +-
builtin/pack-objects.c | 6 +++---
builtin/read-tree.c | 2 +-
builtin/update-index.c | 4 ++--
6 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 369bd43fb2..b842349d86 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
strbuf_release(&config_name);
}
-static int task_option_parse(const struct option *opt,
+static int task_option_parse(const struct option *opt UNUSED,
const char *arg, int unset)
{
int i, num_selected = 0;
diff --git a/builtin/log.c b/builtin/log.c
index fb90d43717..3599063554 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
-static int clear_decorations_callback(const struct option *opt,
- const char *arg, int unset)
+static int clear_decorations_callback(const struct option *opt UNUSED,
+ const char *arg, int unset)
{
string_list_clear(&decorate_refs_include, 0);
string_list_clear(&decorate_refs_exclude, 0);
use_default_decoration_filter = 0;
return 0;
}
-static int decorate_callback(const struct option *opt, const char *arg, int unset)
+static int decorate_callback(const struct option *opt UNUSED, const char *arg,
+ int unset)
{
if (unset)
decoration_style = 0;
@@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
return 0;
}
-static int header_callback(const struct option *opt, const char *arg, int unset)
+static int header_callback(const struct option *opt UNUSED, const char *arg,
+ int unset)
{
if (unset) {
string_list_clear(&extra_hdr, 0);
diff --git a/builtin/merge.c b/builtin/merge.c
index 21363b7985..0436986dab 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
use_strategies[use_strategies_nr++] = s;
}
-static int option_parse_strategy(const struct option *opt,
+static int option_parse_strategy(const struct option *opt UNUSED,
const char *name, int unset)
{
if (unset)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 492372ee5d..91b4b7c177 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
show_object(obj, name, data);
}
-static int option_parse_missing_action(const struct option *opt,
+static int option_parse_missing_action(const struct option *opt UNUSED,
const char *arg, int unset)
{
assert(arg);
@@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
return 0;
}
-static int option_parse_unpack_unreachable(const struct option *opt,
+static int option_parse_unpack_unreachable(const struct option *opt UNUSED,
const char *arg, int unset)
{
if (unset) {
@@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
return 0;
}
-static int option_parse_cruft_expiration(const struct option *opt,
+static int option_parse_cruft_expiration(const struct option *opt UNUSED,
const char *arg, int unset)
{
if (unset) {
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 1fec702a04..8196ca9dd8 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
NULL
};
-static int index_output_cb(const struct option *opt, const char *arg,
+static int index_output_cb(const struct option *opt UNUSED, const char *arg,
int unset)
{
BUG_ON_OPT_NEG(unset);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index aee3cb8cbd..59acae3336 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
return 0;
}
-static int resolve_undo_clear_callback(const struct option *opt,
+static int resolve_undo_clear_callback(const struct option *opt UNUSED,
const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
}
static enum parse_opt_result cacheinfo_callback(
- struct parse_opt_ctx_t *ctx, const struct option *opt,
+ struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
const char *arg, int unset)
{
struct object_id oid;
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 07/10] merge: do not pass unused opt->value parameter
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (5 preceding siblings ...)
2023-08-31 21:21 ` [PATCH v2 06/10] parse-options: mark unused "opt" parameter " Jeff King
@ 2023-08-31 21:21 ` Jeff King
2023-08-31 21:21 ` [PATCH v2 08/10] parse-options: add more BUG_ON() annotations Jeff King
` (2 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
The option_parse_strategy() callback does not look at opt->value;
instead it calls append_strategy(), which manipulates the global
use_strategies array directly. But the OPT_CALLBACK declaration assigns
"&use_strategies" to opt->value.
One could argue this is good, as it tells the reader what we generally
expect the callback to do. But it is also bad, because it can mislead
you into thinking that swapping out "&use_strategies" there might have
any effect. Let's switch it to pass NULL (which is what every other
"does not bother to look at opt->value" callback does). If you want to
know what the callback does, it's easy to read the function itself.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 0436986dab..545da0c8a1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {
OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
OPT_BOOL(0, "verify-signatures", &verify_signatures,
N_("verify that the named commit has a valid GPG signature")),
- OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
+ OPT_CALLBACK('s', "strategy", NULL, N_("strategy"),
N_("merge strategy to use"), option_parse_strategy),
OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
N_("option for selected merge strategy")),
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 08/10] parse-options: add more BUG_ON() annotations
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (6 preceding siblings ...)
2023-08-31 21:21 ` [PATCH v2 07/10] merge: do not pass unused opt->value parameter Jeff King
@ 2023-08-31 21:21 ` Jeff King
2023-08-31 21:22 ` [PATCH v2 09/10] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
2023-08-31 21:22 ` [PATCH v2 10/10] parse-options: mark unused parameters in noop callback Jeff King
9 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
These callbacks are similar to the ones touched by 517fe807d6 (assert
NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were
either missed in that commit (the one in add.c) or were added later (the
one in log.c).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/add.c | 2 ++
builtin/log.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/builtin/add.c b/builtin/add.c
index 4b0dd798df..cf59108523 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -232,6 +232,8 @@ static char *chmod_arg;
static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
{
+ BUG_ON_OPT_ARG(arg);
+
/* if we are told to ignore, we are not adding removals */
*(int *)opt->value = !unset ? 0 : 1;
return 0;
diff --git a/builtin/log.c b/builtin/log.c
index 3599063554..190e1952e9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
static int clear_decorations_callback(const struct option *opt UNUSED,
const char *arg, int unset)
{
+ BUG_ON_OPT_NEG(unset);
+ BUG_ON_OPT_ARG(arg);
string_list_clear(&decorate_refs_include, 0);
string_list_clear(&decorate_refs_exclude, 0);
use_default_decoration_filter = 0;
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 09/10] interpret-trailers: mark unused "unset" parameters in option callbacks
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (7 preceding siblings ...)
2023-08-31 21:21 ` [PATCH v2 08/10] parse-options: add more BUG_ON() annotations Jeff King
@ 2023-08-31 21:22 ` Jeff King
2023-08-31 21:22 ` [PATCH v2 10/10] parse-options: mark unused parameters in noop callback Jeff King
9 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
There are a few parse-option callbacks that do not look at their "unset"
parameters, but also do not set PARSE_OPT_NONEG. At first glance this
seems like a bug, as we'd ignore "--no-if-exists", etc.
But they do work fine, because when "unset" is true, then "arg" is NULL.
And all three functions pass "arg" on to helper functions which do the
right thing with the NULL.
Note that this shortcut would not be correct if any callback used
PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
false). But none of these do.
So the code is fine as-is. But we'll want to mark the unused "unset"
parameters to quiet -Wunused-parameter. I've also added a comment to
make this rather subtle situation more explicit.
Signed-off-by: Jeff King <peff@peff.net>
---
I looked at another BUG_ON here, but it was just so ugly and one-off
that I preferred just adding the comments.
builtin/interpret-trailers.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6aadce6a1e..a110e69f83 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,20 +24,23 @@ static enum trailer_if_exists if_exists;
static enum trailer_if_missing if_missing;
static int option_parse_where(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
+ /* unset implies NULL arg, which is handled in our helper */
return trailer_set_where(opt->value, arg);
}
static int option_parse_if_exists(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
+ /* unset implies NULL arg, which is handled in our helper */
return trailer_set_if_exists(opt->value, arg);
}
static int option_parse_if_missing(const struct option *opt,
- const char *arg, int unset)
+ const char *arg, int unset UNUSED)
{
+ /* unset implies NULL arg, which is handled in our helper */
return trailer_set_if_missing(opt->value, arg);
}
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
` (8 preceding siblings ...)
2023-08-31 21:22 ` [PATCH v2 09/10] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
@ 2023-08-31 21:22 ` Jeff King
2023-09-02 11:37 ` René Scharfe
9 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-08-31 21:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, René Scharfe
Unsurprisingly, the noop options callback doesn't bother to look at any
of its parameters. Let's mark them so that -Wunused-parameter does not
complain.
Another option would be to drop the callback and have parse-options
itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
real benefit.
Signed-off-by: Jeff King <peff@peff.net>
---
parse-options-cb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..bdc7fae497 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
return 0;
}
-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
+int parse_opt_noop_cb(const struct option *opt UNUSED,
+ const char *arg UNUSED,
+ int unset UNUSED)
{
return 0;
}
--
2.42.0.561.gaa987ecc69
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile
2023-08-31 21:20 ` [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile Jeff King
@ 2023-08-31 22:12 ` Junio C Hamano
2023-09-02 6:20 ` René Scharfe
1 sibling, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-08-31 22:12 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
Jeff King <peff@peff.net> writes:
> Using --stage=all requires writing to tempfiles, since we cannot put
> multiple stages into a single file. So --stage=all implies --temp.
This one is new, and it does read really well. Thanks.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option
2023-08-31 21:17 ` [PATCH v2 02/10] merge: simplify parsing of "-n" option Jeff King
@ 2023-09-02 6:20 ` René Scharfe
2023-09-05 6:43 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: René Scharfe @ 2023-09-02 6:20 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
Am 31.08.23 um 23:17 schrieb Jeff King:
> The "-n" option is implemented by an option callback, as it is really a
> "reverse bool". If given, it sets show_diffstat to 0. In theory, when
> negated, it would set the same flag to 1. But it's not possible to
> trigger that, since short options cannot be negated.
>
> So in practice this is really just a SET_INT to 0. Let's use that
> instead, which shortens the code.
>
> Note that negation here would do the wrong thing (as with any SET_INT
> with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof
> ourselves against somebody adding a long option name (which would make
> it possible to negate). But there's not much point:
>
> 1. Nobody is going to do that, because the negated form already
> exists, and is called "--stat" (which is defined separately so that
> "--no-stat" works).
>
> 2. If they did, the BUG() check added by 3284b93862 (parse-options:
> disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
> that check is smart enough to realize that our short-only option is
> OK).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/merge.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 53e9fe70d9..21363b7985 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
> return 0;
> }
>
> -static int option_parse_n(const struct option *opt,
> - const char *arg, int unset)
> -{
> - BUG_ON_OPT_ARG(arg);
> - show_diffstat = unset;
> - return 0;
> -}
Great -- the less custom callbacks, the better.
> -
> static struct option builtin_merge_options[] = {
> - OPT_CALLBACK_F('n', NULL, NULL, NULL,
> - N_("do not show a diffstat at the end of the merge"),
> - PARSE_OPT_NOARG, option_parse_n),
> + OPT_SET_INT('n', NULL, &show_diffstat,
> + N_("do not show a diffstat at the end of the merge"), 0),
> OPT_BOOL(0, "stat", &show_diffstat,
> N_("show a diffstat at the end of the merge")),
Makes it easier to see that we can replace the two complementary
definitions with a single one:
OPT_NEGBIT('n', "no-stat",
N_("do not show a diffstat at the end of the merge"), 1),
Which is a separate topic, of course. And if we did that, however, ...
> OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
... we wouldn't be able to use OPT_ALIAS here anymore (which we arguably
should) because it doesn't allow referencing the negated (or
"positivated") form -- but that is yet another separate topic.
René
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile
2023-08-31 21:20 ` [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile Jeff King
2023-08-31 22:12 ` Junio C Hamano
@ 2023-09-02 6:20 ` René Scharfe
2023-09-05 7:12 ` [PATCH v3 " Jeff King
1 sibling, 1 reply; 47+ messages in thread
From: René Scharfe @ 2023-09-02 6:20 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
Am 31.08.23 um 23:20 schrieb Jeff King:
> @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> state.base_dir = "";
> state.base_dir_len = strlen(state.base_dir);
>
> + if (to_tempfile < 0)
> + to_tempfile = (checkout_stage == CHECKOUT_ALL);
> + if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
> + die("--stage=all and --no-temp are incompatible");
How about making this message translatable from the start and following
the convention from 12909b6b8a (i18n: turn "options are incompatible"
into "cannot be used together", 2022-01-05) to reuse the existing
translations?
René
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks
2023-08-31 21:21 ` [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks Jeff King
@ 2023-09-02 7:34 ` René Scharfe
2023-09-05 6:52 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: René Scharfe @ 2023-09-02 7:34 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
Am 31.08.23 um 23:21 schrieb Jeff King:
> 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!).
Which allows turning global variables into local ones later, so unlocks
more cleanup potential.
> 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).
Makes sense. The types used below look OK to me.
> 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>
> ---
> builtin/checkout-index.c | 8 +++++---
> builtin/describe.c | 6 ++++--
> builtin/fast-export.c | 36 +++++++++++++++++++++---------------
> builtin/fetch.c | 4 ++--
> builtin/interpret-trailers.c | 12 ++++++------
> builtin/pack-objects.c | 21 ++++++++++++---------
> 6 files changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 6687a495ff..6ef6ac4c2e 100644
> --- 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 b28a4a1f82..718b5c3073 100644
> --- 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 56dc69fac1..70aff515ac 100644
> --- 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 8f93529505..fd134ba74d 100644
> --- 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 c5e8345265..6aadce6a1e 100644
> --- 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);
> }
Not your fault, but these all silently exit if "arg" contains an
unrecognized value. Reporting the error would be better.
> 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),
And I wonder if "action" should be replaced by "(after|before|end|start)",
"(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and
"(doNothing|add)", respectively. Gets a bit long in the middle, but would
be more helpful. #leftoverbits
> 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 d2a162d528..492372ee5d 100644
> --- 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,
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
2023-08-31 21:21 ` [PATCH v2 06/10] parse-options: mark unused "opt" parameter " Jeff King
@ 2023-09-02 10:12 ` René Scharfe
2023-09-05 7:05 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: René Scharfe @ 2023-09-02 10:12 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
Am 31.08.23 um 23:21 schrieb Jeff King:
> The previous commit argued that parse-options callbacks should try to
> use opt->value rather than touching globals directly. In some cases,
> however, that's awkward to do. Some callbacks touch multiple variables,
> or may even just call into an abstracted function that does so.
>
> In some of these cases we _could_ convert them by stuffing the multiple
> variables into a single struct and passing the struct pointer through
> opt->value. But that may make other parts of the code less readable,
> as the struct relationship has to be mentioned everywhere.
Does that imply you'd be willing to use other methods? Let's find out
below. :)
>
> Let's just accept that these cases are special and leave them as-is. But
> we do need to mark their "opt" parameters to satisfy -Wunused-parameter.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/gc.c | 2 +-
> builtin/log.c | 10 ++++++----
> builtin/merge.c | 2 +-
> builtin/pack-objects.c | 6 +++---
> builtin/read-tree.c | 2 +-
> builtin/update-index.c | 4 ++--
> 6 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 369bd43fb2..b842349d86 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
> strbuf_release(&config_name);
> }
>
> -static int task_option_parse(const struct option *opt,
> +static int task_option_parse(const struct option *opt UNUSED,
Only the global variable "tasks" seems to be used in here if you don't
count the constant "TASK__COUNT", so you could pass it in. This could
also be converted to OPT_STRING_LIST with parsing and duplicate checking
done later.
And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
on error, but that's a different matter.
> const char *arg, int unset)
> {
> int i, num_selected = 0;
> diff --git a/builtin/log.c b/builtin/log.c
> index fb90d43717..3599063554 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
> static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
> static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
>
> -static int clear_decorations_callback(const struct option *opt,
> - const char *arg, int unset)
> +static int clear_decorations_callback(const struct option *opt UNUSED,
> + const char *arg, int unset)
> {
> string_list_clear(&decorate_refs_include, 0);
> string_list_clear(&decorate_refs_exclude, 0);
> use_default_decoration_filter = 0;
> return 0;
> }
>
Meta: Why do we get seven lines of context in an -U3 patch here? Did
you use --inter-hunk-context?
This patch would be better viewed with --function-context to see that
the callbacks change multiple variables or do other funky things. Only
doubles the line count.
> -static int decorate_callback(const struct option *opt, const char *arg, int unset)
> +static int decorate_callback(const struct option *opt UNUSED, const char *arg,
> + int unset)
> {
> if (unset)
> decoration_style = 0;
Sets decoration_style and decoration_given. Never sets decoration_given
to a negative value. This could be used to initialize decoration_style
to -1 and set decoration_given after parsing, then you could pass in
just decoration_style.
> @@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -static int header_callback(const struct option *opt, const char *arg, int unset)
> +static int header_callback(const struct option *opt UNUSED, const char *arg,
Sorts strings into three string_lists based on prefix. Could become a
vanilla OPT_STRING_LIST with parsing and sorting done later, if it
wouldn't interact with to_callback() and cc_callback().
(--function-context is not enough to see that, it requires a look at the
definition of those other functions as well.)
> + int unset)
> {
> if (unset) {
> string_list_clear(&extra_hdr, 0);
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 21363b7985..0436986dab 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
> use_strategies[use_strategies_nr++] = s;
> }
>
> -static int option_parse_strategy(const struct option *opt,
> +static int option_parse_strategy(const struct option *opt UNUSED,
Could be an OPT_STRING_LIST and parsing done later. Except that
--no-strategy does nothing, which is weird.
> const char *name, int unset)
> {
> if (unset)
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 492372ee5d..91b4b7c177 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
> show_object(obj, name, data);
> }
>
> -static int option_parse_missing_action(const struct option *opt,
> +static int option_parse_missing_action(const struct option *opt UNUSED,
Sets the enum arg_missing_action, fetch_if_missing and fn_show_object.
Could be changed to set only the enum and then you could pass it in.
> const char *arg, int unset)
> {
> assert(arg);
> @@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
> return 0;
> }
>
> -static int option_parse_unpack_unreachable(const struct option *opt,
> +static int option_parse_unpack_unreachable(const struct option *opt UNUSED,
Sets both unpack_unreachable and unpack_unreachable_expiration. Perhaps
could set only unpack_unreachable_expiration and use TIME_MAX if no
argument is given, then set unpack_unreachable after parsing? Not sure.
> const char *arg, int unset)
> {
> if (unset) {
> @@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
> return 0;
> }
>
> -static int option_parse_cruft_expiration(const struct option *opt,
> +static int option_parse_cruft_expiration(const struct option *opt UNUSED,
Does the same as option_parse_unpack_unreachable(), just with two
different variables. And interacts with --cruft.
> const char *arg, int unset)
> {
> if (unset) {
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 1fec702a04..8196ca9dd8 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
> NULL
> };
>
> -static int index_output_cb(const struct option *opt, const char *arg,
> +static int index_output_cb(const struct option *opt UNUSED, const char *arg,
Calls set_alternate_index_output(). Could become an OPT_STRING_F if we
assume that the last value wins and earlier calls have no side effects.
> int unset)
> {
> BUG_ON_OPT_NEG(unset);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index aee3cb8cbd..59acae3336 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
> return 0;
> }
>
> -static int resolve_undo_clear_callback(const struct option *opt,
> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,
Affects the_index. This option (--clear-resolve-undo) is not mentioned
in Documentation/, by the way. Not sure if it interacts with other
callbacks, like the one below. Otherwise could be an OPT_BOOL and its
action done after parsing. Perhaps pass in &the_index?
> const char *arg, int unset)
> {
> BUG_ON_OPT_NEG(unset);
> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
> }
>
> static enum parse_opt_result cacheinfo_callback(
> - struct parse_opt_ctx_t *ctx, const struct option *opt,
> + struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
A low-level callback for a change. Affects the_index. Pass it in?
add_cacheinfo() would need to grow a parameter to receive it. It would
document that this option directly changes the_index.
> const char *arg, int unset)
> {
> struct object_id oid;
Phew, looks like that's stuff for a whole new series or two!
René
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
2023-08-31 21:22 ` [PATCH v2 10/10] parse-options: mark unused parameters in noop callback Jeff King
@ 2023-09-02 11:37 ` René Scharfe
2023-09-05 7:09 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: René Scharfe @ 2023-09-02 11:37 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
Am 31.08.23 um 23:22 schrieb Jeff King:
> Unsurprisingly, the noop options callback doesn't bother to look at any
> of its parameters. Let's mark them so that -Wunused-parameter does not
> complain.
>
> Another option would be to drop the callback and have parse-options
> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
> real benefit.
I'm not sure about that -- we don't need to add special flags or option
types to drop parse_opt_noop_cb().
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> parse-options-cb.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index a24521dee0..bdc7fae497 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
> +int parse_opt_noop_cb(const struct option *opt UNUSED,
> + const char *arg UNUSED,
> + int unset UNUSED)
> {
> return 0;
> }
Your patch is simple and makes sense. The one below is more
complicated, but leaves the code in a slightly better state. I'd go
with that variant if I'd had to add OPT_NOOP_NOARG today, and I
slightly prefer it, but similar to you think that the benefit is low
(though non-zero). So I'm unsure if that's enough to be worth it or
just bikeshedding with some hindsight..
René
--- >8 ---
Subject: [PATCH] parse-options: let NULL callback be a noop
Allow OPTION_CALLBACK options to have a NULL callback function pointer
and do nothing in that case, yet still handle arguments as usual. Use
this to replace parse_opt_noop_cb().
We lose the ability to distinguish between a forgotten function pointer
and intentional noop, but that will be caught by a compiler warning
about an unused function in many cases and having an option do nothing
is a benign failure mode.
We also lose the ability to add a warning to the callback, but we
haven't done that since it was added by 6acec0380b (parseopt: add
OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon. We can
add a callback back when we want to show a warning.
By letting go of features we don't need this patch simplifies the
parse-options API and gets rid of an exported empty function.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options-cb.c | 5 -----
parse-options.c | 7 +++----
parse-options.h | 2 --
3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..e79cd54ec2 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,11 +227,6 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
return 0;
}
-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
-{
- return 0;
-}
-
/**
* Recreates the command-line option in the strbuf.
*/
diff --git a/parse-options.c b/parse-options.c
index 76d2e76b49..5be8de93f5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -201,8 +201,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
}
if (opt->callback)
return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;
- else
+ if (opt->ll_callback)
return (*opt->ll_callback)(p, opt, p_arg, p_unset);
+ return 0;
}
case OPTION_INTEGER:
if (unset) {
@@ -494,9 +495,7 @@ static void parse_options_check(const struct option *opts)
optbug(opts, "should not accept an argument");
break;
case OPTION_CALLBACK:
- if (!opts->callback && !opts->ll_callback)
- optbug(opts, "OPTION_CALLBACK needs one callback");
- else if (opts->callback && opts->ll_callback)
+ if (opts->callback && opts->ll_callback)
optbug(opts, "OPTION_CALLBACK can't have two callbacks");
break;
case OPTION_LOWLEVEL_CALLBACK:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..41bb47120d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -348,7 +348,6 @@ struct option {
.long_name = (l), \
.help = N_("no-op (backward compatibility)"), \
.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, \
- .callback = parse_opt_noop_cb, \
}
#define OPT_ALIAS(s, l, source_long_name) { \
@@ -490,7 +489,6 @@ int parse_opt_commit(const struct option *, const char *, int);
int parse_opt_tertiary(const struct option *, const char *, int);
int parse_opt_string_list(const struct option *, const char *, int);
int parse_opt_strvec(const struct option *, const char *, int);
-int parse_opt_noop_cb(const struct option *, const char *, int);
int parse_opt_passthru(const struct option *, const char *, int);
int parse_opt_passthru_argv(const struct option *, const char *, int);
/* value is enum branch_track* */
--
2.42.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option
2023-09-02 6:20 ` René Scharfe
@ 2023-09-05 6:43 ` Jeff King
0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-09-05 6:43 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
On Sat, Sep 02, 2023 at 08:20:28AM +0200, René Scharfe wrote:
> > static struct option builtin_merge_options[] = {
> > - OPT_CALLBACK_F('n', NULL, NULL, NULL,
> > - N_("do not show a diffstat at the end of the merge"),
> > - PARSE_OPT_NOARG, option_parse_n),
> > + OPT_SET_INT('n', NULL, &show_diffstat,
> > + N_("do not show a diffstat at the end of the merge"), 0),
> > OPT_BOOL(0, "stat", &show_diffstat,
> > N_("show a diffstat at the end of the merge")),
>
> Makes it easier to see that we can replace the two complementary
> definitions with a single one:
>
> OPT_NEGBIT('n', "no-stat",
> N_("do not show a diffstat at the end of the merge"), 1),
>
> Which is a separate topic, of course. And if we did that, however, ...
Ah, I thought we had a "reverse bool" of some kind, but I couldn't find
it. NEGBIT was what I was looking for. But yeah, I agree it gets more
complicated with the various aliases. I think what I have here is a good
stopping point for this series, but if you want to go further on it, be
my guest. :)
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks
2023-09-02 7:34 ` René Scharfe
@ 2023-09-05 6:52 ` Jeff King
0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-09-05 6:52 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote:
> > 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);
> > }
>
> Not your fault, but these all silently exit if "arg" contains an
> unrecognized value. Reporting the error would be better.
Hmm, yeah. On the config side, git_trailer_default_config() issues a
warning after the functions return -1. I'd have guessed we at least
printed a usage message or something here, but we don't even do that.
You get a silent 129 exit code.
Gross, but it's something I think we should fix separately, as it's
orthogonal to this series.
> > 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),
>
> And I wonder if "action" should be replaced by "(after|before|end|start)",
> "(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and
> "(doNothing|add)", respectively. Gets a bit long in the middle, but would
> be more helpful. #leftoverbits
I don't have a strong opinion. It is sometimes nice to provide more
detail to save the user having to look it up separately. But sometimes
those details can be overwhelming and make it hard to read, especially
if they grow over time. I'm thinking less of "-h" output and more of
people like to put:
git foo [--optionA|--optionB|--optionC]
and so on, until eventually the options block is like 4 lines long. Just
saying [options] and then listing them is more friendly. I'm not sure if
we're approaching that kind of problem here or not.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
2023-09-02 10:12 ` René Scharfe
@ 2023-09-05 7:05 ` Jeff King
2023-09-19 7:42 ` René Scharfe
0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-09-05 7:05 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:
> Am 31.08.23 um 23:21 schrieb Jeff King:
> > The previous commit argued that parse-options callbacks should try to
> > use opt->value rather than touching globals directly. In some cases,
> > however, that's awkward to do. Some callbacks touch multiple variables,
> > or may even just call into an abstracted function that does so.
> >
> > In some of these cases we _could_ convert them by stuffing the multiple
> > variables into a single struct and passing the struct pointer through
> > opt->value. But that may make other parts of the code less readable,
> > as the struct relationship has to be mentioned everywhere.
>
> Does that imply you'd be willing to use other methods? Let's find out
> below. :)
Well, I'm not necessarily _opposed_. :) Mostly my cutoff was cases where
the end result was not obviously and immediately more readable. It is
not that big a deal to add an UNUSED annotation. My main goal was to use
the opportunity to check that we aren't papering over an obvious bug.
So for example...
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 369bd43fb2..b842349d86 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
> > strbuf_release(&config_name);
> > }
> >
> > -static int task_option_parse(const struct option *opt,
> > +static int task_option_parse(const struct option *opt UNUSED,
>
> Only the global variable "tasks" seems to be used in here if you don't
> count the constant "TASK__COUNT", so you could pass it in. This could
> also be converted to OPT_STRING_LIST with parsing and duplicate checking
> done later.
...in many cases things can be simplified by parsing into a string, and
then validating or acting on the string later. And sometimes that's even
a better strategy (because it lets the arg parsing handle all the "last
one wins" logic and we just get the result).
But it can also make the code harder to follow, too, because it's now
split it into segments (although one is mostly declarative, which is
nice).
So I generally tried to err on the side of not touching working
code. Both to avoid breaking it, but also to keep the focus on the goal
of the series. And I think that applies to a lot of the other cases you
mentioned below (I won't respond to each; I think some of them could be
fine, but it also feels like writing and review effort for not much
gain. I'm not opposed if you want to dig into them, though).
> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
> on error, but that's a different matter.
Yeah, that doesn't make sense at all.
> > -static int clear_decorations_callback(const struct option *opt,
> > - const char *arg, int unset)
> > +static int clear_decorations_callback(const struct option *opt UNUSED,
> > + const char *arg, int unset)
> > {
> > string_list_clear(&decorate_refs_include, 0);
> > string_list_clear(&decorate_refs_exclude, 0);
> > use_default_decoration_filter = 0;
> > return 0;
> > }
> >
>
> Meta: Why do we get seven lines of context in an -U3 patch here? Did
> you use --inter-hunk-context?
Yes, I set diff.interhunkcontext=1 in my config (and have for many
years; I think this is the first time anybody noticed in a patch). My
rationale is that with "1" the output is never longer, since you save
the hunk header. It is a little funny in this case, since the two
changes aren't really connected.
> This patch would be better viewed with --function-context to see that
> the callbacks change multiple variables or do other funky things. Only
> doubles the line count.
I do sometimes use options like that to make a patch more readable, if I
notice the difference. I didn't happen to in this case (though I don't
disagree with you). As a reviewer, I typically apply and run "git show"
myself if I want to dig more (or just go directly look at the preimage
in my local repo, of course).
> > -static int option_parse_strategy(const struct option *opt,
> > +static int option_parse_strategy(const struct option *opt UNUSED,
>
> Could be an OPT_STRING_LIST and parsing done later. Except that
> --no-strategy does nothing, which is weird.
Yeah, I think the handling of "unset" is a bug (similar to the xopts one
fixed earlier).
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
2023-09-02 11:37 ` René Scharfe
@ 2023-09-05 7:09 ` Jeff King
2023-09-07 20:20 ` René Scharfe
0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-09-05 7:09 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
On Sat, Sep 02, 2023 at 01:37:08PM +0200, René Scharfe wrote:
> --- >8 ---
> Subject: [PATCH] parse-options: let NULL callback be a noop
>
> Allow OPTION_CALLBACK options to have a NULL callback function pointer
> and do nothing in that case, yet still handle arguments as usual. Use
> this to replace parse_opt_noop_cb().
>
> We lose the ability to distinguish between a forgotten function pointer
> and intentional noop, but that will be caught by a compiler warning
> about an unused function in many cases and having an option do nothing
> is a benign failure mode.
Yeah, my concern would be missing an accidental NULL here. I guess that
is pretty unlikely in practice, though. I'd guess the worst case would
be somebody accidentally putting the function into the "opt->value" slot
where the compiler might then think it is used (though I don't know
off-hand if it would complain about an implicit conversion of a function
pointer into a "void *").
> We also lose the ability to add a warning to the callback, but we
> haven't done that since it was added by 6acec0380b (parseopt: add
> OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon. We can
> add a callback back when we want to show a warning.
>
> By letting go of features we don't need this patch simplifies the
> parse-options API and gets rid of an exported empty function.
Your patch looks correct to me. I probably wouldn't have bothered to
write it, but now you did. :) My inclination would be to go with my
dumb-simple one for this series, and it looks like you may have
collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups
on top that could make their own series.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 04/10] checkout-index: delay automatic setting of to_tempfile
2023-09-02 6:20 ` René Scharfe
@ 2023-09-05 7:12 ` Jeff King
0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-09-05 7:12 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
On Sat, Sep 02, 2023 at 08:20:43AM +0200, René Scharfe wrote:
> Am 31.08.23 um 23:20 schrieb Jeff King:
> > @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> > state.base_dir = "";
> > state.base_dir_len = strlen(state.base_dir);
> >
> > + if (to_tempfile < 0)
> > + to_tempfile = (checkout_stage == CHECKOUT_ALL);
> > + if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
> > + die("--stage=all and --no-temp are incompatible");
>
> How about making this message translatable from the start and following
> the convention from 12909b6b8a (i18n: turn "options are incompatible"
> into "cannot be used together", 2022-01-05) to reuse the existing
> translations?
Good catch. I forgot that we had standardized some of these. The other
error messages in the file aren't translated, but I don't think that's
an intentional choice (even though it is plumbing). Some of them are
obviously quite old and don't match our usual style (like starting with
"checkout-index: ").
Rather than re-send the whole series, I _think_ this is the only patch I
would change in a re-roll (if you buy me "sure, go ahead and send it on
top" evasions in my other responses).
So here's a replacement patch 4 that fixes up the message.
-- >8 --
Subject: checkout-index: delay automatic setting of to_tempfile
Using --stage=all requires writing to tempfiles, since we cannot put
multiple stages into a single file. So --stage=all implies --temp.
But we do so by setting to_tempfile in the options callback for --stage,
rather than after all options have been parsed. This leads to two bugs:
1. If you run "checkout-index --stage=all --stage=2", this should not
imply --temp, but it currently does. The callback cannot just unset
to_tempfile when it sees the "2" value, because it no longer knows
if its value was from the earlier --stage call, or if the user
specified --temp explicitly.
2. If you run "checkout-index --stage=all --no-temp", the --no-temp
will overwrite the earlier implied --temp. But this mode of
operation cannot work, and the command will fail with "<path>
already exists" when trying to write the higher stages.
We can fix both by lazily setting to_tempfile. We'll make it a tristate,
with -1 as "not yet given", and have --stage=all enable it only after
all options are parsed. Likewise, after all options are parsed we can
detect and reject the bogus "--no-temp" case.
Note that this does technically change the behavior for "--stage=all
--no-temp" for paths which have only one stage present (which
accidentally worked before, but is now forbidden). But this behavior was
never intended, and you'd have to go out of your way to try to trigger
it.
The new tests cover both cases, as well the general "--stage=all implies
--temp", as most of the other tests explicitly say "--temp". Ironically,
the test "checkout --temp within subdir" is the only one that _doesn't_
use "--temp", and so was implicitly covering this case. But it seems
reasonable to have a more explicit test alongside the other related
ones.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/checkout-index.c | 9 +++++++--
t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..526c210bcb 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -24,7 +24,7 @@
static int nul_term_line;
static int checkout_stage; /* default to checkout stage0 */
static int ignore_skip_worktree; /* default to 0 */
-static int to_tempfile;
+static int to_tempfile = -1;
static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
static struct checkout state = CHECKOUT_INIT;
@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,
BUG_ON_OPT_NEG(unset);
if (!strcmp(arg, "all")) {
- to_tempfile = 1;
checkout_stage = CHECKOUT_ALL;
} else {
int ch = arg[0];
@@ -269,6 +268,12 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
state.base_dir = "";
state.base_dir_len = strlen(state.base_dir);
+ if (to_tempfile < 0)
+ to_tempfile = (checkout_stage == CHECKOUT_ALL);
+ if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--stage=all", "--no-temp");
+
/*
* when --prefix is specified we do not want to update cache.
*/
diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh
index b16d69ca4a..45dd1bc858 100755
--- a/t/t2004-checkout-cache-temp.sh
+++ b/t/t2004-checkout-cache-temp.sh
@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '
test $(cat $s3) = tree3path1)
'
+test_expect_success '--stage=all implies --temp' '
+ rm -f path* .merge_* actual &&
+ git checkout-index --stage=all -- path1 &&
+ test_path_is_missing path1
+'
+
+test_expect_success 'overriding --stage=all resets implied --temp' '
+ rm -f path* .merge_* actual &&
+ git checkout-index --stage=all --stage=2 -- path1 &&
+ echo tree2path1 >expect &&
+ test_cmp expect path1
+'
+
+test_expect_success '--stage=all --no-temp is rejected' '
+ rm -f path* .merge_* actual &&
+ test_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&
+ grep -v "already exists" err &&
+ grep "options .--stage=all. and .--no-temp. cannot be used together" err
+'
+
test_expect_success 'checkout some stages/one file to temporary files' '
rm -f path* .merge_* actual &&
git checkout-index --stage=all --temp -- path2 >actual &&
--
2.42.0.567.gc396a4a104
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
2023-09-05 7:09 ` Jeff King
@ 2023-09-07 20:20 ` René Scharfe
0 siblings, 0 replies; 47+ messages in thread
From: René Scharfe @ 2023-09-07 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Am 05.09.23 um 09:09 schrieb Jeff King:
>
> Your patch looks correct to me. I probably wouldn't have bothered to
> write it, but now you did. :) My inclination would be to go with my
> dumb-simple one for this series,
Fair enough.
> and it looks like you may have
> collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups
> on top that could make their own series.
Well, my aim is more to simplify and improve type safety, but I get
carried away a bit at times.
René
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
2023-09-05 7:05 ` Jeff King
@ 2023-09-19 7:42 ` René Scharfe
0 siblings, 0 replies; 47+ messages in thread
From: René Scharfe @ 2023-09-19 7:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Am 05.09.23 um 09:05 schrieb Jeff King:
> On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:
>
>> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
>> on error, but that's a different matter.
>
> Yeah, that doesn't make sense at all.
Actually it does: any non-zero return from a callback is interpreted as an
error. "return error(...);" would be shorter, but "error(...); return 1;"
is not wrong. Sorry for the noise!
René
^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2023-09-19 7:42 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-31 7:09 [PATCH 0/8] more unused parameters in parseopt callbacks Jeff King
2023-08-31 7:12 ` [PATCH 1/8] merge: make xopts a strvec Jeff King
2023-08-31 7:22 ` Jeff King
2023-08-31 11:18 ` Phillip Wood
2023-08-31 15:46 ` Junio C Hamano
2023-08-31 20:55 ` Taylor Blau
2023-08-31 7:14 ` [PATCH 2/8] merge: simplify parsing of "-n" option Jeff King
2023-08-31 15:56 ` Junio C Hamano
2023-08-31 7:17 ` [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks Jeff King
2023-08-31 16:14 ` Junio C Hamano
2023-08-31 7:18 ` [PATCH 4/8] parse-options: mark unused "opt" parameter " Jeff King
2023-08-31 16:33 ` Junio C Hamano
2023-08-31 17:50 ` Jeff King
2023-08-31 20:48 ` Jeff King
2023-08-31 7:18 ` [PATCH 5/8] merge: do not pass unused opt->value parameter Jeff King
2023-08-31 16:53 ` Junio C Hamano
2023-08-31 7:19 ` [PATCH 6/8] parse-options: add more BUG_ON() annotations Jeff King
2023-08-31 16:58 ` Junio C Hamano
2023-08-31 7:19 ` [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
2023-08-31 17:04 ` Junio C Hamano
2023-08-31 17:56 ` Jeff King
2023-08-31 7:20 ` [PATCH 8/8] parse-options: mark unused parameters in noop callback Jeff King
2023-08-31 17:05 ` Junio C Hamano
2023-08-31 21:16 ` [PATCH v2 0/10] more unused parameters in parseopt callbacks Jeff King
2023-08-31 21:17 ` [PATCH v2 01/10] merge: make xopts a strvec Jeff King
2023-08-31 21:17 ` [PATCH v2 02/10] merge: simplify parsing of "-n" option Jeff King
2023-09-02 6:20 ` René Scharfe
2023-09-05 6:43 ` Jeff King
2023-08-31 21:17 ` [PATCH v2 03/10] format-patch: use OPT_STRING_LIST for to/cc options Jeff King
2023-08-31 21:20 ` [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile Jeff King
2023-08-31 22:12 ` Junio C Hamano
2023-09-02 6:20 ` René Scharfe
2023-09-05 7:12 ` [PATCH v3 " Jeff King
2023-08-31 21:21 ` [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks Jeff King
2023-09-02 7:34 ` René Scharfe
2023-09-05 6:52 ` Jeff King
2023-08-31 21:21 ` [PATCH v2 06/10] parse-options: mark unused "opt" parameter " Jeff King
2023-09-02 10:12 ` René Scharfe
2023-09-05 7:05 ` Jeff King
2023-09-19 7:42 ` René Scharfe
2023-08-31 21:21 ` [PATCH v2 07/10] merge: do not pass unused opt->value parameter Jeff King
2023-08-31 21:21 ` [PATCH v2 08/10] parse-options: add more BUG_ON() annotations Jeff King
2023-08-31 21:22 ` [PATCH v2 09/10] interpret-trailers: mark unused "unset" parameters in option callbacks Jeff King
2023-08-31 21:22 ` [PATCH v2 10/10] parse-options: mark unused parameters in noop callback Jeff King
2023-09-02 11:37 ` René Scharfe
2023-09-05 7:09 ` Jeff King
2023-09-07 20:20 ` René Scharfe
Code repositories for project(s) associated with this public inbox
https://80x24.org/pub/scm/git/git.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).