[RFC] sparse-checkout: be consistent with end of options markers #1625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1625.git.git.1703379611749.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff King wrote (reply to this): On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:
> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> bug. Note that this does mean that anyone who might have been using
> [...]
Thanks for wrapping this up in patch form. It looks good to me,
including the reasoning. You didn't add any tests, but I find it rather
unlikely that we'd later regress here.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <peff@peff.net> writes:
> On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:
>
>> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
>> bug. Note that this does mean that anyone who might have been using
>> [...]
>
> Thanks for wrapping this up in patch form. It looks good to me,
> including the reasoning. You didn't add any tests, but I find it rather
> unlikely that we'd later regress here.
Surely. I am certainly OK with just dropping KEEP_UNKNOWN but I
would strongly prefer to document what we "fixed" (your "misspelt
option name" example) and what (mis|ab)use the people may have been
relying on we have "broken" (the same "misspelt" behaviour that can
be intentional is now forbidden, and we document that this change in
behaviour is intentional) with a new test.
Thanks. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> 93851746 (parse-options: decouple "--end-of-options" and "--",
> 2023-12-06) updated the world order to make callers of parse-options
> that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to
> do with "--end-of-options" they may see after parse_options() returns.
>
> This made a previous bug in sparse-checkout more visible; namely,
> that
>
> git sparse-checkout [add|set] --[no-]cone --end-of-options ...
>
> would simply treat "--end-of-options" as one of the paths to include in
> the sparse-checkout. But this was already problematic before; namely,
>
> git sparse-checkout [add|set| --[no-]cone --sikp-checks ...
>
> would not give an error on the mis-typed "--skip-checks" but instead
> simply treat "--sikp-checks" as a path or pattern to include in the
> sparse-checkout, which is highly unfriendly.
>
> This behavior begain when the command was converted to parse-options in
> 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
> 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was
> renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
> PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify
> that it was only about dashed options; we always keep non-option
> arguments. Looking at that original patch, both Peff and I think that
> the author was simply confused about the mis-named option, and really
> just wanted to keep the non-option arguments. We never should have used
> the flag all along (and the other cases were cargo-culted within the
> file).
>
> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> bug. Note that this does mean that anyone who might have been using
>
> git sparse-checkout [add|set] [--[no-]cone] --foo --bar
>
> to request paths or patterns '--foo' and '--bar' will now have to use
>
> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
>
> That makes sparse-checkout more consistent with other git commands,
> provides users much friendlier error messages and behavior, and is
> consistent with the all-capps warning in git-sparse-checkout.txt that
> this command "is experimental...its behavior...will likely change". :-)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Nicely described.
Apparently we do not have an existing test to cover the case of
"misspelt options becoming a pattern" that this bugfix corrects;
otherwise we would have a s/failure/success/ to update such an older
expectation, and have to wonder if the existing behaviour is
intentional. Since there is no such test, we can feel a bit safer
in "fixing" this odd behaviour.
Also, this corrects "--end-of-options" that becomes one of the
patterns. Such a bug was not detected in our test suite.
So we should at least have two new tests.
(1) Pass "--foo" without the end of options marker and make sure we
error out, instead of making it as one of the patterns.
(2) Pass "--end-of-options foo" and make sure the command succeeds,
an d"--end-of-options" does not become on eof the patterns.
Thanks. |
newren
force-pushed
the
sparse-checkout-end-of-options-consistency
branch
from
c191569
to
0bf4e1a
Compare
9385174 (parse-options: decouple "--end-of-options" and "--", 2023-12-06) updated the world order to make callers of parse-options that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to do with "--end-of-options" they may see after parse_options() returns. This made a previous bug in sparse-checkout more visible; namely, that git sparse-checkout [add|set] --[no-]cone --end-of-options ... would simply treat "--end-of-options" as one of the paths to include in the sparse-checkout. But this was already problematic before; namely, git sparse-checkout [add|set| --[no-]cone --sikp-checks ... would not give an error on the mis-typed "--skip-checks" but instead simply treat "--sikp-checks" as a path or pattern to include in the sparse-checkout, which is highly unfriendly. This behavior began when the command was converted to parse-options in 7bffca9 (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 99d86d6 (parse-options: PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify that it was only about dashed options; we always keep non-option arguments. Looking at that original patch, both Peff and I think that the author was simply confused about the mis-named option, and really just wanted to keep the non-option arguments. We never should have used the flag all along (and the other cases were cargo-culted within the file). Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this bug. Note that this does mean that anyone who might have been using git sparse-checkout [add|set] [--[no-]cone] --foo --bar to request paths or patterns '--foo' and '--bar' will now have to use git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar That makes sparse-checkout more consistent with other git commands, provides users much friendlier error messages and behavior, and is consistent with the all-caps warning in git-sparse-checkout.txt that this command "is experimental...its behavior...will likely change". :-) Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
sparse-checkout-end-of-options-consistency
branch
from
0bf4e1a
to
4481466
Compare
|
On the Git mailing list, Elijah Newren wrote (reply to this): Hi,
On Tue, Dec 26, 2023 at 9:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 93851746 (parse-options: decouple "--end-of-options" and "--",
> > 2023-12-06) updated the world order to make callers of parse-options
> > that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to
> > do with "--end-of-options" they may see after parse_options() returns.
> >
> > This made a previous bug in sparse-checkout more visible; namely,
> > that
> >
> > git sparse-checkout [add|set] --[no-]cone --end-of-options ...
> >
> > would simply treat "--end-of-options" as one of the paths to include in
> > the sparse-checkout. But this was already problematic before; namely,
> >
> > git sparse-checkout [add|set| --[no-]cone --sikp-checks ...
> >
> > would not give an error on the mis-typed "--skip-checks" but instead
> > simply treat "--sikp-checks" as a path or pattern to include in the
> > sparse-checkout, which is highly unfriendly.
> >
> > This behavior begain when the command was converted to parse-options in
> > 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
> > 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was
> > renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
> > PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify
> > that it was only about dashed options; we always keep non-option
> > arguments. Looking at that original patch, both Peff and I think that
> > the author was simply confused about the mis-named option, and really
> > just wanted to keep the non-option arguments. We never should have used
> > the flag all along (and the other cases were cargo-culted within the
> > file).
> >
> > Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> > bug. Note that this does mean that anyone who might have been using
> >
> > git sparse-checkout [add|set] [--[no-]cone] --foo --bar
> >
> > to request paths or patterns '--foo' and '--bar' will now have to use
> >
> > git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
> >
> > That makes sparse-checkout more consistent with other git commands,
> > provides users much friendlier error messages and behavior, and is
> > consistent with the all-capps warning in git-sparse-checkout.txt that
> > this command "is experimental...its behavior...will likely change". :-)
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
>
> Nicely described.
>
> Apparently we do not have an existing test to cover the case of
> "misspelt options becoming a pattern" that this bugfix corrects;
> otherwise we would have a s/failure/success/ to update such an older
> expectation, and have to wonder if the existing behaviour is
> intentional. Since there is no such test, we can feel a bit safer
> in "fixing" this odd behaviour.
>
> Also, this corrects "--end-of-options" that becomes one of the
> patterns. Such a bug was not detected in our test suite.
>
> So we should at least have two new tests.
>
> (1) Pass "--foo" without the end of options marker and make sure we
> error out, instead of making it as one of the patterns.
>
> (2) Pass "--end-of-options foo" and make sure the command succeeds,
> and "--end-of-options" does not become one of the patterns.
>
> Thanks.
I did actually create two such tests last Saturday, but they obviously
somehow went missing from my submission (I guess even if the high
fevers from Wed-Fri last week were gone, I was still more affected
than I realized?) Anyway, I'll resubmit with those test cases. |
|
User |
|
/submit |
|
Submitted as pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> bug. Note that this does mean that anyone who might have been using
>
> git sparse-checkout [add|set] [--[no-]cone] --foo --bar
>
> to request paths or patterns '--foo' and '--bar' will now have to use
>
> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
>
> That makes sparse-checkout more consistent with other git commands,
> provides users much friendlier error messages and behavior, and is
> consistent with the all-caps warning in git-sparse-checkout.txt that
> this command "is experimental...its behavior...will likely change". :-)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Let me drop jc/sparse-checkout-eoo topic and queue this instead, and
then resurrect the "use default for 'set' only when !stdin" as a
separate topic.
Thanks. |
|
This branch is now known as |
|
This patch series was integrated into seen via 479e58f. |
|
This patch series was integrated into seen via 850f9d5. |
|
There was a status update in the "New Topics" section about the branch "git sparse-checkout (add|set) --[no-]cone --end-of-options" did not handle "--end-of-options" correctly after a recent update. Will merge to 'next'. source: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com> |
|
On the Git mailing list, Jeff King wrote (reply to this): On Tue, Dec 26, 2023 at 09:18:14AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:
> >
> >> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> >> bug. Note that this does mean that anyone who might have been using
> >> [...]
> >
> > Thanks for wrapping this up in patch form. It looks good to me,
> > including the reasoning. You didn't add any tests, but I find it rather
> > unlikely that we'd later regress here.
>
> Surely. I am certainly OK with just dropping KEEP_UNKNOWN but I
> would strongly prefer to document what we "fixed" (your "misspelt
> option name" example) and what (mis|ab)use the people may have been
> relying on we have "broken" (the same "misspelt" behaviour that can
> be intentional is now forbidden, and we document that this change in
> behaviour is intentional) with a new test.
Yeah, thank you for talking some sense into me. I do not foresee us
regressing "--sikp-checks", but certainly covering --end-of-options in
more places is worthwhile. As it's handled centrally, it can have
unexpected consequences for various commands.
Elijah's latest version looks good to me.
-Peff |
|
This patch series was integrated into seen via 9fd5ad5. |
|
This patch series was integrated into next via 3ddf2eb. |
|
This patch series was integrated into seen via 6dfaf29. |
|
There was a status update in the "Cooking" section about the branch "git sparse-checkout (add|set) --[no-]cone --end-of-options" did not handle "--end-of-options" correctly after a recent update. Will merge to 'master'. source: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 0841b86. |
|
This patch series was integrated into seen via a6315e9. |
|
This patch series was integrated into seen via 8e599fe. |
|
There was a status update in the "Cooking" section about the branch "git sparse-checkout (add|set) --[no-]cone --end-of-options" did not handle "--end-of-options" correctly after a recent update. Will merge to 'master'. source: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via d73db00. |
|
This patch series was integrated into master via d73db00. |
|
This patch series was integrated into next via d73db00. |
|
Closed via d73db00. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to thread over at https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/, making end of options markers in git-sparse-checkout consistent with how other git commands behave.
Changes since v1:
CC: Randall S. Becker randall.becker@nexbridge.ca
CC: Jeff King peff@peff.net
cc: Elijah Newren newren@gmail.com