[PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

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

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

diff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c
index 80227f3df1..226a458b10 100644
--- c/builtin/sparse-checkout.c
+++ w/builtin/sparse-checkout.c
@@ -776,6 +776,10 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN_OPT);
 
+	if (argc && !strcmp(*argv, "--end-of-options")) {
+		argc--;
+		argv++;
+	}
 	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
@@ -823,6 +827,11 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_set_usage,
 			     PARSE_OPT_KEEP_UNKNOWN_OPT);
 
+	if (argc && !strcmp(*argv, "--end-of-options")) {
+		argc--;
+		argv++;
+	}
+
 	if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
 		return 1;
 
diff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh
index 3a14218b24..5b96716235 100755
--- c/t/t1090-sparse-checkout-scope.sh
+++ w/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 c/t/t1091-sparse-checkout-builtin.sh w/t/t1091-sparse-checkout-builtin.sh
index f67611da28..e33a6ed1b4 100755
--- c/t/t1091-sparse-checkout-builtin.sh
+++ w/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 &&
 	/*
 	!/*/

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Josh Steadmon]

On 2023.12.20 15:19, Junio C Hamano 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.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/sparse-checkout.c          | 9 +++++++++
>  t/t1090-sparse-checkout-scope.sh   | 8 ++++++++
>  t/t1091-sparse-checkout-builtin.sh | 2 +-
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c
> index 80227f3df1..226a458b10 100644
> --- c/builtin/sparse-checkout.c
> +++ w/builtin/sparse-checkout.c
> @@ -776,6 +776,10 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
>  			     builtin_sparse_checkout_add_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN_OPT);
>  
> +	if (argc && !strcmp(*argv, "--end-of-options")) {
> +		argc--;
> +		argv++;
> +	}
>  	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
>  
>  	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
> @@ -823,6 +827,11 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>  			     builtin_sparse_checkout_set_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN_OPT);
>  
> +	if (argc && !strcmp(*argv, "--end-of-options")) {
> +		argc--;
> +		argv++;
> +	}
> +
>  	if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
>  		return 1;
>  
> diff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh
> index 3a14218b24..5b96716235 100755
> --- c/t/t1090-sparse-checkout-scope.sh
> +++ w/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 c/t/t1091-sparse-checkout-builtin.sh w/t/t1091-sparse-checkout-builtin.sh
> index f67611da28..e33a6ed1b4 100755
> --- c/t/t1091-sparse-checkout-builtin.sh
> +++ w/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 &&
>  	/*
>  	!/*/

I can confirm that this fixes an issue noticed by sparse-checkout users
at $DAYJOB. Looks good to me. Thanks!

Reviewed-by: Josh Steadmon <steadmon@google.com>

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> I can confirm that this fixes an issue noticed by sparse-checkout users
> at $DAYJOB. Looks good to me. Thanks!

Heh, there is another one that is not converted in the same file for
"check-rules" subcommand, so the posted patch is way incomplete, I
think.

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Jeff King]

On Wed, Dec 20, 2023 at 06:46:51PM -0800, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
> 
> > I can confirm that this fixes an issue noticed by sparse-checkout users
> > at $DAYJOB. Looks good to me. Thanks!
> 
> Heh, there is another one that is not converted in the same file for
> "check-rules" subcommand, so the posted patch is way incomplete, I
> think.

Yeah. I think it is in the same boat as the other two, in that I believe
that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be
dropped.

-Peff

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Dec 20, 2023 at 06:46:51PM -0800, Junio C Hamano wrote:
>
>> Josh Steadmon <steadmon@google.com> writes:
>> 
>> > I can confirm that this fixes an issue noticed by sparse-checkout users
>> > at $DAYJOB. Looks good to me. Thanks!
>> 
>> Heh, there is another one that is not converted in the same file for
>> "check-rules" subcommand, so the posted patch is way incomplete, I
>> think.
>
> Yeah. I think it is in the same boat as the other two, in that I believe
> that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be
> dropped.

If we dropped KEEP_UNKNOWN_OPT, however, the pattern that is
currently accepted will stop working, e.g.,

 $ git sparse-checkout [add/set] --[no-]cone --foo bar

