@matheusbsbs / matheus-bagliano / commits / 517fe807d6

assert NOARG/NONEG behavior of parse-options callbacks

When 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). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Nov 5, 2018 at 01:45 UTC 517fe807d6903c629a739b23fe0e75b892096998

26 files changed +134 -3

apply.c

+18

@@ -4772,6 +4772,9 @@ static int apply_option_parse_exclude(const struct option *opt,

4772 const char *arg, int unset)

4773 {

4774 struct apply_state *state = opt->value;

4775 +

4776 + BUG_ON_OPT_NEG(unset);

4777 +

4778 add_name_limit(state, arg, 1);

4779 return 0;

4780 }

@@ -4780,6 +4783,9 @@ static int apply_option_parse_include(const struct option *opt,

4783 const char *arg, int unset)

4784 {

4785 struct apply_state *state = opt->value;

4786 +

4787 + BUG_ON_OPT_NEG(unset);

4788 +

4789 add_name_limit(state, arg, 0);

4790 state->has_include = 1;

4791 return 0;

@@ -4790,6 +4796,9 @@ static int apply_option_parse_p(const struct option *opt,

4796 int unset)

4797 {

4798 struct apply_state *state = opt->value;

4799 +

4800 + BUG_ON_OPT_NEG(unset);

4801 +

4802 state->p_value = atoi(arg);

4803 state->p_value_known = 1;

4804 return 0;

@@ -4799,6 +4808,9 @@ static int apply_option_parse_space_change(const struct option *opt,

4808 const char *arg, int unset)

4809 {

4810 struct apply_state *state = opt->value;

4811 +

4812 + BUG_ON_OPT_ARG(arg);

4813 +

4814 if (unset)

4815 state->ws_ignore_action = ignore_ws_none;

4816 else

@@ -4810,6 +4822,9 @@ static int apply_option_parse_whitespace(const struct option *opt,

4822 const char *arg, int unset)

4823 {

4824 struct apply_state *state = opt->value;

4825 +

4826 + BUG_ON_OPT_NEG(unset);

4827 +

4828 state->whitespace_option = arg;

4829 if (parse_whitespace_option(state, arg))

4830 return -1;

@@ -4820,6 +4835,9 @@ static int apply_option_parse_directory(const struct option *opt,

4835 const char *arg, int unset)

4836 {

4837 struct apply_state *state = opt->value;

4838 +

4839 + BUG_ON_OPT_NEG(unset);

4840 +

4841 strbuf_reset(&state->root);

4842 strbuf_addstr(&state->root, arg);

4843 strbuf_complete(&state->root, '/');

builtin/blame.c

@@ -732,6 +732,8 @@ static int blame_copy_callback(const struct option *option, const char *arg, int

732 {

733 int *opt = option->value;

734

735 + BUG_ON_OPT_NEG(unset);

736 +

737 /*

738 * -C enables copy from removed files;

739 * -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

756 {

757 int *opt = option->value;

758

759 + BUG_ON_OPT_NEG(unset);

760 +

761 *opt |= PICKAXE_BLAME_MOVE;

762

763 if (arg)

builtin/cat-file.c

@@ -595,6 +595,8 @@ static int batch_option_callback(const struct option *opt,

595 {

596 struct batch_options *bo = opt->value;

597

598 + BUG_ON_OPT_NEG(unset);

599 +

600 if (bo->enabled) {

601 return error(_("only one batch option may be specified"));

602 }

builtin/checkout-index.c

@@ -132,6 +132,8 @@ static const char * const builtin_checkout_index_usage[] = {

132 static int option_parse_stage(const struct option *opt,

133 const char *arg, int unset)

134 {

135 + BUG_ON_OPT_NEG(unset);

136 +

137 if (!strcmp(arg, "all")) {

138 to_tempfile = 1;

139 checkout_stage = CHECKOUT_ALL;

builtin/clean.c

@@ -140,6 +140,7 @@ static void clean_print_color(enum color_clean ix)

140 static int exclude_cb(const struct option *opt, const char *arg, int unset)

141 {

142 struct string_list *exclude_list = opt->value;

143 + BUG_ON_OPT_NEG(unset);

144 string_list_append(exclude_list, arg);

145 return 0;

146 }

builtin/commit.c

@@ -161,6 +161,9 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)

161 static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)

162 {

163 const char **value = opt->value;

164 +

165 + BUG_ON_OPT_NEG(unset);

166 +

167 if (arg != NULL && *arg == '=')

168 arg = arg + 1;

169

builtin/fetch.c

@@ -98,6 +98,8 @@ static int git_fetch_config(const char *k, const char *v, void *cb)

99 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)

100 {

101 + BUG_ON_OPT_NEG(unset);

102 +

103 /*

104 * "git fetch --refmap='' origin foo"

105 * 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,

712 static int file_callback(const struct option *opt, const char *arg, int unset)

713 {

714 struct grep_opt *grep_opt = opt->value;

715 - int from_stdin = !strcmp(arg, "-");

715 + int from_stdin;

716 FILE *patterns;

717 int lno = 0;

718 struct strbuf sb = STRBUF_INIT;

719

720 + BUG_ON_OPT_NEG(unset);

721 +

722 + from_stdin = !strcmp(arg, "-");

723 patterns = from_stdin ? stdin : fopen(arg, "r");

724 if (!patterns)

725 die_errno(_("cannot open '%s'"), arg);

@@ -737,6 +740,8 @@ static int file_callback(const struct option *opt, const char *arg, int unset)

740 static int not_callback(const struct option *opt, const char *arg, int unset)

741 {

742 struct grep_opt *grep_opt = opt->value;

743 + BUG_ON_OPT_NEG(unset);

744 + BUG_ON_OPT_ARG(arg);

745 append_grep_pattern(grep_opt, "--not", "command line", 0, GREP_NOT);

746 return 0;

747 }

@@ -744,6 +749,8 @@ static int not_callback(const struct option *opt, const char *arg, int unset)

749 static int and_callback(const struct option *opt, const char *arg, int unset)

750 {

751 struct grep_opt *grep_opt = opt->value;

752 + BUG_ON_OPT_NEG(unset);

753 + BUG_ON_OPT_ARG(arg);

754 append_grep_pattern(grep_opt, "--and", "command line", 0, GREP_AND);

755 return 0;

756 }

@@ -751,6 +758,8 @@ static int and_callback(const struct option *opt, const char *arg, int unset)

758 static int open_callback(const struct option *opt, const char *arg, int unset)

759 {

760 struct grep_opt *grep_opt = opt->value;

761 + BUG_ON_OPT_NEG(unset);

762 + BUG_ON_OPT_ARG(arg);

763 append_grep_pattern(grep_opt, "(", "command line", 0, GREP_OPEN_PAREN);

764 return 0;

765 }

@@ -758,6 +767,8 @@ static int open_callback(const struct option *opt, const char *arg, int unset)

767 static int close_callback(const struct option *opt, const char *arg, int unset)

768 {

769 struct grep_opt *grep_opt = opt->value;

770 + BUG_ON_OPT_NEG(unset);

771 + BUG_ON_OPT_ARG(arg);

772 append_grep_pattern(grep_opt, ")", "command line", 0, GREP_CLOSE_PAREN);

773 return 0;

774 }

@@ -766,6 +777,7 @@ static int pattern_callback(const struct option *opt, const char *arg,

777 int unset)

778 {

779 struct grep_opt *grep_opt = opt->value;

780 + BUG_ON_OPT_NEG(unset);

781 append_grep_pattern(grep_opt, arg, "-e option", 0, GREP_PATTERN);

782 return 0;

783 }

builtin/init-db.c

@@ -451,6 +451,7 @@ static int guess_repository_type(const char *git_dir)

451

452 static int shared_callback(const struct option *opt, const char *arg, int unset)

453 {

454 + BUG_ON_OPT_NEG(unset);

455 *((int *) opt->value) = (arg) ? git_config_perm("arg", arg) : PERM_GROUP;

456 return 0;

457 }

builtin/interpret-trailers.c

@@ -80,6 +80,8 @@ static int parse_opt_parse(const struct option *opt, const char *arg,

80 v->only_trailers = 1;

81 v->only_input = 1;

82 v->unfold = 1;

83 + BUG_ON_OPT_NEG(unset);

84 + BUG_ON_OPT_ARG(arg);

85 return 0;

86 }

builtin/log.c

+10

@@ -107,6 +107,8 @@ static int log_line_range_callback(const struct option *option, const char *arg,

107 {

108 struct line_opt_callback_data *data = option->value;

109

110 + BUG_ON_OPT_NEG(unset);

111 +

112 if (!arg)

113 return -1;

114

@@ -1151,6 +1153,8 @@ static int keep_subject = 0;

1153

1154 static int keep_callback(const struct option *opt, const char *arg, int unset)

1155 {

1156 + BUG_ON_OPT_NEG(unset);

1157 + BUG_ON_OPT_ARG(arg);

1158 ((struct rev_info *)opt->value)->total = -1;

1159 keep_subject = 1;

1160 return 0;

@@ -1161,6 +1165,7 @@ static int subject_prefix = 0;

1165 static int subject_prefix_callback(const struct option *opt, const char *arg,

1166 int unset)

1167 {

1168 + BUG_ON_OPT_NEG(unset);

1169 subject_prefix = 1;

1170 ((struct rev_info *)opt->value)->subject_prefix = arg;

1171 return 0;

@@ -1168,6 +1173,8 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,

1173

1174 static int rfc_callback(const struct option *opt, const char *arg, int unset)

1175 {

1176 + BUG_ON_OPT_NEG(unset);

1177 + BUG_ON_OPT_ARG(arg);

1178 return subject_prefix_callback(opt, "RFC PATCH", unset);

1179 }

1180

@@ -1176,6 +1183,7 @@ static int numbered_cmdline_opt = 0;

1183 static int numbered_callback(const struct option *opt, const char *arg,

1184 int unset)

1185 {

1186 + BUG_ON_OPT_ARG(arg);

1187 *(int *)opt->value = numbered_cmdline_opt = unset ? 0 : 1;

1188 if (unset)

1189 auto_number = 0;

@@ -1185,6 +1193,7 @@ static int numbered_callback(const struct option *opt, const char *arg,

1193 static int no_numbered_callback(const struct option *opt, const char *arg,

1194 int unset)

1195 {

1196 + BUG_ON_OPT_NEG(unset);

1197 return numbered_callback(opt, arg, 1);

1198 }

1199

@@ -1192,6 +1201,7 @@ static int output_directory_callback(const struct option *opt, const char *arg,

1201 int unset)

1202 {

1203 const char **dir = (const char **)opt->value;

1204 + BUG_ON_OPT_NEG(unset);

1205 if (*dir)

1206 die(_("Two output directories?"));

1207 *dir = arg;

builtin/ls-files.c

@@ -475,6 +475,8 @@ static int option_parse_exclude(const struct option *opt,

475 {

476 struct string_list *exclude_list = opt->value;

477

478 + BUG_ON_OPT_NEG(unset);

479 +

480 exc_given = 1;

481 string_list_append(exclude_list, arg);

482

@@ -486,6 +488,8 @@ static int option_parse_exclude_from(const struct option *opt,

488 {

489 struct dir_struct *dir = opt->value;

490

491 + BUG_ON_OPT_NEG(unset);

492 +

493 exc_given = 1;

494 add_excludes_from_file(dir, arg);

495

@@ -497,6 +501,9 @@ static int option_parse_exclude_standard(const struct option *opt,

501 {

502 struct dir_struct *dir = opt->value;

503

504 + BUG_ON_OPT_NEG(unset);

505 + BUG_ON_OPT_ARG(arg);

506 +

507 exc_given = 1;

508 setup_standard_excludes(dir);

509

builtin/merge-file.c

@@ -15,6 +15,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset)

15 static int label_count = 0;

16 const char **names = (const char **)opt->value;

18 + BUG_ON_OPT_NEG(unset);

19 +

20 if (label_count >= 3)

21 return error("too many labels on the command line");

22 names[label_count++] = arg;

builtin/merge.c

@@ -224,6 +224,7 @@ static int option_parse_x(const struct option *opt,

224 static int option_parse_n(const struct option *opt,

225 const char *arg, int unset)

226 {

227 + BUG_ON_OPT_ARG(arg);

228 show_diffstat = unset;

229 return 0;

230 }

builtin/notes.c

@@ -215,6 +215,8 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)

215 {

216 struct note_data *d = opt->value;

217

218 + BUG_ON_OPT_NEG(unset);

219 +

220 strbuf_grow(&d->buf, strlen(arg) + 2);

221 if (d->buf.len)

222 strbuf_addch(&d->buf, '\n');

@@ -229,6 +231,8 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)

231 {

232 struct note_data *d = opt->value;

233

234 + BUG_ON_OPT_NEG(unset);

235 +

236 if (d->buf.len)

237 strbuf_addch(&d->buf, '\n');

238 if (!strcmp(arg, "-")) {

@@ -250,6 +254,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)

254 enum object_type type;

255 unsigned long len;

256

257 + BUG_ON_OPT_NEG(unset);

258 +

259 if (d->buf.len)

260 strbuf_addch(&d->buf, '\n');

261

@@ -273,6 +279,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)

279 static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)

280 {

281 struct note_data *d = opt->value;

282 + BUG_ON_OPT_NEG(unset);

283 d->use_editor = 1;

284 return parse_reuse_arg(opt, arg, unset);

285 }

builtin/pack-objects.c

@@ -3206,6 +3206,9 @@ static int option_parse_index_version(const struct option *opt,

3206 {

3207 char *c;

3208 const char *val = arg;

3209 +

3210 + BUG_ON_OPT_NEG(unset);

3211 +

3212 pack_idx_opts.version = strtoul(val, &c, 10);

3213 if (pack_idx_opts.version > 2)

3214 die(_("unsupported index version %s"), val);

builtin/read-tree.c

@@ -44,6 +44,7 @@ static const char * const read_tree_usage[] = {

44 static int index_output_cb(const struct option *opt, const char *arg,

45 int unset)

46 {

47 + BUG_ON_OPT_NEG(unset);

48 set_alternate_index_output(arg);

49 return 0;

50 }

@@ -54,6 +55,8 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,

55 struct dir_struct *dir;

56 struct unpack_trees_options *opts;

58 + BUG_ON_OPT_NEG(unset);

59 +

60 opts = (struct unpack_trees_options *)opt->value;

62 if (opts->dir)

builtin/rebase.c

@@ -703,6 +703,9 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)

703 {

704 struct rebase_options *opts = opt->value;

705

706 + BUG_ON_OPT_NEG(unset);

707 + BUG_ON_OPT_ARG(arg);

708 +

709 if (!is_interactive(opts))

710 opts->type = REBASE_MERGE;

711

@@ -715,6 +718,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,

718 {

719 struct rebase_options *opts = opt->value;

720

721 + BUG_ON_OPT_NEG(unset);

722 + BUG_ON_OPT_ARG(arg);

723 +

724 opts->type = REBASE_INTERACTIVE;

725 opts->flags |= REBASE_INTERACTIVE_EXPLICIT;

726

builtin/show-branch.c

@@ -604,6 +604,7 @@ static int parse_reflog_param(const struct option *opt, const char *arg,

604 {

605 char *ep;

606 const char **base = (const char **)opt->value;

607 + BUG_ON_OPT_NEG(unset);

608 if (!arg)

609 arg = "";

610 reflog = strtoul(arg, &ep, 10);

builtin/show-ref.c

@@ -151,6 +151,7 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)

151 static int exclude_existing_callback(const struct option *opt, const char *arg,

152 int unset)

153 {

154 + BUG_ON_OPT_NEG(unset);

155 exclude_arg = 1;

156 *(const char **)opt->value = arg;

157 return 0;

builtin/tag.c

@@ -338,6 +338,8 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)

338 {

339 struct msg_arg *msg = opt->value;

340

341 + BUG_ON_OPT_NEG(unset);

342 +

343 if (!arg)

344 return -1;

345 if (msg->buf.len)

builtin/update-index.c

+19 -2

@@ -790,12 +790,16 @@ static int refresh(struct refresh_params *o, unsigned int flag)

790 static int refresh_callback(const struct option *opt,

791 const char *arg, int unset)

792 {

793 + BUG_ON_OPT_NEG(unset);

794 + BUG_ON_OPT_ARG(arg);

795 return refresh(opt->value, 0);

796 }

797

798 static int really_refresh_callback(const struct option *opt,

799 const char *arg, int unset)

800 {

801 + BUG_ON_OPT_NEG(unset);

802 + BUG_ON_OPT_ARG(arg);

803 return refresh(opt->value, REFRESH_REALLY);

804 }

805

@@ -803,6 +807,7 @@ static int chmod_callback(const struct option *opt,

807 const char *arg, int unset)

808 {

809 char *flip = opt->value;

810 + BUG_ON_OPT_NEG(unset);

811 if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])

812 return error("option 'chmod' expects \"+x\" or \"-x\"");

813 *flip = arg[0];

@@ -812,6 +817,8 @@ static int chmod_callback(const struct option *opt,

817 static int resolve_undo_clear_callback(const struct option *opt,

818 const char *arg, int unset)

819 {

820 + BUG_ON_OPT_NEG(unset);

821 + BUG_ON_OPT_ARG(arg);

822 resolve_undo_clear();

823 return 0;

824 }

@@ -847,6 +854,8 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,

854 unsigned int mode;

855 const char *path;

856

857 + BUG_ON_OPT_NEG(unset);

858 +

859 if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, &oid, &path)) {

860 if (add_cacheinfo(mode, &oid, path, 0))

861 die("git update-index: --cacheinfo cannot add %s", path);

@@ -869,6 +878,8 @@ static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,

878 {

879 int *nul_term_line = opt->value;

880

881 + BUG_ON_OPT_NEG(unset);

882 +

883 if (ctx->argc != 1)

884 return error("option '%s' must be the last argument", opt->long_name);

885 allow_add = allow_replace = allow_remove = 1;

@@ -881,6 +892,8 @@ static int stdin_callback(struct parse_opt_ctx_t *ctx,

892 {

893 int *read_from_stdin = opt->value;

894

895 + BUG_ON_OPT_NEG(unset);

896 +

897 if (ctx->argc != 1)

898 return error("option '%s' must be the last argument", opt->long_name);

899 *read_from_stdin = 1;

@@ -888,11 +901,13 @@ static int stdin_callback(struct parse_opt_ctx_t *ctx,

901 }

902

903 static int unresolve_callback(struct parse_opt_ctx_t *ctx,

891 - const struct option *opt, int flags)

904 + const struct option *opt, int unset)

905 {

906 int *has_errors = opt->value;

907 const char *prefix = startup_info->prefix;

908

909 + BUG_ON_OPT_NEG(unset);

910 +

911 /* consume remaining arguments. */

912 *has_errors = do_unresolve(ctx->argc, ctx->argv,

913 prefix, prefix ? strlen(prefix) : 0);

@@ -905,11 +920,13 @@ static int unresolve_callback(struct parse_opt_ctx_t *ctx,

920 }

921

922 static int reupdate_callback(struct parse_opt_ctx_t *ctx,

908 - const struct option *opt, int flags)

923 + const struct option *opt, int unset)

924 {

925 int *has_errors = opt->value;

926 const char *prefix = startup_info->prefix;

927

928 + BUG_ON_OPT_NEG(unset);

929 +

930 /* consume remaining arguments. */

931 setup_work_tree();

932 *has_errors = do_reupdate(ctx->argc, ctx->argv,

parse-options-cb.c

@@ -58,6 +58,8 @@ int parse_opt_verbosity_cb(const struct option *opt, const char *arg,

58 {

59 int *target = opt->value;

61 + BUG_ON_OPT_ARG(arg);

62 +

63 if (unset)

64 /* --no-quiet, --no-verbose */

65 *target = 0;

@@ -80,6 +82,8 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)

82 struct object_id oid;

83 struct commit *commit;

85 + BUG_ON_OPT_NEG(unset);

86 +

87 if (!arg)

88 return -1;

89 if (get_oid(arg, &oid))

@@ -110,6 +114,9 @@ int parse_opt_object_name(const struct option *opt, const char *arg, int unset)

114 int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)

115 {

116 int *target = opt->value;

117 +

118 + BUG_ON_OPT_ARG(arg);

119 +

120 *target = unset ? 2 : 1;

121 return 0;

122 }

parse-options.h

+14

@@ -191,6 +191,20 @@ extern int opterror(const struct option *opt, const char *reason, int flags);

191 #define opterror(o,r,f) (opterror((o),(r),(f)), const_error())

192 #endif

193

194 +/*

195 + * Use these assertions for callbacks that expect to be called with NONEG and

196 + * NOARG respectively, and do not otherwise handle the "unset" and "arg"

197 + * parameters.

198 + */

199 +#define BUG_ON_OPT_NEG(unset) do { \

200 + if ((unset)) \

201 + BUG("option callback does not expect negation"); \

202 +} while (0)

203 +#define BUG_ON_OPT_ARG(arg) do { \

204 + if ((arg)) \

205 + BUG("option callback does not expect an argument"); \

206 +} while (0)

207 +

208 /*----- incremental advanced APIs -----*/

209

210 enum {

ref-filter.c

@@ -2316,6 +2316,8 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)

2316 struct object_id oid;

2317 int no_merged = starts_with(opt->long_name, "no");

2318

2319 + BUG_ON_OPT_NEG(unset);

2320 +

2321 if (rf->merge) {

2322 if (no_merged) {

2323 return opterror(opt, "is incompatible with --merged", 0);

t/helper/test-parse-options.c

@@ -36,6 +36,7 @@ static int length_callback(const struct option *opt, const char *arg, int unset)

37 static int number_callback(const struct option *opt, const char *arg, int unset)

38 {

39 + BUG_ON_OPT_NEG(unset);

40 *(int *)opt->value = strtol(arg, NULL, 10);

41 return 0;

42 }