{"thread":{"id":"58986","subject":"[PATCH] dir: check for single file cone patterns","startedAt":"2022-12-20T14:29:19Z","lastAt":"2023-01-05T11:39:11Z","messageCount":8,"participants":["William Sprent via GitGitGadget","Ævar Arnfjörð Bjarmason","William Sprent","Junio C Hamano","Victoria Dye"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469384","messageId":"pull.1446.git.1671546459151.gitgitgadget@gmail.com","threadId":"58986","inReplyTo":null,"subject":"[PATCH] dir: check for single file cone patterns","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-20T14:27:38Z","receivedAt":"2022-12-20T14:29:19Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent <williams@unity3d.com>\n\nThe sparse checkout documentation states that the cone mode pattern set\nis limited to patterns that either recursively include directories or\npatterns that match all files in a directory. In the sparse checkout\nfile, the former manifest in the form:\n\n    /A/B/C/\n\nwhile the latter become a pair of patterns either in the form:\n\n    /A/B/\n    !/A/B/*/\n\nor in the special case of matching the toplevel files:\n\n    /*\n    !/*/\n\nThe 'add_pattern_to_hashsets()' function contains checks which serve to\ndisable cone-mode when non-cone patterns are encountered. However, these\ndo not catch when the pattern list attempts to match a single file or\ndirectory, e.g. a pattern in the form:\n\n    /A/B/C\n\nThis causes sparse-checkout to exhibit unexpected behaviour when such a\npattern is in the sparse-checkout file and cone mode is enabled.\nConcretely, with the pattern like the above, sparse-checkout, in\nnon-cone mode, will only include the directory or file located at\n'/A/B/C'. However, with cone mode enabled, sparse-checkout will instead\njust manifest the toplevel files but not any file located at '/A/B/C'.\n\nRelatedly, issues occur when supplying the same kind of filter when\npartial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will\ncorrectly just include the objects that match the non-cone pattern\nmatching. Which means that checking out the newly cloned repo with the\nsame filter, but with cone mode enabled, fails due to missing objects.\n\nTo fix these issues, add a cone mode pattern check that asserts that\nevery pattern is either a directory match or the pattern '/*'. Add a\ntest to verify the new pattern check and modify another to reflect that\nnon-directory patterns are caught earlier.\n\nSigned-off-by: William Sprent <williams@unity3d.com>\n---\n    dir: check for single file cone patterns\n    \n    I ran into this confusing behaviour when working with sparse-checkout\n    patterns. If my interpretation of the documentation is correct, then I\n    think comes from git not being strict enough when checking cone mode\n    patterns.\n    \n    Thanks, William\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1446\n\n dir.c                              |  7 +++++++\n t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex fbdb24fc819..a122aeb2aa3 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \t\tgoto clear_hashmaps;\n \t}\n \n+\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n+\t    !!strcmp(given->pattern, \"/*\")) {\n+\t\t/* Not a cone pattern. */\n+\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n+\t\tgoto clear_hashmaps;\n+\t}\n+\n \tprev = given->pattern;\n \tcur = given->pattern + 1;\n \tnext = given->pattern + 2;\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex b563d6c263e..627267be153 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' '\n test_expect_success 'cone mode: warn on bad pattern' '\n \ttest_when_finished mv sparse-checkout repo/.git/info/ &&\n \tcp repo/.git/info/sparse-checkout . &&\n-\techo \"!/deep/deeper/*\" >>repo/.git/info/sparse-checkout &&\n+\techo \"!/deep/deeper/*/\" >>repo/.git/info/sparse-checkout &&\n \tgit -C repo read-tree -mu HEAD 2>err &&\n \ttest_i18ngrep \"unrecognized negative pattern\" err\n '\n@@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting \"*\"' '\n \tcheck_read_tree_errors repo \"a deep\" \"disabling cone pattern matching\"\n '\n \n+test_expect_success 'pattern-checks: non directory pattern' '\n+\tcat >repo/.git/info/sparse-checkout <<-\\EOF &&\n+\t/deep/deeper1/a\n+\tEOF\n+\tcheck_read_tree_errors repo deep \"disabling cone pattern matching\" &&\n+\tcheck_files repo/deep deeper1 &&\n+\tcheck_files repo/deep/deeper1 a\n+'\n+\n test_expect_success 'pattern-checks: contained glob characters' '\n \tfor c in \"[a]\" \"\\\\\" \"?\" \"*\"\n \tdo\n\nbase-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n-- \ngitgitgadget\n"},{"id":"469385","messageId":"221220.86sfhaw3c3.gmgdl@evledraar.gmail.com","threadId":"58986","inReplyTo":"pull.1446.git.1671546459151.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir: check for single file cone patterns","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T14:33:51Z","receivedAt":"2022-12-20T14:35:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 20 2022, William Sprent via GitGitGadget wrote:\n\n> From: William Sprent <williams@unity3d.com>\n> [...]\n> +\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n> +\t    !!strcmp(given->pattern, \"/*\")) {\n\nStyle: We use !! to convert 123 to 1 or whatever, but here we just care\nabout \"not matched\", so we can drop this !!strcmp() for strcmp().\n"},{"id":"469434","messageId":"8c9650b7-8e36-909c-c193-2f6713427e57@unity3d.com","threadId":"58986","inReplyTo":"221220.86sfhaw3c3.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] dir: check for single file cone patterns","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2022-12-21T12:51:46Z","receivedAt":"2022-12-21T12:51:56Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 20/12/2022 15.33, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Dec 20 2022, William Sprent via GitGitGadget wrote:\n> \n>> From: William Sprent <williams@unity3d.com>\n>> [...]\n>> +\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n>> +\t    !!strcmp(given->pattern, \"/*\")) {\n> \n> Style: We use !! to convert 123 to 1 or whatever, but here we just care\n> about \"not matched\", so we can drop this !!strcmp() for strcmp().\n\nThanks. I'll drop the '!!' for next version.\n"},{"id":"469758","messageId":"pull.1446.v2.git.1672734059938.gitgitgadget@gmail.com","threadId":"58986","inReplyTo":"pull.1446.git.1671546459151.gitgitgadget@gmail.com","subject":"[PATCH v2] dir: check for single file cone patterns","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-03T08:20:59Z","receivedAt":"2023-01-03T08:21:09Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent <williams@unity3d.com>\n\nThe sparse checkout documentation states that the cone mode pattern set\nis limited to patterns that either recursively include directories or\npatterns that match all files in a directory. In the sparse checkout\nfile, the former manifest in the form:\n\n    /A/B/C/\n\nwhile the latter become a pair of patterns either in the form:\n\n    /A/B/\n    !/A/B/*/\n\nor in the special case of matching the toplevel files:\n\n    /*\n    !/*/\n\nThe 'add_pattern_to_hashsets()' function contains checks which serve to\ndisable cone-mode when non-cone patterns are encountered. However, these\ndo not catch when the pattern list attempts to match a single file or\ndirectory, e.g. a pattern in the form:\n\n    /A/B/C\n\nThis causes sparse-checkout to exhibit unexpected behaviour when such a\npattern is in the sparse-checkout file and cone mode is enabled.\nConcretely, with the pattern like the above, sparse-checkout, in\nnon-cone mode, will only include the directory or file located at\n'/A/B/C'. However, with cone mode enabled, sparse-checkout will instead\njust manifest the toplevel files but not any file located at '/A/B/C'.\n\nRelatedly, issues occur when supplying the same kind of filter when\npartial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will\ncorrectly just include the objects that match the non-cone pattern\nmatching. Which means that checking out the newly cloned repo with the\nsame filter, but with cone mode enabled, fails due to missing objects.\n\nTo fix these issues, add a cone mode pattern check that asserts that\nevery pattern is either a directory match or the pattern '/*'. Add a\ntest to verify the new pattern check and modify another to reflect that\nnon-directory patterns are caught earlier.\n\nSigned-off-by: William Sprent <williams@unity3d.com>\n---\n    dir: check for single file cone patterns\n    \n    Resubmitting this without the superfluous double negation of the\n    'strcmp' output.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1446\n\nRange-diff vs v1:\n\n 1:  cfb4b75c378 ! 1:  d5406e62f6f dir: check for single file cone patterns\n     @@ dir.c: static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_\n       \t}\n       \n      +\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n     -+\t    !!strcmp(given->pattern, \"/*\")) {\n     ++\t    strcmp(given->pattern, \"/*\")) {\n      +\t\t/* Not a cone pattern. */\n      +\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n      +\t\tgoto clear_hashmaps;\n\n\n dir.c                              |  7 +++++++\n t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex fbdb24fc819..4e99f0c868f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \t\tgoto clear_hashmaps;\n \t}\n \n+\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n+\t    strcmp(given->pattern, \"/*\")) {\n+\t\t/* Not a cone pattern. */\n+\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n+\t\tgoto clear_hashmaps;\n+\t}\n+\n \tprev = given->pattern;\n \tcur = given->pattern + 1;\n \tnext = given->pattern + 2;\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex b563d6c263e..627267be153 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' '\n test_expect_success 'cone mode: warn on bad pattern' '\n \ttest_when_finished mv sparse-checkout repo/.git/info/ &&\n \tcp repo/.git/info/sparse-checkout . &&\n-\techo \"!/deep/deeper/*\" >>repo/.git/info/sparse-checkout &&\n+\techo \"!/deep/deeper/*/\" >>repo/.git/info/sparse-checkout &&\n \tgit -C repo read-tree -mu HEAD 2>err &&\n \ttest_i18ngrep \"unrecognized negative pattern\" err\n '\n@@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting \"*\"' '\n \tcheck_read_tree_errors repo \"a deep\" \"disabling cone pattern matching\"\n '\n \n+test_expect_success 'pattern-checks: non directory pattern' '\n+\tcat >repo/.git/info/sparse-checkout <<-\\EOF &&\n+\t/deep/deeper1/a\n+\tEOF\n+\tcheck_read_tree_errors repo deep \"disabling cone pattern matching\" &&\n+\tcheck_files repo/deep deeper1 &&\n+\tcheck_files repo/deep/deeper1 a\n+'\n+\n test_expect_success 'pattern-checks: contained glob characters' '\n \tfor c in \"[a]\" \"\\\\\" \"?\" \"*\"\n \tdo\n\nbase-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n-- \ngitgitgadget\n"},{"id":"469778","messageId":"xmqqfscqvnmy.fsf@gitster.g","threadId":"58986","inReplyTo":"pull.1446.v2.git.1672734059938.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] dir: check for single file cone patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-04T06:07:17Z","receivedAt":"2023-01-04T06:07:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"William Sprent via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: William Sprent <williams@unity3d.com>\n>\n> The sparse checkout documentation states that the cone mode pattern set\n> is limited to patterns that either recursively include directories or\n> patterns that match all files in a directory. In the sparse checkout\n> file, the former manifest in the form:\n>\n>     /A/B/C/\n>\n> while the latter become a pair of patterns either in the form:\n>\n>     /A/B/\n>     !/A/B/*/\n>\n> or in the special case of matching the toplevel files:\n>\n>     /*\n>     !/*/\n>\n> The 'add_pattern_to_hashsets()' function contains checks which serve to\n> disable cone-mode when non-cone patterns are encountered. However, these\n> do not catch when the pattern list attempts to match a single file or\n> directory, e.g. a pattern in the form:\n>\n>     /A/B/C\n>\n> This causes sparse-checkout to exhibit unexpected behaviour when such a\n> pattern is in the sparse-checkout file and cone mode is enabled.\n> Concretely, with the pattern like the above, sparse-checkout, in\n> non-cone mode, will only include the directory or file located at\n> '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead\n> just manifest the toplevel files but not any file located at '/A/B/C'.\n>\n> Relatedly, issues occur when supplying the same kind of filter when\n> partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will\n> correctly just include the objects that match the non-cone pattern\n> matching. Which means that checking out the newly cloned repo with the\n> same filter, but with cone mode enabled, fails due to missing objects.\n>\n> To fix these issues, add a cone mode pattern check that asserts that\n> every pattern is either a directory match or the pattern '/*'. Add a\n> test to verify the new pattern check and modify another to reflect that\n> non-directory patterns are caught earlier.\n>\n> Signed-off-by: William Sprent <williams@unity3d.com>\n> ---\n\nSummoning those who worked in the area, which includes area experts,\nfor further comments, picked out of output from\n\n    $ git shortlog --no-merges --grep=cone --since=2.years -s -n -e\n\n\n> diff --git a/dir.c b/dir.c\n> index fbdb24fc819..4e99f0c868f 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n>  \t\tgoto clear_hashmaps;\n>  \t}\n>  \n> +\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n> +\t    strcmp(given->pattern, \"/*\")) {\n> +\t\t/* Not a cone pattern. */\n> +\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n> +\t\tgoto clear_hashmaps;\n> +\t}\n> +\n>  \tprev = given->pattern;\n>  \tcur = given->pattern + 1;\n>  \tnext = given->pattern + 2;\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index b563d6c263e..627267be153 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' '\n>  test_expect_success 'cone mode: warn on bad pattern' '\n>  \ttest_when_finished mv sparse-checkout repo/.git/info/ &&\n>  \tcp repo/.git/info/sparse-checkout . &&\n> -\techo \"!/deep/deeper/*\" >>repo/.git/info/sparse-checkout &&\n> +\techo \"!/deep/deeper/*/\" >>repo/.git/info/sparse-checkout &&\n>  \tgit -C repo read-tree -mu HEAD 2>err &&\n>  \ttest_i18ngrep \"unrecognized negative pattern\" err\n>  '\n> @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting \"*\"' '\n>  \tcheck_read_tree_errors repo \"a deep\" \"disabling cone pattern matching\"\n>  '\n>  \n> +test_expect_success 'pattern-checks: non directory pattern' '\n> +\tcat >repo/.git/info/sparse-checkout <<-\\EOF &&\n> +\t/deep/deeper1/a\n> +\tEOF\n> +\tcheck_read_tree_errors repo deep \"disabling cone pattern matching\" &&\n> +\tcheck_files repo/deep deeper1 &&\n> +\tcheck_files repo/deep/deeper1 a\n> +'\n> +\n>  test_expect_success 'pattern-checks: contained glob characters' '\n>  \tfor c in \"[a]\" \"\\\\\" \"?\" \"*\"\n>  \tdo\n>\n> base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n"},{"id":"469800","messageId":"99e3c0f9-ecfa-7c26-eea5-685bc324f674@github.com","threadId":"58986","inReplyTo":"pull.1446.v2.git.1672734059938.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] dir: check for single file cone patterns","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-01-04T23:48:24Z","receivedAt":"2023-01-04T23:48:31Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"William Sprent via GitGitGadget wrote:\n> From: William Sprent <williams@unity3d.com>\n> \n> The sparse checkout documentation states that the cone mode pattern set\n> is limited to patterns that either recursively include directories or\n> patterns that match all files in a directory. In the sparse checkout\n> file, the former manifest in the form:\n> \n>     /A/B/C/\n> \n> while the latter become a pair of patterns either in the form:\n> \n>     /A/B/\n>     !/A/B/*/\n> \n> or in the special case of matching the toplevel files:\n> \n>     /*\n>     !/*/\n> \n> The 'add_pattern_to_hashsets()' function contains checks which serve to\n> disable cone-mode when non-cone patterns are encountered. However, these\n> do not catch when the pattern list attempts to match a single file or\n> directory, e.g. a pattern in the form:\n> \n>     /A/B/C\n> \n> This causes sparse-checkout to exhibit unexpected behaviour when such a\n> pattern is in the sparse-checkout file and cone mode is enabled.\n> Concretely, with the pattern like the above, sparse-checkout, in\n> non-cone mode, will only include the directory or file located at\n> '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead\n> just manifest the toplevel files but not any file located at '/A/B/C'.\n> \n> Relatedly, issues occur when supplying the same kind of filter when\n> partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will\n> correctly just include the objects that match the non-cone pattern\n> matching. Which means that checking out the newly cloned repo with the\n> same filter, but with cone mode enabled, fails due to missing objects.\n\nThis clearly explains the issue you're trying to fix: cone mode\nsparse-checkout requires patterns like '/A/B/', !/A/B/*/', '/A/B/C/', etc.,\nbut invalid patterns like '/A/B/C' (no trailing slash) currently don't force\nthe switch to non-cone mode, leading to unexpected behavior.\n\n> \n> To fix these issues, add a cone mode pattern check that asserts that\n> every pattern is either a directory match or the pattern '/*'. Add a\n> test to verify the new pattern check and modify another to reflect that\n> non-directory patterns are caught earlier.\n\nI think this is the best way to maintain the current intended behavior of\ncone mode sparse-checkout. While the idea of single file \"exceptions\" to\ncone patterns has been brought up in the past (I think most recently at the\nContributor's Summit this past September [1]), it'd be a substantial change\nthat's definitely out-of-scope of this small bugfix.\n\n[1] https://lore.kernel.org/git/YzXwOsaoCdBhHsX1@nand.local/\n\n> \n> Signed-off-by: William Sprent <williams@unity3d.com>\n> ---\n>     dir: check for single file cone patterns\n>     \n>     Resubmitting this without the superfluous double negation of the\n>     'strcmp' output.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1446\n> \n> Range-diff vs v1:\n> \n>  1:  cfb4b75c378 ! 1:  d5406e62f6f dir: check for single file cone patterns\n>      @@ dir.c: static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_\n>        \t}\n>        \n>       +\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n>      -+\t    !!strcmp(given->pattern, \"/*\")) {\n>      ++\t    strcmp(given->pattern, \"/*\")) {\n>       +\t\t/* Not a cone pattern. */\n>       +\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n>       +\t\tgoto clear_hashmaps;\n> \n> \n>  dir.c                              |  7 +++++++\n>  t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++-\n>  2 files changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/dir.c b/dir.c\n> index fbdb24fc819..4e99f0c868f 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n>  \t\tgoto clear_hashmaps;\n>  \t}\n>  \n> +\tif (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&\n> +\t    strcmp(given->pattern, \"/*\")) {\n\nThis condition is only entered if:\n\n- the pattern *does not* end in a trailing slash \n  ('!(given->flags & PATTERN_FLAG_MUSTBEDIR)'), AND\n- the pattern *does not* start with '/*' ('strcmp(given->pattern, \"/*\")')\n\nThe '/*'-like patterns are already handled by other parts of\n'add_pattern_to_hashsets()', so only the '/A/B/C'-like patterns will trigger\nthe \"unrecognized\" warning. Looks good!\n\n\n> +\t\t/* Not a cone pattern. */\n> +\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n> +\t\tgoto clear_hashmaps;\n> +\t}\n> +\n>  \tprev = given->pattern;\n>  \tcur = given->pattern + 1;\n>  \tnext = given->pattern + 2;\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index b563d6c263e..627267be153 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' '\n>  test_expect_success 'cone mode: warn on bad pattern' '\n>  \ttest_when_finished mv sparse-checkout repo/.git/info/ &&\n>  \tcp repo/.git/info/sparse-checkout . &&\n> -\techo \"!/deep/deeper/*\" >>repo/.git/info/sparse-checkout &&\n> +\techo \"!/deep/deeper/*/\" >>repo/.git/info/sparse-checkout &&\n\nThis test changes because, without the trailing slash on the pattern, the\nerror would become \"unrecognized pattern: !/deep/deeper/*\" due to the new\ncheck in 'add_pattern_to_hashsets()'. Changing the test pattern ensures\nwe're still exercising what the test was originally intended to exercise,\nand doesn't indicate a regression.\n\n>  \tgit -C repo read-tree -mu HEAD 2>err &&\n>  \ttest_i18ngrep \"unrecognized negative pattern\" err\n>  '\n> @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting \"*\"' '\n>  \tcheck_read_tree_errors repo \"a deep\" \"disabling cone pattern matching\"\n>  '\n>  \n> +test_expect_success 'pattern-checks: non directory pattern' '\n> +\tcat >repo/.git/info/sparse-checkout <<-\\EOF &&\n> +\t/deep/deeper1/a\n> +\tEOF\n> +\tcheck_read_tree_errors repo deep \"disabling cone pattern matching\" &&\n> +\tcheck_files repo/deep deeper1 &&\n> +\tcheck_files repo/deep/deeper1 a\n> +'\n\nAnd this test ensures the new check is working for the appropriate patterns.\n\nThis patch looks good to me, thanks for finding & fixing the bug!\n\n> +\n>  test_expect_success 'pattern-checks: contained glob characters' '\n>  \tfor c in \"[a]\" \"\\\\\" \"?\" \"*\"\n>  \tdo\n> \n> base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n\n"},{"id":"469803","messageId":"xmqq5ydlu3nt.fsf@gitster.g","threadId":"58986","inReplyTo":"99e3c0f9-ecfa-7c26-eea5-685bc324f674@github.com","subject":"Re: [PATCH v2] dir: check for single file cone patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-05T02:16:22Z","receivedAt":"2023-01-05T02:16:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye <vdye@github.com> writes:\n\n> This clearly explains the issue you're trying to fix: cone mode\n> sparse-checkout requires patterns like '/A/B/', !/A/B/*/', '/A/B/C/', etc.,\n> but invalid patterns like '/A/B/C' (no trailing slash) currently don't force\n> the switch to non-cone mode, leading to unexpected behavior.\n> ...\n>> To fix these issues, add a cone mode pattern check that asserts that\n>> every pattern is either a directory match or the pattern '/*'. Add a\n>> test to verify the new pattern check and modify another to reflect that\n>> non-directory patterns are caught earlier.\n>\n> I think this is the best way to maintain the current intended behavior of\n> cone mode sparse-checkout. While the idea of single file \"exceptions\" to\n> cone patterns has been brought up in the past (I think most recently at the\n> Contributor's Summit this past September [1]), it'd be a substantial change\n> that's definitely out-of-scope of this small bugfix.\n> ...\n> And this test ensures the new check is working for the appropriate patterns.\n>\n> This patch looks good to me, thanks for finding & fixing the bug!\n\nThanks for an excellently written review, and a patch that is well\ndone.\n\nWill queue.\n"},{"id":"469806","messageId":"fd873f47-1f7b-b1dc-8a2d-e1bf8fdfcf05@unity3d.com","threadId":"58986","inReplyTo":"99e3c0f9-ecfa-7c26-eea5-685bc324f674@github.com","subject":"Re: [PATCH v2] dir: check for single file cone patterns","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-05T11:39:05Z","receivedAt":"2023-01-05T11:39:11Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 05/01/2023 00.48, Victoria Dye wrote:\n> \n> I think this is the best way to maintain the current intended behavior of\n> cone mode sparse-checkout. While the idea of single file \"exceptions\" to\n> cone patterns has been brought up in the past (I think most recently at the\n> Contributor's Summit this past September [1]), it'd be a substantial change\n> that's definitely out-of-scope of this small bugfix.\n> \n> [1] https://lore.kernel.org/git/YzXwOsaoCdBhHsX1@nand.local/\n> \n\nThanks for the context. I didn't know that it had been discussed.\n\n>>\n>> ...\n>>\n> \n> And this test ensures the new check is working for the appropriate patterns.\n> \n> This patch looks good to me, thanks for finding & fixing the bug!\n> \n\nThanks for taking the time to review!\n\n"}]}