as we would barf with "--foo: unknown option", and the users who are
used to this sloppy command line parsing we had for the past few
years must now write "--end-of-options" before "--foo".  After all,
the reason why the original authors of this code used KEEP_UNKNOWN
is likely to deal with path patterns that begin with dashes.

The patch in the message that started this thread may not be
correct, either, I am afraid.  For either of these:

 $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar
 $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar

we would see that "foo" (or "--foo") is not "--end-of-options", and
we end up using three patterns "foo" (or "--foo"),
"--end-of-options", and "bar", I suspect.  I wonder if we should
notice the "foo" or "--foo" that were not understood and error out,
instead.

But after all, it is not absolutely necessary to notice and barf.
The ONLY practical use of the "--end-of-options" mechanism is to
allow us to write (this applies to any git subcommand):

 #!/bin/sh
 git cmd --hard --coded --options --end-of-options "$@"

in scripts to protect the intended operation from mistaking the
end-user input as options.  And with a script written carefully to
do so, all the args that appear before "--end-of-options" would be
recognizable by the command line parser.  On the other hand, such a
"notice and barf" would not help a script that is written without
"--end-of-options", when it is fed "$@" that happens to begin with
"--end-of-options".  We would silently swallow "--end-of-options"
without any chance to notice the lack of "--end-of-options" in the
script.  So I dunno.

Here is an additional test on top of [1/3]
<20231221065925.3234048-2-gitster@pobox.com>

that demonstrates and summarizes the idea.

 t/t1090-sparse-checkout-scope.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git c/t/t1090-sparse-checkout-scope.sh w/t/t1090-sparse-checkout-scope.sh
index 5b96716235..da9534d222 100755
--- c/t/t1090-sparse-checkout-scope.sh
+++ w/t/t1090-sparse-checkout-scope.sh
@@ -54,6 +54,40 @@ test_expect_success 'return to full checkout of main' '
 	test "$(cat b)" = "modified"
 '
 
+test_expect_success 'sparse-checkout set command line parsing' '
+	# baseline
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone "a*" &&
+	echo "a*" >expect &&
+	test_cmp .git/info/sparse-checkout expect &&
+
+	# unknown string that looks like a dashed option is
+	# taken as a mere pattern
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone --foo "a*" &&
+	test_write_lines "--foo" "a*" >expect &&
+	test_cmp .git/info/sparse-checkout expect &&
+
+	# --end-of-options can be used to protect parameters that
+	# potentially begin with dashes
+	set x --cone "a*" && shift &&
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone --end-of-options "$@" &&
+	test_write_lines "$@" >expect &&
+	test_cmp .git/info/sparse-checkout expect &&
+
+	# but writing --end-of-options after what the command does not
+	# recognize is too late; it becomes one of the patterns, so
+	# that the end-user input that happens to be "--end-of-options"
+	# can be passed through.  To be absoutely sure, you should write
+	# --end-of-options yourself before taking "$@" in.
+	set x --foo --end-of-options "a*" && shift &&
+	git sparse-checkout disable &&
+	git sparse-checkout set --no-cone "$@" &&
+	test_write_lines "$@" >expect &&
+	test_cmp .git/info/sparse-checkout expect
+'
+
 test_expect_success 'skip-worktree on files outside sparse patterns' '
 	git sparse-checkout disable &&
 	git sparse-checkout set --no-cone "a*" &&

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Jeff King]

On Thu, Dec 21, 2023 at 09:02:15AM -0800, Junio C Hamano wrote:

> > Yeah. I think it is in the same boat as the other two, in that I believe
> > that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be
> > dropped.
> 
> If we dropped KEEP_UNKNOWN_OPT, however, the pattern that is
> currently accepted will stop working, e.g.,
> 
>  $ git sparse-checkout [add/set] --[no-]cone --foo bar
> 
> as we would barf with "--foo: unknown option", and the users who are
> used to this sloppy command line parsing we had for the past few
> years must now write "--end-of-options" before "--foo".  After all,
> the reason why the original authors of this code used KEEP_UNKNOWN
> is likely to deal with path patterns that begin with dashes.

Right, that is the "gotcha" I mentioned in my other email. Though that
is the way it has behaved historically, my argument is that users are
unreasonable to expect it to work:

  1. It is not consistent with the rest of Git commands.

  2. It is inconsistent with respect to existing options (and is an
     accident waiting to happen when new options are added).

