Skip to content

Commit ecd2d3e

Browse files
peffgitster
authored andcommitted
pass subcommand "prefix" arguments to parse_options()
Recent commits such as bf0a6b6 (builtin/multi-pack-index.c: let parse-options parse subcommands, 2022-08-19) converted a few functions to match our usual argc/argv/prefix conventions, but the prefix argument remains unused. However, there is a good use for it: they should pass it to their own parse_options() functions, where it may be used to adjust the value of any filename options. In all but one of these functions, there's no behavior change, since they don't use OPT_FILENAME. But this is an actual fix for one option, which you can see by modifying the test suite like so: diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh index 4fe5741..d0974d4371 100755 --- a/t/t5326-multi-pack-bitmaps.sh +++ b/t/t5326-multi-pack-bitmaps.sh @@ -186,7 +186,11 @@ test_expect_success 'writing a bitmap with --refs-snapshot' ' # Then again, but with a refs snapshot which only sees # refs/tags/one. - git multi-pack-index write --bitmap --refs-snapshot=snapshot && + ( + mkdir subdir && + cd subdir && + git multi-pack-index write --bitmap --refs-snapshot=../snapshot + ) && test_path_is_file $midx && test_path_is_file $midx-$(midx_checksum $objdir).bitmap && I'd emphasize that this wasn't broken by bf0a6b6; it has been broken all along, because the sub-function never got to see the prefix. It is that commit which is actually enabling us to fix it (and which also brought attention to the problem because it triggers -Wunused-parameter!) The other functions changed here don't use OPT_FILENAME at all. In their cases this isn't fixing anything visible, but it's following the usual pattern and future-proofing them against somebody adding new options and being surprised. I didn't include a test for the one visible case above. We don't generally test routine parse-options behavior for individual options. The challenge here was finding the problem, and now that this has been done, it's not likely to regress. Likewise, we could apply the patch above to cover it "for free" but it makes reading the rest of the test unnecessarily complicated. Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 398c4ff commit ecd2d3e

File tree

4 files changed

+26
-22
lines changed

4 files changed

+26
-22
lines changed

builtin/commit-graph.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
8080
trace2_cmd_mode("verify");
8181

