Jeff King <peff@xxxxxxxx> writes: > 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. > 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. > 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-<. > 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.
