From 4aaf1a3ea238ee4049a30fd7950374a99e5808e8 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 8 Nov 2021 16:10:13 -0500 Subject: [PATCH 1/3] sparse-checkout: disable cone mode on init with invalid patterns When reusing an existing `.git/info/sparse-checkout` file in `git sparse-checkout init --cone`, warn user and fall back on non-cone mode sparse checkout if the sparse patterns are not cone mode-compatible. This prevents users from ending up with erroneous/corrupted patterns in their cone mode sparse-checkout (assuming no manual edits to the `.git/info/sparse-checkout` file). Additionally, by allowing the `git sparse-checkout init` to continue with cone mode disabled, the user is able to correct or reset their patterns (via `git sparse-checkout set`) and retry `git sparse-checkout init --cone` without requiring manual editing of `.git/info/sparse-checkout`. Signed-off-by: Victoria Dye --- builtin/sparse-checkout.c | 18 +++++++++++++++--- t/t1091-sparse-checkout-builtin.sh | 13 +++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 601cf9dc278d18..300341b8ca4369 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -428,14 +428,26 @@ static int sparse_checkout_init(int argc, const char **argv) } else mode = MODE_ALL_PATTERNS; - if (set_config(mode)) - return 1; - memset(&pl, 0, sizeof(pl)); + pl.use_cone_patterns = core_sparse_checkout_cone; sparse_filename = get_sparse_checkout_filename(); res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL, 0); + /* + * If res >= 0, file already exists. If in cone mode init, verify that the + * patterns are cone mode-compatible (if applicable). Otherwise, fall back + * on non-cone mode sparse checkout. + */ + if (res >= 0 && core_sparse_checkout_cone && !pl.use_cone_patterns) { + warning(_("unable to initialize from existing patterns; disabling cone mode")); + mode = MODE_ALL_PATTERNS; + core_sparse_checkout_cone = 0; + } + + if (set_config(mode)) + return 1; + if (init_opts.sparse_index >= 0) { if (set_sparse_index_config(the_repository, init_opts.sparse_index) < 0) die(_("failed to modify sparse-index config")); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 8f54e69502e2be..51b3bf598e3092 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -712,4 +712,17 @@ test_expect_success 'cone mode clears ignored subdirectories' ' test_cmp expect out ' +test_expect_success 'init with cone mode verifies existing cone patterns' ' + rm -f repo/.git/info/sparse-checkout && + + # Set non-cone mode pattern + git -C repo sparse-checkout init && + git -C repo sparse-checkout set deep/deeper*/ && + git -C repo sparse-checkout disable && + + git -C repo sparse-checkout init --cone 2>err && + test_i18ngrep "disabling cone mode" err && + test_must_fail git -C repo config core.sparsecheckoutcone +' + test_done From ffc78934d917005ca2d868630b85a0bdd0337dbb Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 9 Nov 2021 14:20:16 -0500 Subject: [PATCH 2/3] sparse-checkout: fail cone mode add with existing invalid patterns Add check for `git sparse-checkout add` when existing patterns are not cone mode-compatible, exiting when invalid patterns are found. When a non-cone mode pattern (e.g., one containing glob pattern matching) exists in the on-disk `.git/info/sparse-checkout` file, `git sparse-checkout add` would warn that some patterns were incompatible with cone mode, but otherwise succeed with exit code 0. However, the original (incompatible) patterns would have been removed in the process. For example: $ git sparse-checkout init $ git sparse-checkout add folder*/ $ git config core.sparsecheckoutcone true $ git sparse-checkout add different-folder/ would result in the patterns: /* !/*/ /different-folder/ To prevent the unintentional pattern removal, `git sparse-checkout add` exits with an error if incompatible patterns were found (and therefore not included in the hashmaps used to generate the output sparse pattern file). Signed-off-by: Victoria Dye --- builtin/sparse-checkout.c | 10 +++++++++- t/t1091-sparse-checkout-builtin.sh | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 300341b8ca4369..61bc430f761adb 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -613,13 +613,21 @@ static void add_patterns_cone_mode(int argc, const char **argv, add_patterns_from_input(pl, argc, argv); memset(&existing, 0, sizeof(existing)); - existing.use_cone_patterns = core_sparse_checkout_cone; + existing.use_cone_patterns = 1; if (add_patterns_from_file_to_list(sparse_filename, "", 0, &existing, NULL, 0)) die(_("unable to load existing sparse-checkout patterns")); free(sparse_filename); + /* + * If use_cone_patterns has been disabled, at least one of the existing + * patterns is invalid for cone mode. In that case, the hashmap does not + * correctly reflect patterns, so we must exit early. + */ + if (existing.use_cone_patterns == 0) + die(_("unable to use existing sparse-checkout patterns in cone mode")); + hashmap_for_each_entry(&existing.recursive_hashmap, &iter, pe, ent) { if (!hashmap_contains_parent(&pl->recursive_hashmap, pe->pattern, &buffer) || diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 51b3bf598e3092..e11a917b528f3f 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -725,4 +725,18 @@ test_expect_success 'init with cone mode verifies existing cone patterns' ' test_must_fail git -C repo config core.sparsecheckoutcone ' +test_expect_success 'add with cone mode verifies existing cone patterns' ' + rm -f repo/.git/info/sparse-checkout && + + git -C repo sparse-checkout init --cone && + cat >repo/.git/info/sparse-checkout <<-\EOF && + /* + !/*/ + /deep/deeper*/ + EOF + + test_must_fail git -C repo sparse-checkout add folder1 2>err && + test_i18ngrep "unable to use existing sparse-checkout patterns in cone mode" err +' + test_done From 95fe29ccc9fb5bde74ea6b236ff633cfa3b9ea18 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 9 Nov 2021 13:29:37 -0500 Subject: [PATCH 3/3] sparse-checkout: require starting '/' in cone mode patterns Add check for missing initial '/' when verifying cone patterns in `add_pattern_to_hashsets(...)`, disabling cone patterns if not found. Update tests in `t1091-sparse-checkout-builtin.sh` to ensure the starting '/' is present unless the test verifies behavior when it is missing. The implicit assumption of a starting '/' in cone mode patterns exists throughout the code paths of `git sparse-checkout`, including in the construction of the sparse pattern hashmaps. However, without enforcement of that assumption, the presence of a directory pattern missing the starting '/' would cause any subsequent cone mode `git sparse-checkout add` to enter an infinite recursive loop in `insert_recursive_pattern(...)` (`sparse-checkout.c`). By adding the starting '/' requirement to `add_pattern_to_hashsets(...)` (`dir.c`), cone mode is disabled on the pattern list, forcing `git sparse-checkout add` to exit early. Signed-off-by: Victoria Dye --- dir.c | 5 ++++ t/t1091-sparse-checkout-builtin.sh | 45 ++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index ed3b06da64eed9..fffefd669c9153 100644 --- a/dir.c +++ b/dir.c @@ -715,6 +715,11 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern if (!pl->use_cone_patterns) return; + if (*given->pattern != '/') { + warning(_("unrecognized pattern: '%s'"), given->pattern); + goto clear_hashmaps; + } + if (given->flags & PATTERN_FLAG_NEGATIVE && given->flags & PATTERN_FLAG_MUSTBEDIR && !strcmp(given->pattern, "/*")) { diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index e11a917b528f3f..2789b21c18d385 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -191,7 +191,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 ' @@ -573,7 +573,7 @@ test_expect_success 'pattern-checks: starting "*"' ' cat >repo/.git/info/sparse-checkout <<-\EOF && /* !/*/ - *eep/ + /*eep/ EOF check_read_tree_errors repo "a deep" "disabling cone pattern matching" ' @@ -584,12 +584,21 @@ test_expect_success 'pattern-checks: contained glob characters' ' cat >repo/.git/info/sparse-checkout <<-EOF && /* !/*/ - something$c-else/ + /something$c-else/ EOF check_read_tree_errors repo "a" "disabling cone pattern matching" done ' +test_expect_success 'pattern-checks: starting "/"' ' + cat >repo/.git/info/sparse-checkout <<-\EOF && + /* + !/*/ + deep/ + EOF + check_read_tree_errors repo "a deep" "disabling cone pattern matching" +' + test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' ' git clone repo escaped && TREEOID=$(git -C escaped rev-parse HEAD:folder1) && @@ -739,4 +748,34 @@ test_expect_success 'add with cone mode verifies existing cone patterns' ' test_i18ngrep "unable to use existing sparse-checkout patterns in cone mode" err ' +# NEEDSWORK: in the case of directory patterns like `deep/`, it might be worth trying +# to "correct" the patterns to match a cone mode style. However, that may be more difficult +# for nested directories (like `deep/deeper1/`) in which multiple individual patterns +# would be mapped from the original (`/deep/`, `!/deep/*/`, `/deep/deeper1/`). +test_expect_success 'add cone pattern disallowed with existing non-cone directory pattern' ' + rm -f repo/.git/info/sparse-checkout && + + git -C repo sparse-checkout init --cone && + + # Manually set the sparse checkout pattern to a directory pattern + # without preceding slash + cat >repo/.git/info/sparse-checkout <<-\EOF && + deep/ + EOF + + # `add` fails because `deep/` is not a valid cone pattern. + test_must_fail git -C repo sparse-checkout add folder1/ 2>err && + test_i18ngrep "unable to use existing sparse-checkout patterns in cone mode" err && + + # `set` succeeds with same patterns set properly for cone mode. + git -C repo sparse-checkout set deep/ folder1/ && + cat >expect <<-\EOF && + /* + !/*/ + /deep/ + /folder1/ + EOF + test_cmp expect repo/.git/info/sparse-checkout +' + test_done