{"thread":{"id":"56551","subject":"[PATCH 0/3] Sparse checkout: fix mixed-mode pattern issues","startedAt":"2021-09-21T00:55:17Z","lastAt":"2021-09-21T16:35:23Z","messageCount":10,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"436480","messageId":"pull.1043.git.1632160658.gitgitgadget@gmail.com","threadId":"56551","inReplyTo":null,"subject":"[PATCH 0/3] Sparse checkout: fix mixed-mode pattern issues","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T17:57:35Z","receivedAt":"2021-09-21T00:55:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This fixes a memory leak reported in [1], but also fixes some behavior\nconcerns around the sparse-checkout command when the sparse-checkout file\ndoes not use cone mode patterns, but cone mode is enabled.\n\n[1]\nhttps://lore.kernel.org/git/CAKRwm5a9PyqffEC5N__urSpNcZ-d5vz9GBM2Ei16eGS25B=-FQ@mail.gmail.com/\n\n 1. The first patch fixes the OOM as recommended by Taylor.\n 2. The second patch changes the behavior of 'git sparse-checkout init\n --cone' to overwrite the sparse-checkout file if the patterns are not in\n cone mode.\n 3. The third patch causes 'git sparse-checkout add' to fail if cone mode is\n enabled but the existing patterns are not in cone mode. This also\n requires strengthening our pattern filtering to require the first\n character be a slash ('/'), which should have been there from the start.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n sparse-checkout: fix OOM error with mixed patterns\n sparse-checkout: clear patterns when switching modes\n sparse-checkout: refuse to add to bad patterns\n\n builtin/sparse-checkout.c | 18 +++++++++++++++---\n dir.c | 2 +-\n t/t1091-sparse-checkout-builtin.sh | 22 ++++++++++++++++++++--\n 3 files changed, 36 insertions(+), 6 deletions(-)\n\n\nbase-commit: 4c719308ce59dc70e606f910f40801f2c6051b24\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1043%2Fderrickstolee%2Fsparse-checkout-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1043/derrickstolee/sparse-checkout-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1043\n-- \ngitgitgadget\n"},{"id":"436481","messageId":"d90937b9ac9aaa6170f56d18ddb7a327b2af27d6.1632160658.git.gitgitgadget@gmail.com","threadId":"56551","inReplyTo":"pull.1043.git.1632160658.gitgitgadget@gmail.com","subject":"[PATCH 1/3] sparse-checkout: fix OOM error with mixed patterns","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T17:57:36Z","receivedAt":"2021-09-21T00:55:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nAdd a test to t1091-sparse-checkout-builtin.sh that would result in an\ninfinite loop and out-of-memory error before this change. The issue\nrelies on having non-cone-mode patterns while trying to modify the\npatterns in cone-mode.\n\nThe fix is simple, allowing us to break from the loop when the input\npath does not contain a slash, as the \"dir\" pattern we added does not.\n\nThis is only a fix to the critical out-of-memory error. A better\nresponse to such a strange state will follow in a later change.\n\nReported-by: Calbabreaker \nHelped-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n builtin/sparse-checkout.c | 2 +-\n t/t1091-sparse-checkout-builtin.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 8ba9f13787b..b45fd97a98b 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -389,7 +389,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n \t\tchar *oldpattern = e->pattern;\n \t\tsize_t newlen;\n \n-\t\tif (slash == e->pattern)\n+\t\tif (!slash || slash == e->pattern)\n \t\t\tbreak;\n \n \t\tnewlen = slash - e->pattern;\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 38fc8340f5c..a429d2cc671 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -103,6 +103,14 @@ test_expect_success 'clone --sparse' '\n \tcheck_files clone a\n '\n \n+test_expect_success 'switching to cone mode with non-cone mode patterns' '\n+\tgit init bad-patterns &&\n+\tgit -C bad-patterns sparse-checkout init &&\n+\tgit -C bad-patterns sparse-checkout add dir &&\n+\tgit -C bad-patterns config core.sparseCheckoutCone true &&\n+\tgit -C bad-patterns sparse-checkout add dir\n+'\n+\n test_expect_success 'interaction with clone --no-checkout (unborn index)' '\n \tgit clone --no-checkout \"file://$(pwd)/repo\" clone_no_checkout &&\n \tgit -C clone_no_checkout sparse-checkout init --cone &&\n-- \ngitgitgadget\n\n"},{"id":"436482","messageId":"450b90dad57c42e37a4edc52dac88caf98021fc6.1632160658.git.gitgitgadget@gmail.com","threadId":"56551","inReplyTo":"pull.1043.git.1632160658.gitgitgadget@gmail.com","subject":"[PATCH 2/3] sparse-checkout: clear patterns when switching modes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T17:57:37Z","receivedAt":"2021-09-21T00:55:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nPreviously, when a user runs 'git sparse-checkout init --cone', the\nexisting patterns would remain, even if the patterns were not in cone\nmode. This causes confusion as to how the patterns should work when\nlater 'git sparse-checkout add' commands append to the pattern list.\n\nIn fact, the way these patterns were modified was not even a strict\nappending of patterns, but mutating and reordering patterns because of\nhow the paths were interpreted and rewritten.\n\nAs a first step, we shall start overwriting the pattern set completely\nwhen switching to cone mode, unless the existing patterns already match\ncone mode patterns. The 'use_cone_patterns' member is set to false if\nthe patterns fail to parse using cone mode restrictions.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/sparse-checkout.c | 13 +++++++++++--\n t/t1091-sparse-checkout-builtin.sh | 9 ++++++++-\n 2 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex b45fd97a98b..fe76c3eedda 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -348,8 +348,17 @@ static int sparse_checkout_init(int argc, const char **argv)\n \n \t/* If we already have a sparse-checkout file, use it. */\n \tif (res >= 0) {\n-\t\tfree(sparse_filename);\n-\t\treturn update_working_directory(NULL);\n+\t\tif (pl.use_cone_patterns || !init_opts.cone_mode) {\n+\t\t\tfree(sparse_filename);\n+\t\t\treturn update_working_directory(NULL);\n+\t\t}\n+\n+\t\t/*\n+\t\t * At this point, note that if res >= 0 but pl.use_cone_patterns\n+\t\t * is false, then we want to override the patterns with the\n+\t\t * initial set of cone mode patterns.\n+\t\t */\n+\t\tclear_pattern_list(&pl);\n \t}\n \n \tif (get_oid(\"HEAD\", &oid)) {\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex a429d2cc671..af0acd32bd9 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -108,7 +108,14 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '\n \tgit -C bad-patterns sparse-checkout init &&\n \tgit -C bad-patterns sparse-checkout add dir &&\n \tgit -C bad-patterns config core.sparseCheckoutCone true &&\n-\tgit -C bad-patterns sparse-checkout add dir\n+\tgit -C bad-patterns sparse-checkout add dir &&\n+\n+\tgit -C bad-patterns sparse-checkout init --cone &&\n+\tcat >expect <<-\\EOF &&\n+\t/*\n+\t!/*/\n+\tEOF\n+\ttest_cmp expect bad-patterns/.git/info/sparse-checkout\n '\n \n test_expect_success 'interaction with clone --no-checkout (unborn index)' '\n-- \ngitgitgadget\n\n"},{"id":"436483","messageId":"d513b28b75189d066f9c66de44a1a578cbc38139.1632160658.git.gitgitgadget@gmail.com","threadId":"56551","inReplyTo":"pull.1043.git.1632160658.gitgitgadget@gmail.com","subject":"[PATCH 3/3] sparse-checkout: refuse to add to bad patterns","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T17:57:38Z","receivedAt":"2021-09-21T00:55:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen in cone mode sparse-checkout, it is unclear how 'git\nsparse-checkout add ...' should behave if the existing\nsparse-checkout file does not match the cone mode patterns. Change the\nbehavior to fail with an error message about the existing patterns.\n\nAlso, all cone mode patterns start with a '/' character, so add that\nrestriction. This is necessary for our example test 'cone mode: warn on\nbad pattern', but also requires modifying the example sparse-checkout\nfile we use to test the warnings related to recognizing cone mode\npatterns.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/sparse-checkout.c | 3 +++\n dir.c | 2 +-\n t/t1091-sparse-checkout-builtin.sh | 11 +++++++----\n 3 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex fe76c3eedda..2492ae828a9 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -513,6 +513,9 @@ static void add_patterns_cone_mode(int argc, const char **argv,\n \t\tdie(_(\"unable to load existing sparse-checkout patterns\"));\n \tfree(sparse_filename);\n \n+\tif (!existing.use_cone_patterns)\n+\t\tdie(_(\"existing sparse-checkout patterns do not use cone mode\"));\n+\n \thashmap_for_each_entry(&existing.recursive_hashmap, &iter, pe, ent) {\n \t\tif (!hashmap_contains_parent(&pl->recursive_hashmap,\n \t\t\t\t\tpe->pattern, &buffer) ||\ndiff --git a/dir.c b/dir.c\nindex 03c4d212672..93136442103 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -727,7 +727,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \t}\n \n \tif (given->patternlen < 2 ||\n-\t *given->pattern == '*' ||\n+\t *given->pattern != '/' ||\n \t strstr(given->pattern, \"**\")) {\n \t\t/* Not a cone pattern. */\n \t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex af0acd32bd9..780c6a1aaae 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -108,14 +108,17 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '\n \tgit -C bad-patterns sparse-checkout init &&\n \tgit -C bad-patterns sparse-checkout add dir &&\n \tgit -C bad-patterns config core.sparseCheckoutCone true &&\n-\tgit -C bad-patterns sparse-checkout add dir &&\n+\n+\ttest_must_fail git -C bad-patterns sparse-checkout add dir 2>err &&\n+\tgrep \"existing sparse-checkout patterns do not use cone mode\" err &&\n \n \tgit -C bad-patterns sparse-checkout init --cone &&\n \tcat >expect <<-\\EOF &&\n \t/*\n \t!/*/\n \tEOF\n-\ttest_cmp expect bad-patterns/.git/info/sparse-checkout\n+\ttest_cmp expect bad-patterns/.git/info/sparse-checkout &&\n+\tgit -C bad-patterns sparse-checkout add dir\n '\n \n test_expect_success 'interaction with clone --no-checkout (unborn index)' '\n@@ -182,9 +185,9 @@ test_expect_success 'set sparse-checkout using --stdin' '\n test_expect_success 'add to sparse-checkout' '\n \tcat repo/.git/info/sparse-checkout >expect &&\n \tcat >add <<-\\EOF &&\n-\tpattern1\n+\t/pattern1\n \t/folder1/\n-\tpattern2\n+\t/pattern2\n \tEOF\n \tcat add >>expect &&\n \tgit -C repo sparse-checkout add --stdin From: Derrick Stolee \n>\n> Add a test to t1091-sparse-checkout-builtin.sh that would result in an\n> infinite loop and out-of-memory error before this change. The issue\n> relies on having non-cone-mode patterns while trying to modify the\n> patterns in cone-mode.\n>\n> The fix is simple, allowing us to break from the loop when the input\n> path does not contain a slash, as the \"dir\" pattern we added does not.\n>\n> This is only a fix to the critical out-of-memory error. A better\n> response to such a strange state will follow in a later change.\n>\n> Reported-by: Calbabreaker \n> Helped-by: Taylor Blau \n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/sparse-checkout.c | 2 +-\n> t/t1091-sparse-checkout-builtin.sh | 8 ++++++++\n> 2 files changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 8ba9f13787b..b45fd97a98b 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -389,7 +389,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n> \t\tchar *oldpattern = e->pattern;\n> \t\tsize_t newlen;\n>\n> -\t\tif (slash == e->pattern)\n> +\t\tif (!slash || slash == e->pattern)\n> \t\t\tbreak;\n\nIf we are preparing to make it so that we do not blindly copy patterns\nfrom a sparse checkout without cone mode enabled, then wouldn't this new\ncase be a BUG()?\n\nOf course, users could still tweak the contents of their sparse-checkout\nfile as they wish, but I'd expect that we'd catch those cases, too\n(i.e., by validating the contents of the existing sparse-checkout before\ncalling this function).\n\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 38fc8340f5c..a429d2cc671 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -103,6 +103,14 @@ test_expect_success 'clone --sparse' '\n> \tcheck_files clone a\n> '\n>\n> +test_expect_success 'switching to cone mode with non-cone mode patterns' '\n> +\tgit init bad-patterns &&\n> +\tgit -C bad-patterns sparse-checkout init &&\n> +\tgit -C bad-patterns sparse-checkout add dir &&\n> +\tgit -C bad-patterns config core.sparseCheckoutCone true &&\n\nMakes sense that we'd want to update the config rather than call \"init\n--cone\" here, since a subsequent patch would change the thing that this\nis testing (from \"doesn't OOM in the above-described situation\" to\n\"clears the existing contents of your sparse-checkout\").\n\n> +\tgit -C bad-patterns sparse-checkout add dir\n> +'\n> +\n\nIn other series I've suggested a cosmetic change to move all of these to\na sub-shell that begins with \"cd bad-patterns &&\", but obviously that is\na relatively unimportant suggestion.\n\nThanks,\nTaylor\n"},{"id":"436529","messageId":"YUjYZNqvvxVRctJQ@nand.local","threadId":"56551","inReplyTo":"450b90dad57c42e37a4edc52dac88caf98021fc6.1632160658.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] sparse-checkout: clear patterns when switching modes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T18:52:20Z","receivedAt":"2021-09-21T02:57:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 05:57:37PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> Previously, when a user runs 'git sparse-checkout init --cone', the\n> existing patterns would remain, even if the patterns were not in cone\n> mode. This causes confusion as to how the patterns should work when\n> later 'git sparse-checkout add' commands append to the pattern list.\n>\n> In fact, the way these patterns were modified was not even a strict\n> appending of patterns, but mutating and reordering patterns because of\n> how the paths were interpreted and rewritten.\n>\n> As a first step, we shall start overwriting the pattern set completely\n> when switching to cone mode, unless the existing patterns already match\n> cone mode patterns. The 'use_cone_patterns' member is set to false if\n> the patterns fail to parse using cone mode restrictions.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/sparse-checkout.c | 13 +++++++++++--\n> t/t1091-sparse-checkout-builtin.sh | 9 ++++++++-\n> 2 files changed, 19 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index b45fd97a98b..fe76c3eedda 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -348,8 +348,17 @@ static int sparse_checkout_init(int argc, const char **argv)\n>\n> \t/* If we already have a sparse-checkout file, use it. */\n> \tif (res >= 0) {\n> -\t\tfree(sparse_filename);\n> -\t\treturn update_working_directory(NULL);\n> +\t\tif (pl.use_cone_patterns || !init_opts.cone_mode) {\n\nI traced through this code beginning with sparse_checkout_init() and\nright before the check on pl.use_cone_patterns, and I couldn't find\nanywhere that this variable is set to a non-zero value. Could you let me\nknow if I'm missing something, or is the left-hand side of this or\nredundant?\n\nI guess what this check wants to be saying is \"if the existing\nsparse-checkout was in cone mode or we are transitioning to cone mode,\nthen quit now\".\n\n> +\t\t\tfree(sparse_filename);\n> +\t\t\treturn update_working_directory(NULL);\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * At this point, note that if res >= 0 but pl.use_cone_patterns\n> +\t\t * is false, then we want to override the patterns with the\n> +\t\t * initial set of cone mode patterns.\n> +\t\t */\n> +\t\tclear_pattern_list(&pl);\n\n...or otherwise, we are transitioning into cone mode from a non-cone\nmode state?\n\nIf so, this may be somewhat surprising to users who have their patterns\ncleared after re-initializing. I guess the command is called \"init\", but\nit may be friendlier to have a `--reinitialize` option or similar which\nindicates the user's preference to obliterate existing patterns if\nnecessary.\n\nSee my message at [1] for some more details about a possible suggestion\nthere.\n\n> \t}\n>\n> \tif (get_oid(\"HEAD\", &oid)) {\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index a429d2cc671..af0acd32bd9 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -108,7 +108,14 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '\n> \tgit -C bad-patterns sparse-checkout init &&\n> \tgit -C bad-patterns sparse-checkout add dir &&\n> \tgit -C bad-patterns config core.sparseCheckoutCone true &&\n> -\tgit -C bad-patterns sparse-checkout add dir\n> +\tgit -C bad-patterns sparse-checkout add dir &&\n> +\n> +\tgit -C bad-patterns sparse-checkout init --cone &&\n> +\tcat >expect <<-\\EOF &&\n> +\t/*\n> +\t!/*/\n> +\tEOF\n> +\ttest_cmp expect bad-patterns/.git/info/sparse-checkout\n\n\nMakes sense that we have to look at the contents of\n$GIT_DIR/info/sparse-checkout directly here to see the explicit '/*' and\n'!/*/' patterns.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YUi55%2F3L9nizTVyA@nand.local/\n"},{"id":"436530","messageId":"YUjYyY+1IXOJYvgk@nand.local","threadId":"56551","inReplyTo":"YUjYZNqvvxVRctJQ@nand.local","subject":"Re: [PATCH 2/3] sparse-checkout: clear patterns when switching modes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T18:54:01Z","receivedAt":"2021-09-21T02:57:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 02:52:20PM -0400, Taylor Blau wrote:\n> See my message at [1] for some more details about a possible suggestion\n> there.\n\nOK, I see that in the third patch we do start complaining about adding\npatterns in cone-mode when the existing sparse checkout configuration\ndoes not form a cone.\n\nSo that may indicate that we expect users who wish to blow away their\nconfiguration to just run \"init --cone\". I'd be OK with that (without an\nadditional warning step, although I think that doing that would be\nbetter). But either way, we should document this behavior clearly.\n\n> [1]: https://lore.kernel.org/git/YUi55%2F3L9nizTVyA@nand.local/\n\nThanks,\nTaylor\n"},{"id":"436531","messageId":"YUjaEGtts6R6b/wK@nand.local","threadId":"56551","inReplyTo":"d513b28b75189d066f9c66de44a1a578cbc38139.1632160658.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] sparse-checkout: refuse to add to bad patterns","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T18:59:28Z","receivedAt":"2021-09-21T02:57:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 05:57:38PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> When in cone mode sparse-checkout, it is unclear how 'git\n> sparse-checkout add ...' should behave if the existing\n> sparse-checkout file does not match the cone mode patterns. Change the\n> behavior to fail with an error message about the existing patterns.\n>\n> Also, all cone mode patterns start with a '/' character, so add that\n> restriction. This is necessary for our example test 'cone mode: warn on\n> bad pattern', but also requires modifying the example sparse-checkout\n> file we use to test the warnings related to recognizing cone mode\n> patterns.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/sparse-checkout.c | 3 +++\n> dir.c | 2 +-\n> t/t1091-sparse-checkout-builtin.sh | 11 +++++++----\n> 3 files changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index fe76c3eedda..2492ae828a9 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -513,6 +513,9 @@ static void add_patterns_cone_mode(int argc, const char **argv,\n> \t\tdie(_(\"unable to load existing sparse-checkout patterns\"));\n> \tfree(sparse_filename);\n>\n> +\tif (!existing.use_cone_patterns)\n> +\t\tdie(_(\"existing sparse-checkout patterns do not use cone mode\"));\n> +\n\nMakes sense.\n\n> \thashmap_for_each_entry(&existing.recursive_hashmap, &iter, pe, ent) {\n> \t\tif (!hashmap_contains_parent(&pl->recursive_hashmap,\n> \t\t\t\t\tpe->pattern, &buffer) ||\n> diff --git a/dir.c b/dir.c\n> index 03c4d212672..93136442103 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -727,7 +727,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n> \t}\n>\n> \tif (given->patternlen < 2 ||\n> -\t *given->pattern == '*' ||\n> +\t *given->pattern != '/' ||\n\nMakes sense that we require cone-mode patterns to start with '/', which\nnecessarily means we want to drop the other arm `*given->pattern == '*'`.\n\n> \t strstr(given->pattern, \"**\")) {\n> \t\t/* Not a cone pattern. */\n> \t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index af0acd32bd9..780c6a1aaae 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -108,14 +108,17 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '\n> \tgit -C bad-patterns sparse-checkout init &&\n> \tgit -C bad-patterns sparse-checkout add dir &&\n> \tgit -C bad-patterns config core.sparseCheckoutCone true &&\n> -\tgit -C bad-patterns sparse-checkout add dir &&\n> +\n> +\ttest_must_fail git -C bad-patterns sparse-checkout add dir 2>err &&\n> +\tgrep \"existing sparse-checkout patterns do not use cone mode\" err &&\n>\n> \tgit -C bad-patterns sparse-checkout init --cone &&\n> \tcat >expect <<-\\EOF &&\n> \t/*\n> \t!/*/\n> \tEOF\n> -\ttest_cmp expect bad-patterns/.git/info/sparse-checkout\n> +\ttest_cmp expect bad-patterns/.git/info/sparse-checkout &&\n> +\tgit -C bad-patterns sparse-checkout add dir\n\nGood thinking to make sure that we can add `dir` after clearing. But\nlet's check the contents of the sparse-checkout file to make sure that\nit was added in cone mode.\n\n> '\n>\n> test_expect_success 'interaction with clone --no-checkout (unborn index)' '\n> @@ -182,9 +185,9 @@ test_expect_success 'set sparse-checkout using --stdin' '\n> test_expect_success 'add to sparse-checkout' '\n> \tcat repo/.git/info/sparse-checkout >expect &&\n> \tcat >add <<-\\EOF &&\n> -\tpattern1\n> +\t/pattern1\n> \t/folder1/\n> -\tpattern2\n> +\t/pattern2\n> \tEOF\n> \tcat add >>expect &&\n> \tgit -C repo sparse-checkout add --stdin On Mon, Sep 20, 2021 at 05:57:36PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> Add a test to t1091-sparse-checkout-builtin.sh that would result in an\n>> infinite loop and out-of-memory error before this change. The issue\n>> relies on having non-cone-mode patterns while trying to modify the\n>> patterns in cone-mode.\n>>\n>> The fix is simple, allowing us to break from the loop when the input\n>> path does not contain a slash, as the \"dir\" pattern we added does not.\n>>\n>> This is only a fix to the critical out-of-memory error. A better\n>> response to such a strange state will follow in a later change.\n>>\n>> Reported-by: Calbabreaker \n>> Helped-by: Taylor Blau \n>> Signed-off-by: Derrick Stolee \n>> ---\n>> builtin/sparse-checkout.c | 2 +-\n>> t/t1091-sparse-checkout-builtin.sh | 8 ++++++++\n>> 2 files changed, 9 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n>> index 8ba9f13787b..b45fd97a98b 100644\n>> --- a/builtin/sparse-checkout.c\n>> +++ b/builtin/sparse-checkout.c\n>> @@ -389,7 +389,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n>> \t\tchar *oldpattern = e->pattern;\n>> \t\tsize_t newlen;\n>>\n>> -\t\tif (slash == e->pattern)\n>> +\t\tif (!slash || slash == e->pattern)\n>> \t\t\tbreak;\n> \n> If we are preparing to make it so that we do not blindly copy patterns\n> from a sparse checkout without cone mode enabled, then wouldn't this new\n> case be a BUG()?\n> \n> Of course, users could still tweak the contents of their sparse-checkout\n> file as they wish, but I'd expect that we'd catch those cases, too\n> (i.e., by validating the contents of the existing sparse-checkout before\n> calling this function).\n\nIf I was more confident that we were catching absolutely every possible\ncase of non-cone mode patterns in our parsing logic, then I suppose a BUG()\ncould apply here. At minimum, at this point in time (before fixing the gap\nin parsing in patch 3) the test below would not even work with test_must_fail,\nsince the exit code would be unexpected.\n \n>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n>> index 38fc8340f5c..a429d2cc671 100755\n>> --- a/t/t1091-sparse-checkout-builtin.sh\n>> +++ b/t/t1091-sparse-checkout-builtin.sh\n>> @@ -103,6 +103,14 @@ test_expect_success 'clone --sparse' '\n>> \tcheck_files clone a\n>> '\n>>\n>> +test_expect_success 'switching to cone mode with non-cone mode patterns' '\n>> +\tgit init bad-patterns &&\n>> +\tgit -C bad-patterns sparse-checkout init &&\n>> +\tgit -C bad-patterns sparse-checkout add dir &&\n>> +\tgit -C bad-patterns config core.sparseCheckoutCone true &&\n> \n> Makes sense that we'd want to update the config rather than call \"init\n> --cone\" here, since a subsequent patch would change the thing that this\n> is testing (from \"doesn't OOM in the above-described situation\" to\n> \"clears the existing contents of your sparse-checkout\").\n>\n>> +\tgit -C bad-patterns sparse-checkout add dir\n>> +'\n>> +\n> \n> In other series I've suggested a cosmetic change to move all of these to\n> a sub-shell that begins with \"cd bad-patterns &&\", but obviously that is\n> a relatively unimportant suggestion.\n\nThe only defense I have for not using a subshell and 'cd' is that later\nI can use an \"expect\" file in my current directory without it being \"in\"\nthe repository. It doesn't really matter for this example, but it has in\nthe past, causing me to do this by habit. A tab is smaller than the string\n\" -C bad-patterns\", so it's probably worth changing.\n\nThanks,\n-Stolee\n"},{"id":"436633","messageId":"YUoJxgiWcl/Wcqce@nand.local","threadId":"56551","inReplyTo":"6d565e1d-a31f-7cf0-9c10-a01a87fead41@gmail.com","subject":"Re: [PATCH 1/3] sparse-checkout: fix OOM error with mixed patterns","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T16:35:18Z","receivedAt":"2021-09-21T16:35:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 09:06:59AM -0400, Derrick Stolee wrote:\n> > If we are preparing to make it so that we do not blindly copy patterns\n> > from a sparse checkout without cone mode enabled, then wouldn't this new\n> > case be a BUG()?\n> >\n> > Of course, users could still tweak the contents of their sparse-checkout\n> > file as they wish, but I'd expect that we'd catch those cases, too\n> > (i.e., by validating the contents of the existing sparse-checkout before\n> > calling this function).\n>\n> If I was more confident that we were catching absolutely every possible\n> case of non-cone mode patterns in our parsing logic, then I suppose a BUG()\n> could apply here. At minimum, at this point in time (before fixing the gap\n> in parsing in patch 3) the test below would not even work with test_must_fail,\n> since the exit code would be unexpected.\n\nRight, but it could be a test_must_fail after the second patch, no?\n\nNot calling BUG() is fine with me if you think there may be other cases\nwe haven't discovered. But we should have some way to discover them\ninstead if a user can generate them organically. Maybe a warning()?\n\n> >> +\tgit -C bad-patterns sparse-checkout add dir\n> >> +'\n> >> +\n> >\n> > In other series I've suggested a cosmetic change to move all of these to\n> > a sub-shell that begins with \"cd bad-patterns &&\", but obviously that is\n> > a relatively unimportant suggestion.\n>\n> The only defense I have for not using a subshell and 'cd' is that later\n> I can use an \"expect\" file in my current directory without it being \"in\"\n> the repository. It doesn't really matter for this example, but it has in\n> the past, causing me to do this by habit. A tab is smaller than the string\n> \" -C bad-patterns\", so it's probably worth changing.\n\nYeah. I admit to hardly caring about this (subshell vs. '-C\nbad-patterns') at all. Either is completely fine with me.\n\nThanks,\nTaylor\n"}]}