| Message ID | xmqqbkakqx6s.fsf@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add | expand |
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>
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.
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
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
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*" &&
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.
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
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.
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
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.
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/
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/
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 && /* !/*/
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(-)