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, 17:02 UTC
Message-ID: <xmqqsf3vmqug.fsf@gitster.g>
In-Reply-To: <20231221084011.GB545870@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

> 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*" &&