diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index fd165e11d2..9758ba5275 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -66,12 +66,10 @@ fast-import stream! This option is enabled automatically for remote-helpers that use the `import` capability, as they are already trusted to run their own code. + -Note that this option has to be spelled in full, and has to appear -before any option whose value is separated from it by a space, for -the unsafe `feature` commands in the stream to be allowed. So -`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse -them, while `--allow-unsafe-features --depth 5` and -`--depth=5 --allow-unsafe-features` allow them. +Note that this option has to be spelled in full for the unsafe +`feature` commands in the stream to be allowed. So while +`--allow-unsafe` is accepted as an unambiguous abbreviation of this +option, it still refuses them. `--signed-tags=`:: Specify how to handle signed tags. Behaves in the same way as diff --git a/builtin/bisect.c b/builtin/bisect.c index 1cfb8a794b..ad089b289f 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, return bisect_next(terms, prefix); } +/* + * The options "git bisect start" accepts. Only the ones taking their + * value as a separate argument matter to the scan looking for "--" below, + * as their value has to be skipped along with them. + */ +static const struct early_scan_option bisect_start_early_options[] = { + EARLY_SCAN_SKIP_VALUE("term-good"), + EARLY_SCAN_SKIP_VALUE("term-old"), + EARLY_SCAN_SKIP_VALUE("term-bad"), + EARLY_SCAN_SKIP_VALUE("term-new"), + EARLY_SCAN_END() +}; + static enum bisect_error bisect_start(struct bisect_terms *terms, int argc, const char **argv) { @@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc, /* * Check for one bad and then some good revisions + * + * The scan below has to know about the options taking their value + * as a separate argument, or such a value that happens to be "--" + * would be mistaken for the "--" separating revisions from paths. */ - for (i = 0; i < argc; i++) { - if (!strcmp(argv[i], "--")) { - has_double_dash = 1; - break; - } - } + i = early_scan_options(argc, argv, bisect_start_early_options, + EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL); + if (i < argc) + has_double_dash = 1; for (i = 0; i < argc; i++) { const char *arg = argv[i]; diff --git a/builtin/fast-import.c b/builtin/fast-import.c index fbd919982c..cf0504f01c 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -4120,12 +4120,29 @@ static int option_parse_quiet(const struct option *opt UNUSED, return 0; } +/* + * The only option the early scan below is interested in, as it decides + * whether unsafe "feature" commands from the stream are allowed. + */ +static const char *early_wanted[] = { "allow-unsafe-features", NULL }; + +static int option_parse_early_allow_unsafe( + const struct early_scan_option *opt UNUSED, + const char *value UNUSED, int pos UNUSED, void *data) +{ + struct fast_import_state *state = data; + + state->allow_unsafe_features = 1; + return 0; +} + int cmd_fast_import(int argc, const char **argv, const char *prefix, struct repository *repo) { struct fast_import_state state; + struct early_scan_option *early; struct option fast_import_options[] = { OPT_GROUP(N_("Common")), @@ -4218,23 +4235,20 @@ int cmd_fast_import(int argc, * line to override stream data). But we must do an early parse of any * command-line options that impact how we interpret the feature lines. * - * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features" - * spelling and stops at the first argument that doesn't start with a - * dash. As parse_options() below also accepts unambiguous abbreviations - * and values separated by a space from their option, the two disagree - * for command lines like "--allow-unsafe" or "--depth 5 - * --allow-unsafe-features": parse_options() accepts the option, but - * this scan doesn't see it, so unsafe features from the stream are - * still refused. This errs on the safe side, but should be fixed by - * teaching this scan about the options that take a value. + * NEEDSWORK: This scan only matches the exact + * "--allow-unsafe-features" spelling, while parse_options() below + * also accepts unambiguous abbreviations, so the two disagree for + * a command line like "--allow-unsafe": parse_options() accepts + * the option, but this scan doesn't see it, so unsafe features + * from the stream are still refused. This errs on the safe side. */ - for (int i = 1; i < argc; i++) { - const char *arg = argv[i]; - if (*arg != '-' || !strcmp(arg, "--")) - break; - if (!strcmp(arg, "--allow-unsafe-features")) - state.allow_unsafe_features = 1; - } + early = early_scan_options_from_options(fast_import_options, + early_wanted); + early_scan_options(argc - 1, argv + 1, early, + EARLY_SCAN_STOP_AT_DASHDASH | + EARLY_SCAN_STOP_AT_NON_OPTION, + option_parse_early_allow_unsafe, &state); + free(early); rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free)); for (unsigned int i = 0; i < (cmd_save - 1); i++) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 10938003b5..77d9b400a2 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -695,6 +695,17 @@ static void print_path(const char *path, const char *prefix, strbuf_release(&sb); } +/* + * The options taking their value as a separate argument, which the scan + * looking for "--" below has to skip along with their value. + */ +static const struct early_scan_option rev_parse_early_options[] = { + EARLY_SCAN_SKIP_VALUE("default"), + EARLY_SCAN_SKIP_VALUE("prefix"), + EARLY_SCAN_SKIP_VALUE("resolve-git-dir"), + EARLY_SCAN_END() +}; + int cmd_rev_parse(int argc, const char **argv, const char *prefix, @@ -724,12 +735,15 @@ int cmd_rev_parse(int argc, if (argc > 1 && !strcmp("-h", argv[1])) usage(builtin_rev_parse_usage); - for (i = 1; i < argc; i++) { - if (!strcmp(argv[i], "--")) { - has_dashdash = 1; - break; - } - } + /* + * The scan below has to know about the options taking their value + * as a separate argument, or such a value that happens to be "--" + * would be mistaken for the "--" separating revisions from paths. + */ + i = early_scan_options(argc - 1, argv + 1, rev_parse_early_options, + EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL); + if (i < argc - 1) + has_dashdash = 1; /* No options; just report on whether we're in a git repo or not. */ if (argc == 1) { diff --git a/parse-options.c b/parse-options.c index 51a49792d1..3476e2cfaf 100644 --- a/parse-options.c +++ b/parse-options.c @@ -841,6 +841,26 @@ static void show_negated_gitcomp(const struct option *opts, int show_all, } } +int parse_options_takes_argument(const struct option *opt) +{ + switch (opt->type) { + case OPTION_STRING: + case OPTION_FILENAME: + case OPTION_INTEGER: + case OPTION_UNSIGNED: + case OPTION_CALLBACK: + break; + default: + return 0; + } + + if (opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_OPTARG | + PARSE_OPT_LASTARG_DEFAULT)) + return 0; + + return 1; +} + static int show_gitcomp(const struct option *opts, int show_all) { const struct option *original_opts = opts; @@ -862,20 +882,9 @@ static int show_gitcomp(const struct option *opts, int show_all) break; case OPTION_GROUP: continue; - case OPTION_STRING: - case OPTION_FILENAME: - case OPTION_INTEGER: - case OPTION_UNSIGNED: - case OPTION_CALLBACK: - if (opts->flags & PARSE_OPT_NOARG) - break; - if (opts->flags & PARSE_OPT_OPTARG) - break; - if (opts->flags & PARSE_OPT_LASTARG_DEFAULT) - break; - suffix = "="; - break; default: + if (parse_options_takes_argument(opts)) + suffix = "="; break; } if (opts->flags & PARSE_OPT_COMP_ARG) @@ -1246,6 +1255,115 @@ int parse_options(int argc, const char **argv, return parse_options_end(&ctx); } +/* + * Look for `arg` among `options`. On success, return the matching option + * and set `value` to the value stuck to it, if any, or to NULL. + */ +static const struct early_scan_option * +find_early_scan_option(const char *arg, + const struct early_scan_option *options, + const char **value) +{ + if (!skip_prefix(arg, "--", &arg)) + return NULL; + + for (; options->name; options++) { + const char *rest; + + if (!skip_prefix(arg, options->name, &rest)) + continue; + if (!*rest) { + *value = NULL; + return options; + } + /* Only an option taking a value can be stuck to one. */ + if (*rest == '=' && options->takes_value) { + *value = rest + 1; + return options; + } + } + + return NULL; +} + +int early_scan_options(int argc, const char **argv, + const struct early_scan_option *options, + enum early_scan_flags flags, + early_scan_fn *fn, void *data) +{ + for (int i = 0; i < argc; i++) { + const char *arg = argv[i]; + const char *value; + const struct early_scan_option *opt; + int pos = i; + + if ((flags & EARLY_SCAN_STOP_AT_DASHDASH) && + !strcmp(arg, "--")) + return i; + + opt = find_early_scan_option(arg, options, &value); + if (!opt) { + if ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) && + (*arg != '-' || !arg[1])) + return i; + continue; + } + + /* + * When an option takes a value, but that value is not + * stuck to it with '=', then the next argument is the + * value and it has to be skipped so that it isn't + * taken for an option itself. + */ + if (opt->takes_value && !value && i + 1 < argc) + value = argv[++i]; + + if (opt->wanted && fn(opt, value, pos, data)) + return i; + } + + return argc; +} + +struct early_scan_option * +early_scan_options_from_options(const struct option *options, + const char **wanted) +{ + struct early_scan_option *early; + size_t nr = 0; + + for (const struct option *opt = options; opt->type != OPTION_END; opt++) + if (opt->long_name) + nr++; + + CALLOC_ARRAY(early, nr + 1); + + nr = 0; + for (const struct option *opt = options; opt->type != OPTION_END; opt++) { + if (!opt->long_name) + continue; + early[nr].name = opt->long_name; + early[nr].takes_value = !!parse_options_takes_argument(opt); + nr++; + } + + for (; wanted && *wanted; wanted++) { + size_t i; + + for (i = 0; i < nr; i++) { + if (strcmp(early[i].name, *wanted)) + continue; + early[i].wanted = 1; + break; + } + if (i == nr) + BUG("wanted option '%s' is not in the options array", + *wanted); + } + + return early; +} + static int usage_argh(const struct option *opts, FILE *outfile) { const char *s; diff --git a/parse-options.h b/parse-options.h index a0b30f3c04..2fdb3b6098 100644 --- a/parse-options.h +++ b/parse-options.h @@ -423,6 +423,16 @@ int parse_options(int argc, const char **argv, const char *prefix, const char * const usagestr[], enum parse_opt_flags flags); +/* + * Return non-zero if `opt` takes a value, which means that it consumes + * the next argument when that value is not stuck to it with an '='. + * + * Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the + * next argument when it isn't the last one, so it is not considered as + * taking a value here. + */ +int parse_options_takes_argument(const struct option *opt); + NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); @@ -494,6 +504,88 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name, BUG("option callback expects an argument"); \ } while(0) +/*----- Early scan: scanning argv before the actual option parsing -----*/ + +/* + * Some commands need to look at a few options before they can parse + * their command line for real, for example because the result decides + * whether a repository is needed at all. + * + * Such an early scan has to know which options take their value as a + * separate argument, or it could mistake such a value for an option. The + * `struct early_scan_option` array passed to early_scan_options() below + * describes the options to look for, as well as the ones that only need + * to be skipped along with their value. + */ +struct early_scan_option { + const char *name; /* Option name, without the leading dashes */ + unsigned takes_value:1; /* "--option=value" or "--option value" expected? */ + unsigned wanted:1; /* Report option to callback? */ +}; + +#define EARLY_SCAN_SKIP_VALUE(n) { .name = (n), .takes_value = 1 } +#define EARLY_SCAN_WANT(n) { .name = (n), .wanted = 1 } +#define EARLY_SCAN_WANT_VALUE(n) { .name = (n), .takes_value = 1, .wanted = 1 } +#define EARLY_SCAN_END() { NULL } + +/* + * Called by early_scan_options() for each argument matching a + * `struct early_scan_option` that has its `wanted` bit set. + * + * `option` is the matching option, `value` its value or NULL if it + * doesn't take one, and `pos` the index of the option in argv. + * + * Returning a non-zero value stops the scan. + */ +typedef int early_scan_fn(const struct early_scan_option *option, + const char *value, int pos, void *data); + +enum early_scan_flags { + EARLY_SCAN_STOP_AT_DASHDASH = 1 << 0, /* Stop at "--" */ + EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 1, +}; + +/* + * Scan `argv` for the options described by `options`, calling `fn` + * for each of those that are `wanted`. `argv` is not modified. + * + * `fn` may be NULL when no option is `wanted`, which is useful to only + * find out where the scan stops. + * + * Note that abbreviated options are not recognized, as a scan cannot + * know about the options it hasn't been told about, and would then + * resolve abbreviations differently from the actual option parsing. + * + * Returns the index at which the scan stopped, which is `argc` when the + * whole array was scanned. + */ +int early_scan_options(int argc, const char **argv, + const struct early_scan_option *options, + enum early_scan_flags flags, + early_scan_fn *fn, void *data); + +/* + * Build the `struct early_scan_option` array to pass to + * early_scan_options() from the `options` array that the actual option + * parsing uses, so that both agree on which options take a value. + * + * Note some intentional limitations to keep the scan simple and fast: + * short options are ignored, options with PARSE_OPT_LASTARG_DEFAULT or + * PARSE_OPT_OPTARG are treated as not taking a separate value, negated + * options ("--no-...") are not automatically generated, and abbreviated + * options will not be matched. + * + * The options named in the NULL terminated `wanted` array get their + * `wanted` bit set, the other ones are only there to be skipped along + * with their value. It is a BUG() for a name in `wanted` not to appear + * in `options`. + * + * The returned array is allocated and should be free()d by the caller. + */ +struct early_scan_option * +early_scan_options_from_options(const struct option *options, + const char **wanted); + /*----- incremental advanced APIs -----*/ struct parse_opt_cmdmode_list; diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index f181f0c02d..0187a25ccb 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -383,3 +383,74 @@ int cmd__parse_subcommand(int argc, const char **argv) return parse_subcommand__cmd(argc, argv, test_flags); } + +static int show_early_option(const struct early_scan_option *opt, + const char *value, int pos, void *data UNUSED) +{ + printf("found: %s at %d", opt->name, pos); + if (value) + printf(" value: %s", value); + putchar('\n'); + return 0; +} + +int cmd__early_scan_options(int argc, const char **argv) +{ + static const struct early_scan_option options[] = { + EARLY_SCAN_WANT("wanted"), + EARLY_SCAN_WANT_VALUE("wanted-value"), + EARLY_SCAN_SKIP_VALUE("skipped-value"), + EARLY_SCAN_END() + }; + enum early_scan_flags flags = 0; + int stopped; + + while (argc > 1 && *argv[1] == '-') { + if (!strcmp(argv[1], "--stop-at-dashdash")) + flags |= EARLY_SCAN_STOP_AT_DASHDASH; + else if (!strcmp(argv[1], "--stop-at-non-option")) + flags |= EARLY_SCAN_STOP_AT_NON_OPTION; + else + break; + argc--; + argv++; + } + + stopped = early_scan_options(argc - 1, argv + 1, options, flags, + show_early_option, NULL); + printf("stopped at: %d of %d\n", stopped, argc - 1); + + return 0; +} + +int cmd__early_scan_from_options(int argc, const char **argv) +{ + int an_int = 0, a_bool = 0; + char *a_string = NULL; + const struct option options[] = { + OPT_STRING(0, "string", &a_string, "str", "get a string"), + OPT_INTEGER(0, "int", &an_int, "get an integer"), + OPT_BOOL(0, "bool", &a_bool, "get a boolean"), + OPT_STRING_F(0, "optarg", &a_string, "str", + "string with an optional value", + PARSE_OPT_OPTARG), + OPT_END() + }; + static const char *wanted[] = { "bool", NULL }; + struct early_scan_option *early; + int stopped; + + early = early_scan_options_from_options(options, wanted); + + for (const struct early_scan_option *o = early; o->name; o++) + printf("option: %s takes_value: %d wanted: %d\n", + o->name, o->takes_value, o->wanted); + + stopped = early_scan_options(argc - 1, argv + 1, early, 0, + show_early_option, NULL); + printf("stopped at: %d of %d\n", stopped, argc - 1); + + free(early); + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index b71a22b43b..f1b208a5af 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -50,6 +50,8 @@ static struct test_cmd cmds[] = { { "pack-mtimes", cmd__pack_mtimes }, { "parse-options", cmd__parse_options }, { "parse-options-flags", cmd__parse_options_flags }, + { "early-scan-options", cmd__early_scan_options }, + { "early-scan-from-options", cmd__early_scan_from_options }, { "parse-pathspec-file", cmd__parse_pathspec_file }, { "parse-subcommand", cmd__parse_subcommand }, { "partial-clone", cmd__partial_clone }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index f2885b33d5..97334ce3c6 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -43,6 +43,8 @@ int cmd__pack_deltas(int argc, const char **argv); int cmd__pack_mtimes(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); int cmd__parse_options_flags(int argc, const char **argv); +int cmd__early_scan_options(int argc, const char **argv); +int cmd__early_scan_from_options(int argc, const char **argv); int cmd__parse_pathspec_file(int argc, const char** argv); int cmd__parse_subcommand(int argc, const char **argv); int cmd__partial_clone(int argc, const char **argv); diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 449fff4d34..bb72a6544d 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -845,4 +845,107 @@ test_expect_success 'u16 limits range' ' test_grep "value 65536 for option .u16. not in range \[0,65535\]" err ' +test_expect_success 'early_scan_options() finds a wanted option' ' + test-tool early-scan-options --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 0 + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() reads a stuck or separate value' ' + test-tool early-scan-options --wanted-value=one >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: one + stopped at: 1 of 1 + EOF + test_cmp expect actual && + test-tool early-scan-options --wanted-value two >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: two + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() skips the value of other options' ' + test-tool early-scan-options --skipped-value --wanted >actual && + cat >expect <<-\EOF && + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --skipped-value one --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() can stop at "--"' ' + test-tool early-scan-options --stop-at-dashdash -- --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-dashdash \ + --skipped-value -- --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() can stop at a non-option' ' + test-tool early-scan-options --stop-at-non-option \ + arg --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-non-option \ + --skipped-value arg --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() ignores abbreviated options' ' + test-tool early-scan-options --want >actual && + cat >expect <<-\EOF && + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options_from_options() derives takes_value' ' + test-tool early-scan-from-options >actual && + cat >expect <<-\EOF && + option: string takes_value: 1 wanted: 0 + option: int takes_value: 1 wanted: 0 + option: bool takes_value: 0 wanted: 1 + option: optarg takes_value: 0 wanted: 0 + stopped at: 0 of 0 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options_from_options() skips values' ' + test-tool early-scan-from-options --string --bool >out && + tail -1 out >actual && + echo "stopped at: 2 of 2" >expect && + test_cmp expect actual && + test-tool early-scan-from-options --string v --bool >out && + tail -2 out >actual && + cat >expect <<-\EOF && + found: bool at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + test_done diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index 30bf8a4d0a..b60be318ee 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -383,4 +383,9 @@ test_expect_success ':/ and HEAD^{/} favor more recent matching commits' ' ) ' +test_expect_success 'rev-parse with "--" as an option value' ' + test_must_fail git rev-parse --default -- notarev 2>err && + test_grep "ambiguous argument .notarev." err +' + test_done diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index a7588222a8..464ca53b42 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' ' test_cmp expected actual ' +test_expect_success 'bisect start with "--" as a term name' ' + git bisect reset && + git bisect start --term-good -- hello && + git bisect terms --term-good >actual && + echo -- >expected && + test_cmp expected actual +' + # Bisect is started with --term-new and --term-old arguments, # then skip. The HEAD should be changed. test_expect_success 'bisect skip works with --term*' ' diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index d9de2ef0d8..1a37f2b8e6 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2344,6 +2344,20 @@ test_expect_success 'R: export-marks options can be overridden by commandline op test_path_is_missing feature-sub ' +test_expect_success 'R: --allow-unsafe-features found after a value' ' + echo "feature import-marks-if-exists=nonexistent.marks" >input && + git fast-import --allow-unsafe-features input && + test_must_fail git fast-import --allow-unsafe err && + test_grep "forbidden in input without --allow-unsafe-features" err +' + test_expect_success 'R: catch typo in marks file name' ' test_must_fail git fast-import --import-marks=nonexistent.marks