sparse-checkout: be consistent with end of options markers

From: Elijah Newren <[email protected]>

93851746 (parse-options: decouple "--end-of-options" and "--", 2023-12-06) updated the world order to make callers of parse-options that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to do with "--end-of-options" they may see after parse_options() returns.

This made a previous bug in sparse-checkout more visible; namely, that

git sparse-checkout [add|set] --[no-]cone --end-of-options ...

would simply treat "--end-of-options" as one of the paths to include in the sparse-checkout. But this was already problematic before; namely,

git sparse-checkout [add|set| --[no-]cone --sikp-checks ...

would not give an error on the mis-typed "--skip-checks" but instead simply treat "--sikp-checks" as a path or pattern to include in the sparse-checkout, which is highly unfriendly.

This behavior begain 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. Looking at that original patch, both Peff and I think that the author was simply 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).

Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this bug. Note that this does mean that anyone who might have been using

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

to request paths or patterns '--foo' and '--bar' will now have to use

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

That makes sparse-checkout more consistent with other git commands, provides users much friendlier error messages and behavior, and is consistent with the all-capps warning in git-sparse-checkout.txt that this command "is experimental...its behavior...will likely change". :-)

---
[RFC] sparse-checkout: be consistent with end of options markers

Follow-up to thread over at
https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1625%2Fgitgitgadget%2Fsparse-checkout-end-of-options-consistency-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1625/gitgitgadget/sparse-checkout-end-of-options-consistency-v1
Pull-Request: https://github.com/git/git/pull/1625

 builtin/sparse-checkout.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

index 5c8ffb1f759..0e68e9b0b0d 100644
@@ -777,8 +777,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_add_options,
-			     builtin_sparse_checkout_add_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_add_usage, 0);
 
 	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 

@@ -824,8 +823,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_set_options,
-			     builtin_sparse_checkout_set_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_set_usage, 0);
 
 	if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
 		return 1;

@@ -996,8 +994,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_check_rules_options,
-			     builtin_sparse_checkout_check_rules_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_check_rules_usage, 0);
 
 	if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
 		check_rules_opts.cone_mode = 1;

base-commit: 055bb6e9969085777b7fab83e3fee0017654f134

gitgitgadget

Thanks for wrapping this up in patch form. It looks good to me, including the reasoning. You didn't add any tests, but I find it rather unlikely that we'd later regress here.

-Peff

Surely. I am certainly OK with just dropping KEEP_UNKNOWN but I would strongly prefer to document what we "fixed" (your "misspelt option name" example) and what (mis|ab)use the people may have been relying on we have "broken" (the same "misspelt" behaviour that can be intentional is now forbidden, and we document that this change in behaviour is intentional) with a new test.

Thanks.

Yeah, thank you for talking some sense into me. I do not foresee us regressing "--sikp-checks", but certainly covering --end-of-options in more places is worthwhile. As it's handled centrally, it can have unexpected consequences for various commands.

Elijah's latest version looks good to me.

-Peff

Nicely described.

Apparently we do not have an existing test to cover the case of "misspelt options becoming a pattern" that this bugfix corrects; otherwise we would have a s/failure/success/ to update such an older expectation, and have to wonder if the existing behaviour is intentional. Since there is no such test, we can feel a bit safer in "fixing" this odd behaviour.

Also, this corrects "--end-of-options" that becomes one of the patterns. Such a bug was not detected in our test suite.

So we should at least have two new tests.

(1) Pass "--foo" without the end of options marker and make sure we error out, instead of making it as one of the patterns.

(2) Pass "--end-of-options foo" and make sure the command succeeds, an d"--end-of-options" does not become on eof the patterns.

Thanks.

Hi,

I did actually create two such tests last Saturday, but they obviously somehow went missing from my submission (I guess even if the high fevers from Wed-Fri last week were gone, I was still more affected than I realized?) Anyway, I'll resubmit with those test cases.

From: Elijah Newren <[email protected]>

93851746 (parse-options: decouple "--end-of-options" and "--", 2023-12-06) updated the world order to make callers of parse-options that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to do with "--end-of-options" they may see after parse_options() returns.

This made a previous bug in sparse-checkout more visible; namely, that

git sparse-checkout [add|set] --[no-]cone --end-of-options ...

would simply treat "--end-of-options" as one of the paths to include in the sparse-checkout. But this was already problematic before; namely,

git sparse-checkout [add|set| --[no-]cone --sikp-checks ...

would not give an error on the mis-typed "--skip-checks" but instead simply treat "--sikp-checks" as a path or pattern to include in the sparse-checkout, which is highly unfriendly.

This behavior began 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. Looking at that original patch, both Peff and I think that the author was simply 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).

Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this bug. Note that this does mean that anyone who might have been using

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

