* [PATCH] dir: check for single file cone patterns @ 2022-12-20 14:27 William Sprent via GitGitGadget 2022-12-20 14:33 ` Ævar Arnfjörð Bjarmason 2023-01-03 8:20 ` [PATCH v2] " William Sprent via GitGitGadget 0 siblings, 2 replies; 8+ messages in thread From: William Sprent via GitGitGadget @ 2022-12-20 14:27 UTC (permalink / raw) To: git; +Cc: William Sprent, William Sprent From: William Sprent <williams@unity3d.com> The sparse checkout documentation states that the cone mode pattern set is limited to patterns that either recursively include directories or patterns that match all files in a directory. In the sparse checkout file, the former manifest in the form: /A/B/C/ while the latter become a pair of patterns either in the form: /A/B/ !/A/B/*/ or in the special case of matching the toplevel files: /* !/*/ The 'add_pattern_to_hashsets()' function contains checks which serve to disable cone-mode when non-cone patterns are encountered. However, these do not catch when the pattern list attempts to match a single file or directory, e.g. a pattern in the form: /A/B/C This causes sparse-checkout to exhibit unexpected behaviour when such a pattern is in the sparse-checkout file and cone mode is enabled. Concretely, with the pattern like the above, sparse-checkout, in non-cone mode, will only include the directory or file located at '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead just manifest the toplevel files but not any file located at '/A/B/C'. Relatedly, issues occur when supplying the same kind of filter when partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will correctly just include the objects that match the non-cone pattern matching. Which means that checking out the newly cloned repo with the same filter, but with cone mode enabled, fails due to missing objects. To fix these issues, add a cone mode pattern check that asserts that every pattern is either a directory match or the pattern '/*'. Add a test to verify the new pattern check and modify another to reflect that non-directory patterns are caught earlier. Signed-off-by: William Sprent <williams@unity3d.com> --- dir: check for single file cone patterns I ran into this confusing behaviour when working with sparse-checkout patterns. If my interpretation of the documentation is correct, then I think comes from git not being strict enough when checking cone mode patterns. Thanks, William Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1446 dir.c | 7 +++++++ t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/dir.c b/dir.c index fbdb24fc819..a122aeb2aa3 100644 --- a/dir.c +++ b/dir.c @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern goto clear_hashmaps; } + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && + !!strcmp(given->pattern, "/*")) { + /* Not a cone pattern. */ + warning(_("unrecognized pattern: '%s'"), given->pattern); + goto clear_hashmaps; + } + prev = given->pattern; cur = given->pattern + 1; next = given->pattern + 2; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index b563d6c263e..627267be153 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' ' test_expect_success 'cone mode: warn on bad pattern' ' test_when_finished mv sparse-checkout repo/.git/info/ && cp repo/.git/info/sparse-checkout . && - echo "!/deep/deeper/*" >>repo/.git/info/sparse-checkout && + echo "!/deep/deeper/*/" >>repo/.git/info/sparse-checkout && git -C repo read-tree -mu HEAD 2>err && test_i18ngrep "unrecognized negative pattern" err ' @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting "*"' ' check_read_tree_errors repo "a deep" "disabling cone pattern matching" ' +test_expect_success 'pattern-checks: non directory pattern' ' + cat >repo/.git/info/sparse-checkout <<-\EOF && + /deep/deeper1/a + EOF + check_read_tree_errors repo deep "disabling cone pattern matching" && + check_files repo/deep deeper1 && + check_files repo/deep/deeper1 a +' + test_expect_success 'pattern-checks: contained glob characters' ' for c in "[a]" "\\" "?" "*" do base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7 -- gitgitgadget ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] dir: check for single file cone patterns 2022-12-20 14:27 [PATCH] dir: check for single file cone patterns William Sprent via GitGitGadget @ 2022-12-20 14:33 ` Ævar Arnfjörð Bjarmason 2022-12-21 12:51 ` William Sprent 2023-01-03 8:20 ` [PATCH v2] " William Sprent via GitGitGadget 1 sibling, 1 reply; 8+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-12-20 14:33 UTC (permalink / raw) To: William Sprent via GitGitGadget; +Cc: git, William Sprent On Tue, Dec 20 2022, William Sprent via GitGitGadget wrote: > From: William Sprent <williams@unity3d.com> > [...] > + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && > + !!strcmp(given->pattern, "/*")) { Style: We use !! to convert 123 to 1 or whatever, but here we just care about "not matched", so we can drop this !!strcmp() for strcmp(). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dir: check for single file cone patterns 2022-12-20 14:33 ` Ævar Arnfjörð Bjarmason @ 2022-12-21 12:51 ` William Sprent 0 siblings, 0 replies; 8+ messages in thread From: William Sprent @ 2022-12-21 12:51 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, William Sprent via GitGitGadget; +Cc: git On 20/12/2022 15.33, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Dec 20 2022, William Sprent via GitGitGadget wrote: > >> From: William Sprent <williams@unity3d.com> >> [...] >> + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && >> + !!strcmp(given->pattern, "/*")) { > > Style: We use !! to convert 123 to 1 or whatever, but here we just care > about "not matched", so we can drop this !!strcmp() for strcmp(). Thanks. I'll drop the '!!' for next version. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] dir: check for single file cone patterns 2022-12-20 14:27 [PATCH] dir: check for single file cone patterns William Sprent via GitGitGadget 2022-12-20 14:33 ` Ævar Arnfjörð Bjarmason @ 2023-01-03 8:20 ` William Sprent via GitGitGadget 2023-01-04 6:07 ` Junio C Hamano 2023-01-04 23:48 ` Victoria Dye 1 sibling, 2 replies; 8+ messages in thread From: William Sprent via GitGitGadget @ 2023-01-03 8:20 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason, William Sprent, William Sprent From: William Sprent <williams@unity3d.com> The sparse checkout documentation states that the cone mode pattern set is limited to patterns that either recursively include directories or patterns that match all files in a directory. In the sparse checkout file, the former manifest in the form: /A/B/C/ while the latter become a pair of patterns either in the form: /A/B/ !/A/B/*/ or in the special case of matching the toplevel files: /* !/*/ The 'add_pattern_to_hashsets()' function contains checks which serve to disable cone-mode when non-cone patterns are encountered. However, these do not catch when the pattern list attempts to match a single file or directory, e.g. a pattern in the form: /A/B/C This causes sparse-checkout to exhibit unexpected behaviour when such a pattern is in the sparse-checkout file and cone mode is enabled. Concretely, with the pattern like the above, sparse-checkout, in non-cone mode, will only include the directory or file located at '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead just manifest the toplevel files but not any file located at '/A/B/C'. Relatedly, issues occur when supplying the same kind of filter when partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will correctly just include the objects that match the non-cone pattern matching. Which means that checking out the newly cloned repo with the same filter, but with cone mode enabled, fails due to missing objects. To fix these issues, add a cone mode pattern check that asserts that every pattern is either a directory match or the pattern '/*'. Add a test to verify the new pattern check and modify another to reflect that non-directory patterns are caught earlier. Signed-off-by: William Sprent <williams@unity3d.com> --- dir: check for single file cone patterns Resubmitting this without the superfluous double negation of the 'strcmp' output. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1446 Range-diff vs v1: 1: cfb4b75c378 ! 1: d5406e62f6f dir: check for single file cone patterns @@ dir.c: static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_ } + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && -+ !!strcmp(given->pattern, "/*")) { ++ strcmp(given->pattern, "/*")) { + /* Not a cone pattern. */ + warning(_("unrecognized pattern: '%s'"), given->pattern); + goto clear_hashmaps; dir.c | 7 +++++++ t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/dir.c b/dir.c index fbdb24fc819..4e99f0c868f 100644 --- a/dir.c +++ b/dir.c @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern goto clear_hashmaps; } + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && + strcmp(given->pattern, "/*")) { + /* Not a cone pattern. */ + warning(_("unrecognized pattern: '%s'"), given->pattern); + goto clear_hashmaps; + } + prev = given->pattern; cur = given->pattern + 1; next = given->pattern + 2; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index b563d6c263e..627267be153 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' ' test_expect_success 'cone mode: warn on bad pattern' ' test_when_finished mv sparse-checkout repo/.git/info/ && cp repo/.git/info/sparse-checkout . && - echo "!/deep/deeper/*" >>repo/.git/info/sparse-checkout && + echo "!/deep/deeper/*/" >>repo/.git/info/sparse-checkout && git -C repo read-tree -mu HEAD 2>err && test_i18ngrep "unrecognized negative pattern" err ' @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting "*"' ' check_read_tree_errors repo "a deep" "disabling cone pattern matching" ' +test_expect_success 'pattern-checks: non directory pattern' ' + cat >repo/.git/info/sparse-checkout <<-\EOF && + /deep/deeper1/a + EOF + check_read_tree_errors repo deep "disabling cone pattern matching" && + check_files repo/deep deeper1 && + check_files repo/deep/deeper1 a +' + test_expect_success 'pattern-checks: contained glob characters' ' for c in "[a]" "\\" "?" "*" do base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7 -- gitgitgadget ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] dir: check for single file cone patterns 2023-01-03 8:20 ` [PATCH v2] " William Sprent via GitGitGadget @ 2023-01-04 6:07 ` Junio C Hamano 2023-01-04 23:48 ` Victoria Dye 1 sibling, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2023-01-04 6:07 UTC (permalink / raw) To: Derrick Stolee, Elijah Newren, Victoria Dye Cc: git, Ævar Arnfjörð Bjarmason, William Sprent, William Sprent via GitGitGadget "William Sprent via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: William Sprent <williams@unity3d.com> > > The sparse checkout documentation states that the cone mode pattern set > is limited to patterns that either recursively include directories or > patterns that match all files in a directory. In the sparse checkout > file, the former manifest in the form: > > /A/B/C/ > > while the latter become a pair of patterns either in the form: > > /A/B/ > !/A/B/*/ > > or in the special case of matching the toplevel files: > > /* > !/*/ > > The 'add_pattern_to_hashsets()' function contains checks which serve to > disable cone-mode when non-cone patterns are encountered. However, these > do not catch when the pattern list attempts to match a single file or > directory, e.g. a pattern in the form: > > /A/B/C > > This causes sparse-checkout to exhibit unexpected behaviour when such a > pattern is in the sparse-checkout file and cone mode is enabled. > Concretely, with the pattern like the above, sparse-checkout, in > non-cone mode, will only include the directory or file located at > '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead > just manifest the toplevel files but not any file located at '/A/B/C'. > > Relatedly, issues occur when supplying the same kind of filter when > partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will > correctly just include the objects that match the non-cone pattern > matching. Which means that checking out the newly cloned repo with the > same filter, but with cone mode enabled, fails due to missing objects. > > To fix these issues, add a cone mode pattern check that asserts that > every pattern is either a directory match or the pattern '/*'. Add a > test to verify the new pattern check and modify another to reflect that > non-directory patterns are caught earlier. > > Signed-off-by: William Sprent <williams@unity3d.com> > --- Summoning those who worked in the area, which includes area experts, for further comments, picked out of output from $ git shortlog --no-merges --grep=cone --since=2.years -s -n -e > diff --git a/dir.c b/dir.c > index fbdb24fc819..4e99f0c868f 100644 > --- a/dir.c > +++ b/dir.c > @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern > goto clear_hashmaps; > } > > + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && > + strcmp(given->pattern, "/*")) { > + /* Not a cone pattern. */ > + warning(_("unrecognized pattern: '%s'"), given->pattern); > + goto clear_hashmaps; > + } > + > prev = given->pattern; > cur = given->pattern + 1; > next = given->pattern + 2; > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index b563d6c263e..627267be153 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' ' > test_expect_success 'cone mode: warn on bad pattern' ' > test_when_finished mv sparse-checkout repo/.git/info/ && > cp repo/.git/info/sparse-checkout . && > - echo "!/deep/deeper/*" >>repo/.git/info/sparse-checkout && > + echo "!/deep/deeper/*/" >>repo/.git/info/sparse-checkout && > git -C repo read-tree -mu HEAD 2>err && > test_i18ngrep "unrecognized negative pattern" err > ' > @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting "*"' ' > check_read_tree_errors repo "a deep" "disabling cone pattern matching" > ' > > +test_expect_success 'pattern-checks: non directory pattern' ' > + cat >repo/.git/info/sparse-checkout <<-\EOF && > + /deep/deeper1/a > + EOF > + check_read_tree_errors repo deep "disabling cone pattern matching" && > + check_files repo/deep deeper1 && > + check_files repo/deep/deeper1 a > +' > + > test_expect_success 'pattern-checks: contained glob characters' ' > for c in "[a]" "\\" "?" "*" > do > > base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] dir: check for single file cone patterns 2023-01-03 8:20 ` [PATCH v2] " William Sprent via GitGitGadget 2023-01-04 6:07 ` Junio C Hamano @ 2023-01-04 23:48 ` Victoria Dye 2023-01-05 2:16 ` Junio C Hamano 2023-01-05 11:39 ` William Sprent 1 sibling, 2 replies; 8+ messages in thread From: Victoria Dye @ 2023-01-04 23:48 UTC (permalink / raw) To: William Sprent via GitGitGadget, git Cc: Ævar Arnfjörð Bjarmason, William Sprent, Derrick Stolee, Elijah Newren, Junio C Hamano William Sprent via GitGitGadget wrote: > From: William Sprent <williams@unity3d.com> > > The sparse checkout documentation states that the cone mode pattern set > is limited to patterns that either recursively include directories or > patterns that match all files in a directory. In the sparse checkout > file, the former manifest in the form: > > /A/B/C/ > > while the latter become a pair of patterns either in the form: > > /A/B/ > !/A/B/*/ > > or in the special case of matching the toplevel files: > > /* > !/*/ > > The 'add_pattern_to_hashsets()' function contains checks which serve to > disable cone-mode when non-cone patterns are encountered. However, these > do not catch when the pattern list attempts to match a single file or > directory, e.g. a pattern in the form: > > /A/B/C > > This causes sparse-checkout to exhibit unexpected behaviour when such a > pattern is in the sparse-checkout file and cone mode is enabled. > Concretely, with the pattern like the above, sparse-checkout, in > non-cone mode, will only include the directory or file located at > '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead > just manifest the toplevel files but not any file located at '/A/B/C'. > > Relatedly, issues occur when supplying the same kind of filter when > partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will > correctly just include the objects that match the non-cone pattern > matching. Which means that checking out the newly cloned repo with the > same filter, but with cone mode enabled, fails due to missing objects. This clearly explains the issue you're trying to fix: cone mode sparse-checkout requires patterns like '/A/B/', !/A/B/*/', '/A/B/C/', etc., but invalid patterns like '/A/B/C' (no trailing slash) currently don't force the switch to non-cone mode, leading to unexpected behavior. > > To fix these issues, add a cone mode pattern check that asserts that > every pattern is either a directory match or the pattern '/*'. Add a > test to verify the new pattern check and modify another to reflect that > non-directory patterns are caught earlier. I think this is the best way to maintain the current intended behavior of cone mode sparse-checkout. While the idea of single file "exceptions" to cone patterns has been brought up in the past (I think most recently at the Contributor's Summit this past September [1]), it'd be a substantial change that's definitely out-of-scope of this small bugfix. [1] https://lore.kernel.org/git/YzXwOsaoCdBhHsX1@nand.local/ > > Signed-off-by: William Sprent <williams@unity3d.com> > --- > dir: check for single file cone patterns > > Resubmitting this without the superfluous double negation of the > 'strcmp' output. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1446 > > Range-diff vs v1: > > 1: cfb4b75c378 ! 1: d5406e62f6f dir: check for single file cone patterns > @@ dir.c: static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_ > } > > + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && > -+ !!strcmp(given->pattern, "/*")) { > ++ strcmp(given->pattern, "/*")) { > + /* Not a cone pattern. */ > + warning(_("unrecognized pattern: '%s'"), given->pattern); > + goto clear_hashmaps; > > > dir.c | 7 +++++++ > t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/dir.c b/dir.c > index fbdb24fc819..4e99f0c868f 100644 > --- a/dir.c > +++ b/dir.c > @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern > goto clear_hashmaps; > } > > + if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) && > + strcmp(given->pattern, "/*")) { This condition is only entered if: - the pattern *does not* end in a trailing slash ('!(given->flags & PATTERN_FLAG_MUSTBEDIR)'), AND - the pattern *does not* start with '/*' ('strcmp(given->pattern, "/*")') The '/*'-like patterns are already handled by other parts of 'add_pattern_to_hashsets()', so only the '/A/B/C'-like patterns will trigger the "unrecognized" warning. Looks good! > + /* Not a cone pattern. */ > + warning(_("unrecognized pattern: '%s'"), given->pattern); > + goto clear_hashmaps; > + } > + > prev = given->pattern; > cur = given->pattern + 1; > next = given->pattern + 2; > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index b563d6c263e..627267be153 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' ' > test_expect_success 'cone mode: warn on bad pattern' ' > test_when_finished mv sparse-checkout repo/.git/info/ && > cp repo/.git/info/sparse-checkout . && > - echo "!/deep/deeper/*" >>repo/.git/info/sparse-checkout && > + echo "!/deep/deeper/*/" >>repo/.git/info/sparse-checkout && This test changes because, without the trailing slash on the pattern, the error would become "unrecognized pattern: !/deep/deeper/*" due to the new check in 'add_pattern_to_hashsets()'. Changing the test pattern ensures we're still exercising what the test was originally intended to exercise, and doesn't indicate a regression. > git -C repo read-tree -mu HEAD 2>err && > test_i18ngrep "unrecognized negative pattern" err > ' > @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting "*"' ' > check_read_tree_errors repo "a deep" "disabling cone pattern matching" > ' > > +test_expect_success 'pattern-checks: non directory pattern' ' > + cat >repo/.git/info/sparse-checkout <<-\EOF && > + /deep/deeper1/a > + EOF > + check_read_tree_errors repo deep "disabling cone pattern matching" && > + check_files repo/deep deeper1 && > + check_files repo/deep/deeper1 a > +' And this test ensures the new check is working for the appropriate patterns. This patch looks good to me, thanks for finding & fixing the bug! > + > test_expect_success 'pattern-checks: contained glob characters' ' > for c in "[a]" "\\" "?" "*" > do > > base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] dir: check for single file cone patterns 2023-01-04 23:48 ` Victoria Dye @ 2023-01-05 2:16 ` Junio C Hamano 2023-01-05 11:39 ` William Sprent 1 sibling, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2023-01-05 2:16 UTC (permalink / raw) To: Victoria Dye Cc: William Sprent via GitGitGadget, git, Ævar Arnfjörð Bjarmason, William Sprent, Derrick Stolee, Elijah Newren Victoria Dye <vdye@github.com> writes: > This clearly explains the issue you're trying to fix: cone mode > sparse-checkout requires patterns like '/A/B/', !/A/B/*/', '/A/B/C/', etc., > but invalid patterns like '/A/B/C' (no trailing slash) currently don't force > the switch to non-cone mode, leading to unexpected behavior. > ... >> To fix these issues, add a cone mode pattern check that asserts that >> every pattern is either a directory match or the pattern '/*'. Add a >> test to verify the new pattern check and modify another to reflect that >> non-directory patterns are caught earlier. > > I think this is the best way to maintain the current intended behavior of > cone mode sparse-checkout. While the idea of single file "exceptions" to > cone patterns has been brought up in the past (I think most recently at the > Contributor's Summit this past September [1]), it'd be a substantial change > that's definitely out-of-scope of this small bugfix. > ... > And this test ensures the new check is working for the appropriate patterns. > > This patch looks good to me, thanks for finding & fixing the bug! Thanks for an excellently written review, and a patch that is well done. Will queue. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] dir: check for single file cone patterns 2023-01-04 23:48 ` Victoria Dye 2023-01-05 2:16 ` Junio C Hamano @ 2023-01-05 11:39 ` William Sprent 1 sibling, 0 replies; 8+ messages in thread From: William Sprent @ 2023-01-05 11:39 UTC (permalink / raw) To: Victoria Dye, William Sprent via GitGitGadget, git Cc: Ævar Arnfjörð Bjarmason, Derrick Stolee, Elijah Newren, Junio C Hamano On 05/01/2023 00.48, Victoria Dye wrote: > > I think this is the best way to maintain the current intended behavior of > cone mode sparse-checkout. While the idea of single file "exceptions" to > cone patterns has been brought up in the past (I think most recently at the > Contributor's Summit this past September [1]), it'd be a substantial change > that's definitely out-of-scope of this small bugfix. > > [1] https://lore.kernel.org/git/YzXwOsaoCdBhHsX1@nand.local/ > Thanks for the context. I didn't know that it had been discussed. >> >> ... >> > > And this test ensures the new check is working for the appropriate patterns. > > This patch looks good to me, thanks for finding & fixing the bug! > Thanks for taking the time to review! ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-05 11:39 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-20 14:27 [PATCH] dir: check for single file cone patterns William Sprent via GitGitGadget 2022-12-20 14:33 ` Ævar Arnfjörð Bjarmason 2022-12-21 12:51 ` William Sprent 2023-01-03 8:20 ` [PATCH v2] " William Sprent via GitGitGadget 2023-01-04 6:07 ` Junio C Hamano 2023-01-04 23:48 ` Victoria Dye 2023-01-05 2:16 ` Junio C Hamano 2023-01-05 11:39 ` William Sprent
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).