So I'd consider it a bug-fix.

> The patch in the message that started this thread may not be
> correct, either, I am afraid.  For either of these:
> 
>  $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar
>  $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar
> 
> we would see that "foo" (or "--foo") is not "--end-of-options", and
> we end up using three patterns "foo" (or "--foo"),
> "--end-of-options", and "bar", I suspect.  I wonder if we should
> notice the "foo" or "--foo" that were not understood and error out,
> instead.

Yes, I agree that "--foo --end-of-options" should barf. And of course
that happens naturally if you just let parse-options do its job by not
passing the KEEP_UNKNOWN_OPT flag. ;)

I'm not sure about "foo". We do allow out-of-order options/args, so
isn't it correct to keep it as a non-option argument?

> But after all, it is not absolutely necessary to notice and barf.
> The ONLY practical use of the "--end-of-options" mechanism is to
> allow us to write (this applies to any git subcommand):
> 
>  #!/bin/sh
>  git cmd --hard --coded --options --end-of-options "$@"
> 
> in scripts to protect the intended operation from mistaking the
> end-user input as options.  And with a script written carefully to
> do so, all the args that appear before "--end-of-options" would be
> recognizable by the command line parser.

Yes, but if you misspell "--otpions", it magically becomes a parameter
rather than having the command barf and complain. That is not the end of
the world, but it is unfriendly and inconsistent with the rest of Git.

-Peff

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Right, that is the "gotcha" I mentioned in my other email. Though that
> is the way it has behaved historically, my argument is that users are
> unreasonable to expect it to work:
>
>   1. It is not consistent with the rest of Git commands.
>
>   2. It is inconsistent with respect to existing options (and is an
>      accident waiting to happen when new options are added).
>
> So I'd consider it a bug-fix.

So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
deliberately break them^W^W^Wrealign their expectations?

I do not have much stake in sparse-checkout, so I am fine with that
direction.  But I suspect other folks on the list would have users
of their own who would rather loudly complain to them if we broke
them ;-) 

I'll see how bad the fallout would be if we go that route, using the
test modification I made for the previous rounds as yardsticks.

Thanks.

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Jeff King]

On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Right, that is the "gotcha" I mentioned in my other email. Though that
> > is the way it has behaved historically, my argument is that users are
> > unreasonable to expect it to work:
> >
> >   1. It is not consistent with the rest of Git commands.
> >
> >   2. It is inconsistent with respect to existing options (and is an
> >      accident waiting to happen when new options are added).
> >
> > So I'd consider it a bug-fix.
> 
> So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
> deliberately break them^W^W^Wrealign their expectations?

Yes. :) But keep in mind we are un-breaking other people, like those who
typo:

  git sparse-checkout --sikp-checks

and don't see an error (instead, we make a garbage entry in the sparse
checkout file).

> I do not have much stake in sparse-checkout, so I am fine with that
> direction.  But I suspect other folks on the list would have users
> of their own who would rather loudly complain to them if we broke
> them ;-) 

Likewise, I have never used sparse-checkout myself, and I don't care
_that_ much. My interest is mostly in having various Git commands behave
consistently. This whole discussion started because the centralized
--end-of-options fix interacted badly with this unusual behavior.

-Peff

RE: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[rsbecker]

On Saturday, December 23, 2023 5:02 AM, Peff wrote:
>On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> > Right, that is the "gotcha" I mentioned in my other email. Though
>> > that is the way it has behaved historically, my argument is that
>> > users are unreasonable to expect it to work:
>> >
>> >   1. It is not consistent with the rest of Git commands.
>> >
>> >   2. It is inconsistent with respect to existing options (and is an
>> >      accident waiting to happen when new options are added).
>> >
>> > So I'd consider it a bug-fix.
>>
>> So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
>> deliberately break them^W^W^Wrealign their expectations?
>
>Yes. :) But keep in mind we are un-breaking other people, like those who
>typo:
>
>  git sparse-checkout --sikp-checks

I don't see why 

git sparse-checkout -- --sikp-checks

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

