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

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

[Elijah Newren via GitGitGadget]

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>
---
    [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(-)

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;

base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
-- 
gitgitgadget

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

"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.

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

[Elijah Newren]

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.

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

[Elijah Newren via GitGitGadget]

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 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".  :-)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    [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".  :-)
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
     @@ 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(-)

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;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index f67611da28e..e49b8024ac5 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -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

Re: [PATCH v2] sparse-checkout: be consistent with end of options markers

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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".  :-)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

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.

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

[Jeff King]

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