Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 21, 2023, 18:20 UTC
Message-ID: <xmqqfrzvmn7p.fsf@gitster.g>
In-Reply-To: <20231221083643.GA545870@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> Which would mean that simply dropping --end-of-options from the list, as
> your patch does, would be similarly unsafe. It is papering over the
> problem of:
>
>   git sparse-checkout --end-of-options foo
>
> but leaving:
>
>   git sparse-checkout --end-of-options --foo
>
> broken.

Hmph, I do not understand. The latter case we want to take "--foo" as the sole parameter to the subcommand, no?

> But the plot thickens! Curiously, in both of these cases, we do not do
> any further parsing of the options at all. We just treat them as
> pattern arguments with no special meaning.

Exactly.

Show 9 quoted lines

> So your patch is actually OK, but I would argue that the correct fix
> here is to drop the use of PARSE_OPT_KEEP_UNKNOWN_OPT at all. I cannot
> find any indication in the history on why it was ever used. It was added
> when the command was converted to parse-options in 7bffca95ea
> (sparse-checkout: add '--stdin' option to set subcommand, 2019-11-21).
> Back then it was just called KEEP_UNKNOWN. Later it was renamed to
> KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options: PARSE_OPT_KEEP_UNKNOWN
> only applies to --options, 2022-08-19) to clarify that it was only about
> dashed options; we always keep non-option arguments.

Yes.

> So looking at that original patch, it makes me think that the author was
> confused about the mis-named option, and really just wanted to keep the
> non-option arguments. We never should have used the flag all along (and
> the other cases were cargo-culted within the file).

That is something only the original author can answer, by my working theory has been they wanted to have a cheap way to allow any string, even the ones that happen to begin with a dash, to be passed as one of the patterns.

Show 8 quoted lines

> There is one minor gotcha, though. Unlike most other Git commands,
> sparse-checkout will happily accept random option names and treat them
> as non-option arguments. E.g. this works:
>
>   git sparse-checkout add --foo --bar
>
> and will add "--foo" and "--bar" as patterns. If we remove the flag,
> we'd be breaking that.

Exactly. The user _should_ protect these "patterns" by using "--end-of-options" before "--foo" above, but we have been taking the patterns with such a loose parser for quite some time, so tightening would be taken as a regression X-<.

Show 9 quoted lines

> But I'd argue that anybody relying on that is
> asking for trouble. For example, this does not work in the same way:
>
>   git sparse-checkout add --skip-checks --foo
>
> because "--skip-checks" is a real option. Ditto for any other options,
> including those we add in the future. If you don't trust the contents of
> your arguments, you should be using "--" or "--end-of-options" to
> communicate the intent to the command.

Exactly. My response to the other message, with a new test, summarizes my position.

Thanks.