From 64ae1e12f359aa6e6962ab5c85e0fa893b15a30f 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 f97b6f6e2cee11c7b98aa5fd557eeaf96e75af7b 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 3e24c00140e810..643aa03cd06ea7 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -618,6 +618,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 65d64744ec06825cd64ad7243385db84ea2500ff 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 643aa03cd06ea7..16077f60b711da 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1101,6 +1101,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 2c0563ca96c1a5f6f02aa187bc812b5591b337f9 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 ee2831494bafac..9aaba14073672c 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 6332a0c9d75689f26010464e43d61f1e194f46d1 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 16077f60b711da..823546d04cbabb 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1111,6 +1111,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