Re: [PATCH v4] rev-list: handle arguments in early option scan

[Junio C Hamano <gitster@pobox.com>]

Azuama Odinaka Maximus <azuama69@gmail.com> writes:

> The early option scan in cmd_rev_list() looks for options that
> affect missing-object handling before setup_revisions() parses the
> command line. It must not interpret an option's separate argument
> as another option.
>
> Use parse_long_opt() to recognize revision options that take
> separate arguments, and skip those arguments. Handle -n and
> --default as well. Stop scanning at --, where subsequent arguments
> are pathspecs.
>
> Add tests to ensure that --grep and --author arguments beginning
> with -- are not mistaken for options during the early scan.

The commit log message explains the motivation well, and the attempt
to address the feedback from the previous round by handling all 19
options from revision.c is appreciated.

But the approach of hardcoding an array of option strings that take
arguments is inherently fragile and scales poorly.  By duplicating
parser knowledge into the early scan, this patch falls into the
exact trap described by the NEEDSWORK comment just below it.

> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 071a0e3cb5..4dd646050a 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -744,6 +744,47 @@ static void prepare_maximal_independent(struct rev_info *revs)
>  	revs->no_walk = 1;
>  }
>  
> +static int rev_list_option_takes_argument(const char **argv)
> +{
> +	static const char * const options[] = {
> +		"max-count",
> +		"max-count-oldest",
> +		"skip",
> +		"max-age",
> +		"since",
> +		"since-as-filter",
> +		"after",
> +		"min-age",
> +		"before",
> +		"until",
> +		"date",
> +		"author",
> +		"committer",
> +		"grep-reflog",
> +		"grep",
> +		"encoding",
> +		"glob",
> +		"exclude",
> +		"exclude-hidden",
> +	};

Is this hardcoded list complete?  It misses options parsed via
diff_merges_parse_opts() in diff-merges.c (e.g., --diff-merges), as
well as options handled in diff.c (e.g., --src-prefix and
--dst-prefix), which also take separate arguments and are accepted
by git rev-list.  Would

    $ git rev-list --src-prefix --missing-only HEAD

take --missing-only as an extra option or a prefix?

Thanks.