8282
opts.progress = isatty(2);
83-
argc = parse_options(argc, argv, NULL,
83+
argc = parse_options(argc, argv, prefix,
8484
options,
8585
builtin_commit_graph_verify_usage, 0);
8686
if (argc)
@@ -241,7 +241,7 @@ static int graph_write(int argc, const char **argv, const char *prefix)
241241

242242
git_config(git_commit_graph_write_config, &opts);
243243

244-
argc = parse_options(argc, argv, NULL,
244+
argc = parse_options(argc, argv, prefix,
245245
options,
246246
builtin_commit_graph_write_usage, 0);
247247
if (argc)

builtin/multi-pack-index.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
133133

134134
if (isatty(2))
135135
opts.flags |= MIDX_PROGRESS;
136-
argc = parse_options(argc, argv, NULL,
136+
argc = parse_options(argc, argv, prefix,
137137
options, builtin_multi_pack_index_write_usage,
138138
0);
139139
if (argc)
@@ -176,7 +176,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv,
176176

177177
if (isatty(2))
178178
opts.flags |= MIDX_PROGRESS;
179-
argc = parse_options(argc, argv, NULL,
179+
argc = parse_options(argc, argv, prefix,
180180
options, builtin_multi_pack_index_verify_usage,
181181
0);
182182
if (argc)
@@ -203,7 +203,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv,
203203

204204
if (isatty(2))
205205
opts.flags |= MIDX_PROGRESS;
206-
argc = parse_options(argc, argv, NULL,
206+
argc = parse_options(argc, argv, prefix,
207207
options, builtin_multi_pack_index_expire_usage,
208208
0);
209209
if (argc)
@@ -233,7 +233,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv,
233233

234234
if (isatty(2))
235235
opts.flags |= MIDX_PROGRESS;
236-
argc = parse_options(argc, argv, NULL,
236+
argc = parse_options(argc, argv, prefix,
237237
options,
238238
builtin_multi_pack_index_repack_usage,
239239
0);

builtin/remote.c

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,8 @@ static int add(int argc, const char **argv, const char *prefix)
177177
OPT_END()
178178
};
179179

180-
argc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,
181-
0);
180+
argc = parse_options(argc, argv, prefix, options,
181+
builtin_remote_add_usage, 0);
182182

183183
if (argc != 2)
184184
usage_with_options(builtin_remote_add_usage, options);
@@ -695,7 +695,7 @@ static int mv(int argc, const char **argv, const char *prefix)
695695
int i, refs_renamed_nr = 0, refspec_updated = 0;
696696
struct progress *progress = NULL;
697697

698-
argc = parse_options(argc, argv, NULL, options,
698+
argc = parse_options(argc, argv, prefix, options,
699699
builtin_remote_rename_usage, 0);
700700

701701
if (argc != 2)
@@ -1264,7 +1264,8 @@ static int show(int argc, const char **argv, const char *prefix)
12641264
};
12651265
struct show_info info = SHOW_INFO_INIT;
12661266

1267-
argc = parse_options(argc, argv, NULL, options, builtin_remote_show_usage,
1267+
argc = parse_options(argc, argv, prefix, options,
1268+
builtin_remote_show_usage,
12681269
0);
12691270

12701271
if (argc < 1)
@@ -1371,8 +1372,8 @@ static int set_head(int argc, const char **argv, const char *prefix)
13711372
N_("delete refs/remotes/<name>/HEAD")),
13721373
OPT_END()
13731374
};
1374-
argc = parse_options(argc, argv, NULL, options, builtin_remote_sethead_usage,
1375-
0);
1375+
argc = parse_options(argc, argv, prefix, options,
1376+
builtin_remote_sethead_usage, 0);
13761377
if (argc)
13771378
strbuf_addf(&buf, "refs/remotes/%s/HEAD", argv[0]);
13781379

@@ -1471,8 +1472,8 @@ static int prune(int argc, const char **argv, const char *prefix)
14711472
OPT_END()
14721473
};
14731474

1474-
argc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,
1475-
0);
1475+
argc = parse_options(argc, argv, prefix, options,
1476+
builtin_remote_prune_usage, 0);
14761477

14771478
if (argc < 1)
14781479
usage_with_options(builtin_remote_prune_usage, options);
@@ -1504,7 +1505,8 @@ static int update(int argc, const char **argv, const char *prefix)
15041505
int default_defined = 0;
15051506
int retval;
15061507

1507-
argc = parse_options(argc, argv, NULL, options, builtin_remote_update_usage,
1508+
argc = parse_options(argc, argv, prefix, options,
1509+
builtin_remote_update_usage,
15081510
PARSE_OPT_KEEP_ARGV0);
15091511

15101512
strvec_push(&fetch_argv, "fetch");
@@ -1583,7 +1585,7 @@ static int set_branches(int argc, const char **argv, const char *prefix)
15831585
OPT_END()
15841586
};
15851587

1586-
argc = parse_options(argc, argv, NULL, options,
1588+
argc = parse_options(argc, argv, prefix, options,
15871589
builtin_remote_setbranches_usage, 0);
15881590
if (argc == 0) {
15891591
error(_("no remote specified"));
@@ -1608,7 +1610,8 @@ static int get_url(int argc, const char **argv, const char *prefix)
16081610
N_("return all URLs")),
16091611
OPT_END()
16101612
};
1611-
argc = parse_options(argc, argv, NULL, options, builtin_remote_geturl_usage, 0);
1613+
argc = parse_options(argc, argv, prefix, options,
1614+
builtin_remote_geturl_usage, 0);
16121615

16131616
if (argc != 1)
16141617
usage_with_options(builtin_remote_geturl_usage, options);
@@ -1668,7 +1671,8 @@ static int set_url(int argc, const char **argv, const char *prefix)
16681671
N_("delete URLs")),
16691672
OPT_END()
16701673
};
1671-
argc = parse_options(argc, argv, NULL, options, builtin_remote_seturl_usage,
1674+
argc = parse_options(argc, argv, prefix, options,
1675+
builtin_remote_seturl_usage,
16721676
PARSE_OPT_KEEP_ARGV0);
16731677

16741678
if (add_mode && delete_mode)

builtin/sparse-checkout.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix)
6060
if (!core_apply_sparse_checkout)
6161
die(_("this worktree is not sparse"));
6262

63-
argc = parse_options(argc, argv, NULL,
63+
argc = parse_options(argc, argv, prefix,
6464
builtin_sparse_checkout_list_options,
6565
builtin_sparse_checkout_list_usage, 0);
6666

@@ -452,7 +452,7 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)
452452
init_opts.cone_mode = -1;
453453
init_opts.sparse_index = -1;
454454

455-
argc = parse_options(argc, argv, NULL,
455+
argc = parse_options(argc, argv, prefix,
456456
builtin_sparse_checkout_init_options,
457457
builtin_sparse_checkout_init_usage, 0);
458458

@@ -860,7 +860,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
860860
reapply_opts.cone_mode = -1;
861861
reapply_opts.sparse_index = -1;
862862

863-
argc = parse_options(argc, argv, NULL,
863+
argc = parse_options(argc, argv, prefix,
864864
builtin_sparse_checkout_reapply_options,
865865
builtin_sparse_checkout_reapply_usage, 0);
866866

@@ -897,7 +897,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
897897
* forcibly return to a dense checkout regardless of initial state.
898898
*/
899899

900-
argc = parse_options(argc, argv, NULL,
900+
argc = parse_options(argc, argv, prefix,
901901
builtin_sparse_checkout_disable_options,
902902
builtin_sparse_checkout_disable_usage, 0);
903903

0 commit comments

Comments
 (0)