{"thread":{"id":"60651","subject":"[PATCH] sparse-checkout: be consistent with end of options markers","startedAt":"2023-12-24T01:00:15Z","lastAt":"2023-12-28T11:43:32Z","messageCount":8,"participants":["Elijah Newren via GitGitGadget","Jeff King","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486006","messageId":"pull.1625.git.git.1703379611749.gitgitgadget@gmail.com","threadId":"60651","inReplyTo":null,"subject":"[PATCH] sparse-checkout: be consistent with end of options markers","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-24T01:00:11Z","receivedAt":"2023-12-24T01:00:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n93851746 (parse-options: decouple \"--end-of-options\" and \"--\",\n2023-12-06) updated the world order to make callers of parse-options\nthat set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to\ndo with \"--end-of-options\" they may see after parse_options() returns.\n\nThis made a previous bug in sparse-checkout more visible; namely,\nthat\n\n git sparse-checkout [add|set] --[no-]cone --end-of-options ...\n\nwould simply treat \"--end-of-options\" as one of the paths to include in\nthe sparse-checkout. But this was already problematic before; namely,\n\n git sparse-checkout [add|set| --[no-]cone --sikp-checks ...\n\nwould not give an error on the mis-typed \"--skip-checks\" but instead\nsimply treat \"--sikp-checks\" as a path or pattern to include in the\nsparse-checkout, which is highly unfriendly.\n\nThis behavior begain when the command was converted to parse-options in\n7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,\n2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was\nrenamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:\nPARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify\nthat it was only about dashed options; we always keep non-option\narguments. Looking at that original patch, both Peff and I think that\nthe author was simply confused about the mis-named option, and really\njust wanted to keep the non-option arguments. We never should have used\nthe flag all along (and the other cases were cargo-culted within the\nfile).\n\nRemove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\nbug. Note that this does mean that anyone who might have been using\n\n git sparse-checkout [add|set] [--[no-]cone] --foo --bar\n\nto request paths or patterns '--foo' and '--bar' will now have to use\n\n git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar\n\nThat makes sparse-checkout more consistent with other git commands,\nprovides users much friendlier error messages and behavior, and is\nconsistent with the all-capps warning in git-sparse-checkout.txt that\nthis command \"is experimental...its behavior...will likely change\". :-)\n\nSigned-off-by: Elijah Newren \n---\n [RFC] sparse-checkout: be consistent with end of options markers\n \n Follow-up to thread over at\n https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1625%2Fgitgitgadget%2Fsparse-checkout-end-of-options-consistency-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1625/gitgitgadget/sparse-checkout-end-of-options-consistency-v1\nPull-Request: https://github.com/git/git/pull/1625\n\n builtin/sparse-checkout.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 5c8ffb1f759..0e68e9b0b0d 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -777,8 +777,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_sparse_checkout_add_options,\n-\t\t\t builtin_sparse_checkout_add_usage,\n-\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\t\t\t builtin_sparse_checkout_add_usage, 0);\n \n \tsanitize_paths(argc, argv, prefix, add_opts.skip_checks);\n \n@@ -824,8 +823,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_sparse_checkout_set_options,\n-\t\t\t builtin_sparse_checkout_set_usage,\n-\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\t\t\t builtin_sparse_checkout_set_usage, 0);\n \n \tif (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))\n \t\treturn 1;\n@@ -996,8 +994,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_sparse_checkout_check_rules_options,\n-\t\t\t builtin_sparse_checkout_check_rules_usage,\n-\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\t\t\t builtin_sparse_checkout_check_rules_usage, 0);\n \n \tif (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)\n \t\tcheck_rules_opts.cone_mode = 1;\n\nbase-commit: 055bb6e9969085777b7fab83e3fee0017654f134\n-- \ngitgitgadget\n"},{"id":"486012","messageId":"20231224083206.GA2053380@coredump.intra.peff.net","threadId":"60651","inReplyTo":"pull.1625.git.git.1703379611749.gitgitgadget@gmail.com","subject":"Re: [PATCH] sparse-checkout: be consistent with end of options markers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-24T08:32:06Z","receivedAt":"2023-12-24T08:32:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\n> bug. Note that this does mean that anyone who might have been using\n> [...]\n\nThanks for wrapping this up in patch form. It looks good to me,\nincluding the reasoning. You didn't add any tests, but I find it rather\nunlikely that we'd later regress here.\n\n-Peff\n"},{"id":"486028","messageId":"xmqqfrzoj31l.fsf@gitster.g","threadId":"60651","inReplyTo":"20231224083206.GA2053380@coredump.intra.peff.net","subject":"Re: [PATCH] sparse-checkout: be consistent with end of options markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T17:18:14Z","receivedAt":"2023-12-26T17:18:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n>> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\n>> bug. Note that this does mean that anyone who might have been using\n>> [...]\n>\n> Thanks for wrapping this up in patch form. It looks good to me,\n> including the reasoning. You didn't add any tests, but I find it rather\n> unlikely that we'd later regress here.\n\nSurely. I am certainly OK with just dropping KEEP_UNKNOWN but I\nwould strongly prefer to document what we \"fixed\" (your \"misspelt\noption name\" example) and what (mis|ab)use the people may have been\nrelying on we have \"broken\" (the same \"misspelt\" behaviour that can\nbe intentional is now forbidden, and we document that this change in\nbehaviour is intentional) with a new test.\n\nThanks.\n"},{"id":"486029","messageId":"xmqq8r5gj2o3.fsf@gitster.g","threadId":"60651","inReplyTo":"pull.1625.git.git.1703379611749.gitgitgadget@gmail.com","subject":"Re: [PATCH] sparse-checkout: be consistent with end of options markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T17:26:20Z","receivedAt":"2023-12-26T17:26:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> 93851746 (parse-options: decouple \"--end-of-options\" and \"--\",\n> 2023-12-06) updated the world order to make callers of parse-options\n> that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to\n> do with \"--end-of-options\" they may see after parse_options() returns.\n>\n> This made a previous bug in sparse-checkout more visible; namely,\n> that\n>\n> git sparse-checkout [add|set] --[no-]cone --end-of-options ...\n>\n> would simply treat \"--end-of-options\" as one of the paths to include in\n> the sparse-checkout. But this was already problematic before; namely,\n>\n> git sparse-checkout [add|set| --[no-]cone --sikp-checks ...\n>\n> would not give an error on the mis-typed \"--skip-checks\" but instead\n> simply treat \"--sikp-checks\" as a path or pattern to include in the\n> sparse-checkout, which is highly unfriendly.\n>\n> This behavior begain when the command was converted to parse-options in\n> 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,\n> 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was\n> renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:\n> PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify\n> that it was only about dashed options; we always keep non-option\n> arguments. Looking at that original patch, both Peff and I think that\n> the author was simply confused about the mis-named option, and really\n> just wanted to keep the non-option arguments. We never should have used\n> the flag all along (and the other cases were cargo-culted within the\n> file).\n>\n> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\n> bug. Note that this does mean that anyone who might have been using\n>\n> git sparse-checkout [add|set] [--[no-]cone] --foo --bar\n>\n> to request paths or patterns '--foo' and '--bar' will now have to use\n>\n> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar\n>\n> That makes sparse-checkout more consistent with other git commands,\n> provides users much friendlier error messages and behavior, and is\n> consistent with the all-capps warning in git-sparse-checkout.txt that\n> this command \"is experimental...its behavior...will likely change\". :-)\n>\n> Signed-off-by: Elijah Newren \n> ---\n\nNicely described.\n\nApparently we do not have an existing test to cover the case of\n\"misspelt options becoming a pattern\" that this bugfix corrects;\notherwise we would have a s/failure/success/ to update such an older\nexpectation, and have to wonder if the existing behaviour is\nintentional. Since there is no such test, we can feel a bit safer\nin \"fixing\" this odd behaviour.\n\nAlso, this corrects \"--end-of-options\" that becomes one of the\npatterns. Such a bug was not detected in our test suite.\n\nSo we should at least have two new tests.\n\n (1) Pass \"--foo\" without the end of options marker and make sure we\n error out, instead of making it as one of the patterns.\n\n (2) Pass \"--end-of-options foo\" and make sure the command succeeds,\n an d\"--end-of-options\" does not become on eof the patterns.\n\nThanks.\n"},{"id":"486034","messageId":"CABPp-BGSUytAZ+mCWRsP7NoHbO3sBBtnbeAs=UZo6eVMa62XbQ@mail.gmail.com","threadId":"60651","inReplyTo":"xmqq8r5gj2o3.fsf@gitster.g","subject":"Re: [PATCH] sparse-checkout: be consistent with end of options markers","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-12-26T19:19:34Z","receivedAt":"2023-12-26T19:19:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Dec 26, 2023 at 9:26 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > 93851746 (parse-options: decouple \"--end-of-options\" and \"--\",\n> > 2023-12-06) updated the world order to make callers of parse-options\n> > that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to\n> > do with \"--end-of-options\" they may see after parse_options() returns.\n> >\n> > This made a previous bug in sparse-checkout more visible; namely,\n> > that\n> >\n> > git sparse-checkout [add|set] --[no-]cone --end-of-options ...\n> >\n> > would simply treat \"--end-of-options\" as one of the paths to include in\n> > the sparse-checkout. But this was already problematic before; namely,\n> >\n> > git sparse-checkout [add|set| --[no-]cone --sikp-checks ...\n> >\n> > would not give an error on the mis-typed \"--skip-checks\" but instead\n> > simply treat \"--sikp-checks\" as a path or pattern to include in the\n> > sparse-checkout, which is highly unfriendly.\n> >\n> > This behavior begain when the command was converted to parse-options in\n> > 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,\n> > 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was\n> > renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:\n> > PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify\n> > that it was only about dashed options; we always keep non-option\n> > arguments. Looking at that original patch, both Peff and I think that\n> > the author was simply confused about the mis-named option, and really\n> > just wanted to keep the non-option arguments. We never should have used\n> > the flag all along (and the other cases were cargo-culted within the\n> > file).\n> >\n> > Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\n> > bug. Note that this does mean that anyone who might have been using\n> >\n> > git sparse-checkout [add|set] [--[no-]cone] --foo --bar\n> >\n> > to request paths or patterns '--foo' and '--bar' will now have to use\n> >\n> > git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar\n> >\n> > That makes sparse-checkout more consistent with other git commands,\n> > provides users much friendlier error messages and behavior, and is\n> > consistent with the all-capps warning in git-sparse-checkout.txt that\n> > this command \"is experimental...its behavior...will likely change\". :-)\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n>\n> Nicely described.\n>\n> Apparently we do not have an existing test to cover the case of\n> \"misspelt options becoming a pattern\" that this bugfix corrects;\n> otherwise we would have a s/failure/success/ to update such an older\n> expectation, and have to wonder if the existing behaviour is\n> intentional. Since there is no such test, we can feel a bit safer\n> in \"fixing\" this odd behaviour.\n>\n> Also, this corrects \"--end-of-options\" that becomes one of the\n> patterns. Such a bug was not detected in our test suite.\n>\n> So we should at least have two new tests.\n>\n> (1) Pass \"--foo\" without the end of options marker and make sure we\n> error out, instead of making it as one of the patterns.\n>\n> (2) Pass \"--end-of-options foo\" and make sure the command succeeds,\n> and \"--end-of-options\" does not become one of the patterns.\n>\n> Thanks.\n\nI did actually create two such tests last Saturday, but they obviously\nsomehow went missing from my submission (I guess even if the high\nfevers from Wed-Fri last week were gone, I was still more affected\nthan I realized?) Anyway, I'll resubmit with those test cases.\n"},{"id":"486036","messageId":"pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com","threadId":"60651","inReplyTo":"pull.1625.git.git.1703379611749.gitgitgadget@gmail.com","subject":"[PATCH v2] sparse-checkout: be consistent with end of options markers","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-26T19:39:22Z","receivedAt":"2023-12-26T19:39:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n93851746 (parse-options: decouple \"--end-of-options\" and \"--\",\n2023-12-06) updated the world order to make callers of parse-options\nthat set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to\ndo with \"--end-of-options\" they may see after parse_options() returns.\n\nThis made a previous bug in sparse-checkout more visible; namely,\nthat\n\n git sparse-checkout [add|set] --[no-]cone --end-of-options ...\n\nwould simply treat \"--end-of-options\" as one of the paths to include in\nthe sparse-checkout. But this was already problematic before; namely,\n\n git sparse-checkout [add|set| --[no-]cone --sikp-checks ...\n\nwould not give an error on the mis-typed \"--skip-checks\" but instead\nsimply treat \"--sikp-checks\" as a path or pattern to include in the\nsparse-checkout, which is highly unfriendly.\n\nThis behavior began when the command was converted to parse-options in\n7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,\n2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was\nrenamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:\nPARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify\nthat it was only about dashed options; we always keep non-option\narguments. Looking at that original patch, both Peff and I think that\nthe author was simply confused about the mis-named option, and really\njust wanted to keep the non-option arguments. We never should have used\nthe flag all along (and the other cases were cargo-culted within the\nfile).\n\nRemove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\nbug. Note that this does mean that anyone who might have been using\n\n git sparse-checkout [add|set] [--[no-]cone] --foo --bar\n\nto request paths or patterns '--foo' and '--bar' will now have to use\n\n git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar\n\nThat makes sparse-checkout more consistent with other git commands,\nprovides users much friendlier error messages and behavior, and is\nconsistent with the all-caps warning in git-sparse-checkout.txt that\nthis command \"is experimental...its behavior...will likely change\". :-)\n\nSigned-off-by: Elijah Newren \n---\n [RFC] sparse-checkout: be consistent with end of options markers\n \n Follow-up to thread over at\n https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/,\n making end of options markers in git-sparse-checkout consistent with how\n other git commands behave.\n \n Changes since v1:\n \n * Added some testcases\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1625%2Fgitgitgadget%2Fsparse-checkout-end-of-options-consistency-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1625/gitgitgadget/sparse-checkout-end-of-options-consistency-v2\nPull-Request: https://github.com/git/git/pull/1625\n\nRange-diff vs v1:\n\n 1: c19156919a6 ! 1: 448146637a9 sparse-checkout: be consistent with end of options markers\n @@ Commit message\n simply treat \"--sikp-checks\" as a path or pattern to include in the\n sparse-checkout, which is highly unfriendly.\n \n - This behavior begain when the command was converted to parse-options in\n + This behavior began when the command was converted to parse-options in\n 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,\n 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was\n renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:\n @@ Commit message\n \n That makes sparse-checkout more consistent with other git commands,\n provides users much friendlier error messages and behavior, and is\n - consistent with the all-capps warning in git-sparse-checkout.txt that\n + consistent with the all-caps warning in git-sparse-checkout.txt that\n this command \"is experimental...its behavior...will likely change\". :-)\n \n Signed-off-by: Elijah Newren \n @@ builtin/sparse-checkout.c: static int sparse_checkout_check_rules(int argc, cons\n \n \tif (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)\n \t\tcheck_rules_opts.cone_mode = 1;\n +\n + ## t/t1091-sparse-checkout-builtin.sh ##\n +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'cone mode: set with nested folders' '\n + \n + test_expect_success 'cone mode: add independent path' '\n + \tgit -C repo sparse-checkout set deep/deeper1 &&\n +-\tgit -C repo sparse-checkout add folder1 &&\n ++\tgit -C repo sparse-checkout add --end-of-options folder1 &&\n + \tcat >expect <<-\\EOF &&\n + \t/*\n + \t!/*/\n +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'by default, cone mode will error out when passed files' '\n + \tgrep \".gitignore.*is not a directory\" error\n + '\n + \n ++test_expect_success 'error on mistyped command line options' '\n ++\ttest_must_fail git -C repo sparse-checkout add --sikp-checks .gitignore 2>error &&\n ++\n ++\tgrep \"unknown option.*sikp-checks\" error\n ++'\n ++\n + test_expect_success 'by default, non-cone mode will warn on individual files' '\n + \tgit -C repo sparse-checkout reapply --no-cone &&\n + \tgit -C repo sparse-checkout add .gitignore 2>warning &&\n\n\n builtin/sparse-checkout.c | 9 +++------\n t/t1091-sparse-checkout-builtin.sh | 8 +++++++-\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 5c8ffb1f759..0e68e9b0b0d 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -777,8 +777,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_sparse_checkout_add_options,\n-\t\t\t builtin_sparse_checkout_add_usage,\n-\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\t\t\t builtin_sparse_checkout_add_usage, 0);\n \n \tsanitize_paths(argc, argv, prefix, add_opts.skip_checks);\n \n@@ -824,8 +823,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_sparse_checkout_set_options,\n-\t\t\t builtin_sparse_checkout_set_usage,\n-\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\t\t\t builtin_sparse_checkout_set_usage, 0);\n \n \tif (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))\n \t\treturn 1;\n@@ -996,8 +994,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_sparse_checkout_check_rules_options,\n-\t\t\t builtin_sparse_checkout_check_rules_usage,\n-\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\t\t\t builtin_sparse_checkout_check_rules_usage, 0);\n \n \tif (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)\n \t\tcheck_rules_opts.cone_mode = 1;\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex f67611da28e..e49b8024ac5 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -334,7 +334,7 @@ test_expect_success 'cone mode: set with nested folders' '\n \n test_expect_success 'cone mode: add independent path' '\n \tgit -C repo sparse-checkout set deep/deeper1 &&\n-\tgit -C repo sparse-checkout add folder1 &&\n+\tgit -C repo sparse-checkout add --end-of-options folder1 &&\n \tcat >expect <<-\\EOF &&\n \t/*\n \t!/*/\n@@ -886,6 +886,12 @@ test_expect_success 'by default, cone mode will error out when passed files' '\n \tgrep \".gitignore.*is not a directory\" error\n '\n \n+test_expect_success 'error on mistyped command line options' '\n+\ttest_must_fail git -C repo sparse-checkout add --sikp-checks .gitignore 2>error &&\n+\n+\tgrep \"unknown option.*sikp-checks\" error\n+'\n+\n test_expect_success 'by default, non-cone mode will warn on individual files' '\n \tgit -C repo sparse-checkout reapply --no-cone &&\n \tgit -C repo sparse-checkout add .gitignore 2>warning &&\n\nbase-commit: 055bb6e9969085777b7fab83e3fee0017654f134\n-- \ngitgitgadget\n"},{"id":"486038","messageId":"xmqqil4khgdz.fsf@gitster.g","threadId":"60651","inReplyTo":"pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] sparse-checkout: be consistent with end of options markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T20:12:56Z","receivedAt":"2023-12-26T20:13:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\n> bug. Note that this does mean that anyone who might have been using\n>\n> git sparse-checkout [add|set] [--[no-]cone] --foo --bar\n>\n> to request paths or patterns '--foo' and '--bar' will now have to use\n>\n> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar\n>\n> That makes sparse-checkout more consistent with other git commands,\n> provides users much friendlier error messages and behavior, and is\n> consistent with the all-caps warning in git-sparse-checkout.txt that\n> this command \"is experimental...its behavior...will likely change\". :-)\n>\n> Signed-off-by: Elijah Newren \n> ---\n\nLet me drop jc/sparse-checkout-eoo topic and queue this instead, and\nthen resurrect the \"use default for 'set' only when !stdin\" as a\nseparate topic.\n\nThanks.\n"},{"id":"486117","messageId":"20231228114325.GA627960@coredump.intra.peff.net","threadId":"60651","inReplyTo":"xmqqfrzoj31l.fsf@gitster.g","subject":"Re: [PATCH] sparse-checkout: be consistent with end of options markers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-28T11:43:25Z","receivedAt":"2023-12-28T11:43:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 26, 2023 at 09:18:14AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:\n> >\n> >> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this\n> >> bug. Note that this does mean that anyone who might have been using\n> >> [...]\n> >\n> > Thanks for wrapping this up in patch form. It looks good to me,\n> > including the reasoning. You didn't add any tests, but I find it rather\n> > unlikely that we'd later regress here.\n> \n> Surely. I am certainly OK with just dropping KEEP_UNKNOWN but I\n> would strongly prefer to document what we \"fixed\" (your \"misspelt\n> option name\" example) and what (mis|ab)use the people may have been\n> relying on we have \"broken\" (the same \"misspelt\" behaviour that can\n> be intentional is now forbidden, and we document that this change in\n> behaviour is intentional) with a new test.\n\nYeah, thank you for talking some sense into me. I do not foresee us\nregressing \"--sikp-checks\", but certainly covering --end-of-options in\nmore places is worthwhile. As it's handled centrally, it can have\nunexpected consequences for various commands.\n\nElijah's latest version looks good to me.\n\n-Peff\n"}]}