to request paths or patterns '--foo' and '--bar' will now have to use

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

That makes sparse-checkout more consistent with other git commands, provides users much friendlier error messages and behavior, and is consistent with the all-caps warning in git-sparse-checkout.txt that this command "is experimental...its behavior...will likely change". :-)

---
[RFC] sparse-checkout: be consistent with end of options markers

Follow-up to thread over at
https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/,
making end of options markers in git-sparse-checkout consistent with how other git commands behave.

Changes since v1:

* Added some testcases

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1625%2Fgitgitgadget%2Fsparse-checkout-end-of-options-consistency-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1625/gitgitgadget/sparse-checkout-end-of-options-consistency-v2
Pull-Request: https://github.com/git/git/pull/1625

Range-diff vs v1:

1: c19156919a6 ! 1: 448146637a9 sparse-checkout: be consistent with end of options markers
@@ Commit message
simply treat "--sikp-checks" as a path or pattern to include in the sparse-checkout, which is highly unfriendly.

- This behavior begain when the command was converted to parse-options in
+ This behavior began 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:
@@ Commit message

That makes sparse-checkout more consistent with other git commands, provides users much friendlier error messages and behavior, and is
- consistent with the all-capps warning in git-sparse-checkout.txt that
+ consistent with the all-caps warning in git-sparse-checkout.txt that
this command "is experimental...its behavior...will likely change". :-)

@@ builtin/sparse-checkout.c: static int sparse_checkout_check_rules(int argc, cons

if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
check_rules_opts.cone_mode = 1;
+
+ ## t/t1091-sparse-checkout-builtin.sh ##
+@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'cone mode: set with nested folders' ' +
+ test_expect_success 'cone mode: add independent path' '
+ git -C repo sparse-checkout set deep/deeper1 &&
+- git -C repo sparse-checkout add folder1 &&
++ git -C repo sparse-checkout add --end-of-options folder1 &&
+ cat >expect <<-\EOF &&
+ /*
+ !/*/
+@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'by default, cone mode will error out when passed files' '
+ grep ".gitignore.*is not a directory" error
+ '
+
++test_expect_success 'error on mistyped command line options' '
++ test_must_fail git -C repo sparse-checkout add --sikp-checks .gitignore 2>error &&
++
++ grep "unknown option.*sikp-checks" error
++'
++
+ test_expect_success 'by default, non-cone mode will warn on individual files' '
+ git -C repo sparse-checkout reapply --no-cone &&
+ git -C repo sparse-checkout add .gitignore 2>warning &&

 builtin/sparse-checkout.c          | 9 +++------
 t/t1091-sparse-checkout-builtin.sh | 8 +++++++-
 2 files changed, 10 insertions(+), 7 deletions(-)

index 5c8ffb1f759..0e68e9b0b0d 100644
@@ -777,8 +777,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_add_options,
-			     builtin_sparse_checkout_add_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_add_usage, 0);
 
 	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 

@@ -824,8 +823,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_set_options,
-			     builtin_sparse_checkout_set_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_set_usage, 0);
 
 	if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
 		return 1;

@@ -996,8 +994,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_check_rules_options,
-			     builtin_sparse_checkout_check_rules_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_check_rules_usage, 0);
 
 	if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
 		check_rules_opts.cone_mode = 1;

index f67611da28e..e49b8024ac5 100755
@@ -334,7 +334,7 @@ test_expect_success 'cone mode: set with nested folders' '
 
 test_expect_success 'cone mode: add independent path' '
 	git -C repo sparse-checkout set deep/deeper1 &&
-	git -C repo sparse-checkout add folder1 &&
+	git -C repo sparse-checkout add --end-of-options folder1 &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/

@@ -886,6 +886,12 @@ test_expect_success 'by default, cone mode will error out when passed files' '
 	grep ".gitignore.*is not a directory" error
 '
 
+test_expect_success 'error on mistyped command line options' '
+	test_must_fail git -C repo sparse-checkout add --sikp-checks .gitignore 2>error &&
+
+	grep "unknown option.*sikp-checks" error
+'
+
 test_expect_success 'by default, non-cone mode will warn on individual files' '
 	git -C repo sparse-checkout reapply --no-cone &&
 	git -C repo sparse-checkout add .gitignore 2>warning &&

base-commit: 055bb6e9969085777b7fab83e3fee0017654f134

gitgitgadget

Let me drop jc/sparse-checkout-eoo topic and queue this instead, and then resurrect the "use default for 'set' only when !stdin" as a separate topic.

Thanks.