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

Christian Couder [off-list ref] 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.