[PATCH 0/3] sparse-checkout with --end-of-options

[PATCH 0/3] sparse-checkout with --end-of-options

[Junio C Hamano]

I only wanted to make sure 

    git sparse-checkout set --end-of-options path

work fine, but it turns out that there were other sloppiness that
has nothing to do with the end-of-options handling that needs to be
adjusted to the new world order, so I ended up addressing that as
well.

Junio C Hamano (3):
  sparse-checkout: take care of "--end-of-options" in set/add
  sparse-checkout: allow default patterns for 'set' only !stdin
  sparse-checkout: tighten add_patterns_from_input()

 builtin/sparse-checkout.c          |  9 ++++++++-
 parse-options.c                    |  8 ++++++++
 parse-options.h                    |  2 ++
 t/t1090-sparse-checkout-scope.sh   |  8 ++++++++
 t/t1091-sparse-checkout-builtin.sh | 13 +++++++++++--
 5 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.43.0-174-g055bb6e996

[PATCH 1/3] sparse-checkout: take care of "--end-of-options" in set/add/check-rules

[Junio C Hamano]

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 unfortunately broke "sparse-checkout set/add", and from this
invocation,

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

we now see "--end-of-options" listed in .git/info/sparse-checkout as if
it is one of the path patterns.

A breakage that results from the same cause exists in the check-rules
subcommand, but check-rules has a few other problems that need to be
corrected before it can fully work with --end-of-options safely,
which will be addressed later.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/sparse-checkout.c          | 3 +++
 parse-options.c                    | 8 ++++++++
 parse-options.h                    | 2 ++
 t/t1090-sparse-checkout-scope.sh   | 8 ++++++++
 t/t1091-sparse-checkout-builtin.sh | 2 +-
 5 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 5c8ffb1f75..8f55127202 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -779,6 +779,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_options,
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+	parse_opt_skip_end_of_options(&argc, &argv);
 
 	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
@@ -826,6 +827,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_set_options,
 			     builtin_sparse_checkout_set_usage,
 			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+	parse_opt_skip_end_of_options(&argc, &argv);
 
 	if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
 		return 1;
@@ -998,6 +1000,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
 			     builtin_sparse_checkout_check_rules_options,
 			     builtin_sparse_checkout_check_rules_usage,
 			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+	parse_opt_skip_end_of_options(&argc, &argv);
 
 	if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
 		check_rules_opts.cone_mode = 1;
diff --git a/parse-options.c b/parse-options.c
index d50962062e..fe265bbf68 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1321,3 +1321,11 @@ void die_for_incompatible_opt4(int opt1, const char *opt1_name,
 		break;
 	}
 }
+
+void parse_opt_skip_end_of_options(int *argc, const char ***argv)
+{
+	if (*argc && !strcmp(**argv, "--end-of-options")) {
+		(*argc)--;
+		(*argv)++;
+	}
+}
diff --git a/parse-options.h b/parse-options.h
index bd62e20268..0d3354d4a8 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -498,6 +498,8 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);
 /* value is enum branch_track* */
 int parse_opt_tracking_mode(const struct option *, const char *, int);
 
+void parse_opt_skip_end_of_options(int *argc, const char ***argv);
+
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
 #define OPT__VERBOSITY(var) { \
diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh
index 3a14218b24..5b96716235 100755
--- a/t/t1090-sparse-checkout-scope.sh
+++ b/t/t1090-sparse-checkout-scope.sh
@@ -57,6 +57,14 @@ test_expect_success 'return to full checkout of main' '
 test_expect_success 'skip-worktree on files outside sparse patterns' '
 	git sparse-checkout disable &&
 	git sparse-checkout set --no-cone "a*" &&
+	cat .git/info/sparse-checkout >wo-eoo &&
+
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone --end-of-options "a*" &&
+	cat .git/info/sparse-checkout >w-eoo &&
+
+	test_cmp wo-eoo w-eoo &&
+
 	git checkout-index --all --ignore-skip-worktree-bits &&
 
 	git ls-files -t >output &&
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index f67611da28..e33a6ed1b4 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 &&
 	/*
 	!/*/
-- 
2.43.0-174-g055bb6e996

[PATCH 2/3] sparse-checkout: use default patterns for 'set' only !stdin

[Junio C Hamano]

"git sparse-checkout set ---no-cone" uses default patterns when none
is given from the command line, but it should do so ONLY when
--stdin is not being used.  Right now, add_patterns_from_input()
called when reading from the standard input is sloppy and does not
check if there are extra command line parameters that the command
will silently ignore, but that will change soon and not setting this
unnecessary and unused default patterns start to matter when it gets
fixed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This came from f2e3a218 (sparse-checkout: enable `set` to
   initialize sparse-checkout mode, 2021-12-14) by Elijah.

 builtin/sparse-checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8f55127202..04ae81fce8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -837,7 +837,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 	 * non-cone mode, if nothing is specified, manually select just the
 	 * top-level directory (much as 'init' would do).
 	 */
