parseopt fixes from -Wunused-parameters
cover parseopt fixes from -Wunused-parameters
Continuing my exploration of what -Wunused-parameters can show us, here are some bug-fixes related to parse-options callbacks.
This is the last of the actual bug-fixes I've found. After this, I have about 60 patches worth of cleanups (i.e., dropping the unused parameters), and then I have a series to annotate parameters that must be unused (e.g., for functions that must conform to callback interfaces). After we can start compiling with -Wunused-parameters, assuming we don't find the annotations too cumbersome.
But this series fixes real bugs. These first four fix segfaults:
[01/13]: apply: mark include/exclude options as NONEG [02/13]: am: handle --no-patch-format option [03/13]: ls-files: mark exclude options as NONEG [04/13]: pack-objects: mark index-version option as NONEG
And these four fix cases where we just quietly do the wrong thing:
[05/13]: cat-file: mark batch options with NONEG [06/13]: status: mark --find-renames option with NONEG [07/13]: format-patch: mark "--no-numbered" option with NONEG [08/13]: show-branch: mark --reflog option as NONEG
These ones are just message improvements:
[09/13]: tag: mark "--message" option with NONEG
[10/13]: cat-file: report an error on multiple --batch options [11/13]: apply: return -1 from option callback instead of calling exit(1)
This one is a segfault, but it has no callers. ;)
[12/13]: parse-options: drop OPT_DATE()
And then this last one is mostly about annotating the callbacks. It doesn't strictly need to happen here, but the alternative is that I'd eventually have to deal with it in the later series I mentioned.
[13/13]: assert NOARG/NONEG behavior of parse-options callbacks
apply.c | 24 +++++++++++++++++++++--- builtin/am.c | 4 +++- builtin/blame.c | 4 ++++ builtin/cat-file.c | 10 +++++++--- builtin/checkout-index.c | 2 ++ builtin/clean.c | 1 + builtin/commit.c | 5 ++++- builtin/fetch.c | 2 ++ builtin/grep.c | 14 +++++++++++++- builtin/init-db.c | 1 + builtin/interpret-trailers.c | 2 ++ builtin/log.c | 12 +++++++++++- builtin/ls-files.c | 14 +++++++++++--- builtin/merge-file.c | 2 ++ builtin/merge.c | 1 + builtin/notes.c | 7 +++++++ builtin/pack-objects.c | 5 ++++- builtin/read-tree.c | 3 +++ builtin/rebase.c | 6 ++++++ builtin/show-branch.c | 3 ++- builtin/show-ref.c | 1 + builtin/tag.c | 6 ++++-- builtin/update-index.c | 21 +++++++++++++++++++-- parse-options-cb.c | 14 +++++++------- parse-options.h | 18 ++++++++++++++---- ref-filter.c | 2 ++ t/helper/test-parse-options.c | 2 +- t/t0040-parse-options.sh | 22 ---------------------- 28 files changed, 155 insertions(+), 53 deletions(-)
-Peff
PATCH 1/13 apply: mark include/exclude options as NONEG
The options callback for "git apply --no-include" is not ready to handle the "unset" parameter, and as a result will segfault when it adds a NULL argument to the include list (likewise for "--no-exclude").
In theory this might be used to clear the list, but since both "--include" and "--exclude" add to the same list, it's not immediately obvious what the semantics should be. Let's punt on that for now and just disallow the broken options.
--- apply.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
apply.c +2 -2
@@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,struct option builtin_apply_options[] = { {OPTION_CALLBACK ,0 ,"exclude" , state,N_ ("path" ),N_ ("don't apply changes matching the given path" ), -0 , apply_option_parse_exclude }, +PARSE_OPT_NONEG , apply_option_parse_exclude }, {OPTION_CALLBACK ,0 ,"include" , state,N_ ("path" ),N_ ("apply changes matching the given path" ), -0 , apply_option_parse_include }, +PARSE_OPT_NONEG , apply_option_parse_include }, {OPTION_CALLBACK ,'p' ,NULL , state,N_ ("num" ),N_ ("remove <num> leading slashes from traditional diff paths" ),0 , apply_option_parse_p },
Signature · 1 line
2.19.1.1505.g9cd28186cf
Re: [PATCH 01/13] apply: mark include/exclude options as NONEG
Quoted 8 lines · Jeff King
The options callback for "git apply --no-include" is not ready to handle the "unset" parameter, and as a result will segfault when it adds a NULL argument to the include list (likewise for "--no-exclude").
In theory this might be used to clear the list, but since both "--include" and "--exclude" add to the same list, it's not immediately obvious what the semantics should be. Let's punt on that for now and just disallow the broken options.
Thanks. I agree with the conclusion to leave it to later outside this series to define what --no-(include|exclude) should do.
I suspect something along the lines of
Each element on the single list is marked as either include or exclude, and "--no-include" would remove the accumulated "include" entries in the list without touching any "exclude" elements.
would be sufficiently clear and useful, perhaps.
Quoted 23 lines
Signed-off-by Jeff King <[email protected]>--- apply.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)apply.c +2
-2@@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,struct option builtin_apply_options[] = { {OPTION_CALLBACK ,0 ,"exclude" , state,N_ ("path" ),N_ ("don't apply changes matching the given path" ), -0 , apply_option_parse_exclude }, +PARSE_OPT_NONEG , apply_option_parse_exclude }, {OPTION_CALLBACK ,0 ,"include" , state,N_ ("path" ),N_ ("apply changes matching the given path" ), -0 , apply_option_parse_include }, +PARSE_OPT_NONEG , apply_option_parse_include }, {OPTION_CALLBACK ,'p' ,NULL , state,N_ ("num" ),N_ ("remove <num> leading slashes from traditional diff paths" ),0 , apply_option_parse_p },
PATCH 2/13 am: handle --no-patch-format option
In reply to Jeff KingRunning "git am --no-patch-format" will currently segfault, since it tries to parse a NULL argument. Instead, let's have it cancel any previous --patch-format option.
--- builtin/am.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/am.c +3 -1
@@ -2165,7 +2165,9 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int {int *opt_value = opt->value; -if (!strcmp (arg,"mbox" )) +if (unset) + *opt_value =PATCH_FORMAT_UNKNOWN ; +else if (!strcmp (arg,"mbox" )) *opt_value =PATCH_FORMAT_MBOX ;else if (!strcmp (arg,"stgit" )) *opt_value =PATCH_FORMAT_STGIT ;
Signature · 1 line
2.19.1.1505.g9cd28186cf
Re: [PATCH 02/13] am: handle --no-patch-format option
Quoted 3 lines · Jeff King
Running "git am --no-patch-format" will currently segfault, since it tries to parse a NULL argument. Instead, let's have it cancel any previous --patch-format option.
Makes perfect sense.
Quoted 21 lines
Signed-off-by Jeff King <[email protected]>--- builtin/am.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)builtin/am.c +3
-1@@ -2165,7 +2165,9 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int {int *opt_value = opt->value; -if (!strcmp (arg,"mbox" )) +if (unset) + *opt_value =PATCH_FORMAT_UNKNOWN ; +else if (!strcmp (arg,"mbox" )) *opt_value =PATCH_FORMAT_MBOX ;else if (!strcmp (arg,"stgit" )) *opt_value =PATCH_FORMAT_STGIT ;
PATCH 3/13 ls-files: mark exclude options as NONEG
In reply to Jeff KingRunning "git ls-files --no-exclude" will currently segfault, as its option callback does not handle the "unset" parameter.
In theory this could be used to clear the exclude list, but it is not clear how that would interact with the other exclude options, nor is the current code capable of clearing the list. Let's just disable the broken option.
Note that --no-exclude-from will similarly segfault, but --no-exclude-standard will not. It just silently does the wrong thing (pretending as if --exclude-standard was specified).
--- builtin/ls-files.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/ls-files.c +4 -3
@@ -548,15 +548,16 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) N_("show resolve-undo information")), {OPTION_CALLBACK ,'x' ,"exclude" , &exclude_list,N_ ("pattern" ),N_ ("skip files matching pattern" ), - 0, option_parse_exclude }, + PARSE_OPT_NONEG, option_parse_exclude }, { OPTION_CALLBACK, 'X', "exclude-from", &dir, N_("file"),N_ ("exclude patterns are read from <file>" ), - 0, option_parse_exclude_from }, + PARSE_OPT_NONEG, option_parse_exclude_from },OPT_STRING (0 ,"exclude-per-directory" , &dir.exclude_per_dir,N_ ("file" ),N_ ("read additional per-directory exclude patterns in <file>" )), { OPTION_CALLBACK, 0, "exclude-standard", &dir, NULL,N_ ("add the standard git exclusions" ), - PARSE_OPT_NOARG, option_parse_exclude_standard }, +PARSE_OPT_NOARG |PARSE_OPT_NONEG , + option_parse_exclude_standard },OPT_SET_INT_F (0 ,"full-name" , &prefix_len,N_ ("make the output relative to the project top directory" ), 0, PARSE_OPT_NONEG),
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 4/13 pack-objects: mark index-version option as NONEG
In reply to Jeff KingRunning "git pack-objects --no-index-version" will segfault, since the callback is not prepared to handle the "unset" flag.
In theory this might be used to counteract an earlier "--index-version", or override a pack.indexversion config setting. But the semantics aren't immediately obvious, and it's unlikely anybody wants this. Let's just disable the broken option for now.
--- builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/pack-objects.c +1 -1
@@ -3252,7 +3252,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) N_("similar to --all-progress when progress meter is shown")), {OPTION_CALLBACK ,0 ,"index-version" ,NULL ,N_ ("<version>[,<offset>]" ),N_ ("write the pack index file in the specified idx format version" ), - 0, option_parse_index_version }, + PARSE_OPT_NONEG, option_parse_index_version },OPT_MAGNITUDE (0 ,"max-pack-size" , &pack_size_limit,N_ ("maximum size of each output pack file" )), OPT_BOOL(0, "local", &local,
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 5/13 cat-file: mark batch options with NONEG
In reply to Jeff KingRunning "cat-file --no-batch" will behave as if "--batch" was given, since the option callback does not handle the "unset" flag (likewise for "--no-batch-check").
In theory this might be used to cancel an earlier --batch, but it's not immediately obvious how that would interact with --batch-check. Let's just disallow the negated form of both options.
--- builtin/cat-file.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
builtin/cat-file.c +4 -2
@@ -631,10 +631,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)OPT_BOOL (0 ,"buffer" , &batch.buffer_output,N_ ("buffer --batch output" )), { OPTION_CALLBACK, 0, "batch", &batch, "format",N_ ("show info and content of objects fed from the standard input" ), - PARSE_OPT_OPTARG, batch_option_callback }, +PARSE_OPT_OPTARG |PARSE_OPT_NONEG , + batch_option_callback }, { OPTION_CALLBACK, 0, "batch-check", &batch, "format",N_ ("show info about objects fed from the standard input" ), - PARSE_OPT_OPTARG, batch_option_callback }, +PARSE_OPT_OPTARG |PARSE_OPT_NONEG , + batch_option_callback },OPT_BOOL (0 ,"follow-symlinks" , &batch.follow_symlinks,N_ ("follow in-tree symlinks (used with --batch or --batch-check)" )), OPT_BOOL(0, "batch-all-objects", &batch.all_objects,
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 6/13 status: mark --find-renames option with NONEG
In reply to Jeff KingIf you run "git status --no-find-renames", it will behave the same as "--find-renames", because we ignore the "unset" parameter (we see a NULL "arg", but since the score argument is optional, we just think that the user did not provide a score).
We already have a separate "--no-renames" to disable renames, so there's not much point in supporting "--no-find-renames". Let's just flag it as an error.
--- builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/commit.c +1 -1
@@ -1335,7 +1335,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)OPT_BOOL (0 ,"no-renames" , &no_renames,N_ ("do not detect renames" )), { OPTION_CALLBACK, 'M', "find-renames", &rename_score_arg,N_ ("n" ),N_ ("detect renames, optionally set similarity index" ), - PARSE_OPT_OPTARG, opt_parse_rename_score }, + PARSE_OPT_OPTARG | PARSE_OPT_NONEG, opt_parse_rename_score }, OPT_END(), };
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 7/13 format-patch: mark "--no-numbered" option with NONEG
In reply to Jeff KingWe have separate parse-options entries for "numbered" and "no-numbered", which means that we accept "--no-no-numbered". It does not behave sensibly, though (it ignores the "unset" flag and acts like "--no-numbered").
We could fix that, but obviously this is silly and unintentional. Let's just disallow it.
--- builtin/log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/log.c +1 -1
@@ -1508,7 +1508,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) PARSE_OPT_NOARG, numbered_callback }, {OPTION_CALLBACK ,'N' ,"no-numbered" , &numbered,NULL ,N_ ("use [PATCH] even with multiple patches" ), - PARSE_OPT_NOARG, no_numbered_callback }, + PARSE_OPT_NOARG | PARSE_OPT_NONEG, no_numbered_callback },OPT_BOOL ('s' ,"signoff" , &do_signoff,N_ ("add Signed-off-by:" )),OPT_BOOL (0 ,"stdout" , &use_stdout, N_("print patches to standard out")),
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 8/13 show-branch: mark --reflog option as NONEG
In reply to Jeff KingRunning "git show-branch --no-reflog" will behave as if "--reflog" was given with no options, which makes no sense.
In theory this option might be used to cancel an earlier "--reflog" option, but the semantics are not clear. Let's punt on it and just disallow the broken option.
---
On most of these I tried to decide whether there was a sensible behavior, and actually do the fix. But I have to admit I have no idea how "show-branch -g" is supposed to work, which is the main reason I punted here. If somebody cares enough to write a patch, they can supersede this (but we should do something, because the current behavior is nonsense).
builtin/show-branch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/show-branch.c +1 -1
@@ -674,7 +674,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) {OPTION_CALLBACK ,'g' ,"reflog" , &reflog_base,N_ ("<n>[,<base>]" ),N_ ("show <n> most recent ref-log entries starting at " "base" ), -PARSE_OPT_OPTARG , +PARSE_OPT_OPTARG |PARSE_OPT_NONEG , parse_reflog_param },OPT_END () };
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 9/13 tag: mark "--message" option with NONEG
In reply to Jeff KingWe do not allow "--no-message" to work now, as the option callback returns "-1" when it sees a NULL arg. However, that will cause parse-options to exit(129) without printing anything further, leaving the user confused about what happened.
Instead, let's explicitly mark it as PARSE_OPT_NONEG, which will give a useful error message (and print the usual -h output).
In theory this could be used to override an earlier "-m", but it's not clear how it would interact with other message options (e.g., would it also clear data read for "-F"?). Since it's already disabled and nobody is asking for it, let's punt on that and just improve the error message.
--- builtin/tag.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/tag.c +2 -2
@@ -390,8 +390,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)OPT_GROUP (N_ ("Tag creation options" )),OPT_BOOL ('a' ,"annotate" , &annotate,N_ ("annotated tag, needs a message" )), -OPT_CALLBACK ('m' ,"message" , &msg,N_ ("message" ), -N_ ("tag message" ), parse_msg_arg), + { OPTION_CALLBACK, 'm', "message", &msg, N_("message"), + N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg },OPT_FILENAME ('F' ,"file" , &msgfile,N_ ("read message from file" )),OPT_BOOL ('e' ,"edit" , &edit_flag,N_ ("force edit of tag message" )), OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 10/13 cat-file: report an error on multiple --batch options
In reply to Jeff KingThe options callback for --batch and --batch-check detects when the two mutually incompatible options are used. But it simply returns an error code to parse-options, meaning the program will quit without any kind of message to the user.
Instead, let's use error() to print something and return -1. Note that this flips the error return from 1 to -1, but negative values are more idiomatic here (and parse-options treats them the same).
--- builtin/cat-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/cat-file.c +1 -1
@@ -596,7 +596,7 @@ static int batch_option_callback(const struct option *opt,struct batch_options *bo = opt->value;if (bo->enabled) { -return 1 ; +return error (_ ("only one batch option may be specified" )); } bo->enabled =1 ;
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 11/13 apply: return -1 from option callback instead of calling exit(1)
In reply to Jeff KingThe option callback for "apply --whitespace" exits with status "1" on error. It makes more sense for it to just return an error to parse-options. That code will exit, too, but it will use status "129" that is customary for option errors.
The exit() dates back to aaf6c447aa (builtin/apply: make parse_whitespace_option() return -1 instead of die()ing, 2016-08-08). That commit gives no reason why we'd prefer the current exit status (it looks like it was just bumping the "die" up a level in the callstack, but did not go as far as it could have).
--- apply.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
apply.c +1 -1
@@ -4812,7 +4812,7 @@ static int apply_option_parse_whitespace(const struct option *opt,struct apply_state *state = opt->value; state->whitespace_option = arg;if (parse_whitespace_option (state, arg)) -exit (1 ); +return -1 ;return 0 ; }
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 12/13 parse-options: drop OPT_DATE()
In reply to Jeff KingThere are no users of OPT_DATE except for test-parse-options; its only caller went away in 27ec394a97 (prune: introduce OPT_EXPIRY_DATE() and use it, 2013-04-25).
It also has a bug: it does not specify PARSE_OPT_NONEG, but its callback does not respect the "unset" flag, and will feed NULL to approxidate() and segfault. Probably this should be marked with NONEG, or the callback should set the timestamp to some sentinel value (e.g,. "0", or "(time_t)-1").
But since there are no callers, deleting it means we don't even have to think about what the right behavior should be.
--- parse-options-cb.c | 7 ------- parse-options.h | 4 ---- t/helper/test-parse-options.c | 1 - t/t0040-parse-options.sh | 22 ---------------------- 4 files changed, 34 deletions(-)
parse-options-cb.c +0 -7
@@ -28,13 +28,6 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)return 0 ; } -int parse_opt_approxidate_cb (const struct option *opt,const char *arg, -int unset) -{ - *(timestamp_t *)(opt->value) =approxidate (arg); -return 0 ; -} -int parse_opt_expiry_date_cb (const struct option *opt,const char *arg,int unset) {
parse-options.h +0 -4
@@ -150,9 +150,6 @@ struct option { (h), 0, &parse_opt_string_list }#define OPT_UYN (s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG, &parse_opt_tertiary } -#define OPT_DATE (s, l, v, h) \ - { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0, \ - parse_opt_approxidate_cb }#define OPT_EXPIRY_DATE (s, l, v, h) \ { OPTION_CALLBACK, (s), (l), (v), N_("expiry-date"),(h), 0, \ parse_opt_expiry_date_cb }
@@ -232,7 +229,6 @@ extern struct option *parse_options_concat(struct option *a, struct option *b);/*----- some often used options -----*/ extern int parse_opt_abbrev_cb (const struct option *,const char *,int ); -extern int parse_opt_approxidate_cb (const struct option *,const char *,int );extern int parse_opt_expiry_date_cb (const struct option *,const char *,int );extern int parse_opt_color_flag_cb (const struct option *,const char *,int );extern int parse_opt_verbosity_cb (const struct option *,const char *,int );
t/helper/test-parse-options.c +0 -1
@@ -119,7 +119,6 @@ int cmd__parse_options(int argc, const char **argv)OPT_INTEGER ('j' ,NULL , &integer,"get a integer, too" ),OPT_MAGNITUDE ('m' ,"magnitude" , &magnitude,"get a magnitude" ),OPT_SET_INT (0 ,"set23" , &integer,"set integer to 23" ,23 ), -OPT_DATE ('t' ,NULL , ×tamp,"get timestamp of <time>" ),OPT_CALLBACK ('L' ,"length" , &integer,"str" ,"get length of <str>" , length_callback), OPT_FILENAME('F', "file", &file, "set file to <file>"),
t/t0040-parse-options.sh +0 -22
@@ -23,7 +23,6 @@ usage: test-tool parse-options <options> -j <n> get a integer, too -m, --magnitude <n> get a magnitude --set23 set integer to 23 - -t <time> get timestamp of <time> -L, --length <str> get length of <str> -F, --file <file> set file to <file>
@@ -245,27 +244,6 @@ test_expect_success 'keep some options as arguments' ' test-tool parse-options --expect="arg 00: --quux" --quux ' -cat >expect <<\EOF -boolean: 0 -integer: 0 -magnitude: 0 -timestamp: 1 -string: (not set) -abbrev: 7 -verbose: -1 -quiet: 1 -dry run: no -file: (not set) -arg 00: foo -EOF - -test_expect_success 'OPT_DATE() works' ' - test-tool parse-options -t "1970-01-01 00:00:01 +0000" \ - foo -q >output 2>output.err && - test_must_be_empty output.err && - test_cmp expect output -' - cat >expect <<\EOF Callback: "four", 0 boolean: 5
Signature · 1 line
2.19.1.1505.g9cd28186cf
PATCH 1/1 parse-options: deprecate OPT_DATE
--- Documentation/technical/api-parse-options.txt | 4 ---- 1 file changed, 4 deletions(-)
Documentation/technical/api-parse-options.txt +0 -4
@@ -183,10 +183,6 @@ There are some macros to easily define options: scale the provided value by 1024, 1024^2 or 1024^3 respectively. The scaled value is put into `unsigned_long_var`. -`OPT_DATE(short, long, ×tamp_t_var, description)`:: - Introduce an option with date argument, see `approxidate()`. - The timestamp is put into `timestamp_t_var`. - `OPT_EXPIRY_DATE(short, long, ×tamp_t_var, description)`:: Introduce an option with expiry date argument, see `parse_expiry_date()`. The timestamp is put into `timestamp_t_var`.
Signature · 1 line
2.19.1.816.gcd69ec8cd
Re: [PATCH] parse-options: deprecate OPT_DATE
Quoted 10 lines · Carlo Marcelo Arenas Belón
Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- Documentation/technical/api-parse-options.txt | 4 ---- 1 file changed, 4 deletions(-)Documentation/technical/api-parse-options.txt +0
-0
Documentation/technical/api-parse-options.txt There are some macros to easily define options: · line 188
Quoted 10 lines
Documentation/technical/api-parse-options.txt +0
-4@@ -183,10 +183,6 @@ There are some macros to easily define options: scale the provided value by 1024, 1024^2 or 1024^3 respectively. The scaled value is put into `unsigned_long_var`. -`OPT_DATE(short, long, ×tamp_t_var, description)`:: - Introduce an option with date argument, see `approxidate()`. - The timestamp is put into `timestamp_t_var`. - `OPT_EXPIRY_DATE(short, long, ×tamp_t_var, description)`:: Introduce an option with expiry date argument, see `parse_expiry_date()`. The timestamp is put into `timestamp_t_var`.
Thank you! I forgot to check the documentation.
-Peff
PATCH 13/13 assert NOARG/NONEG behavior of parse-options callbacks
In reply to Jeff KingWhen we define a parse-options callback, the flags we put in the option struct must match what the callback expects. For example, a callback which does not handle the "unset" parameter should only be used with PARSE_OPT_NONEG. But since the callback and the option struct are not defined next to each other, it's easy to get this wrong (as earlier patches in this series show).
Fortunately, the compiler can help us here: compiling with -Wunused-parameters can show us which callbacks ignore their "unset" parameters (and likewise, ones that ignore "arg" expect to be triggered with PARSE_OPT_NOARG).
But after we've inspected a callback and determined that all of its callers use the right flags, what do we do next? We'd like to silence the compiler warning, but do so in a way that will catch any wrong calls in the future.
We can do that by actually checking those variables and asserting that they match our expectations. Because this is such a common pattern, we'll introduce some helper macros. The resulting messages aren't as descriptive as we could make them, but the file/line information from BUG() is enough to identify the problem (and anyway, the point is that these should never be seen).
Each of the annotated callbacks in this patch triggers -Wunused-parameters, and was manually inspected to make sure all callers use the correct options (so none of these BUGs should be triggerable).
--- apply.c | 18 ++++++++++++++++++ builtin/blame.c | 4 ++++ builtin/cat-file.c | 2 ++ builtin/checkout-index.c | 2 ++ builtin/clean.c | 1 + builtin/commit.c | 3 +++ builtin/fetch.c | 2 ++ builtin/grep.c | 14 +++++++++++++- builtin/init-db.c | 1 + builtin/interpret-trailers.c | 2 ++ builtin/log.c | 10 ++++++++++ builtin/ls-files.c | 7 +++++++ builtin/merge-file.c | 2 ++ builtin/merge.c | 1 + builtin/notes.c | 7 +++++++ builtin/pack-objects.c | 3 +++ builtin/read-tree.c | 3 +++ builtin/rebase.c | 6 ++++++ builtin/show-branch.c | 1 + builtin/show-ref.c | 1 + builtin/tag.c | 2 ++ builtin/update-index.c | 21 +++++++++++++++++++-- parse-options-cb.c | 7 +++++++ parse-options.h | 14 ++++++++++++++ ref-filter.c | 2 ++ t/helper/test-parse-options.c | 1 + 26 files changed, 134 insertions(+), 3 deletions(-)
apply.c +18 -0
@@ -4772,6 +4772,9 @@ static int apply_option_parse_exclude(const struct option *opt, const char *arg, int unset) {struct apply_state *state = opt->value; + +BUG_ON_OPT_NEG (unset); +add_name_limit (state, arg,1 );return 0 ; }
@@ -4780,6 +4783,9 @@ static int apply_option_parse_include(const struct option *opt, const char *arg, int unset) {struct apply_state *state = opt->value; + +BUG_ON_OPT_NEG (unset); +add_name_limit (state, arg,0 ); state->has_include =1 ;return 0 ;
@@ -4790,6 +4796,9 @@ static int apply_option_parse_p(const struct option *opt, int unset) {struct apply_state *state = opt->value; + +BUG_ON_OPT_NEG (unset); + state->p_value =atoi (arg); state->p_value_known =1 ;return 0 ;
@@ -4799,6 +4808,9 @@ static int apply_option_parse_space_change(const struct option *opt, const char *arg, int unset) {struct apply_state *state = opt->value; + +BUG_ON_OPT_ARG (arg); +if (unset) state->ws_ignore_action = ignore_ws_none;else
@@ -4810,6 +4822,9 @@ static int apply_option_parse_whitespace(const struct option *opt, const char *arg, int unset) {struct apply_state *state = opt->value; + +BUG_ON_OPT_NEG (unset); + state->whitespace_option = arg;if (parse_whitespace_option (state, arg))return -1 ;
@@ -4820,6 +4835,9 @@ static int apply_option_parse_directory(const struct option *opt, const char *arg, int unset) {struct apply_state *state = opt->value; + +BUG_ON_OPT_NEG (unset); +strbuf_reset (&state->root);strbuf_addstr (&state->root, arg);strbuf_complete (&state->root,'/' );
builtin/blame.c +4 -0
@@ -732,6 +732,8 @@ static int blame_copy_callback(const struct option *option, const char *arg, int {int *opt = option->value; +BUG_ON_OPT_NEG (unset); +/* * -C enables copy from removed files; * -C -C enables copy from existing files, but only
@@ -754,6 +756,8 @@ static int blame_move_callback(const struct option *option, const char *arg, int {int *opt = option->value; +BUG_ON_OPT_NEG (unset); + *opt |=PICKAXE_BLAME_MOVE ;if (arg)
builtin/cat-file.c +2 -0
@@ -595,6 +595,8 @@ static int batch_option_callback(const struct option *opt, {struct batch_options *bo = opt->value; +BUG_ON_OPT_NEG (unset); +if (bo->enabled) {return error (_ ("only one batch option may be specified" )); }
builtin/checkout-index.c +2 -0
@@ -132,6 +132,8 @@ static const char * const builtin_checkout_index_usage[] = {static int option_parse_stage (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +if (!strcmp (arg,"all" )) { to_tempfile =1 ; checkout_stage =CHECKOUT_ALL ;
builtin/clean.c +1 -0
@@ -140,6 +140,7 @@ static void clean_print_color(enum color_clean ix)static int exclude_cb (const struct option *opt,const char *arg,int unset) {struct string_list *exclude_list = opt->value; +BUG_ON_OPT_NEG (unset);string_list_append (exclude_list, arg);return 0 ; }
builtin/commit.c +3 -0
@@ -161,6 +161,9 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)static int opt_parse_rename_score (const struct option *opt,const char *arg,int unset) {const char **value = opt->value; + +BUG_ON_OPT_NEG (unset); +if (arg !=NULL && *arg =='=' ) arg = arg +1 ;
builtin/fetch.c +2 -0
@@ -98,6 +98,8 @@ static int git_fetch_config(const char *k, const char *v, void *cb)static int parse_refmap_arg (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +/* * "git fetch --refmap='' origin foo" * can be used to tell the command not to store anywhere
builtin/grep.c +13 -1
@@ -712,11 +712,14 @@ static int context_callback(const struct option *opt, const char *arg,static int file_callback (const struct option *opt,const char *arg,int unset) {struct grep_opt *grep_opt = opt->value; -int from_stdin = !strcmp (arg,"-" ); +int from_stdin;FILE *patterns;int lno =0 ;struct strbuf sb =STRBUF_INIT ; +BUG_ON_OPT_NEG (unset); + + from_stdin = !strcmp (arg,"-" ); patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg);
@@ -737,6 +740,8 @@ static int file_callback(const struct option *opt, const char *arg, int unset)static int not_callback (const struct option *opt,const char *arg,int unset) {struct grep_opt *grep_opt = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);append_grep_pattern (grep_opt,"--not" ,"command line" ,0 ,GREP_NOT );return 0 ; }
@@ -744,6 +749,8 @@ static int not_callback(const struct option *opt, const char *arg, int unset)static int and_callback (const struct option *opt,const char *arg,int unset) {struct grep_opt *grep_opt = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);append_grep_pattern (grep_opt,"--and" ,"command line" ,0 ,GREP_AND );return 0 ; }
@@ -751,6 +758,8 @@ static int and_callback(const struct option *opt, const char *arg, int unset)static int open_callback (const struct option *opt,const char *arg,int unset) {struct grep_opt *grep_opt = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);append_grep_pattern (grep_opt,"(" ,"command line" ,0 ,GREP_OPEN_PAREN );return 0 ; }
@@ -758,6 +767,8 @@ static int open_callback(const struct option *opt, const char *arg, int unset)static int close_callback (const struct option *opt,const char *arg,int unset) {struct grep_opt *grep_opt = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);append_grep_pattern (grep_opt,")" ,"command line" ,0 ,GREP_CLOSE_PAREN );return 0 ; }
@@ -766,6 +777,7 @@ static int pattern_callback(const struct option *opt, const char *arg, int unset) {struct grep_opt *grep_opt = opt->value; +BUG_ON_OPT_NEG (unset);append_grep_pattern (grep_opt, arg,"-e option" ,0 ,GREP_PATTERN );return 0 ; }
builtin/init-db.c +1 -0
@@ -451,6 +451,7 @@ static int guess_repository_type(const char *git_dir)static int shared_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); *((int *) opt->value) = (arg) ?git_config_perm ("arg" , arg) :PERM_GROUP ;return 0 ; }
builtin/interpret-trailers.c +2 -0
@@ -80,6 +80,8 @@ static int parse_opt_parse(const struct option *opt, const char *arg, v->only_trailers =1 ; v->only_input =1 ; v->unfold =1 ; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);return 0 ; }
builtin/log.c +10 -0
@@ -107,6 +107,8 @@ static int log_line_range_callback(const struct option *option, const char *arg, {struct line_opt_callback_data *data = option->value; +BUG_ON_OPT_NEG (unset); +if (!arg)return -1 ;
@@ -1151,6 +1153,8 @@ static int keep_subject = 0;static int keep_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg); ((struct rev_info *)opt->value)->total =-1 ; keep_subject =1 ;return 0 ;
@@ -1161,6 +1165,7 @@ static int subject_prefix = 0;static int subject_prefix_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); subject_prefix =1 ; ((struct rev_info *)opt->value)->subject_prefix = arg;return 0 ;
@@ -1168,6 +1173,8 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,static int rfc_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);return subject_prefix_callback (opt,"RFC PATCH" , unset); }
@@ -1176,6 +1183,7 @@ static int numbered_cmdline_opt = 0;static int numbered_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_ARG (arg); *(int *)opt->value = numbered_cmdline_opt = unset ?0 :1 ;if (unset) auto_number =0 ;
@@ -1185,6 +1193,7 @@ static int numbered_callback(const struct option *opt, const char *arg,static int no_numbered_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset);return numbered_callback (opt, arg,1 ); }
@@ -1192,6 +1201,7 @@ static int output_directory_callback(const struct option *opt, const char *arg, int unset) {const char **dir = (const char **)opt->value; +BUG_ON_OPT_NEG (unset);if (*dir)die (_ ("Two output directories?" )); *dir = arg;
builtin/ls-files.c +7 -0
@@ -475,6 +475,8 @@ static int option_parse_exclude(const struct option *opt, {struct string_list *exclude_list = opt->value; +BUG_ON_OPT_NEG (unset); + exc_given =1 ;string_list_append (exclude_list, arg);
@@ -486,6 +488,8 @@ static int option_parse_exclude_from(const struct option *opt, {struct dir_struct *dir = opt->value; +BUG_ON_OPT_NEG (unset); + exc_given =1 ;add_excludes_from_file (dir, arg);
@@ -497,6 +501,9 @@ static int option_parse_exclude_standard(const struct option *opt, {struct dir_struct *dir = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg); + exc_given =1 ;setup_standard_excludes (dir);
builtin/merge-file.c +2 -0
@@ -15,6 +15,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset)static int label_count =0 ;const char **names = (const char **)opt->value; +BUG_ON_OPT_NEG (unset); +if (label_count >=3 )return error ("too many labels on the command line" ); names[label_count++] = arg;
builtin/merge.c +1 -0
@@ -224,6 +224,7 @@ static int option_parse_x(const struct option *opt,static int option_parse_n (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_ARG (arg); show_diffstat = unset;return 0 ; }
builtin/notes.c +7 -0
@@ -215,6 +215,8 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) {struct note_data *d = opt->value; +BUG_ON_OPT_NEG (unset); +strbuf_grow (&d->buf,strlen (arg) +2 );if (d->buf.len)strbuf_addch (&d->buf,'\n' );
@@ -229,6 +231,8 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset) {struct note_data *d = opt->value; +BUG_ON_OPT_NEG (unset); +if (d->buf.len)strbuf_addch (&d->buf,'\n' );if (!strcmp (arg,"-" )) {
@@ -250,6 +254,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)enum object_type type;unsigned long len; +BUG_ON_OPT_NEG (unset); +if (d->buf.len)strbuf_addch (&d->buf,'\n' );
@@ -273,6 +279,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)static int parse_reedit_arg (const struct option *opt,const char *arg,int unset) {struct note_data *d = opt->value; +BUG_ON_OPT_NEG (unset); d->use_editor =1 ;return parse_reuse_arg (opt, arg, unset); }
builtin/pack-objects.c +3 -0
@@ -3206,6 +3206,9 @@ static int option_parse_index_version(const struct option *opt, {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 )die (_ ("unsupported index version %s" ), val);
builtin/read-tree.c +3 -0
@@ -44,6 +44,7 @@ static const char * const read_tree_usage[] = {static int index_output_cb (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset);set_alternate_index_output (arg);return 0 ; }
@@ -54,6 +55,8 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,struct dir_struct *dir;struct unpack_trees_options *opts; +BUG_ON_OPT_NEG (unset); + opts = (struct unpack_trees_options *)opt->value;if (opts->dir)
builtin/rebase.c +6 -0
@@ -703,6 +703,9 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset) {struct rebase_options *opts = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg); +if (!is_interactive (opts)) opts->type =REBASE_MERGE ;
@@ -715,6 +718,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg, {struct rebase_options *opts = opt->value; +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg); + opts->type =REBASE_INTERACTIVE ; opts->flags |=REBASE_INTERACTIVE_EXPLICIT ;
builtin/show-branch.c +1 -0
@@ -604,6 +604,7 @@ static int parse_reflog_param(const struct option *opt, const char *arg, {char *ep;const char **base = (const char **)opt->value; +BUG_ON_OPT_NEG (unset);if (!arg) arg ="" ; reflog =strtoul (arg, &ep,10 );
builtin/show-ref.c +1 -0
@@ -151,6 +151,7 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)static int exclude_existing_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); exclude_arg =1 ; *(const char **)opt->value = arg;return 0 ;
builtin/tag.c +2 -0
@@ -338,6 +338,8 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) {struct msg_arg *msg = opt->value; +BUG_ON_OPT_NEG (unset); +if (!arg)return -1 ;if (msg->buf.len)
builtin/update-index.c +19 -2
@@ -790,12 +790,16 @@ static int refresh(struct refresh_params *o, unsigned int flag)static int refresh_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);return refresh (opt->value,0 ); }static int really_refresh_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);return refresh (opt->value,REFRESH_REALLY ); }
@@ -803,6 +807,7 @@ static int chmod_callback(const struct option *opt, const char *arg, int unset) { char *flip = opt->value; +BUG_ON_OPT_NEG (unset);if ((arg[0 ] !='-' && arg[0 ] !='+' ) || arg[1 ] !='x' || arg[2 ])return error ("option 'chmod' expects \"+x\" or \"-x\"" ); *flip = arg[0 ];
@@ -812,6 +817,8 @@ static int chmod_callback(const struct option *opt,static int resolve_undo_clear_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); +BUG_ON_OPT_ARG (arg);resolve_undo_clear ();return 0 ; }
@@ -847,6 +854,8 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,unsigned int mode;const char *path; +BUG_ON_OPT_NEG (unset); +if (!parse_new_style_cacheinfo (ctx->argv[1 ], &mode, &oid, &path)) {if (add_cacheinfo (mode, &oid, path,0 ))die ("git update-index: --cacheinfo cannot add %s" , path);
@@ -869,6 +878,8 @@ static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx, {int *nul_term_line = opt->value; +BUG_ON_OPT_NEG (unset); +if (ctx->argc !=1 )return error ("option '%s' must be the last argument" , opt->long_name); allow_add = allow_replace = allow_remove =1 ;
@@ -881,6 +892,8 @@ static int stdin_callback(struct parse_opt_ctx_t *ctx, {int *read_from_stdin = opt->value; +BUG_ON_OPT_NEG (unset); +if (ctx->argc !=1 )return error ("option '%s' must be the last argument" , opt->long_name); *read_from_stdin =1 ;
@@ -888,11 +901,13 @@ static int stdin_callback(struct parse_opt_ctx_t *ctx, }static int unresolve_callback (struct parse_opt_ctx_t *ctx, -const struct option *opt,int flags) +const struct option *opt,int unset) {int *has_errors = opt->value;const char *prefix = startup_info->prefix; +BUG_ON_OPT_NEG (unset); +/* consume remaining arguments. */ *has_errors =do_unresolve (ctx->argc, ctx->argv, prefix, prefix ?strlen (prefix) :0 );
@@ -905,11 +920,13 @@ static int unresolve_callback(struct parse_opt_ctx_t *ctx, }static int reupdate_callback (struct parse_opt_ctx_t *ctx, -const struct option *opt,int flags) +const struct option *opt,int unset) {int *has_errors = opt->value;const char *prefix = startup_info->prefix; +BUG_ON_OPT_NEG (unset); +/* consume remaining arguments. */ setup_work_tree (); *has_errors = do_reupdate(ctx->argc, ctx->argv,
parse-options-cb.c +7 -0
@@ -58,6 +58,8 @@ int parse_opt_verbosity_cb(const struct option *opt, const char *arg, {int *target = opt->value; +BUG_ON_OPT_ARG (arg); +if (unset)/* --no-quiet, --no-verbose */ *target =0 ;
@@ -80,6 +82,8 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)struct object_id oid;struct commit *commit; +BUG_ON_OPT_NEG (unset); +if (!arg)return -1 ;if (get_oid (arg, &oid))
@@ -110,6 +114,9 @@ int parse_opt_object_name(const struct option *opt, const char *arg, int unset)int parse_opt_tertiary (const struct option *opt,const char *arg,int unset) {int *target = opt->value; + +BUG_ON_OPT_ARG (arg); + *target = unset ?2 :1 ;return 0 ; }
parse-options.h +14 -0
@@ -191,6 +191,20 @@ extern int opterror(const struct option *opt, const char *reason, int flags);#define opterror (o,r,f) (opterror((o),(r),(f)), const_error())#endif +/* +* Use these assertions for callbacks that expect to be called with NONEG and +* NOARG respectively, and do not otherwise handle the "unset" and "arg" +* parameters. +*/ +#define BUG_ON_OPT_NEG (unset) do { \ + if ((unset)) \ + BUG("option callback does not expect negation"); \ +} while (0) +#define BUG_ON_OPT_ARG (arg) do { \ + if ((arg)) \ + BUG("option callback does not expect an argument"); \ +} while (0) +/*----- incremental advanced APIs -----*/ enum {
ref-filter.c +2 -0
@@ -2316,6 +2316,8 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)struct object_id oid;int no_merged =starts_with (opt->long_name,"no" ); +BUG_ON_OPT_NEG (unset); +if (rf->merge) {if (no_merged) {return opterror (opt,"is incompatible with --merged" ,0 );
t/helper/test-parse-options.c +1 -0
@@ -36,6 +36,7 @@ static int length_callback(const struct option *opt, const char *arg, int unset)static int number_callback (const struct option *opt,const char *arg,int unset) { +BUG_ON_OPT_NEG (unset); *(int *)opt->value =strtol (arg,NULL ,10 );return 0 ; }
Signature · 1 line
2.19.1.1505.g9cd28186cf
Quoted 10 lines · Jeff King
Continuing my exploration of what -Wunused-parameters can show us, here are some bug-fixes related to parse-options callbacks.
This is the last of the actual bug-fixes I've found. After this, I have about 60 patches worth of cleanups (i.e., dropping the unused parameters), and then I have a series to annotate parameters that must be unused (e.g., for functions that must conform to callback interfaces). After we can start compiling with -Wunused-parameters, assuming we don't find the annotations too cumbersome.
Another good thing from this series is there are fewer --no-options to complete.
About the annotating unused parameters related to segfault bug fixes in this series. Should we just add something like
if (unset)
BUG("This code does not support --no-stuff");which could be done in the same patches that fix the segfault, and it extends the diff context a bit to see what these callbacks do without opening up the editor, and also serves as a kind of annotation?
Signature · 1 line
Duy
Quoted 17 lines · Duy Nguyen
Quoted 10 lines · Jeff King
Continuing my exploration of what -Wunused-parameters can show us, here are some bug-fixes related to parse-options callbacks.
This is the last of the actual bug-fixes I've found. After this, I have about 60 patches worth of cleanups (i.e., dropping the unused parameters), and then I have a series to annotate parameters that must be unused (e.g., for functions that must conform to callback interfaces). After we can start compiling with -Wunused-parameters, assuming we don't find the annotations too cumbersome.
Another good thing from this series is there are fewer --no-options to complete.
About the annotating unused parameters related to segfault bug fixes in this series. Should we just add something like
Quoted 6 lines
if (unset)
BUG("This code does not support --no-stuff");which could be done in the same patches that fix the segfault, and it extends the diff context a bit to see what these callbacks do without opening up the editor, and also serves as a kind of annotation?
That's done in the final patch. I did originally do it alongside the actual segfault fixes, but since it needs doing in so many other callbacks, too, it made sense to me to do it all together.
-Peff
Quoted 23 lines · Jeff King
Quoted 20 lines · Duy Nguyen
Quoted 10 lines · Jeff King
Continuing my exploration of what -Wunused-parameters can show us, here are some bug-fixes related to parse-options callbacks.
This is the last of the actual bug-fixes I've found. After this, I have about 60 patches worth of cleanups (i.e., dropping the unused parameters), and then I have a series to annotate parameters that must be unused (e.g., for functions that must conform to callback interfaces). After we can start compiling with -Wunused-parameters, assuming we don't find the annotations too cumbersome.
Another good thing from this series is there are fewer --no-options to complete.
About the annotating unused parameters related to segfault bug fixes in this series. Should we just add something like
if (unset)
BUG("This code does not support --no-stuff");
Quoted 7 lines
Quoted 3 lines
which could be done in the same patches that fix the segfault, and it extends the diff context a bit to see what these callbacks do without opening up the editor, and also serves as a kind of annotation?
That's done in the final patch. I did originally do it alongside the actual segfault fixes, but since it needs doing in so many other callbacks, too, it made sense to me to do it all together.
Oops. I guess I stopped reading the series too early. Sorry for the noise.
Signature · 1 line
Duy