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

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

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

> I've investigated the relevant parsing paths further:
>
> 1. Revision options are handled by handle_revision_opt() in
> revision.c. It already determines argument consumption through helpers
> such as parse_long_opt(), which returns whether an option uses an
> attached or separate argument. However, the function also applies the
> option's effects to struct rev_info, so calling it again purely to
> determine argument consumption would be unsafe.

Correct.  There is no dry-run mode for this.

> 2. Revision pseudo-options are handled separately by
> handle_revision_pseudo_opt().

Yes, this is called from various code paths.  No dry-run, either.

> 3. Diff-merge options are handled by diff_merges_parse_opts() in diff-merges.c.
>
> 4. Diff options are defined through add_diff_options() in diff.c,
> which provides an existing struct option table. This includes options
> such as --src-prefix and --dst-prefix.

Correct.  I do not think there is any dry-run to any of these.

I am not sure if the above is exhausitve, though.  I didn't check.

> This suggests that the early scan needs to obtain argument-consumption
> information from the same option definitions or recognition logic used
> by normal parsing, rather than maintaining another list of option
> names.

It is what the NEEDSWORK comment boils down to, I think.  I do not
see how the above survey helps with that conclusion, though.

> One direction I'm considering is separating option recognition and
> argument consumption from option application, so that the early scan
> and normal parser can share the recognition logic without executing
> parsing side effects twice.
>
> I have not settled on the implementation yet, particularly how best to
> accommodate the hand-written revision and diff-merge parsers alongside
> the diff option table.
>
> Would you consider this a reasonable direction? Or do you see an
> existing Git parser interface or abstraction that would be preferable
> to extend for this purpose?

All of the above assumes that we want to keep a separate "early
scan", but I wonder if that is really a good direction to go.  If
you can clearly identify the problem the "early scan" wanted to
solve, and can come up with a way to solve that problem without
scanning the list of arguments twice (i.e., not involving an "early
scan" at all), would that give you a cleaner implementation?

If the default of an option (is it '.fetch_if_missing' alone, or are
there many other settings involved?) needs to depend on other
options, perhaps fixing it is simpler if the presence of an explicit
option that sets '.fetch_if_missing' is recorded in a separate
member of the struct, and then at the very end after all the command
line options have been seen, take these other options and the
explicit option setting for it into account to synthesize the
result?  If '.fetch_if_missing' was explicitly set from what the
user said from the command line, there is no need to look at any
other options, and if not, then the settings left by parsing other
options that the misdesigned "early scan" pays attention to can be
consulted to decide what the desirable value should be.

Just thinking out loud.  It seems that this started at df11e19648
(rev-list: support termination at promisor objects, 2017-12-08), and
then other needs may have been piled on top, but going back to that
original commit may be a good place to start understanding and
reevaluating if the "early scan" was the best approach to solve
whatever problem it was trying to solve.

> Thanks again for the guidance.
>
> On Fri, 6 Nov 2026 at 01:36, Junio C Hamano <gitster@pobox.com> wrote:

Never top-post.