>and don't see an error (instead, we make a garbage entry in the sparse checkout
>file).
>
>> I do not have much stake in sparse-checkout, so I am fine with that
>> direction.  But I suspect other folks on the list would have users of
>> their own who would rather loudly complain to them if we broke them
>> ;-)
>
>Likewise, I have never used sparse-checkout myself, and I don't care _that_ much.
>My interest is mostly in having various Git commands behave consistently. This
>whole discussion started because the centralized --end-of-options fix interacted
>badly with this unusual behavior.

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

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Elijah Newren]

Hi,

On Sat, Dec 23, 2023 at 2:02 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > Right, that is the "gotcha" I mentioned in my other email. Though that
> > > is the way it has behaved historically, my argument is that users are
> > > unreasonable to expect it to work:
> > >
> > >   1. It is not consistent with the rest of Git commands.
> > >
> > >   2. It is inconsistent with respect to existing options (and is an
> > >      accident waiting to happen when new options are added).
> > >
> > > So I'd consider it a bug-fix.
> >
> > So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
> > deliberately break them^W^W^Wrealign their expectations?
>
> Yes. :) But keep in mind we are un-breaking other people, like those who
> typo:
>
>   git sparse-checkout --sikp-checks
>
> and don't see an error (instead, we make a garbage entry in the sparse
> checkout file).

I think you mean
     git sparse-checkout set --sikp-checks
or
     git sparse-checkout add --sikp-checks
(i.e. with either 'set' or 'add' in there)

Neither of these are currently an error, but I agree both definitely
ought to be.  (Without the 'set' or 'add', you currently do get an
error, as we'd expect.)

> > I do not have much stake in sparse-checkout, so I am fine with that
> > direction.  But I suspect other folks on the list would have users
> > of their own who would rather loudly complain to them if we broke
> > them ;-)
>
> Likewise, I have never used sparse-checkout myself, and I don't care
> _that_ much. My interest is mostly in having various Git commands behave
> consistently. This whole discussion started because the centralized
> --end-of-options fix interacted badly with this unusual behavior.

Well, I care quite a bit about sparse-checkout, and I would rather see
Peff's proposed fix, even if some users might have to tweak their
commands a little.

Of course, I'm not the only sparse-checkout user representative, but
Randall commented elsewhere in this thread that he used
sparse-checkout quite a bit, and he feels that "without the --,
--sikp-checks should result in an error."  In other words, he is
basically agreeing that taking Peff's fix seems more important than
strict backward compatibility here.  (His wording may not sound like
that at first glance, probably because of the 'set'/'add' omission in
the example command, but I think what he said actually amounts to
agreeing with this change.)

Finally, git-sparse-checkout(1) does say "THIS COMMAND IS
EXPERIMENTAL.  ITS BEHAVIOR...WILL LIKELY CHANGE".  I apologize for
being pulled from my intent to work on [*], which I think would allow
us to finally drop this warning (I'll still get back to it, one way or
another...), but I think this is a good case where we should take
advantage of the existing warning and simply fix the command.

Anyway, just my $0.02,
Elijah

[*] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Elijah Newren]

On Sat, Dec 23, 2023 at 2:45 PM Elijah Newren <newren@gmail.com> wrote:
>
> Hi,
>
> On Sat, Dec 23, 2023 at 2:02 AM Jeff King <peff@peff.net> wrote:
> >
> > On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:
> >
> > > Jeff King <peff@peff.net> writes:
> > >
> > > > Right, that is the "gotcha" I mentioned in my other email. Though that
> > > > is the way it has behaved historically, my argument is that users are
> > > > unreasonable to expect it to work:
> > > >
> > > >   1. It is not consistent with the rest of Git commands.
> > > >
> > > >   2. It is inconsistent with respect to existing options (and is an
> > > >      accident waiting to happen when new options are added).
> > > >
> > > > So I'd consider it a bug-fix.
> > >
> > > So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
> > > deliberately break them^W^W^Wrealign their expectations?
> >
> > Yes. :) But keep in mind we are un-breaking other people, like those who
> > typo:
> >
> >   git sparse-checkout --sikp-checks
> >
> > and don't see an error (instead, we make a garbage entry in the sparse
> > checkout file).
>
> I think you mean
>      git sparse-checkout set --sikp-checks
> or
>      git sparse-checkout add --sikp-checks
> (i.e. with either 'set' or 'add' in there)
>
> Neither of these are currently an error, but I agree both definitely
> ought to be.  (Without the 'set' or 'add', you currently do get an
> error, as we'd expect.)
>
> > > I do not have much stake in sparse-checkout, so I am fine with that
> > > direction.  But I suspect other folks on the list would have users
> > > of their own who would rather loudly complain to them if we broke
> > > them ;-)
> >
> > Likewise, I have never used sparse-checkout myself, and I don't care
> > _that_ much. My interest is mostly in having various Git commands behave
> > consistently. This whole discussion started because the centralized
> > --end-of-options fix interacted badly with this unusual behavior.
>
> Well, I care quite a bit about sparse-checkout, and I would rather see
> Peff's proposed fix, even if some users might have to tweak their
> commands a little.

