From bb01b26dec69f5f287f0d36cbe4c765fe7f7b053 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:04:58 +0000 Subject: [PATCH 1/9] reset: fix validation in sparse index test Update t1092 test 'reset with pathspecs outside sparse definition' to verify index contents. The use of `rev-parse` verifies the contents of HEAD, not the index, providing no real validation of the reset results. Conversely, `ls-files` reports the contents of the index (OIDs, flags, filenames), which are then compared across checkouts to ensure compatible index states. Fixes 741a2c9ffa (reset: expand test coverage for sparse checkouts, 2021-09-27). Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1092-sparse-checkout-compatibility.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 49f70a6569..d5167e7ed6 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -596,13 +596,11 @@ test_expect_success 'reset with pathspecs outside sparse definition' ' test_sparse_match git reset update-folder1 -- folder1 && git -C full-checkout reset update-folder1 -- folder1 && - test_sparse_match git status --porcelain=v2 && - test_all_match git rev-parse HEAD:folder1 && + test_all_match git ls-files -s -- folder1 && test_sparse_match git reset update-folder2 -- folder2/a && git -C full-checkout reset update-folder2 -- folder2/a && - test_sparse_match git status --porcelain=v2 && - test_all_match git rev-parse HEAD:folder2/a + test_all_match git ls-files -s -- folder2/a ' test_expect_success 'reset with wildcard pathspec' ' From 1624333ec1486378c44ce38e4f8ae9d02c07d15a Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:04:59 +0000 Subject: [PATCH 2/9] reset: reorder wildcard pathspec conditions Rearrange conditions in method determining whether index expansion is necessary when a pathspec is specified for `git reset`, placing less expensive condition first. Additionally, add details & examples to related code comments to help with readability. Helped-by: Elijah Newren Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/reset.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index b1ff699b43..79b40385b9 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -204,10 +204,16 @@ static int pathspec_needs_expanded_index(const struct pathspec *pathspec) /* * Special case: if the pattern is a path inside the cone * followed by only wildcards, the pattern cannot match - * partial sparse directories, so we don't expand the index. + * partial sparse directories, so we know we don't need to + * expand the index. + * + * Examples: + * - in-cone/foo***: doesn't need expanded index + * - not-in-cone/bar*: may need expanded index + * - **.c: may need expanded index */ - if (path_in_cone_mode_sparse_checkout(item.original, &the_index) && - strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len) + if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len && + path_in_cone_mode_sparse_checkout(item.original, &the_index)) continue; for (pos = 0; pos < active_nr; pos++) { From 1e9e10e04891a13e5ccd52b36cfadc55dfaa5066 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:00 +0000 Subject: [PATCH 3/9] clean: integrate with sparse index Remove full index requirement for `git clean` and test to ensure the index is not expanded in `git clean`. Add to existing test for `git clean` to verify cleanup of untracked files in sparse directories is consistent between sparse index and non-sparse index checkouts. Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/clean.c | 3 +++ t/t1092-sparse-checkout-compatibility.sh | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/builtin/clean.c b/builtin/clean.c index 98a2860409..5628fc7103 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -983,6 +983,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix) dir.flags |= DIR_KEEP_UNTRACKED_CONTENTS; } + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + if (read_cache() < 0) die(_("index file corrupt")); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index d5167e7ed6..0558736145 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -764,23 +764,42 @@ test_expect_success 'clean' ' test_all_match git commit -m "ignore bogus files" && run_on_sparse mkdir folder1 && + run_on_all mkdir -p deep/untracked-deep && run_on_all touch folder1/bogus && + run_on_all touch folder1/untracked && + run_on_all touch deep/untracked-deep/bogus && + run_on_all touch deep/untracked-deep/untracked && test_all_match git status --porcelain=v2 && test_all_match git clean -f && test_all_match git status --porcelain=v2 && test_sparse_match ls && test_sparse_match ls folder1 && + run_on_all test_path_exists folder1/bogus && + run_on_all test_path_is_missing folder1/untracked && + run_on_all test_path_exists deep/untracked-deep/bogus && + run_on_all test_path_exists deep/untracked-deep/untracked && + + test_all_match git clean -fd && + test_all_match git status --porcelain=v2 && + test_sparse_match ls && + test_sparse_match ls folder1 && + run_on_all test_path_exists folder1/bogus && + run_on_all test_path_exists deep/untracked-deep/bogus && + run_on_all test_path_is_missing deep/untracked-deep/untracked && test_all_match git clean -xf && test_all_match git status --porcelain=v2 && test_sparse_match ls && test_sparse_match ls folder1 && + run_on_all test_path_is_missing folder1/bogus && + run_on_all test_path_exists deep/untracked-deep/bogus && test_all_match git clean -xdf && test_all_match git status --porcelain=v2 && test_sparse_match ls && test_sparse_match ls folder1 && + run_on_all test_path_is_missing deep/untracked-deep/bogus && test_sparse_match test_path_is_dir folder1 ' @@ -920,6 +939,8 @@ test_expect_success 'sparse-index is not expanded' ' # Wildcard identifies only full sparse directories, no index expansion ensure_not_expanded reset deepest -- folder\* && + ensure_not_expanded clean -fd && + ensure_not_expanded checkout -f update-deep && test_config -C sparse-index pull.twohead ort && ( From b553ef674965f41bfff4e0a2c330f9087b3cd6b7 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:01 +0000 Subject: [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests Add tests to cover `checkout-index`, with a focus on cases interesting in a sparse checkout (e.g., files specified outside sparse checkout definition). New tests are intended to serve as a baseline for existing and/or expected behavior and performance when integrating `checkout-index` with the sparse index. Note that the test 'checkout-index --all' is marked as 'test_expect_failure', indicating that `update-index --all` will be modified in a subsequent patch to behave as the test expects. Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/perf/p2000-sparse-operations.sh | 1 + t/t1092-sparse-checkout-compatibility.sh | 54 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh index cb777c74a2..54f8602f3c 100755 --- a/t/perf/p2000-sparse-operations.sh +++ b/t/perf/p2000-sparse-operations.sh @@ -117,5 +117,6 @@ test_perf_on_all git diff test_perf_on_all git diff --cached test_perf_on_all git blame $SPARSE_CONE/a test_perf_on_all git blame $SPARSE_CONE/f3/a +test_perf_on_all git checkout-index -f --all test_done diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 0558736145..db7ad41109 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -755,6 +755,60 @@ test_expect_success 'cherry-pick with conflicts' ' test_all_match test_must_fail git cherry-pick to-cherry-pick ' +test_expect_success 'checkout-index inside sparse definition' ' + init_repos && + + run_on_all rm -f deep/a && + test_all_match git checkout-index -- deep/a && + test_all_match git status --porcelain=v2 && + + echo test >>new-a && + run_on_all cp ../new-a a && + test_all_match test_must_fail git checkout-index -- a && + test_all_match git checkout-index -f -- a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'checkout-index outside sparse definition' ' + init_repos && + + # File does not exist on disk yet for sparse checkouts, so checkout-index + # succeeds without -f + test_sparse_match git checkout-index -- folder1/a && + test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && + test_cmp sparse-checkout/folder1/a full-checkout/folder1/a && + + run_on_sparse rm -rf folder1 && + echo test >new-a && + run_on_sparse mkdir -p folder1 && + run_on_all cp ../new-a folder1/a && + + test_all_match test_must_fail git checkout-index -- folder1/a && + test_all_match git checkout-index -f -- folder1/a && + test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && + test_cmp sparse-checkout/folder1/a full-checkout/folder1/a +' + +test_expect_success 'checkout-index with folders' ' + init_repos && + + # Inside checkout definition + test_all_match test_must_fail git checkout-index -f -- deep/ && + + # Outside checkout definition + test_all_match test_must_fail git checkout-index -f -- folder1/ +' + +# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all +# files (even those outside the sparse definition) on disk. However, these files +# don't appear in the percentage of tracked files in git status. +test_expect_failure 'checkout-index --all' ' + init_repos && + + test_all_match git checkout-index --all && + test_sparse_match test_path_is_missing folder1 +' + test_expect_success 'clean' ' init_repos && From 88078f543b769dc13ae9796372651178584a25a0 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:02 +0000 Subject: [PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option Update `checkout-index` to no longer refresh files that have the `skip-worktree` bit set, exiting with an error if `skip-worktree` filenames are directly provided to `checkout-index`. The newly-added `--ignore-skip-worktree-bits` option provides a mechanism to replicate the old behavior, checking out *all* files specified (even those with `skip-worktree` enabled). The ability to toggle whether files should be checked-out based on `skip-worktree` already exists in `git checkout` and `git restore` (both of which have an `--ignore-skip-worktree-bits` option). The change to, by default, ignore `skip-worktree` files is especially helpful for sparse-checkout; it prevents inadvertent creation of files outside the sparse definition on disk and eliminates the need to expand a sparse index when using the `--all` option. Internal usage of `checkout-index` in `git stash` and `git filter-branch` do not make explicit use of files with `skip-worktree` enabled, so `--ignore-skip-worktree-bits` is not added to them. Helped-by: Elijah Newren Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- Documentation/git-checkout-index.txt | 10 +++++++-- builtin/checkout-index.c | 13 ++++++++++++ t/t1092-sparse-checkout-compatibility.sh | 27 +++++++++++++++--------- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/Documentation/git-checkout-index.txt b/Documentation/git-checkout-index.txt index 4d33e7be0f..01dbd5cbf5 100644 --- a/Documentation/git-checkout-index.txt +++ b/Documentation/git-checkout-index.txt @@ -12,6 +12,7 @@ SYNOPSIS 'git checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=] [--stage=|all] [--temp] + [--ignore-skip-worktree-bits] [-z] [--stdin] [--] [...] @@ -37,8 +38,9 @@ OPTIONS -a:: --all:: - checks out all files in the index. Cannot be used - together with explicit filenames. + checks out all files in the index except for those with the + skip-worktree bit set (see `--ignore-skip-worktree-bits`). + Cannot be used together with explicit filenames. -n:: --no-create:: @@ -59,6 +61,10 @@ OPTIONS write the content to temporary files. The temporary name associations will be written to stdout. +--ignore-skip-worktree-bits:: + Check out all files, including those with the skip-worktree bit + set. + --stdin:: Instead of taking list of paths from the command line, read list of paths from the standard input. Paths are diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index e21620d964..615a118e2f 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -7,6 +7,7 @@ #define USE_THE_INDEX_COMPATIBILITY_MACROS #include "builtin.h" #include "config.h" +#include "dir.h" #include "lockfile.h" #include "quote.h" #include "cache-tree.h" @@ -17,6 +18,7 @@ #define CHECKOUT_ALL 4 static int nul_term_line; static int checkout_stage; /* default to checkout stage0 */ +static int ignore_skip_worktree; /* default to 0 */ static int to_tempfile; static char topath[4][TEMPORARY_FILENAME_LENGTH + 1]; @@ -65,6 +67,7 @@ static int checkout_file(const char *name, const char *prefix) int namelen = strlen(name); int pos = cache_name_pos(name, namelen); int has_same_name = 0; + int is_skipped = 1; int did_checkout = 0; int errs = 0; @@ -78,6 +81,9 @@ static int checkout_file(const char *name, const char *prefix) break; has_same_name = 1; pos++; + if (!ignore_skip_worktree && ce_skip_worktree(ce)) + break; + is_skipped = 0; if (ce_stage(ce) != checkout_stage && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce))) continue; @@ -106,6 +112,9 @@ static int checkout_file(const char *name, const char *prefix) fprintf(stderr, "git checkout-index: %s ", name); if (!has_same_name) fprintf(stderr, "is not in the cache"); + else if (is_skipped) + fprintf(stderr, "has skip-worktree enabled; " + "use '--ignore-skip-worktree-bits' to checkout"); else if (checkout_stage) fprintf(stderr, "does not exist at stage %d", checkout_stage); @@ -125,6 +134,8 @@ static int checkout_all(const char *prefix, int prefix_length) ensure_full_index(&the_index); for (i = 0; i < active_nr ; i++) { struct cache_entry *ce = active_cache[i]; + if (!ignore_skip_worktree && ce_skip_worktree(ce)) + continue; if (ce_stage(ce) != checkout_stage && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce))) continue; @@ -185,6 +196,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) struct option builtin_checkout_index_options[] = { OPT_BOOL('a', "all", &all, N_("check out all files in the index")), + OPT_BOOL(0, "ignore-skip-worktree-bits", &ignore_skip_worktree, + N_("do not skip files with skip-worktree set")), OPT__FORCE(&force, N_("force overwrite of existing files"), 0), OPT__QUIET(&quiet, N_("no warning for existing files and files not in index")), diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index db7ad41109..434ef0433c 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -772,9 +772,14 @@ test_expect_success 'checkout-index inside sparse definition' ' test_expect_success 'checkout-index outside sparse definition' ' init_repos && - # File does not exist on disk yet for sparse checkouts, so checkout-index - # succeeds without -f - test_sparse_match git checkout-index -- folder1/a && + # Without --ignore-skip-worktree-bits, outside-of-cone files will trigger + # an error + test_sparse_match test_must_fail git checkout-index -- folder1/a && + test_i18ngrep "folder1/a has skip-worktree enabled" sparse-checkout-err && + test_path_is_missing folder1/a && + + # With --ignore-skip-worktree-bits, outside-of-cone files are checked out + test_sparse_match git checkout-index --ignore-skip-worktree-bits -- folder1/a && test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && test_cmp sparse-checkout/folder1/a full-checkout/folder1/a && @@ -783,8 +788,8 @@ test_expect_success 'checkout-index outside sparse definition' ' run_on_sparse mkdir -p folder1 && run_on_all cp ../new-a folder1/a && - test_all_match test_must_fail git checkout-index -- folder1/a && - test_all_match git checkout-index -f -- folder1/a && + test_all_match test_must_fail git checkout-index --ignore-skip-worktree-bits -- folder1/a && + test_all_match git checkout-index -f --ignore-skip-worktree-bits -- folder1/a && test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && test_cmp sparse-checkout/folder1/a full-checkout/folder1/a ' @@ -799,14 +804,16 @@ test_expect_success 'checkout-index with folders' ' test_all_match test_must_fail git checkout-index -f -- folder1/ ' -# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all -# files (even those outside the sparse definition) on disk. However, these files -# don't appear in the percentage of tracked files in git status. -test_expect_failure 'checkout-index --all' ' +test_expect_success 'checkout-index --all' ' init_repos && test_all_match git checkout-index --all && - test_sparse_match test_path_is_missing folder1 + test_sparse_match test_path_is_missing folder1 && + + # --ignore-skip-worktree-bits will cause `skip-worktree` files to be + # checked out, causing the outside-of-cone `folder1` to exist on-disk + test_all_match git checkout-index --ignore-skip-worktree-bits --all && + test_all_match test_path_exists folder1 ' test_expect_success 'clean' ' From 35682ada44554e136677649ac3da8c92342cdae2 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:03 +0000 Subject: [PATCH 6/9] checkout-index: integrate with sparse index Add repository settings to allow usage of the sparse index. When using the `--all` option, sparse directories are ignored by default due to the `skip-worktree` flag, so there is no need to expand the index. If `--ignore-skip-worktree-bits` is specified, the index is expanded in order to check out all files. When checking out individual files, existing behavior in a full index is to exit with an error if a directory is specified (as the directory name will not match an index entry). However, it is possible in a sparse index to match a directory name to a sparse directory index entry, but checking out that sparse directory still results in an error on checkout. To reduce some potential confusion for users, `checkout_file(...)` explicitly exits with an informative error if provided with a sparse directory name. The test corresponding to this scenario verifies the error message, which now differs between sparse index and non-sparse index checkouts. Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 28 ++++++++++++++++++++++-- t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 615a118e2f..97e06e8c52 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -67,6 +67,7 @@ static int checkout_file(const char *name, const char *prefix) int namelen = strlen(name); int pos = cache_name_pos(name, namelen); int has_same_name = 0; + int is_file = 0; int is_skipped = 1; int did_checkout = 0; int errs = 0; @@ -81,6 +82,9 @@ static int checkout_file(const char *name, const char *prefix) break; has_same_name = 1; pos++; + if (S_ISSPARSEDIR(ce->ce_mode)) + break; + is_file = 1; if (!ignore_skip_worktree && ce_skip_worktree(ce)) break; is_skipped = 0; @@ -112,6 +116,8 @@ static int checkout_file(const char *name, const char *prefix) fprintf(stderr, "git checkout-index: %s ", name); if (!has_same_name) fprintf(stderr, "is not in the cache"); + else if (!is_file) + fprintf(stderr, "is a sparse directory"); else if (is_skipped) fprintf(stderr, "has skip-worktree enabled; " "use '--ignore-skip-worktree-bits' to checkout"); @@ -130,10 +136,25 @@ static int checkout_all(const char *prefix, int prefix_length) int i, errs = 0; struct cache_entry *last_ce = NULL; - /* TODO: audit for interaction with sparse-index. */ - ensure_full_index(&the_index); for (i = 0; i < active_nr ; i++) { struct cache_entry *ce = active_cache[i]; + + if (S_ISSPARSEDIR(ce->ce_mode)) { + if (!ce_skip_worktree(ce)) + BUG("sparse directory '%s' does not have skip-worktree set", ce->name); + + /* + * If the current entry is a sparse directory and skip-worktree + * entries are being checked out, expand the index and continue + * the loop on the current index position (now pointing to the + * first entry inside the expanded sparse directory). + */ + if (ignore_skip_worktree) { + ensure_full_index(&the_index); + ce = active_cache[i]; + } + } + if (!ignore_skip_worktree && ce_skip_worktree(ce)) continue; if (ce_stage(ce) != checkout_stage @@ -225,6 +246,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); prefix_length = prefix ? strlen(prefix) : 0; + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + if (read_cache() < 0) { die("invalid cache"); } diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 434ef0433c..0c72c854d8 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -801,7 +801,14 @@ test_expect_success 'checkout-index with folders' ' test_all_match test_must_fail git checkout-index -f -- deep/ && # Outside checkout definition - test_all_match test_must_fail git checkout-index -f -- folder1/ + # Note: although all tests fail (as expected), the messaging differs. For + # non-sparse index checkouts, the error is that the "file" does not appear + # in the index; for sparse checkouts, the error is explicitly that the + # entry is a sparse directory. + run_on_all test_must_fail git checkout-index -f -- folder1/ && + test_cmp full-checkout-err sparse-checkout-err && + ! test_cmp full-checkout-err sparse-index-err && + grep "is a sparse directory" sparse-index-err ' test_expect_success 'checkout-index --all' ' @@ -972,6 +979,8 @@ test_expect_success 'sparse-index is not expanded' ' echo >>sparse-index/untracked.txt && ensure_not_expanded add . && + ensure_not_expanded checkout-index -f a && + ensure_not_expanded checkout-index -f --all && for ref in update-deep update-folder1 update-folder2 update-deep do echo >>sparse-index/README.md && From e015d4d9614f728cc454900fde52af2076d5ef63 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:04 +0000 Subject: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility Introduce tests for a variety of `git update-index` use cases, including performance scenarios. Tests are intended to exercise `update-index` with options that change the commands interaction with the index (e.g., `--again`) and with files/directories inside and outside a sparse checkout cone. Of note is that these tests clearly establish the behavior of `git update-index --add` with untracked, outside-of-cone files. Unlike `git add`, which fails with an error when provided with such files, `update-index` succeeds in adding them to the index. Additionally, the `skip-worktree` flag is *not* automatically added to the new entry. Although this is pre-existing behavior, there are a couple of reasons to avoid changing it in favor of consistency with e.g. `git add`: * `update-index` is low-level command for modifying the index; while it can perform operations similar to those of `add`, it traditionally has fewer "guardrails" preventing a user from doing something they may not want to do (in this case, adding an outside-of-cone, non-`skip-worktree` file to the index) * `update-index` typically only exits with an error code if it is incapable of performing an operation (e.g., if an internal function call fails); adding a new file outside the sparse checkout definition is still a valid operation, albeit an inadvisable one * `update-index` does not implicitly set flags (e.g., `skip-worktree`) when creating new index entries with `--add`; if flags need to be updated, options like `--[no-]skip-worktree` allow a user to intentionally set them All this to say that, while there are valid reasons to consider changing the treatment of outside-of-cone files in `update-index`, there are also sufficient reasons for leaving it as-is. Co-authored-by: Derrick Stolee Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/perf/p2000-sparse-operations.sh | 1 + t/t1092-sparse-checkout-compatibility.sh | 167 +++++++++++++++++++++++ 2 files changed, 168 insertions(+) diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh index 54f8602f3c..2a7106b949 100755 --- a/t/perf/p2000-sparse-operations.sh +++ b/t/perf/p2000-sparse-operations.sh @@ -118,5 +118,6 @@ test_perf_on_all git diff --cached test_perf_on_all git blame $SPARSE_CONE/a test_perf_on_all git blame $SPARSE_CONE/f3/a test_perf_on_all git checkout-index -f --all +test_perf_on_all git update-index --add --remove $SPARSE_CONE/a test_done diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 0c72c854d8..91f849f541 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -630,6 +630,173 @@ test_expect_success 'reset with wildcard pathspec' ' test_all_match git ls-files -s -- folder1 ' +test_expect_success 'update-index modify outside sparse definition' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + # Create & modify folder1/a + # Note that this setup is a manual way of reaching the erroneous + # condition in which a `skip-worktree` enabled, outside-of-cone file + # exists on disk. It is used here to ensure `update-index` is stable + # and behaves predictably if such a condition occurs. + run_on_sparse mkdir -p folder1 && + run_on_sparse cp ../initial-repo/folder1/a folder1/a && + run_on_all ../edit-contents folder1/a && + + # If file has skip-worktree enabled, update-index does not modify the + # index entry + test_sparse_match git update-index folder1/a && + test_sparse_match git status --porcelain=v2 && + test_must_be_empty sparse-checkout-out && + + # When skip-worktree is disabled (even on files outside sparse cone), file + # is updated in the index + test_sparse_match git update-index --no-skip-worktree folder1/a && + test_all_match git status --porcelain=v2 && + test_all_match git update-index folder1/a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'update-index --add outside sparse definition' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + # Create folder1, add new file + run_on_sparse mkdir -p folder1 && + run_on_all ../edit-contents folder1/b && + + # The *untracked* out-of-cone file is added to the index because it does + # not have a `skip-worktree` bit to signal that it should be ignored + # (unlike in `git add`, which will fail due to the file being outside + # the sparse checkout definition). + test_all_match git update-index --add folder1/b && + test_all_match git status --porcelain=v2 +' + +# NEEDSWORK: `--remove`, unlike the rest of `update-index`, does not ignore +# `skip-worktree` entries by default and will remove them from the index. +# The `--ignore-skip-worktree-entries` flag must be used in conjunction with +# `--remove` to ignore the `skip-worktree` entries and prevent their removal +# from the index. +test_expect_success 'update-index --remove outside sparse definition' ' + init_repos && + + # When --ignore-skip-worktree-entries is _not_ specified: + # out-of-cone, not-on-disk files are removed from the index + test_sparse_match git update-index --remove folder1/a && + cat >expect <<-EOF && + D folder1/a + EOF + test_sparse_match git diff --cached --name-status && + test_cmp expect sparse-checkout-out && + + # Reset the state + test_all_match git reset --hard && + + # When --ignore-skip-worktree-entries is specified, out-of-cone + # (skip-worktree) files are ignored + test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a && + test_sparse_match git diff --cached --name-status && + test_must_be_empty sparse-checkout-out && + + # Reset the state + test_all_match git reset --hard && + + # --force-remove supercedes --ignore-skip-worktree-entries, removing + # a skip-worktree file from the index (and disk) when both are specified + # with --remove + test_sparse_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a && + cat >expect <<-EOF && + D folder1/a + EOF + test_sparse_match git diff --cached --name-status && + test_cmp expect sparse-checkout-out +' + +test_expect_success 'update-index with directories' ' + init_repos && + + # update-index will exit silently when provided with a directory name + # containing a trailing slash + test_all_match git update-index deep/ folder1/ && + grep "Ignoring path deep/" sparse-checkout-err && + grep "Ignoring path folder1/" sparse-checkout-err && + + # When update-index is given a directory name WITHOUT a trailing slash, it will + # behave in different ways depending on the status of the directory on disk: + # * if it exists, the command exits with an error ("add individual files instead") + # * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a + # file and either triggers an error ("does not exist and --remove not passed") + # or is ignored completely (when using --remove) + test_all_match test_must_fail git update-index deep && + run_on_all test_must_fail git update-index folder1 && + test_must_fail git -C full-checkout update-index --remove folder1 && + test_sparse_match git update-index --remove folder1 && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'update-index --again file outside sparse definition' ' + init_repos && + + test_all_match git checkout -b test-reupdate && + + # Update HEAD without modifying the index to introduce a difference in + # folder1/a + test_sparse_match git reset --soft update-folder1 && + + # Because folder1/a differs in the index vs HEAD, + # `git update-index --no-skip-worktree --again` will effectively perform + # `git update-index --no-skip-worktree folder1/a` and remove the skip-worktree + # flag from folder1/a + test_sparse_match git update-index --no-skip-worktree --again && + test_sparse_match git status --porcelain=v2 && + + cat >expect <<-EOF && + D folder1/a + EOF + test_sparse_match git diff --name-status && + test_cmp expect sparse-checkout-out +' + +test_expect_success 'update-index --cacheinfo' ' + init_repos && + + deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) && + folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) && + folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) && + + test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a && + test_all_match git status --porcelain=v2 && + + # Cannot add sparse directory, even in sparse index case + test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ && + + # Sparse match only: the new outside-of-cone entry is added *without* skip-worktree, + # so `git status` reports it as "deleted" in the worktree + test_sparse_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a && + test_sparse_match git status --porcelain=v2 && + cat >expect <<-EOF && + MD folder1/a + EOF + test_sparse_match git status --short -- folder1/a && + test_cmp expect sparse-checkout-out && + + # To return folder1/a to "normal" for a sparse checkout (ignored & + # outside-of-cone), add the skip-worktree flag. + test_sparse_match git update-index --skip-worktree folder1/a && + cat >expect <<-EOF && + S folder1/a + EOF + test_sparse_match git ls-files -t -- folder1/a && + test_cmp expect sparse-checkout-out +' + test_expect_success 'merge, cherry-pick, and rebase' ' init_repos && From c35e9f5ecd00f0c003dc9120d3c68e95e2ba3bd7 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:05 +0000 Subject: [PATCH 8/9] update-index: integrate with sparse index Enable use of the sparse index with `update-index`. Most variations of `update-index` work without explicitly expanding the index or making any other updates in or outside of `update-index.c`. The one usage requiring additional changes is `--cacheinfo`; if a file inside a sparse directory was specified, the index would not be expanded until after the cache tree is invalidated, leading to a mismatch between the index and cache tree. This scenario is handled by rearranging `add_index_entry_with_check`, allowing `index_name_stage_pos` to expand the index *before* attempting to invalidate the relevant cache tree path, avoiding cache tree/index corruption. Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/update-index.c | 3 +++ read-cache.c | 10 +++++++--- t/t1092-sparse-checkout-compatibility.sh | 15 +++++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 187203e8bb..605cc693bb 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1077,6 +1077,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); + prepare_repo_settings(r); + the_repository->settings.command_requires_full_index = 0; + /* we will diagnose later if it turns out that we need to update it */ newfd = hold_locked_index(&lock_file, 0); if (newfd < 0) diff --git a/read-cache.c b/read-cache.c index cbe73f14e5..b4600e954b 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1339,9 +1339,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK; int new_only = option & ADD_CACHE_NEW_ONLY; - if (!(option & ADD_CACHE_KEEP_CACHE_TREE)) - cache_tree_invalidate_path(istate, ce->name); - /* * If this entry's path sorts after the last entry in the index, * we can avoid searching for it. @@ -1352,6 +1349,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e else pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), EXPAND_SPARSE); + /* + * Cache tree path should be invalidated only after index_name_stage_pos, + * in case it expands a sparse index. + */ + if (!(option & ADD_CACHE_KEEP_CACHE_TREE)) + cache_tree_invalidate_path(istate, ce->name); + /* existing match? Just replace it. */ if (pos >= 0) { if (!new_only) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 91f849f541..fceaba7101 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1253,6 +1253,21 @@ test_expect_success 'sparse index is not expanded: diff' ' ensure_not_expanded diff --cached ' +test_expect_success 'sparse index is not expanded: update-index' ' + init_repos && + + deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) && + ensure_not_expanded update-index --cacheinfo 100644 $deep_a_oid deep/a && + + echo "test" >sparse-index/README.md && + echo "test2" >sparse-index/a && + rm -f sparse-index/deep/a && + + ensure_not_expanded update-index --add README.md && + ensure_not_expanded update-index a && + ensure_not_expanded update-index --remove deep/a +' + test_expect_success 'sparse index is not expanded: blame' ' init_repos && From b9ca5e26579ceb820103b49648c01187a4a0dddd Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 11 Jan 2022 18:05:06 +0000 Subject: [PATCH 9/9] update-index: reduce scope of index expansion in do_reupdate Replace unconditional index expansion in 'do_reupdate()' with one scoped to only where a full index is needed. A full index is only required in 'do_reupdate()' when a sparse directory in the index differs from HEAD; in that case, the index is expanded and the operation restarted. Because the index should only be expanded if a sparse directory is modified, add a test ensuring the index is not expanded when differences only exist within the sparse cone. Signed-off-by: Victoria Dye Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/update-index.c | 14 +++++++++++--- t/t1092-sparse-checkout-compatibility.sh | 5 ++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 605cc693bb..52ecc714d9 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -606,7 +606,7 @@ static struct cache_entry *read_one_ent(const char *which, error("%s: not in %s branch.", path, which); return NULL; } - if (mode == S_IFDIR) { + if (!the_index.sparse_index && mode == S_IFDIR) { if (which) error("%s: not a blob in %s branch.", path, which); return NULL; @@ -743,8 +743,6 @@ static int do_reupdate(int ac, const char **av, */ has_head = 0; redo: - /* TODO: audit for interaction with sparse-index. */ - ensure_full_index(&the_index); for (pos = 0; pos < active_nr; pos++) { const struct cache_entry *ce = active_cache[pos]; struct cache_entry *old = NULL; @@ -761,6 +759,16 @@ static int do_reupdate(int ac, const char **av, discard_cache_entry(old); continue; /* unchanged */ } + + /* At this point, we know the contents of the sparse directory are + * modified with respect to HEAD, so we expand the index and restart + * to process each path individually + */ + if (S_ISSPARSEDIR(ce->ce_mode)) { + ensure_full_index(&the_index); + goto redo; + } + /* Be careful. The working tree may not have the * path anymore, in which case, under 'allow_remove', * or worse yet 'allow_replace', active_nr may decrease. diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index fceaba7101..53f84881de 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1265,7 +1265,10 @@ test_expect_success 'sparse index is not expanded: update-index' ' ensure_not_expanded update-index --add README.md && ensure_not_expanded update-index a && - ensure_not_expanded update-index --remove deep/a + ensure_not_expanded update-index --remove deep/a && + + ensure_not_expanded reset --soft update-deep && + ensure_not_expanded update-index --add --remove --again ' test_expect_success 'sparse index is not expanded: blame' '