{"thread":{"id":"60641","subject":"[PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","startedAt":"2023-12-20T23:19:29Z","lastAt":"2023-12-24T01:03:04Z","messageCount":19,"participants":["Junio C Hamano","Josh Steadmon","René Scharfe","Jeff King","rsbecker@nexbridge.com","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485899","messageId":"xmqqbkakqx6s.fsf@gitster.g","threadId":"60641","inReplyTo":null,"subject":"[PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-20T23:19:23Z","receivedAt":"2023-12-20T23:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"93851746 (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 unfortunately broke \"sparse-checkout set/add\", and from this\ninvocation,\n\n \"git sparse-checkout [add|set] --[no-]cone --end-of-options pattern...\"\n\nwe now see \"--end-of-options\" listed in .git/info/sparse-checkout as if\nit is one of the path patterns.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/sparse-checkout.c | 9 +++++++++\n t/t1090-sparse-checkout-scope.sh | 8 ++++++++\n t/t1091-sparse-checkout-builtin.sh | 2 +-\n 3 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c\nindex 80227f3df1..226a458b10 100644\n--- c/builtin/sparse-checkout.c\n+++ w/builtin/sparse-checkout.c\n@@ -776,6 +776,10 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_sparse_checkout_add_usage,\n \t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n \n+\tif (argc && !strcmp(*argv, \"--end-of-options\")) {\n+\t\targc--;\n+\t\targv++;\n+\t}\n \tsanitize_paths(argc, argv, prefix, add_opts.skip_checks);\n \n \treturn modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);\n@@ -823,6 +827,11 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_sparse_checkout_set_usage,\n \t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n \n+\tif (argc && !strcmp(*argv, \"--end-of-options\")) {\n+\t\targc--;\n+\t\targv++;\n+\t}\n+\n \tif (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))\n \t\treturn 1;\n \ndiff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh\nindex 3a14218b24..5b96716235 100755\n--- c/t/t1090-sparse-checkout-scope.sh\n+++ w/t/t1090-sparse-checkout-scope.sh\n@@ -57,6 +57,14 @@ test_expect_success 'return to full checkout of main' '\n test_expect_success 'skip-worktree on files outside sparse patterns' '\n \tgit sparse-checkout disable &&\n \tgit sparse-checkout set --no-cone \"a*\" &&\n+\tcat .git/info/sparse-checkout >wo-eoo &&\n+\n+\tgit sparse-checkout disable &&\n+\tgit sparse-checkout set --no-cone --end-of-options \"a*\" &&\n+\tcat .git/info/sparse-checkout >w-eoo &&\n+\n+\ttest_cmp wo-eoo w-eoo &&\n+\n \tgit checkout-index --all --ignore-skip-worktree-bits &&\n \n \tgit ls-files -t >output &&\ndiff --git c/t/t1091-sparse-checkout-builtin.sh w/t/t1091-sparse-checkout-builtin.sh\nindex f67611da28..e33a6ed1b4 100755\n--- c/t/t1091-sparse-checkout-builtin.sh\n+++ w/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"},{"id":"485900","messageId":"ZYN-5H-2NNoRRpf-@google.com","threadId":"60641","inReplyTo":"xmqqbkakqx6s.fsf@gitster.g","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-12-20T23:55:16Z","receivedAt":"2023-12-20T23:55:22Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.12.20 15:19, Junio C Hamano wrote:\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 unfortunately broke \"sparse-checkout set/add\", and from this\n> invocation,\n> \n> \"git sparse-checkout [add|set] --[no-]cone --end-of-options pattern...\"\n> \n> we now see \"--end-of-options\" listed in .git/info/sparse-checkout as if\n> it is one of the path patterns.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/sparse-checkout.c | 9 +++++++++\n> t/t1090-sparse-checkout-scope.sh | 8 ++++++++\n> t/t1091-sparse-checkout-builtin.sh | 2 +-\n> 3 files changed, 18 insertions(+), 1 deletion(-)\n> \n> diff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c\n> index 80227f3df1..226a458b10 100644\n> --- c/builtin/sparse-checkout.c\n> +++ w/builtin/sparse-checkout.c\n> @@ -776,6 +776,10 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)\n> \t\t\t builtin_sparse_checkout_add_usage,\n> \t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n> \n> +\tif (argc && !strcmp(*argv, \"--end-of-options\")) {\n> +\t\targc--;\n> +\t\targv++;\n> +\t}\n> \tsanitize_paths(argc, argv, prefix, add_opts.skip_checks);\n> \n> \treturn modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);\n> @@ -823,6 +827,11 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)\n> \t\t\t builtin_sparse_checkout_set_usage,\n> \t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n> \n> +\tif (argc && !strcmp(*argv, \"--end-of-options\")) {\n> +\t\targc--;\n> +\t\targv++;\n> +\t}\n> +\n> \tif (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))\n> \t\treturn 1;\n> \n> diff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh\n> index 3a14218b24..5b96716235 100755\n> --- c/t/t1090-sparse-checkout-scope.sh\n> +++ w/t/t1090-sparse-checkout-scope.sh\n> @@ -57,6 +57,14 @@ test_expect_success 'return to full checkout of main' '\n> test_expect_success 'skip-worktree on files outside sparse patterns' '\n> \tgit sparse-checkout disable &&\n> \tgit sparse-checkout set --no-cone \"a*\" &&\n> +\tcat .git/info/sparse-checkout >wo-eoo &&\n> +\n> +\tgit sparse-checkout disable &&\n> +\tgit sparse-checkout set --no-cone --end-of-options \"a*\" &&\n> +\tcat .git/info/sparse-checkout >w-eoo &&\n> +\n> +\ttest_cmp wo-eoo w-eoo &&\n> +\n> \tgit checkout-index --all --ignore-skip-worktree-bits &&\n> \n> \tgit ls-files -t >output &&\n> diff --git c/t/t1091-sparse-checkout-builtin.sh w/t/t1091-sparse-checkout-builtin.sh\n> index f67611da28..e33a6ed1b4 100755\n> --- c/t/t1091-sparse-checkout-builtin.sh\n> +++ w/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\nI can confirm that this fixes an issue noticed by sparse-checkout users\nat $DAYJOB. Looks good to me. Thanks!\n\nReviewed-by: Josh Steadmon \n"},{"id":"485901","messageId":"xmqqttocp98r.fsf@gitster.g","threadId":"60641","inReplyTo":"xmqqbkakqx6s.fsf@gitster.g","subject":"[RFC/PATCH] archive: \"--list\" does not take further options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-21T02:41:56Z","receivedAt":"2023-12-21T02:42:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git archive --list blah\" should notice an extra command line\nparameter that goes unused. Make it so.\n\nSigned-off-by: Junio C Hamano \n---\n * This was done to convince myself that even though cmd_archive()\n calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then\n uses the resulting argc/argv without apparent filtering of the\n \"--end-of-options\" thing, it is correctly handling it, either\n locally or remotely.\n\n - locally, write_archive() uses parse_archive_args(), which calls\n parse_options() without KEEP_UNKNOWN_OPT and \"--end-of-options\"\n is handled there just fine.\n\n - remotely, run_remote_archiver() relays the local parameters,\n including \"--end-of-options\" via the \"argument\" packet. Then\n the arguments are assembled into a strvec and used by the\n upload-archive running on the other side to spawn an\n upload-archive--writer process with.\n cmd_upload_archive_writer() then makes the same write_archive()\n call; \"--end-of-options\" would still be in argv[] if the\n original \"git archive --remote\" invocation had one, but it is\n consumed the same way as the local case in write_archive() by\n calling parse_archive_args().\n\n I do not like the remote error behaviour this one adds at all.\n Do we use a more proper mechanism to propagate a remote error\n back for other subcommands we can reuse here?\n\n archive.c | 7 +++++++\n t/t5000-tar-tree.sh | 14 ++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git c/archive.c w/archive.c\nindex 9aeaf2bd87..3244e9f9f2 100644\n--- c/archive.c\n+++ w/archive.c\n@@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,\n \t\tbase = \"\";\n \n \tif (list) {\n+\t\tif (argc) {\n+\t\t\tif (!is_remote)\n+\t\t\t\tdie(_(\"extra command line parameter '%s'\"), *argv);\n+\t\t\telse\n+\t\t\t\tprintf(\"!ERROR! extra command line parameter '%s'\\n\",\n+\t\t\t\t *argv);\n+\t\t}\n \t\tfor (i = 0; i < nr_archivers; i++)\n \t\t\tif (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)\n \t\t\t\tprintf(\"%s\\n\", archivers[i]->name);\ndiff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh\nindex 918a2fc7c6..04592f45b0 100755\n--- c/t/t5000-tar-tree.sh\n+++ w/t/t5000-tar-tree.sh\n@@ -124,6 +124,20 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n+test_expect_success '--list notices extra parameters' '\n+\ttest_must_fail git archive --list blah &&\n+\t# NEEDSWORK: remote error does not result in non-zero\n+\t# exit, which we might want to change later.\n+\tgit archive --remote=. --list blah >remote-out &&\n+\tgrep \"!ERROR! \" remote-out\n+'\n+\n+test_expect_success 'end-of-options is correctly eaten' '\n+\tgit archive --list --end-of-options &&\n+\tgit archive --remote=. --list --end-of-options >remote-out &&\n+\t! grep \"!ERROR! \" remote-out\n+'\n+\n test_expect_success 'populate workdir' '\n \tmkdir a &&\n \techo simple textfile >a/a &&\n"},{"id":"485902","messageId":"xmqqplz0p90k.fsf@gitster.g","threadId":"60641","inReplyTo":"ZYN-5H-2NNoRRpf-@google.com","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-21T02:46:51Z","receivedAt":"2023-12-21T02:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> I can confirm that this fixes an issue noticed by sparse-checkout users\n> at $DAYJOB. Looks good to me. Thanks!\n\nHeh, there is another one that is not converted in the same file for\n\"check-rules\" subcommand, so the posted patch is way incomplete, I\nthink.\n\n\n"},{"id":"485907","messageId":"296e8d69-c1d7-4ad2-943a-dfc54940abc2@web.de","threadId":"60641","inReplyTo":"xmqqttocp98r.fsf@gitster.g","subject":"Re: [RFC/PATCH] archive: \"--list\" does not take further options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-21T07:30:36Z","receivedAt":"2023-12-21T07:30:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.12.23 um 03:41 schrieb Junio C Hamano:\n> \"git archive --list blah\" should notice an extra command line\n> parameter that goes unused. Make it so.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> * This was done to convince myself that even though cmd_archive()\n> calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then\n> uses the resulting argc/argv without apparent filtering of the\n> \"--end-of-options\" thing, it is correctly handling it, either\n> locally or remotely.\n>\n> - locally, write_archive() uses parse_archive_args(), which calls\n> parse_options() without KEEP_UNKNOWN_OPT and \"--end-of-options\"\n> is handled there just fine.\n>\n> - remotely, run_remote_archiver() relays the local parameters,\n> including \"--end-of-options\" via the \"argument\" packet. Then\n> the arguments are assembled into a strvec and used by the\n> upload-archive running on the other side to spawn an\n> upload-archive--writer process with.\n> cmd_upload_archive_writer() then makes the same write_archive()\n> call; \"--end-of-options\" would still be in argv[] if the\n> original \"git archive --remote\" invocation had one, but it is\n> consumed the same way as the local case in write_archive() by\n> calling parse_archive_args().\n>\n> I do not like the remote error behaviour this one adds at all.\n> Do we use a more proper mechanism to propagate a remote error\n> back for other subcommands we can reuse here?\n\nDon't we have one? It would affect other unsupported options as well,\nand this seems to work just fine, e.g.:\n\n $ git archive --remote=. --format=foo HEAD\n remote: fatal: Unknown archive format 'foo'\n remote: git upload-archive: archiver died with error\n fatal: sent error to the client: git upload-archive: archiver died with error\n\n>\n> archive.c | 7 +++++++\n> t/t5000-tar-tree.sh | 14 ++++++++++++++\n> 2 files changed, 21 insertions(+)\n>\n> diff --git c/archive.c w/archive.c\n> index 9aeaf2bd87..3244e9f9f2 100644\n> --- c/archive.c\n> +++ w/archive.c\n> @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,\n> \t\tbase = \"\";\n>\n> \tif (list) {\n> +\t\tif (argc) {\n> +\t\t\tif (!is_remote)\n> +\t\t\t\tdie(_(\"extra command line parameter '%s'\"), *argv);\n> +\t\t\telse\n> +\t\t\t\tprintf(\"!ERROR! extra command line parameter '%s'\\n\",\n> +\t\t\t\t *argv);\n> +\t\t}\n\nSo just call die() here?\n\n> \t\tfor (i = 0; i < nr_archivers; i++)\n> \t\t\tif (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)\n> \t\t\t\tprintf(\"%s\\n\", archivers[i]->name);\n> diff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh\n> index 918a2fc7c6..04592f45b0 100755\n> --- c/t/t5000-tar-tree.sh\n> +++ w/t/t5000-tar-tree.sh\n> @@ -124,6 +124,20 @@ test_expect_success 'setup' '\n> \tEOF\n> '\n>\n> +test_expect_success '--list notices extra parameters' '\n> +\ttest_must_fail git archive --list blah &&\n> +\t# NEEDSWORK: remote error does not result in non-zero\n> +\t# exit, which we might want to change later.\n> +\tgit archive --remote=. --list blah >remote-out &&\n> +\tgrep \"!ERROR! \" remote-out\n\n... and use test_must_fail in both cases?\n\n> +'\n> +\n> +test_expect_success 'end-of-options is correctly eaten' '\n> +\tgit archive --list --end-of-options &&\n> +\tgit archive --remote=. --list --end-of-options >remote-out &&\n> +\t! grep \"!ERROR! \" remote-out\n> +'\n> +\n> test_expect_success 'populate workdir' '\n> \tmkdir a &&\n> \techo simple textfile >a/a &&\n"},{"id":"485908","messageId":"20231221083643.GA545870@coredump.intra.peff.net","threadId":"60641","inReplyTo":"xmqqbkakqx6s.fsf@gitster.g","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T08:36:43Z","receivedAt":"2023-12-21T08:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 20, 2023 at 03:19:23PM -0800, Junio C Hamano wrote:\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 unfortunately broke \"sparse-checkout set/add\", and from this\n> invocation,\n> \n> \"git sparse-checkout [add|set] --[no-]cone --end-of-options pattern...\"\n> \n> we now see \"--end-of-options\" listed in .git/info/sparse-checkout as if\n> it is one of the path patterns.\n\nThanks for finding this. Though I only half-agree with the\n\"unfortunately\" part. ;)\n\nAs argued in 93851746, any caller passing KEEP_UNKNOWN_OPT but _not_\nhandling the now-returned end-of-options is rather suspicious. Because\nif they are planning to parse those options further, they will have no\nidea that we saw --end-of-options, and will err in the unsafe direction\nof still treating any elements with dashes as options.\n\nWhich would mean that simply dropping --end-of-options from the list, as\nyour patch does, would be similarly unsafe. It is papering over the\nproblem of:\n\n git sparse-checkout --end-of-options foo\n\nbut leaving:\n\n git sparse-checkout --end-of-options --foo\n\nbroken.\n\nBut the plot thickens! Curiously, in both of these cases, we do not do\nany further parsing of the options at all. We just treat them as\npattern arguments with no special meaning.\n\nSo your patch is actually OK, but I would argue that the correct fix\nhere is to drop the use of PARSE_OPT_KEEP_UNKNOWN_OPT at all. I cannot\nfind any indication in the history on why it was ever used. It was added\nwhen the command was converted to parse-options in 7bffca95ea\n(sparse-checkout: add '--stdin' option to set subcommand, 2019-11-21).\nBack then it was just called KEEP_UNKNOWN. Later it was renamed to\nKEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options: PARSE_OPT_KEEP_UNKNOWN\nonly applies to --options, 2022-08-19) to clarify that it was only about\ndashed options; we always keep non-option arguments.\n\nSo looking at that original patch, it makes me think that the author was\nconfused about the mis-named option, and really just wanted to keep the\nnon-option arguments. We never should have used the flag all along (and\nthe other cases were cargo-culted within the file).\n\nThere is one minor gotcha, though. Unlike most other Git commands,\nsparse-checkout will happily accept random option names and treat them\nas non-option arguments. E.g. this works:\n\n git sparse-checkout add --foo --bar\n\nand will add \"--foo\" and \"--bar\" as patterns. If we remove the flag,\nwe'd be breaking that. But I'd argue that anybody relying on that is\nasking for trouble. For example, this does not work in the same way:\n\n git sparse-checkout add --skip-checks --foo\n\nbecause \"--skip-checks\" is a real option. Ditto for any other options,\nincluding those we add in the future. If you don't trust the contents of\nyour arguments, you should be using \"--\" or \"--end-of-options\" to\ncommunicate the intent to the command.\n\n-Peff\n"},{"id":"485909","messageId":"20231221084011.GB545870@coredump.intra.peff.net","threadId":"60641","inReplyTo":"xmqqplz0p90k.fsf@gitster.g","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T08:40:11Z","receivedAt":"2023-12-21T08:40:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 20, 2023 at 06:46:51PM -0800, Junio C Hamano wrote:\n\n> Josh Steadmon writes:\n> \n> > I can confirm that this fixes an issue noticed by sparse-checkout users\n> > at $DAYJOB. Looks good to me. Thanks!\n> \n> Heh, there is another one that is not converted in the same file for\n> \"check-rules\" subcommand, so the posted patch is way incomplete, I\n> think.\n\nYeah. I think it is in the same boat as the other two, in that I believe\nthat the KEEP_UNKNOWN_OPT flag is counter-productive and should just be\ndropped.\n\n-Peff\n"},{"id":"485910","messageId":"20231221085808.GC545870@coredump.intra.peff.net","threadId":"60641","inReplyTo":"xmqqttocp98r.fsf@gitster.g","subject":"Re: [RFC/PATCH] archive: \"--list\" does not take further options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T08:58:08Z","receivedAt":"2023-12-21T08:58:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 20, 2023 at 06:41:56PM -0800, Junio C Hamano wrote:\n\n> ---\n> * This was done to convince myself that even though cmd_archive()\n> calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then\n> uses the resulting argc/argv without apparent filtering of the\n> \"--end-of-options\" thing, it is correctly handling it, either\n> locally or remotely.\n> \n> - locally, write_archive() uses parse_archive_args(), which calls\n> parse_options() without KEEP_UNKNOWN_OPT and \"--end-of-options\"\n> is handled there just fine.\n\nRight. Not only is it handled by the second parser, but _not_ keeping it\nwould be a bug, because that second parser would have no idea that we\nsaw end-of-options (and so \"git archive --end-of-options --foo\" would\ntry to treat \"--foo\" as an option).\n\nThe recent commit 9385174627 did fix a case like this for fast-export,\nbut git-archive was not changed. It passed KEEP_DASHDASH along with\nKEEP_UNKNOWN_OPT, so it already retained and passed along\n--end-of-options.\n\n> - remotely, run_remote_archiver() relays the local parameters,\n> including \"--end-of-options\" via the \"argument\" packet. Then\n> the arguments are assembled into a strvec and used by the\n> upload-archive running on the other side to spawn an\n> upload-archive--writer process with.\n> cmd_upload_archive_writer() then makes the same write_archive()\n> call; \"--end-of-options\" would still be in argv[] if the\n> original \"git archive --remote\" invocation had one, but it is\n> consumed the same way as the local case in write_archive() by\n> calling parse_archive_args().\n\nRight, and this is just the same case but with a lot of complicated\nnetwork-ferrying in between. :) We must retain --end-of-options so that\nthe next parser knows to stop treating them as arguments. And because it\ndoesn't use KEEP_UNKNOWN_OPT, the ferried \"--end-of-options\" is removed\nthen.\n\n> diff --git c/archive.c w/archive.c\n> index 9aeaf2bd87..3244e9f9f2 100644\n> --- c/archive.c\n> +++ w/archive.c\n> @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,\n> \t\tbase = \"\";\n> \n> \tif (list) {\n> +\t\tif (argc) {\n> +\t\t\tif (!is_remote)\n> +\t\t\t\tdie(_(\"extra command line parameter '%s'\"), *argv);\n> +\t\t\telse\n> +\t\t\t\tprintf(\"!ERROR! extra command line parameter '%s'\\n\",\n> +\t\t\t\t *argv);\n> +\t\t}\n\nThis general direction seems reasonable to me, since we're letting the\nuser know that their extra argument was ignored (though note that if it\nwas a misspelled option, like \"--otuput\", we would complain about that).\nIt's largely orthogonal to end-of-options, but I see how you got here by\nwondering about it. :)\n\n-Peff\n"},{"id":"485911","messageId":"20231221085948.GD545870@coredump.intra.peff.net","threadId":"60641","inReplyTo":"296e8d69-c1d7-4ad2-943a-dfc54940abc2@web.de","subject":"Re: [RFC/PATCH] archive: \"--list\" does not take further options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T08:59:48Z","receivedAt":"2023-12-21T08:59:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 21, 2023 at 08:30:36AM +0100, René Scharfe wrote:\n\n> > I do not like the remote error behaviour this one adds at all.\n> > Do we use a more proper mechanism to propagate a remote error\n> > back for other subcommands we can reuse here?\n> \n> Don't we have one? It would affect other unsupported options as well,\n> and this seems to work just fine, e.g.:\n> \n> $ git archive --remote=. --format=foo HEAD\n> remote: fatal: Unknown archive format 'foo'\n> remote: git upload-archive: archiver died with error\n> fatal: sent error to the client: git upload-archive: archiver died with error\n\nRight. The whole idea of upload-archive is to spawn a separate writer\nprocess and mux the conversation (including errors) back over the wire.\nThere are a zillion reasons it can die (including bad arguments) and we\ncatch and report them in the muxing process.\n\n> > \tif (list) {\n> > +\t\tif (argc) {\n> > +\t\t\tif (!is_remote)\n> > +\t\t\t\tdie(_(\"extra command line parameter '%s'\"), *argv);\n> > +\t\t\telse\n> > +\t\t\t\tprintf(\"!ERROR! extra command line parameter '%s'\\n\",\n> > +\t\t\t\t *argv);\n> > +\t\t}\n> \n> So just call die() here?\n\nYes, exactly.\n\n-Peff\n"},{"id":"485943","messageId":"xmqqsf3vmqug.fsf@gitster.g","threadId":"60641","inReplyTo":"20231221084011.GB545870@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-21T17:02:15Z","receivedAt":"2023-12-21T17:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Dec 20, 2023 at 06:46:51PM -0800, Junio C Hamano wrote:\n>\n>> Josh Steadmon writes:\n>> \n>> > I can confirm that this fixes an issue noticed by sparse-checkout users\n>> > at $DAYJOB. Looks good to me. Thanks!\n>> \n>> Heh, there is another one that is not converted in the same file for\n>> \"check-rules\" subcommand, so the posted patch is way incomplete, I\n>> think.\n>\n> Yeah. I think it is in the same boat as the other two, in that I believe\n> that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be\n> dropped.\n\nIf we dropped KEEP_UNKNOWN_OPT, however, the pattern that is\ncurrently accepted will stop working, e.g.,\n\n $ git sparse-checkout [add/set] --[no-]cone --foo bar\n\nas we would barf with \"--foo: unknown option\", and the users who are\nused to this sloppy command line parsing we had for the past few\nyears must now write \"--end-of-options\" before \"--foo\". After all,\nthe reason why the original authors of this code used KEEP_UNKNOWN\nis likely to deal with path patterns that begin with dashes.\n\nThe patch in the message that started this thread may not be\ncorrect, either, I am afraid. For either of these:\n\n $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar\n $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar\n\nwe would see that \"foo\" (or \"--foo\") is not \"--end-of-options\", and\nwe end up using three patterns \"foo\" (or \"--foo\"),\n\"--end-of-options\", and \"bar\", I suspect. I wonder if we should\nnotice the \"foo\" or \"--foo\" that were not understood and error out,\ninstead.\n\nBut after all, it is not absolutely necessary to notice and barf.\nThe ONLY practical use of the \"--end-of-options\" mechanism is to\nallow us to write (this applies to any git subcommand):\n\n #!/bin/sh\n git cmd --hard --coded --options --end-of-options \"$@\"\n\nin scripts to protect the intended operation from mistaking the\nend-user input as options. And with a script written carefully to\ndo so, all the args that appear before \"--end-of-options\" would be\nrecognizable by the command line parser. On the other hand, such a\n\"notice and barf\" would not help a script that is written without\n\"--end-of-options\", when it is fed \"$@\" that happens to begin with\n\"--end-of-options\". We would silently swallow \"--end-of-options\"\nwithout any chance to notice the lack of \"--end-of-options\" in the\nscript. So I dunno.\n\nHere is an additional test on top of [1/3]\n<20231221065925.3234048-2-gitster@pobox.com>\n\nthat demonstrates and summarizes the idea.\n\n t/t1090-sparse-checkout-scope.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh\nindex 5b96716235..da9534d222 100755\n--- c/t/t1090-sparse-checkout-scope.sh\n+++ w/t/t1090-sparse-checkout-scope.sh\n@@ -54,6 +54,40 @@ test_expect_success 'return to full checkout of main' '\n \ttest \"$(cat b)\" = \"modified\"\n '\n \n+test_expect_success 'sparse-checkout set command line parsing' '\n+\t# baseline\n+\tgit sparse-checkout disable &&\n+\tgit sparse-checkout set --no-cone \"a*\" &&\n+\techo \"a*\" >expect &&\n+\ttest_cmp .git/info/sparse-checkout expect &&\n+\n+\t# unknown string that looks like a dashed option is\n+\t# taken as a mere pattern\n+\tgit sparse-checkout disable &&\n+\tgit sparse-checkout set --no-cone --foo \"a*\" &&\n+\ttest_write_lines \"--foo\" \"a*\" >expect &&\n+\ttest_cmp .git/info/sparse-checkout expect &&\n+\n+\t# --end-of-options can be used to protect parameters that\n+\t# potentially begin with dashes\n+\tset x --cone \"a*\" && shift &&\n+\tgit sparse-checkout disable &&\n+\tgit sparse-checkout set --no-cone --end-of-options \"$@\" &&\n+\ttest_write_lines \"$@\" >expect &&\n+\ttest_cmp .git/info/sparse-checkout expect &&\n+\n+\t# but writing --end-of-options after what the command does not\n+\t# recognize is too late; it becomes one of the patterns, so\n+\t# that the end-user input that happens to be \"--end-of-options\"\n+\t# can be passed through. To be absoutely sure, you should write\n+\t# --end-of-options yourself before taking \"$@\" in.\n+\tset x --foo --end-of-options \"a*\" && shift &&\n+\tgit sparse-checkout disable &&\n+\tgit sparse-checkout set --no-cone \"$@\" &&\n+\ttest_write_lines \"$@\" >expect &&\n+\ttest_cmp .git/info/sparse-checkout expect\n+'\n+\n test_expect_success 'skip-worktree on files outside sparse patterns' '\n \tgit sparse-checkout disable &&\n \tgit sparse-checkout set --no-cone \"a*\" &&\n"},{"id":"485948","messageId":"xmqqmsu3mnix.fsf@gitster.g","threadId":"60641","inReplyTo":"20231221085948.GD545870@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] archive: \"--list\" does not take further options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-21T18:13:58Z","receivedAt":"2023-12-21T18:14:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 21, 2023 at 08:30:36AM +0100, René Scharfe wrote:\n>> ...\n>> Don't we have one? It would affect other unsupported options as well,\n>> and this seems to work just fine, e.g.:\n>> ...\n> Right. The whole idea of upload-archive is to spawn a separate writer\n> process and mux the conversation (including errors) back over the wire.\n\nThanks, both. Just to tie the loose end, let me queue this and\nmerge it to 'next'.\n\n----- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] archive: \"--list\" does not take further options\n\n\"git archive --list blah\" should notice an extra command line\nparameter that goes unused. Make it so.\n\nSigned-off-by: Junio C Hamano \n---\n archive.c | 2 ++\n t/t5000-tar-tree.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/archive.c b/archive.c\nindex ca11db185b..8da820d1ce 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -685,6 +685,8 @@ static int parse_archive_args(int argc, const char **argv,\n \t\tbase = \"\";\n \n \tif (list) {\n+\t\tif (argc)\n+\t\t\tdie(_(\"extra command line parameter '%s'\"), *argv);\n \t\tfor (i = 0; i < nr_archivers; i++)\n \t\t\tif (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)\n \t\t\t\tprintf(\"%s\\n\", archivers[i]->name);\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 4b4c3315d8..72b8d0ff02 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -124,6 +124,16 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n+test_expect_success '--list notices extra parameters' '\n+\ttest_must_fail git archive --list blah &&\n+\ttest_must_fail git archive --remote=. --list blah\n+'\n+\n+test_expect_success 'end-of-options is correctly eaten' '\n+\tgit archive --list --end-of-options &&\n+\tgit archive --remote=. --list --end-of-options\n+'\n+\n test_expect_success 'populate workdir' '\n \tmkdir a &&\n \techo simple textfile >a/a &&\n-- \n2.43.0-174-g055bb6e996\n\n"},{"id":"485949","messageId":"xmqqfrzvmn7p.fsf@gitster.g","threadId":"60641","inReplyTo":"20231221083643.GA545870@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-21T18:20:42Z","receivedAt":"2023-12-21T18:20:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Which would mean that simply dropping --end-of-options from the list, as\n> your patch does, would be similarly unsafe. It is papering over the\n> problem of:\n>\n> git sparse-checkout --end-of-options foo\n>\n> but leaving:\n>\n> git sparse-checkout --end-of-options --foo\n>\n> broken.\n\nHmph, I do not understand. The latter case we want to take \"--foo\"\nas the sole parameter to the subcommand, no?\n\n> But the plot thickens! Curiously, in both of these cases, we do not do\n> any further parsing of the options at all. We just treat them as\n> pattern arguments with no special meaning.\n\nExactly.\n\n> So your patch is actually OK, but I would argue that the correct fix\n> here is to drop the use of PARSE_OPT_KEEP_UNKNOWN_OPT at all. I cannot\n> find any indication in the history on why it was ever used. It was added\n> when the command was converted to parse-options in 7bffca95ea\n> (sparse-checkout: add '--stdin' option to set subcommand, 2019-11-21).\n> Back then it was just called KEEP_UNKNOWN. Later it was renamed to\n> KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options: PARSE_OPT_KEEP_UNKNOWN\n> only applies to --options, 2022-08-19) to clarify that it was only about\n> dashed options; we always keep non-option arguments.\n\nYes.\n\n> So looking at that original patch, it makes me think that the author was\n> confused about the mis-named option, and really just wanted to keep the\n> non-option arguments. We never should have used the flag all along (and\n> the other cases were cargo-culted within the file).\n\nThat is something only the original author can answer, by my working\ntheory has been they wanted to have a cheap way to allow any string,\neven the ones that happen to begin with a dash, to be passed as one\nof the patterns.\n\n> There is one minor gotcha, though. Unlike most other Git commands,\n> sparse-checkout will happily accept random option names and treat them\n> as non-option arguments. E.g. this works:\n>\n> git sparse-checkout add --foo --bar\n>\n> and will add \"--foo\" and \"--bar\" as patterns. If we remove the flag,\n> we'd be breaking that.\n\nExactly. The user _should_ protect these \"patterns\" by using\n\"--end-of-options\" before \"--foo\" above, but we have been taking the\npatterns with such a loose parser for quite some time, so tightening\nwould be taken as a regression X-<.\n\n> But I'd argue that anybody relying on that is\n> asking for trouble. For example, this does not work in the same way:\n>\n> git sparse-checkout add --skip-checks --foo\n>\n> because \"--skip-checks\" is a real option. Ditto for any other options,\n> including those we add in the future. If you don't trust the contents of\n> your arguments, you should be using \"--\" or \"--end-of-options\" to\n> communicate the intent to the command.\n\nExactly. My response to the other message, with a new test,\nsummarizes my position.\n\nThanks.\n"},{"id":"485960","messageId":"20231221213543.GC1446091@coredump.intra.peff.net","threadId":"60641","inReplyTo":"xmqqmsu3mnix.fsf@gitster.g","subject":"Re: [RFC/PATCH] archive: \"--list\" does not take further options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T21:35:43Z","receivedAt":"2023-12-21T21:35:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 21, 2023 at 10:13:58AM -0800, Junio C Hamano wrote:\n\n> Thanks, both. Just to tie the loose end, let me queue this and\n> merge it to 'next'.\n> \n> ----- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] archive: \"--list\" does not take further options\n\nThis looks fine to me. It does mean that this sequence of commands no\nlonger works:\n\n [let's try to make a \"foo\" archive]\n $ git archive --format=foo HEAD\n fatal: Unknown archive format 'foo'\n\n [oops, that didn't work. What formats are supported?]\n $ !! --list\n git archive --format=foo HEAD --list\n tar\n tgz\n tar.gz\n zip\n\nI think that's OK in practice. The increased error checking is worth it\n(and matches many other commands, which tend to warn about confusing\nextra bits).\n\n-Peff\n"},{"id":"485961","messageId":"20231221214550.GD1446091@coredump.intra.peff.net","threadId":"60641","inReplyTo":"xmqqsf3vmqug.fsf@gitster.g","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T21:45:50Z","receivedAt":"2023-12-21T21:45:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 21, 2023 at 09:02:15AM -0800, Junio C Hamano wrote:\n\n> > Yeah. I think it is in the same boat as the other two, in that I believe\n> > that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be\n> > dropped.\n> \n> If we dropped KEEP_UNKNOWN_OPT, however, the pattern that is\n> currently accepted will stop working, e.g.,\n> \n> $ git sparse-checkout [add/set] --[no-]cone --foo bar\n> \n> as we would barf with \"--foo: unknown option\", and the users who are\n> used to this sloppy command line parsing we had for the past few\n> years must now write \"--end-of-options\" before \"--foo\". After all,\n> the reason why the original authors of this code used KEEP_UNKNOWN\n> is likely to deal with path patterns that begin with dashes.\n\nRight, that is the \"gotcha\" I mentioned in my other email. Though that\nis the way it has behaved historically, my argument is that users are\nunreasonable to expect it to work:\n\n 1. It is not consistent with the rest of Git commands.\n\n 2. It is inconsistent with respect to existing options (and is an\n accident waiting to happen when new options are added).\n\nSo I'd consider it a bug-fix.\n\n> The patch in the message that started this thread may not be\n> correct, either, I am afraid. For either of these:\n> \n> $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar\n> $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar\n> \n> we would see that \"foo\" (or \"--foo\") is not \"--end-of-options\", and\n> we end up using three patterns \"foo\" (or \"--foo\"),\n> \"--end-of-options\", and \"bar\", I suspect. I wonder if we should\n> notice the \"foo\" or \"--foo\" that were not understood and error out,\n> instead.\n\nYes, I agree that \"--foo --end-of-options\" should barf. And of course\nthat happens naturally if you just let parse-options do its job by not\npassing the KEEP_UNKNOWN_OPT flag. ;)\n\nI'm not sure about \"foo\". We do allow out-of-order options/args, so\nisn't it correct to keep it as a non-option argument?\n\n> But after all, it is not absolutely necessary to notice and barf.\n> The ONLY practical use of the \"--end-of-options\" mechanism is to\n> allow us to write (this applies to any git subcommand):\n> \n> #!/bin/sh\n> git cmd --hard --coded --options --end-of-options \"$@\"\n> \n> in scripts to protect the intended operation from mistaking the\n> end-user input as options. And with a script written carefully to\n> do so, all the args that appear before \"--end-of-options\" would be\n> recognizable by the command line parser.\n\nYes, but if you misspell \"--otpions\", it magically becomes a parameter\nrather than having the command barf and complain. That is not the end of\nthe world, but it is unfriendly and inconsistent with the rest of Git.\n\n-Peff\n"},{"id":"485966","messageId":"xmqqle9njjp3.fsf@gitster.g","threadId":"60641","inReplyTo":"20231221214550.GD1446091@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-21T22:04:56Z","receivedAt":"2023-12-21T22:05:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Right, that is the \"gotcha\" I mentioned in my other email. Though that\n> is the way it has behaved historically, my argument is that users are\n> unreasonable to expect it to work:\n>\n> 1. It is not consistent with the rest of Git commands.\n>\n> 2. It is inconsistent with respect to existing options (and is an\n> accident waiting to happen when new options are added).\n>\n> So I'd consider it a bug-fix.\n\nSo the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and\ndeliberately break them^W^W^Wrealign their expectations?\n\nI do not have much stake in sparse-checkout, so I am fine with that\ndirection. But I suspect other folks on the list would have users\nof their own who would rather loudly complain to them if we broke\nthem ;-) \n\nI'll see how bad the fallout would be if we go that route, using the\ntest modification I made for the previous rounds as yardsticks.\n\nThanks.\n"},{"id":"485986","messageId":"20231223100229.GA2016274@coredump.intra.peff.net","threadId":"60641","inReplyTo":"xmqqle9njjp3.fsf@gitster.g","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-23T10:02:29Z","receivedAt":"2023-12-23T10:02:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Right, that is the \"gotcha\" I mentioned in my other email. Though that\n> > is the way it has behaved historically, my argument is that users are\n> > unreasonable to expect it to work:\n> >\n> > 1. It is not consistent with the rest of Git commands.\n> >\n> > 2. It is inconsistent with respect to existing options (and is an\n> > accident waiting to happen when new options are added).\n> >\n> > So I'd consider it a bug-fix.\n> \n> So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and\n> deliberately break them^W^W^Wrealign their expectations?\n\nYes. :) But keep in mind we are un-breaking other people, like those who\ntypo:\n\n git sparse-checkout --sikp-checks\n\nand don't see an error (instead, we make a garbage entry in the sparse\ncheckout file).\n\n> I do not have much stake in sparse-checkout, so I am fine with that\n> direction. But I suspect other folks on the list would have users\n> of their own who would rather loudly complain to them if we broke\n> them ;-) \n\nLikewise, I have never used sparse-checkout myself, and I don't care\n_that_ much. My interest is mostly in having various Git commands behave\nconsistently. This whole discussion started because the centralized\n--end-of-options fix interacted badly with this unusual behavior.\n\n-Peff\n"},{"id":"485989","messageId":"08b801da35b6$22c6e0f0$6854a2d0$@nexbridge.com","threadId":"60641","inReplyTo":"20231223100229.GA2016274@coredump.intra.peff.net","subject":"RE: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-12-23T15:38:48Z","receivedAt":"2023-12-23T15:39:40Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Saturday, December 23, 2023 5:02 AM, Peff wrote:\n>On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:\n>> Jeff King writes:\n>> > Right, that is the \"gotcha\" I mentioned in my other email. Though\n>> > that is the way it has behaved historically, my argument is that\n>> > users are unreasonable to expect it to work:\n>> >\n>> > 1. It is not consistent with the rest of Git commands.\n>> >\n>> > 2. It is inconsistent with respect to existing options (and is an\n>> > accident waiting to happen when new options are added).\n>> >\n>> > So I'd consider it a bug-fix.\n>>\n>> So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and\n>> deliberately break them^W^W^Wrealign their expectations?\n>\n>Yes. :) But keep in mind we are un-breaking other people, like those who\n>typo:\n>\n> git sparse-checkout --sikp-checks\n\nI don't see why \n\ngit sparse-checkout -- --sikp-checks\n\nwould be the only way to get that typo into a garbage entry, to be consistent with other commands. Without the --, --sikp-checks should result in an error.\n\n>and don't see an error (instead, we make a garbage entry in the sparse checkout\n>file).\n>\n>> I do not have much stake in sparse-checkout, so I am fine with that\n>> direction. But I suspect other folks on the list would have users of\n>> their own who would rather loudly complain to them if we broke them\n>> ;-)\n>\n>Likewise, I have never used sparse-checkout myself, and I don't care _that_ much.\n>My interest is mostly in having various Git commands behave consistently. This\n>whole discussion started because the centralized --end-of-options fix interacted\n>badly with this unusual behavior.\n\nI use sparse-checkout every day and do depend on it working predictably (although I would take a fix to see it work as above, with the -- path separator), so personally, I care about this a whole lot -- in terms of consistency.\n\n"},{"id":"486005","messageId":"CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com","threadId":"60641","inReplyTo":"20231223100229.GA2016274@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-12-23T22:45:53Z","receivedAt":"2023-12-23T22:46:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sat, Dec 23, 2023 at 2:02 AM Jeff King wrote:\n>\n> On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > Right, that is the \"gotcha\" I mentioned in my other email. Though that\n> > > is the way it has behaved historically, my argument is that users are\n> > > unreasonable to expect it to work:\n> > >\n> > > 1. It is not consistent with the rest of Git commands.\n> > >\n> > > 2. It is inconsistent with respect to existing options (and is an\n> > > accident waiting to happen when new options are added).\n> > >\n> > > So I'd consider it a bug-fix.\n> >\n> > So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and\n> > deliberately break them^W^W^Wrealign their expectations?\n>\n> Yes. :) But keep in mind we are un-breaking other people, like those who\n> typo:\n>\n> git sparse-checkout --sikp-checks\n>\n> and don't see an error (instead, we make a garbage entry in the sparse\n> checkout file).\n\nI think you mean\n git sparse-checkout set --sikp-checks\nor\n git sparse-checkout add --sikp-checks\n(i.e. with either 'set' or 'add' in there)\n\nNeither of these are currently an error, but I agree both definitely\nought to be. (Without the 'set' or 'add', you currently do get an\nerror, as we'd expect.)\n\n> > I do not have much stake in sparse-checkout, so I am fine with that\n> > direction. But I suspect other folks on the list would have users\n> > of their own who would rather loudly complain to them if we broke\n> > them ;-)\n>\n> Likewise, I have never used sparse-checkout myself, and I don't care\n> _that_ much. My interest is mostly in having various Git commands behave\n> consistently. This whole discussion started because the centralized\n> --end-of-options fix interacted badly with this unusual behavior.\n\nWell, I care quite a bit about sparse-checkout, and I would rather see\nPeff's proposed fix, even if some users might have to tweak their\ncommands a little.\n\nOf course, I'm not the only sparse-checkout user representative, but\nRandall commented elsewhere in this thread that he used\nsparse-checkout quite a bit, and he feels that \"without the --,\n--sikp-checks should result in an error.\" In other words, he is\nbasically agreeing that taking Peff's fix seems more important than\nstrict backward compatibility here. (His wording may not sound like\nthat at first glance, probably because of the 'set'/'add' omission in\nthe example command, but I think what he said actually amounts to\nagreeing with this change.)\n\nFinally, git-sparse-checkout(1) does say \"THIS COMMAND IS\nEXPERIMENTAL. ITS BEHAVIOR...WILL LIKELY CHANGE\". I apologize for\nbeing pulled from my intent to work on [*], which I think would allow\nus to finally drop this warning (I'll still get back to it, one way or\nanother...), but I think this is a good case where we should take\nadvantage of the existing warning and simply fix the command.\n\nAnyway, just my $0.02,\nElijah\n\n[*] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/\n"},{"id":"486007","messageId":"CABPp-BG640jNdAdJic0wqmYXwz1HyzcBB6-W9Ef70AHqtPSECA@mail.gmail.com","threadId":"60641","inReplyTo":"CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com","subject":"Re: [PATCH/RFC] sparse-checkout: take care of \"--end-of-options\" in set/add","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-12-24T01:02:49Z","receivedAt":"2023-12-24T01:03:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Dec 23, 2023 at 2:45 PM Elijah Newren wrote:\n>\n> Hi,\n>\n> On Sat, Dec 23, 2023 at 2:02 AM Jeff King wrote:\n> >\n> > On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:\n> >\n> > > Jeff King writes:\n> > >\n> > > > Right, that is the \"gotcha\" I mentioned in my other email. Though that\n> > > > is the way it has behaved historically, my argument is that users are\n> > > > unreasonable to expect it to work:\n> > > >\n> > > > 1. It is not consistent with the rest of Git commands.\n> > > >\n> > > > 2. It is inconsistent with respect to existing options (and is an\n> > > > accident waiting to happen when new options are added).\n> > > >\n> > > > So I'd consider it a bug-fix.\n> > >\n> > > So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and\n> > > deliberately break them^W^W^Wrealign their expectations?\n> >\n> > Yes. :) But keep in mind we are un-breaking other people, like those who\n> > typo:\n> >\n> > git sparse-checkout --sikp-checks\n> >\n> > and don't see an error (instead, we make a garbage entry in the sparse\n> > checkout file).\n>\n> I think you mean\n> git sparse-checkout set --sikp-checks\n> or\n> git sparse-checkout add --sikp-checks\n> (i.e. with either 'set' or 'add' in there)\n>\n> Neither of these are currently an error, but I agree both definitely\n> ought to be. (Without the 'set' or 'add', you currently do get an\n> error, as we'd expect.)\n>\n> > > I do not have much stake in sparse-checkout, so I am fine with that\n> > > direction. But I suspect other folks on the list would have users\n> > > of their own who would rather loudly complain to them if we broke\n> > > them ;-)\n> >\n> > Likewise, I have never used sparse-checkout myself, and I don't care\n> > _that_ much. My interest is mostly in having various Git commands behave\n> > consistently. This whole discussion started because the centralized\n> > --end-of-options fix interacted badly with this unusual behavior.\n>\n> Well, I care quite a bit about sparse-checkout, and I would rather see\n> Peff's proposed fix, even if some users might have to tweak their\n> commands a little.\n\nAnd I wrote it up as a patch over at\nhttps://lore.kernel.org/git/pull.1625.git.git.1703379611749.gitgitgadget@gmail.com/\n"}]}