And I wrote it up as a patch over at
https://lore.kernel.org/git/pull.1625.git.git.1703379611749.gitgitgadget@gmail.com/

[RFC/PATCH] archive: "--list" does not take further options

[Junio C Hamano]

"git archive --list blah" should notice an extra command line
parameter that goes unused.  Make it so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This was done to convince myself that even though cmd_archive()
   calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then
   uses the resulting argc/argv without apparent filtering of the
   "--end-of-options" thing, it is correctly handling it, either
   locally or remotely.

   - locally, write_archive() uses parse_archive_args(), which calls
     parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options"
     is handled there just fine.

   - remotely, run_remote_archiver() relays the local parameters,
     including "--end-of-options" via the "argument" packet.  Then
     the arguments are assembled into a strvec and used by the
     upload-archive running on the other side to spawn an
     upload-archive--writer process with.
     cmd_upload_archive_writer() then makes the same write_archive()
     call; "--end-of-options" would still be in argv[] if the
     original "git archive --remote" invocation had one, but it is
     consumed the same way as the local case in write_archive() by
     calling parse_archive_args().

   I do not like the remote error behaviour this one adds at all.
   Do we use a more proper mechanism to propagate a remote error
   back for other subcommands we can reuse here?

 archive.c           |  7 +++++++
 t/t5000-tar-tree.sh | 14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git c/archive.c w/archive.c
index 9aeaf2bd87..3244e9f9f2 100644
--- c/archive.c
+++ w/archive.c
@@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,
 		base = "";
 
 	if (list) {
+		if (argc) {
+			if (!is_remote)
+				die(_("extra command line parameter '%s'"), *argv);
+			else
+				printf("!ERROR! extra command line parameter '%s'\n",
+				       *argv);
+		}
 		for (i = 0; i < nr_archivers; i++)
 			if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)
 				printf("%s\n", archivers[i]->name);
diff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh
index 918a2fc7c6..04592f45b0 100755
--- c/t/t5000-tar-tree.sh
+++ w/t/t5000-tar-tree.sh
@@ -124,6 +124,20 @@ test_expect_success 'setup' '
 	EOF
 '
 
+test_expect_success '--list notices extra parameters' '
+	test_must_fail git archive --list blah &&
+	# NEEDSWORK: remote error does not result in non-zero
+	# exit, which we might want to change later.
+	git archive --remote=. --list blah >remote-out &&
+	grep "!ERROR! " remote-out
+'
+
+test_expect_success 'end-of-options is correctly eaten' '
+	git archive --list --end-of-options &&
+	git archive --remote=. --list --end-of-options >remote-out &&
+	! grep "!ERROR! " remote-out
+'
+
 test_expect_success 'populate workdir' '
 	mkdir a &&
 	echo simple textfile >a/a &&

Re: [RFC/PATCH] archive: "--list" does not take further options

[René Scharfe]

