Re: [PATCH 0/6] Standardize early option scanning to fix argument parsing bugs

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

Christian Couder <christian.couder@gmail.com> writes:

> A number of commands perform an early scan of their arguments to look
> for specific flags or structural separators (like `--`).
>
> These hand-rolled early scans are often fragile. They especially fail
> to account for options that take their value as a separate
> argument. This leads to disagreements between the early scan and the
> actual parse_options() pass. For example, the early scanner might miss
> a special option entirely, or mistakenly treat an option's value as
> the `--` path separator.
>
> To allow these commands to safely skip option values during their
> early scans, this series introduces a new "early-scan" sub-API into
> the existing "parse-options" API.

Yay.

> This is deliberately implemented as a new simple and fast scan, which
> has some limitations, instead of a full refactor and reuse of the
> parse_options() code,

Sigh.  In other words, we hate these ad-hoc prescan that are buggy
badly enough to replace them all with yet another ad-hoc prescan
that is know to behave differently from the real thing?

>  - `git bisect start --term-good -- <not-a-rev>` mistook the term name
>    `--` for the revision/path separator, so <not-a-rev> was rejected
>    as an invalid revision instead of being treated as a path.

Sorry, I fail to see much practical value in this.

>  - `git rev-parse --default -- <not-a-rev>` did the same, reporting
>    "bad revision <notarev>" while any other default value gives the
>    usual more helpful "ambiguous argument" error.

Neither in this one.

>  - `git fast-import --depth 5 --allow-unsafe-features` silently
>    ignored `--allow-unsafe-features`, refusing unsafe features from
>    the stream.

On the other hand, this may be a very good thing.

Is the reason why the ad-hoc pre-scan failed to see it was because
it did not realize 5 is a value to the --depth option?

> All of these commands call parse_options(), but for `git bisect` and
> `git rev-parse`, the specific functions doing the early scan
> (bisect_start() and cmd_rev_parse()'s main loop) parse their own
> options by hand after the early scan and have no `struct option` array
> for those options.
>
> If bisect_start() and cmd_rev_parse() were converted to use
> `struct option`, they could use early_scan_options_from_options() and
> would not be affected by limitations 1), 2) and 3) above, as both use
> the early scan only to locate `--`.

I imagine that in the long term we would rather see a properly
refactored parse-options machinery perform the prescan (perhaps with
some kind of "dry-run" option given to the machinery) than yet
another ad-hoc parser like this topic introduces.  It would be very
good if this interim solution at least took the same 'options[]'
array so that when we have the real thing in the future we do not
have to redo the conversion effort.

By the way, how does this interact with your other topic that has
been stalled for quite some time?  Would moving this one forward
help the other, or do they not have much relevance to each other?  I
would rather not see two topics of non-trivial size stalled on a
single author at the same time, so ...

Thanks.