From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Derrick Stolee <stolee@gmail•com>,
git@vger•kernel.org, Josh Steadmon <steadmon@google•com>
Subject: Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add
Date: Thu, 21 Dec 2023 10:20:42 -0800 [thread overview]
Message-ID: <xmqqfrzvmn7p.fsf@gitster.g> (raw)
In-Reply-To: <20231221083643.GA545870@coredump.intra.peff.net> (Jeff King's message of "Thu, 21 Dec 2023 03:36:43 -0500")
Jeff King <peff@peff•net> 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.
prev parent reply other threads:[~2023-12-21 18:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-20 23:19 [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add Junio C Hamano
2023-12-20 23:55 ` Josh Steadmon
2023-12-21 2:46 ` Junio C Hamano
2023-12-21 8:40 ` Jeff King
2023-12-21 17:02 ` Junio C Hamano
2023-12-21 21:45 ` Jeff King
2023-12-21 22:04 ` Junio C Hamano
2023-12-23 10:02 ` Jeff King
2023-12-23 15:38 ` rsbecker
2023-12-23 22:45 ` Elijah Newren
2023-12-24 1:02 ` Elijah Newren
2023-12-21 2:41 ` [RFC/PATCH] archive: "--list" does not take further options Junio C Hamano
2023-12-21 7:30 ` René Scharfe
2023-12-21 8:59 ` Jeff King
2023-12-21 18:13 ` Junio C Hamano
2023-12-21 21:35 ` Jeff King
2023-12-21 8:58 ` Jeff King
2023-12-21 8:36 ` [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add Jeff King
2023-12-21 18:20 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqfrzvmn7p.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=steadmon@google$(echo .)com \
--cc=stolee@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox