| Message ID | pull.1625.git.git.1703379611749.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sparse-checkout: be consistent with end of options markers | expand |
On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote: > 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 > [...] 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
Jeff King <peff@peff.net> writes: > On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote: > >> 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 >> [...] > > 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. 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.
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > 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". :-) > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- 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, On Tue, Dec 26, 2023 at 9:26 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > 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". :-) > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > 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, > and "--end-of-options" does not become one of the patterns. > > Thanks. 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.
On Tue, Dec 26, 2023 at 09:18:14AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote: > > > >> 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 > >> [...] > > > > 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. > > 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. 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
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 5c8ffb1f759..0e68e9b0b0d 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -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;