Merge branch 'cc/early-scan-options' into seen

The process of parsing command-line options in commands that
perform an early scan over their arguments (such as 'git bisect',
'git rev-parse', and 'git fast-import') has been unified using a
new early-scan sub-API, which parses and skips known options taking
separate values to prevent logic bugs.

* cc/early-scan-options:
  fast-import: use early_scan_options() for --allow-unsafe-features
  parse-options: build early scan options from a struct option array
  parse-options: add parse_options_takes_argument()
  rev-parse: fix "--" detection when it is an option value
  bisect: fix "--" detection when a term name is "--"
  parse-options: add early_scan_options()
Junio C Hamano 2026-09-17 12:54:42 -07:00
commit d208fd9f90
13 changed files with 503 additions and 47 deletions

View File

@ -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=<mode>`::
Specify how to handle signed tags. Behaves in the same way as

View File

@ -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];

View File

@ -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++)

View File

@ -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) {

View File

@ -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;

View File

@ -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;

View File

@ -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;
}

View File

@ -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 },

View File

@ -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);

View File

@ -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

View File

@ -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

View File

@ -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*' '

View File

@ -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 &&
git fast-import --depth=5 --allow-unsafe-features <input &&
git fast-import --depth 5 --allow-unsafe-features <input &&
git fast-import --date-format raw --allow-unsafe-features <input
'

test_expect_success 'R: --allow-unsafe-features has to be spelled in full' '
echo "feature import-marks-if-exists=nonexistent.marks" >input &&
test_must_fail git fast-import --allow-unsafe <input 2>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 </dev/null &&
echo "feature import-marks=nonexistent.marks" |