Am 21.12.23 um 03:41 schrieb Junio C Hamano:
> "git archive --list blah" should notice an extra command line
> parameter that goes unused.  Make it so.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * This was done to convince myself that even though cmd_archive()
>    calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then
>    uses the resulting argc/argv without apparent filtering of the
>    "--end-of-options" thing, it is correctly handling it, either
>    locally or remotely.
>
>    - locally, write_archive() uses parse_archive_args(), which calls
>      parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options"
>      is handled there just fine.
>
>    - remotely, run_remote_archiver() relays the local parameters,
>      including "--end-of-options" via the "argument" packet.  Then
>      the arguments are assembled into a strvec and used by the
>      upload-archive running on the other side to spawn an
>      upload-archive--writer process with.
>      cmd_upload_archive_writer() then makes the same write_archive()
>      call; "--end-of-options" would still be in argv[] if the
>      original "git archive --remote" invocation had one, but it is
>      consumed the same way as the local case in write_archive() by
>      calling parse_archive_args().
>
>    I do not like the remote error behaviour this one adds at all.
>    Do we use a more proper mechanism to propagate a remote error
>    back for other subcommands we can reuse here?

Don't we have one?  It would affect other unsupported options as well,
and this seems to work just fine, e.g.:

   $ git archive --remote=. --format=foo HEAD
   remote: fatal: Unknown archive format 'foo'
   remote: git upload-archive: archiver died with error
   fatal: sent error to the client: git upload-archive: archiver died with error

>
>  archive.c           |  7 +++++++
>  t/t5000-tar-tree.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git c/archive.c w/archive.c
> index 9aeaf2bd87..3244e9f9f2 100644
> --- c/archive.c
> +++ w/archive.c
> @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,
>  		base = "";
>
>  	if (list) {
> +		if (argc) {
> +			if (!is_remote)
> +				die(_("extra command line parameter '%s'"), *argv);
> +			else
> +				printf("!ERROR! extra command line parameter '%s'\n",
> +				       *argv);
> +		}

So just call die() here?

>  		for (i = 0; i < nr_archivers; i++)
>  			if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)
>  				printf("%s\n", archivers[i]->name);
> diff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh
> index 918a2fc7c6..04592f45b0 100755
> --- c/t/t5000-tar-tree.sh
> +++ w/t/t5000-tar-tree.sh
> @@ -124,6 +124,20 @@ test_expect_success 'setup' '
>  	EOF
>  '
>
> +test_expect_success '--list notices extra parameters' '
> +	test_must_fail git archive --list blah &&
> +	# NEEDSWORK: remote error does not result in non-zero
> +	# exit, which we might want to change later.
> +	git archive --remote=. --list blah >remote-out &&
> +	grep "!ERROR! " remote-out

... and use test_must_fail in both cases?

> +'
> +
> +test_expect_success 'end-of-options is correctly eaten' '
> +	git archive --list --end-of-options &&
> +	git archive --remote=. --list --end-of-options >remote-out &&
> +	! grep "!ERROR! " remote-out
> +'
> +
>  test_expect_success 'populate workdir' '
>  	mkdir a &&
>  	echo simple textfile >a/a &&

Re: [RFC/PATCH] archive: "--list" does not take further options

[Jeff King]

On Thu, Dec 21, 2023 at 08:30:36AM +0100, René Scharfe wrote:

> >    I do not like the remote error behaviour this one adds at all.
> >    Do we use a more proper mechanism to propagate a remote error
> >    back for other subcommands we can reuse here?
> 
> Don't we have one?  It would affect other unsupported options as well,
> and this seems to work just fine, e.g.:
> 
>    $ git archive --remote=. --format=foo HEAD
>    remote: fatal: Unknown archive format 'foo'
>    remote: git upload-archive: archiver died with error
>    fatal: sent error to the client: git upload-archive: archiver died with error

Right. The whole idea of upload-archive is to spawn a separate writer
process and mux the conversation (including errors) back over the wire.
There are a zillion reasons it can die (including bad arguments) and we
catch and report them in the muxing process.