-	if (!core_sparse_checkout_cone && argc == 0) {
+	if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
 		argv = default_patterns;
 		argc = default_patterns_nr;
 	} else {
-- 
2.43.0-174-g055bb6e996

[PATCH 3/3] sparse-checkout: tighten add_patterns_from_input()

[Junio C Hamano]

The add_patterns_from_input() function was introduced at 6fb705ab
(sparse-checkout: extract add_patterns_from_input(), 2020-02-11) and
then modified by 00408ade (builtin/sparse-checkout: add check-rules
command, 2023-03-27).  Throughout its life, it either allowed to
read patterns from the file (before 00408ade, it only allowed the
standard input, after 00408ade, an arbitrary FILE *) or from argv[],
but never both.  However, when we read from a file, the function
never checked that there is nothing in argv[] (in other words, the
command line arguments have fully been consumed), resulting in
excess arguments silently getting ignored.

This caused commands like "git sparse-checkout set [--stdin]" and
"git sparse-checkout check-rules [--rules-file <file>]" to silently
ignore the rest of the command line arguments without reporting.

The fix finally makes the --end-of-options handling for this
subcommand also work, so add test for it, too.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/sparse-checkout.c          |  4 ++++
 t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 04ae81fce8..1166e1e298 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -555,6 +555,10 @@ static void add_patterns_from_input(struct pattern_list *pl,
 				    FILE *file)
 {
 	int i;
+
+	if (file && argc)
+		die(_("excess command line parameter '%s'"), argv[0]);
+
 	if (core_sparse_checkout_cone) {
 		struct strbuf line = STRBUF_INIT;
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index e33a6ed1b4..107ed170ac 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -937,6 +937,10 @@ test_expect_success 'check-rules cone mode' '
 	EOF
 
 	git -C bare ls-tree -r --name-only HEAD >all-files &&
+
+	test_must_fail git -C bare sparse-checkout check-rules --cone \
+		--rules-file ../rules excess args <all-files &&
+
 	git -C bare sparse-checkout check-rules --cone \
 		--rules-file ../rules >check-rules-file <all-files &&
 
@@ -947,6 +951,7 @@ test_expect_success 'check-rules cone mode' '
 	git -C repo sparse-checkout check-rules >check-rules-default <all-files &&
 
 	test_grep "deep/deeper1/deepest/a" check-rules-file &&
+	test_grep ! "end-of-options" check-rules-file &&
 	test_grep ! "deep/deeper2" check-rules-file &&
 
 	test_cmp check-rules-file ls-files &&
@@ -959,8 +964,12 @@ test_expect_success 'check-rules non-cone mode' '
 	EOF
 
 	git -C bare ls-tree -r --name-only HEAD >all-files &&
+
+	test_must_fail git -C bare sparse-checkout check-rules --no-cone \
+		--rules-file ../rules excess args <all-files &&
+
 	git -C bare sparse-checkout check-rules --no-cone --rules-file ../rules\
-		>check-rules-file <all-files &&
+		--end-of-options >check-rules-file <all-files &&
 
 	cat rules | git -C repo sparse-checkout set --no-cone --stdin &&
 	git -C repo ls-files -t >out &&
-- 
2.43.0-174-g055bb6e996

Re: [PATCH 2/3] sparse-checkout: use default patterns for 'set' only !stdin

[Elijah Newren]

On Wed, Dec 20, 2023 at 10:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "git sparse-checkout set ---no-cone" uses default patterns when none
> is given from the command line, but it should do so ONLY when
> --stdin is not being used.  Right now, add_patterns_from_input()
> called when reading from the standard input is sloppy and does not
> check if there are extra command line parameters that the command
> will silently ignore, but that will change soon and not setting this
> unnecessary and unused default patterns start to matter when it gets
> fixed.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * This came from f2e3a218 (sparse-checkout: enable `set` to
>    initialize sparse-checkout mode, 2021-12-14) by Elijah.
>
>  builtin/sparse-checkout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 8f55127202..04ae81fce8 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -837,7 +837,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>          * non-cone mode, if nothing is specified, manually select just the
>          * top-level directory (much as 'init' would do).
>          */
> -       if (!core_sparse_checkout_cone && argc == 0) {
> +       if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
>                 argv = default_patterns;
>                 argc = default_patterns_nr;
>         } else {
> --
> 2.43.0-174-g055bb6e996

Thanks for catching this; the fix looks good to me.

Re: [PATCH 1/3] sparse-checkout: take care of "--end-of-options" in set/add/check-rules

[Elijah Newren]

On Wed, Dec 20, 2023 at 11:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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 unfortunately broke "sparse-checkout set/add", and from this
> invocation,
>
>   "git sparse-checkout [add|set] --[no-]cone --end-of-options pattern..."
>
> we now see "--end-of-options" listed in .git/info/sparse-checkout as if
> it is one of the path patterns.
>
> A breakage that results from the same cause exists in the check-rules
> subcommand, but check-rules has a few other problems that need to be
> corrected before it can fully work with --end-of-options safely,
> which will be addressed later.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/sparse-checkout.c          | 3 +++
>  parse-options.c                    | 8 ++++++++
>  parse-options.h                    | 2 ++
>  t/t1090-sparse-checkout-scope.sh   | 8 ++++++++
>  t/t1091-sparse-checkout-builtin.sh | 2 +-
>  5 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 5c8ffb1f75..8f55127202 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -779,6 +779,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
>                              builtin_sparse_checkout_add_options,
>                              builtin_sparse_checkout_add_usage,
>                              PARSE_OPT_KEEP_UNKNOWN_OPT);
> +       parse_opt_skip_end_of_options(&argc, &argv);
>
>         sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
>
> @@ -826,6 +827,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>                              builtin_sparse_checkout_set_options,
>                              builtin_sparse_checkout_set_usage,
>                              PARSE_OPT_KEEP_UNKNOWN_OPT);
> +       parse_opt_skip_end_of_options(&argc, &argv);
>
>         if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
>                 return 1;
> @@ -998,6 +1000,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
>                              builtin_sparse_checkout_check_rules_options,
>                              builtin_sparse_checkout_check_rules_usage,
>                              PARSE_OPT_KEEP_UNKNOWN_OPT);
> +       parse_opt_skip_end_of_options(&argc, &argv);
>
>         if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
>                 check_rules_opts.cone_mode = 1;
> diff --git a/parse-options.c b/parse-options.c
> index d50962062e..fe265bbf68 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1321,3 +1321,11 @@ void die_for_incompatible_opt4(int opt1, const char *opt1_name,
>                 break;
>         }
>  }
> +
> +void parse_opt_skip_end_of_options(int *argc, const char ***argv)
> +{
> +       if (*argc && !strcmp(**argv, "--end-of-options")) {
> +               (*argc)--;
> +               (*argv)++;
> +       }
> +}
> diff --git a/parse-options.h b/parse-options.h
> index bd62e20268..0d3354d4a8 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -498,6 +498,8 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);
>  /* value is enum branch_track* */
>  int parse_opt_tracking_mode(const struct option *, const char *, int);
>
> +void parse_opt_skip_end_of_options(int *argc, const char ***argv);
> +
>  #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
>  #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
>  #define OPT__VERBOSITY(var) { \
> diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh
> index 3a14218b24..5b96716235 100755
> --- a/t/t1090-sparse-checkout-scope.sh
> +++ b/t/t1090-sparse-checkout-scope.sh
> @@ -57,6 +57,14 @@ test_expect_success 'return to full checkout of main' '
>  test_expect_success 'skip-worktree on files outside sparse patterns' '
>         git sparse-checkout disable &&
>         git sparse-checkout set --no-cone "a*" &&
> +       cat .git/info/sparse-checkout >wo-eoo &&
> +
> +       git sparse-checkout disable &&
> +       git sparse-checkout set --no-cone --end-of-options "a*" &&
> +       cat .git/info/sparse-checkout >w-eoo &&
> +
> +       test_cmp wo-eoo w-eoo &&
> +
>         git checkout-index --all --ignore-skip-worktree-bits &&
>
>         git ls-files -t >output &&
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index f67611da28..e33a6ed1b4 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 &&
>         /*
>         !/*/
> --
> 2.43.0-174-g055bb6e996

I've got a counter-proposal for this patch at
https://lore.kernel.org/git/pull.1625.git.git.1703379611749.gitgitgadget@gmail.com/,
based on further thread discussion over at
https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/.

Re: [PATCH 2/3] sparse-checkout: use default patterns for 'set' only !stdin

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 8f55127202..04ae81fce8 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -837,7 +837,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>>          * non-cone mode, if nothing is specified, manually select just the
>>          * top-level directory (much as 'init' would do).
>>          */
>> -       if (!core_sparse_checkout_cone && argc == 0) {
>> +       if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
>>                 argv = default_patterns;
>>                 argc = default_patterns_nr;
>>         } else {
>> --
>> 2.43.0-174-g055bb6e996
>
> Thanks for catching this; the fix looks good to me.

Actually I am not so sure.

An obvious alternative would be to collect the patterns, either from
the command line or from the standard input, and then use the
default when we collected nothing.

But I guess those who use the standard input should be able to
specify everything fully, so it would probably be OK.