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

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

Jeff King <peff@peff.net> writes:

> On Wed, Dec 20, 2023 at 06:46:51PM -0800, Junio C Hamano wrote:
>
>> Josh Steadmon <steadmon@google.com> writes:
>> 
>> > I can confirm that this fixes an issue noticed by sparse-checkout users
>> > at $DAYJOB. Looks good to me. Thanks!
>> 
>> Heh, there is another one that is not converted in the same file for
>> "check-rules" subcommand, so the posted patch is way incomplete, I
>> think.
>
> Yeah. I think it is in the same boat as the other two, in that I believe
> that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be
> dropped.

If we dropped KEEP_UNKNOWN_OPT, however, the pattern that is
currently accepted will stop working, e.g.,

 $ git sparse-checkout [add/set] --[no-]cone --foo bar

as we would barf with "--foo: unknown option", and the users who are
used to this sloppy command line parsing we had for the past few
years must now write "--end-of-options" before "--foo".  After all,
the reason why the original authors of this code used KEEP_UNKNOWN
is likely to deal with path patterns that begin with dashes.

The patch in the message that started this thread may not be
correct, either, I am afraid.  For either of these:

 $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar
 $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar

we would see that "foo" (or "--foo") is not "--end-of-options", and
we end up using three patterns "foo" (or "--foo"),
"--end-of-options", and "bar", I suspect.  I wonder if we should
notice the "foo" or "--foo" that were not understood and error out,
instead.

But after all, it is not absolutely necessary to notice and barf.
The ONLY practical use of the "--end-of-options" mechanism is to
allow us to write (this applies to any git subcommand):

 #!/bin/sh
 git cmd --hard --coded --options --end-of-options "$@"

in scripts to protect the intended operation from mistaking the
end-user input as options.  And with a script written carefully to
do so, all the args that appear before "--end-of-options" would be
recognizable by the command line parser.  On the other hand, such a
"notice and barf" would not help a script that is written without
"--end-of-options", when it is fed "$@" that happens to begin with
"--end-of-options".  We would silently swallow "--end-of-options"
without any chance to notice the lack of "--end-of-options" in the
script.  So I dunno.

Here is an additional test on top of [1/3]
<20231221065925.3234048-2-gitster@pobox.com>

that demonstrates and summarizes the idea.

 t/t1090-sparse-checkout-scope.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh
index 5b96716235..da9534d222 100755
--- c/t/t1090-sparse-checkout-scope.sh
+++ w/t/t1090-sparse-checkout-scope.sh
@@ -54,6 +54,40 @@ test_expect_success 'return to full checkout of main' '
 	test "$(cat b)" = "modified"
 '
 
+test_expect_success 'sparse-checkout set command line parsing' '
+	# baseline
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone "a*" &&
+	echo "a*" >expect &&
+	test_cmp .git/info/sparse-checkout expect &&
+
+	# unknown string that looks like a dashed option is
+	# taken as a mere pattern
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone --foo "a*" &&
+	test_write_lines "--foo" "a*" >expect &&
+	test_cmp .git/info/sparse-checkout expect &&
+
+	# --end-of-options can be used to protect parameters that
+	# potentially begin with dashes
+	set x --cone "a*" && shift &&
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone --end-of-options "$@" &&
+	test_write_lines "$@" >expect &&
+	test_cmp .git/info/sparse-checkout expect &&
+
+	# but writing --end-of-options after what the command does not
+	# recognize is too late; it becomes one of the patterns, so
+	# that the end-user input that happens to be "--end-of-options"
+	# can be passed through.  To be absoutely sure, you should write
+	# --end-of-options yourself before taking "$@" in.
+	set x --foo --end-of-options "a*" && shift &&
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone "$@" &&
+	test_write_lines "$@" >expect &&
+	test_cmp .git/info/sparse-checkout expect
+'
+
 test_expect_success 'skip-worktree on files outside sparse patterns' '
 	git sparse-checkout disable &&
 	git sparse-checkout set --no-cone "a*" &&