> >  	if (list) {
> > +		if (argc) {
> > +			if (!is_remote)
> > +				die(_("extra command line parameter '%s'"), *argv);
> > +			else
> > +				printf("!ERROR! extra command line parameter '%s'\n",
> > +				       *argv);
> > +		}
> 
> So just call die() here?

Yes, exactly.

-Peff

Re: [RFC/PATCH] archive: "--list" does not take further options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 21, 2023 at 08:30:36AM +0100, René Scharfe wrote:
>> ...
>> Don't we have one?  It would affect other unsupported options as well,
>> and this seems to work just fine, e.g.:
>> ...
> Right. The whole idea of upload-archive is to spawn a separate writer
> process and mux the conversation (including errors) back over the wire.

Thanks, both.  Just to tie the loose end, let me queue this and
merge it to 'next'.

----- >8 --------- >8 --------- >8 -----
Subject: [PATCH] archive: "--list" does not take further options

"git archive --list blah" should notice an extra command line
parameter that goes unused.  Make it so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 archive.c           |  2 ++
 t/t5000-tar-tree.sh | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/archive.c b/archive.c
index ca11db185b..8da820d1ce 100644
--- a/archive.c
+++ b/archive.c
@@ -685,6 +685,8 @@ static int parse_archive_args(int argc, const char **argv,
 		base = "";
 
 	if (list) {
+		if (argc)
+			die(_("extra command line parameter '%s'"), *argv);
 		for (i = 0; i < nr_archivers; i++)
 			if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)
 				printf("%s\n", archivers[i]->name);
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 4b4c3315d8..72b8d0ff02 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -124,6 +124,16 @@ test_expect_success 'setup' '
 	EOF
 '
 
+test_expect_success '--list notices extra parameters' '
+	test_must_fail git archive --list blah &&
+	test_must_fail git archive --remote=. --list blah
+'
+
+test_expect_success 'end-of-options is correctly eaten' '
+	git archive --list --end-of-options &&
+	git archive --remote=. --list --end-of-options
+'
+
 test_expect_success 'populate workdir' '
 	mkdir a &&
 	echo simple textfile >a/a &&
-- 
2.43.0-174-g055bb6e996

Re: [RFC/PATCH] archive: "--list" does not take further options

[Jeff King]

On Thu, Dec 21, 2023 at 10:13:58AM -0800, Junio C Hamano wrote:

> Thanks, both.  Just to tie the loose end, let me queue this and
> merge it to 'next'.
> 
> ----- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] archive: "--list" does not take further options

This looks fine to me. It does mean that this sequence of commands no
longer works:

  [let's try to make a "foo" archive]
  $ git archive --format=foo HEAD
  fatal: Unknown archive format 'foo'

  [oops, that didn't work. What formats are supported?]
  $ !! --list
  git archive --format=foo HEAD --list
  tar
  tgz
  tar.gz
  zip

I think that's OK in practice. The increased error checking is worth it
(and matches many other commands, which tend to warn about confusing
extra bits).

-Peff

Re: [RFC/PATCH] archive: "--list" does not take further options

[Jeff King]

On Wed, Dec 20, 2023 at 06:41:56PM -0800, Junio C Hamano wrote:

> ---
>  * This was done to convince myself that even though cmd_archive()
>    calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then
>    uses the resulting argc/argv without apparent filtering of the
>    "--end-of-options" thing, it is correctly handling it, either
>    locally or remotely.
> 
>    - locally, write_archive() uses parse_archive_args(), which calls
>      parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options"
>      is handled there just fine.

Right. Not only is it handled by the second parser, but _not_ keeping it
would be a bug, because that second parser would have no idea that we
saw end-of-options (and so "git archive --end-of-options --foo" would
try to treat "--foo" as an option).

The recent commit 9385174627 did fix a case like this for fast-export,
but git-archive was not changed. It passed KEEP_DASHDASH along with
KEEP_UNKNOWN_OPT, so it already retained and passed along
--end-of-options.

>    - remotely, run_remote_archiver() relays the local parameters,
>      including "--end-of-options" via the "argument" packet.  Then
>      the arguments are assembled into a strvec and used by the
>      upload-archive running on the other side to spawn an
>      upload-archive--writer process with.
>      cmd_upload_archive_writer() then makes the same write_archive()
>      call; "--end-of-options" would still be in argv[] if the
>      original "git archive --remote" invocation had one, but it is
>      consumed the same way as the local case in write_archive() by
>      calling parse_archive_args().

Right, and this is just the same case but with a lot of complicated
network-ferrying in between. :) We must retain --end-of-options so that
the next parser knows to stop treating them as arguments. And because it
doesn't use KEEP_UNKNOWN_OPT, the ferried "--end-of-options" is removed
then.

> diff --git c/archive.c w/archive.c
> index 9aeaf2bd87..3244e9f9f2 100644
> --- c/archive.c
> +++ w/archive.c
> @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,
>  		base = "";
>  
>  	if (list) {
> +		if (argc) {
> +			if (!is_remote)
> +				die(_("extra command line parameter '%s'"), *argv);
> +			else
> +				printf("!ERROR! extra command line parameter '%s'\n",
> +				       *argv);
> +		}

This general direction seems reasonable to me, since we're letting the
user know that their extra argument was ignored (though note that if it
was a misspelled option, like "--otuput", we would complain about that).
It's largely orthogonal to end-of-options, but I see how you got here by
wondering about it. :)

-Peff

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Jeff King]

On Wed, Dec 20, 2023 at 03:19:23PM -0800, Junio C Hamano 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.

Thanks for finding this. Though I only half-agree with the
"unfortunately" part. ;)

