From 0590b931a6825c49b2da84153d11b46ec872a0af Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 9 Sep 2021 20:18:00 -0400 Subject: [PATCH 1/5] update-index (bugfix): `--cacheinfo` should block directories Add explicit check for directory entry mode, throwing an error if found. Without the check, sparse directory cache entries could be added to any index (including non-sparse). In a sparse index, this would at least cause inconsistencies in the cache tree; for a non-sparse index, such an entry being present at all is a bug. Signed-off-by: Victoria Dye --- builtin/update-index.c | 3 +++ t/t2107-update-index-basic.sh | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/builtin/update-index.c b/builtin/update-index.c index d10174bb8cc0b3..a9cc562c6af8c6 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -410,6 +410,9 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, if (!verify_path(path, mode)) return error("Invalid path '%s'", path); + if (S_ISSPARSEDIR(mode)) + return error("%s: cannot add directory as cache entry", path); + len = strlen(path); ce = make_empty_cache_entry(&the_index, len); diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index a30b7ca6bc90c9..ea1eac5d278053 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -64,6 +64,14 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' ' test_cmp expect actual ' +test_expect_success '--cacheinfo does not accept directory mode' ' + mkdir folder1 && + echo content >folder1/content && + git add folder1 && + folder1_oid=$(git ls-files -s folder1 | git hash-object --stdin) && + test_must_fail git update-index --add --cacheinfo 040000 $folder1_oid folder1/ +' + test_expect_success '.lock files cleaned up' ' mkdir cleanup && ( From 80c1683fb7046cf1facf2b473d5a9505b452a6ba Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 9 Sep 2021 16:54:05 -0400 Subject: [PATCH 2/5] update-index: add tests for sparse-checkout compatibility In preparation of integrating 'git update-index' with the sparse index, introduce tests for a variety of 'git update-index' uses. Some (namely those in update-index add/remove`) are focused specifically on how 'git stash' uses 'git update-index' as a subcommand. Signed-off-by: Victoria Dye --- t/t1092-sparse-checkout-compatibility.sh | 150 +++++++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 1cf4f1f0c8ccf9..c83f6c121e3e0b 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -737,6 +737,156 @@ test_expect_success 'reset with wildcard pathspec' ' test_all_match git status --porcelain=v2 ' +# NEEDSWORK: although update-index executes without error on files outside +# the sparse checkout definition, it does not actually add the file to the +# index. This is also true when "--no-ignore-skip-worktree-entries" is +# specified. +test_expect_success 'update-index add outside sparse definition' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + run_on_sparse mkdir -p folder1 && + run_on_sparse cp ../initial-repo/folder1/a folder1/a && + + # Edit the file only in sparse checkouts so that, when checking the status + # of the index, the unmodified full-checkout is compared to the "modified" + # sparse checkouts. + run_on_sparse ../edit-contents folder1/a && + + test_sparse_match git update-index --add folder1/a && + test_all_match git status --porcelain=v2 && + test_sparse_match git update-index --add --no-ignore-skip-worktree-entries folder1/a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'update-index remove outside sparse definition' ' + init_repos && + + # When --remove is specified, files outside the sparse checkout definition + # are considered "removed". + rm -f full-checkout/folder1/a && + test_all_match git update-index --remove folder1/a && + test_all_match git status --porcelain=v2 && + + git reset --hard && + + # When --ignore-skip-worktree-entries is explicitly specified, a file + # outside the sparse definition is not added to the index as "removed" + # (thus matching the unmodified full-checkout). + test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a && + test_all_match git status --porcelain=v2 && + + git reset --hard && + + # --force-remove supercedes --ignore-skip-worktree-entries and always + # removes the file from the index. + test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'update-index folder add/remove' ' + init_repos && + + test_all_match test_must_fail git update-index --add --remove deep && + test_all_match test_must_fail git update-index --add --remove deep/ && + + # NEEDSWORK: attempting to update-index on an existing folder outside the + # sparse checkout definition does not throw an error (as it does for folders + # inside the definition, or in the full checkout). However, it is a no-op. + test_sparse_match git update-index --add --remove folder1 && + test_sparse_match git update-index --add --remove folder1/ && + test_sparse_match git update-index --force-remove folder1/ && + test_all_match git status --porcelain=v2 && + + # New folders, even in sparse checkouts, throw an error on update-index + run_on_all mkdir folder3 && + run_on_all cp a folder3/a && + run_on_all test_must_fail git update-index --add --remove folder3 +' + +test_expect_success 'update-index with updated flags' ' + init_repos && + + # NEEDSWORK: updating flags runs inconsistently on directories, performing no + # operation with warning text specifying the path being ignored if a trailing + # slash is in the path, but throwing an error if there is no trailing slash. + test_all_match test_must_fail git update-index --no-skip-worktree folder1 && + test_all_match git update-index --no-skip-worktree folder1/ && + test_all_match git status --porcelain=v2 && + + # Removing the skip-worktree bit from a file outside the sparse checkout + # will cause the file to appear as unstaged and deleted. + test_sparse_match git update-index --no-skip-worktree folder1/a && + rm -f full-checkout/folder1/a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'update-index --again file outside sparse definition' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + # When a file is manually added and modified outside the checkout + # definition, it will not be changed with `--again` because its changes are + # not tracked in the index. + run_on_sparse mkdir -p folder1 && + run_on_sparse ../edit-contents folder1/a && + test_sparse_match git update-index --again && + test_sparse_match git status --porcelain=v2 && + + # Update the sparse checkouts so that folder1/a is no longer skipped and + # exists on-disk + run_on_sparse cp ../initial-repo/folder1/a folder1/a && + test_sparse_match git update-index --no-skip-worktree folder1/a && + test_all_match git status --porcelain=v2 && + + # Stage change for commit + run_on_all ../edit-contents folder1/a && + test_all_match git update-index folder1/a && + test_all_match git status --porcelain=v2 && + + # Modify the file + run_on_all ../edit-contents folder1/a && + test_all_match git status --porcelain=v2 && + + # Run update-index --again, which re-stages the local changes + test_all_match git update-index --again && + test_all_match git ls-files -s folder1/a && + test_all_match git status --porcelain=v2 && + + # Running update-index --again with staged changes after manually deleting + # the file on disk will cause it to fail if --remove is not also specified + run_on_all rm -f folder1/a && + test_all_match test_must_fail git update-index --again folder1 && + test_all_match git update-index --remove --again && + test_all_match git status --porcelain=v2 +' + +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 - because folder1/a is outside the sparse checkout + # definition (and thus not on-disk), it will appear as "deleted" in + # unstaged changes. + test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a && + test_sparse_match git status --porcelain=v2 +' + test_expect_success 'merge, cherry-pick, and rebase' ' init_repos && From eaf37a0a39fb176656bcb28e0e4f41c3c81950b6 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 9 Sep 2021 16:56:14 -0400 Subject: [PATCH 3/5] update-index: integrate with sparse index Enable usage of the sparse index with `update-index`. Most variations of `update-index` work without explicitly expanding the index (due to the implicit expansion performed when attempting to find an existing entry). However, in order to prevent manually creating a sparse directory cache entry (current behavior is to throw an error when presented with a directory using `--cacheinfo`), the index is expanded before the the entry is added. Signed-off-by: Victoria Dye --- builtin/update-index.c | 7 +++++++ t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/builtin/update-index.c b/builtin/update-index.c index a9cc562c6af8c6..1947acbd3b51e1 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -425,6 +425,10 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, ce->ce_flags |= CE_VALID; option = allow_add ? ADD_CACHE_OK_TO_ADD : 0; option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0; + + /* TODO: ensure full index to maintain expected behavior */ + ensure_full_index(&the_index); + if (add_cache_entry(ce, option)) return error("%s: cannot add to the index - missing --add option?", path); @@ -1082,6 +1086,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/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index c83f6c121e3e0b..08d87b68352a52 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1382,6 +1382,18 @@ test_expect_success 'sparse index is not expanded: sparse-checkout' ' ensure_not_expanded sparse-checkout set ' +test_expect_success 'sparse index is not expanded: update-index' ' + init_repos && + + 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 && +' + # NEEDSWORK: a sparse-checkout behaves differently from a full checkout # in this scenario, but it shouldn't. test_expect_success 'reset mixed and checkout orphan' ' From 1831e1b1f460f4ba15ef3e184cd5a29553d68555 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 9 Sep 2021 18:24:43 -0400 Subject: [PATCH 4/5] update-index: remove sparse index expansion for --cacheinfo Rearrange `add_index_entry_with_check` to allow `index_name_stage_pos` to expand the index _before_ attempting to invalidate the relevant cache tree path. This permits implicit index expansion when adding a cache entry outside the sparse checkout definition. Signed-off-by: Victoria Dye --- builtin/update-index.c | 4 ---- read-cache.c | 10 +++++++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 1947acbd3b51e1..bd96c716d16602 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -425,10 +425,6 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, ce->ce_flags |= CE_VALID; option = allow_add ? ADD_CACHE_OK_TO_ADD : 0; option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0; - - /* TODO: ensure full index to maintain expected behavior */ - ensure_full_index(&the_index); - if (add_cache_entry(ce, option)) return error("%s: cannot add to the index - missing --add option?", path); diff --git a/read-cache.c b/read-cache.c index 92033406b51d62..c1a6abb8168aa9 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1352,9 +1352,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. @@ -1365,6 +1362,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) From 3bca4a2765ecdb8f99adb938c75c054577226c56 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 9 Sep 2021 19:35:06 -0400 Subject: [PATCH 5/5] update-index: remove unnecessary index expansion in do_reupdate Instead of ensuring the full index, only need to prevent reupdating sparse directory entries to maintain expected behavior. Corresponding addition to index expansion test verifies the sparse index is not expanded. Signed-off-by: Victoria Dye --- builtin/update-index.c | 12 +++++++++--- t/t1092-sparse-checkout-compatibility.sh | 3 +++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index bd96c716d16602..5b1ae4bdfdf163 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -747,17 +747,23 @@ static int do_reupdate(int ac, const char **av, * commit. Update everything in the index. */ 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; int save_nr; char *path; - if (ce_stage(ce) || !ce_path_match(&the_index, ce, &pathspec, NULL)) + /* + * We can safely skip re-updating sparse directories because if there + * were any changes to re-update inside of the sparse directory, it + * would not be sparse. + */ + if (S_ISSPARSEDIR(ce->ce_mode) || ce_stage(ce) || + !ce_path_match(&the_index, ce, &pathspec, NULL)) continue; + if (has_head) old = read_one_ent(NULL, &head_oid, ce->name, ce_namelen(ce), 0); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 08d87b68352a52..8ddacab279185b 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1392,6 +1392,9 @@ 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 && + + rm -f sparse-index/README.md sparse-index/a && + ensure_not_expanded update-index --add --remove --again ' # NEEDSWORK: a sparse-checkout behaves differently from a full checkout