As argued in 93851746, any caller passing KEEP_UNKNOWN_OPT but _not_
handling the now-returned end-of-options is rather suspicious. Because
if they are planning to parse those options further, they will have no
idea that we saw --end-of-options, and will err in the unsafe direction
of still treating any elements with dashes as options.

Which would mean that simply dropping --end-of-options from the list, as
your patch does, would be similarly unsafe. It is papering over the
problem of:

  git sparse-checkout --end-of-options foo

but leaving:

  git sparse-checkout --end-of-options --foo

broken.

But the plot thickens! Curiously, in both of these cases, we do not do
any further parsing of the options at all. We just treat them as
pattern arguments with no special meaning.

So your patch is actually OK, but I would argue that the correct fix
here is to drop the use of PARSE_OPT_KEEP_UNKNOWN_OPT at all. I cannot
find any indication in the history on why it was ever used. It was added
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.

So looking at that original patch, it makes me think that the author was
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).

There is one minor gotcha, though. Unlike most other Git commands,
sparse-checkout will happily accept random option names and treat them
as non-option arguments. E.g. this works:

  git sparse-checkout add --foo --bar

and will add "--foo" and "--bar" as patterns. If we remove the flag,
we'd be breaking that. But I'd argue that anybody relying on that is
asking for trouble. For example, this does not work in the same way:

  git sparse-checkout add --skip-checks --foo

because "--skip-checks" is a real option. Ditto for any other options,
including those we add in the future. If you don't trust the contents of
your arguments, you should be using "--" or "--end-of-options" to
communicate the intent to the command.

-Peff

Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Which would mean that simply dropping --end-of-options from the list, as
> your patch does, would be similarly unsafe. It is papering over the
> problem of:
>
>   git sparse-checkout --end-of-options foo
>
> but leaving:
>
>   git sparse-checkout --end-of-options --foo
>
> broken.

Hmph, I do not understand.  The latter case we want to take "--foo"
as the sole parameter to the subcommand, no?

> But the plot thickens! Curiously, in both of these cases, we do not do
> any further parsing of the options at all. We just treat them as
> pattern arguments with no special meaning.

Exactly.

> So your patch is actually OK, but I would argue that the correct fix
> here is to drop the use of PARSE_OPT_KEEP_UNKNOWN_OPT at all. I cannot
> find any indication in the history on why it was ever used. It was added
> 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.

Yes.

> So looking at that original patch, it makes me think that the author was
> 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).

That is something only the original author can answer, by my working
theory has been they wanted to have a cheap way to allow any string,
even the ones that happen to begin with a dash, to be passed as one
of the patterns.

> There is one minor gotcha, though. Unlike most other Git commands,
> sparse-checkout will happily accept random option names and treat them
> as non-option arguments. E.g. this works:
>
>   git sparse-checkout add --foo --bar
>
> and will add "--foo" and "--bar" as patterns. If we remove the flag,
> we'd be breaking that.

Exactly.  The user _should_ protect these "patterns" by using
"--end-of-options" before "--foo" above, but we have been taking the
patterns with such a loose parser for quite some time, so tightening
would be taken as a regression X-<.

> But I'd argue that anybody relying on that is
> asking for trouble. For example, this does not work in the same way:
>
>   git sparse-checkout add --skip-checks --foo
>
> because "--skip-checks" is a real option. Ditto for any other options,
> including those we add in the future. If you don't trust the contents of
> your arguments, you should be using "--" or "--end-of-options" to
> communicate the intent to the command.

Exactly.  My response to the other message, with a new test,
summarizes my position.

Thanks.