From e0fddaa74b9772aef6d6c2051eefb4902b6b9287 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 1 Sep 2021 16:01:55 -0400 Subject: [PATCH 1/6] reset: add test demonstrating mixed reset outside sparse checkout Mixed reset in a sparse checkout (cone mode or otherwise) will, if a file outside of the sparse definition has differences between the working tree and target, write the file to the local working copy of the repository. Since this behavior conflicts with the typical expectations of a strict checkout definition/cone, a test is added demonstrating (and explaining) it. Signed-off-by: Victoria Dye --- t/t1092-sparse-checkout-compatibility.sh | 26 ++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 458c3dde007119..65839d06d8ca57 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -480,6 +480,32 @@ test_expect_success 'checkout and reset (mixed) [sparse]' ' test_sparse_match git reset update-folder2 ' +# NEEDSWORK: with mixed reset, files with differences between HEAD and +# will be added to the work tree even if outside the sparse checkout +# definition, and even if the file is modified to a state of having no local +# changes. The file is "re-ignored" if a hard reset is executed. We may want to +# change this behavior in the future and enforce that files are not written +# outside of the sparse checkout definition. +test_expect_success 'checkout and mixed reset file tracking [sparse]' ' + init_repos && + + test_all_match git checkout -b reset-test update-deep && + test_all_match git reset update-folder1 && + test_all_match git reset update-deep && + + # At this point, there are no changes in the working tree. However, + # folder1/a now exists locally (even though it is outside of the sparse + # paths). + run_on_sparse test_path_exists folder1 && + + run_on_all rm folder1/a && + test_all_match git status --porcelain=v2 && + + test_all_match git reset --hard update-deep && + run_on_sparse test_path_is_missing folder1 && + test_path_exists full-checkout/folder1 +' + test_expect_success 'merge, cherry-pick, and rebase' ' init_repos && From e934f4f256135c08cbb95da9f33deb882dfd209f Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 1 Sep 2021 16:46:51 -0400 Subject: [PATCH 2/6] reset: expand test coverage in sparse checkout Add tests for `--merge` and `--keep` modes, as well as (mixed) reset with pathspecs both inside and outside of the sparse checkout definition. Signed-off-by: Victoria Dye --- t/t1092-sparse-checkout-compatibility.sh | 81 ++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 65839d06d8ca57..a9170cda4bff54 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -506,6 +506,87 @@ test_expect_success 'checkout and mixed reset file tracking [sparse]' ' test_path_exists full-checkout/folder1 ' +test_expect_success 'checkout and reset (merge)' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + test_all_match git checkout -b reset-test update-deep && + run_on_all ../edit-contents a && + test_all_match git reset --merge deepest && + test_all_match git status --porcelain=v2 && + + test_all_match git reset --hard update-deep && + run_on_all ../edit-contents deep/a && + test_all_match test_must_fail git reset --merge deepest +' + +test_expect_success 'checkout and reset (keep)' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + test_all_match git checkout -b reset-test update-deep && + run_on_all ../edit-contents a && + test_all_match git reset --keep deepest && + test_all_match git status --porcelain=v2 && + + test_all_match git reset --hard update-deep && + run_on_all ../edit-contents deep/a && + test_all_match test_must_fail git reset --keep deepest +' + +test_expect_success 'reset with pathspecs inside sparse definition' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + test_all_match git checkout -b reset-test update-deep && + run_on_all ../edit-contents deep/a && + + test_all_match git reset base -- deep/a && + test_all_match git status --porcelain=v2 && + + test_all_match git reset base -- nonexistent-file && + test_all_match git status --porcelain=v2 && + + test_all_match git reset deepest -- deep && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'reset with sparse directory pathspec outside definition' ' + init_repos && + + test_all_match git checkout -b reset-test update-deep && + test_all_match git reset --hard update-folder1 && + test_all_match git reset base -- folder1 && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'reset with file pathspec outside sparse definition' ' + init_repos && + + test_all_match git checkout -b reset-test update-deep && + test_all_match git reset --hard update-folder1 && + test_all_match git reset base -- folder1/a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'reset with wildcard pathspec' ' + init_repos && + + test_all_match git checkout -b reset-test update-deep && + test_all_match git reset --hard update-folder1 && + test_all_match git reset base -- */a && + test_all_match git status --porcelain=v2 +' + test_expect_success 'merge, cherry-pick, and rebase' ' init_repos && From 8d8143a7a2b6d02e6c34a0e456b34447a14e043d Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 1 Sep 2021 16:10:20 -0400 Subject: [PATCH 3/6] sparse-index: use `read-tree` for expand/collapse test In anticipation of `reset` being fully integrated with sparse index, the test for index expand and collapse for non-sparse index integrated commands is updated to use `read-tree`. Signed-off-by: Victoria Dye --- t/t1092-sparse-checkout-compatibility.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index a9170cda4bff54..ab4a69302c5040 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -750,7 +750,7 @@ test_expect_success 'sparse-index is expanded and converted back' ' init_repos && GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \ - git -C sparse-index -c core.fsmonitor="" reset --hard && + git -C sparse-index -c core.fsmonitor="" read-tree -mu HEAD && test_region index convert_to_sparse trace2.txt && test_region index ensure_full_index trace2.txt ' From 33e9a2072c0a7265da53f6768857899ceae9427c Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 25 Aug 2021 15:01:14 -0400 Subject: [PATCH 4/6] sparse-index: integrate with reset This change enables running the `reset` builtin command without expanding the full index in a cone-mode sparse checkout. `reset --soft` does not modify the index, so no compatibility changes are needed for it to function without expanding the index. For all other reset modes (`--mixed`, `--hard`, `--keep`, `--merge`), the full index is explicitly expanded with `ensure_full_index` to maintain current behavior. Additionally, the `read_cache()` check verifying an uncorrupted index is moved after argument parsing and preparing the repo settings. The index is not used by the preceding argument handling, but `read_cache()` does need to be run after enabling sparse index for the command and before using the index for the reset. Signed-off-by: Victoria Dye --- builtin/reset.c | 10 +++++++--- cache-tree.c | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index fe67be37ada4c2..a456d1e5a3d7e4 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -204,6 +204,7 @@ static int read_from_tree(const struct pathspec *pathspec, opt.flags.override_submodule_config = 1; opt.repo = the_repository; + ensure_full_index(&the_index); if (do_diff_cache(tree_oid, &opt)) return 1; diffcore_std(&opt); @@ -281,9 +282,6 @@ static void parse_args(struct pathspec *pathspec, } *rev_ret = rev; - if (read_cache() < 0) - die(_("index file corrupt")); - parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL | (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0), @@ -440,6 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (intent_to_add && reset_type != MIXED) die(_("-N can only be used with --mixed")); + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + + if (read_cache() < 0) + die(_("index file corrupt")); + /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset * the index file to the tree object we are switching to. */ diff --git a/cache-tree.c b/cache-tree.c index 9e472c63ee69d8..2742d136bf759e 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -811,6 +811,7 @@ void prime_cache_tree(struct repository *r, cache_tree_free(&istate->cache_tree); istate->cache_tree = cache_tree(); + ensure_full_index(istate); prime_cache_tree_rec(r, istate->cache_tree, tree); istate->cache_changed |= CACHE_TREE_CHANGED; trace2_region_leave("cache-tree", "prime_cache_tree", r); From 9a65b6ad78381eef2e8679a2b6cc2a58f94d6c31 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 1 Sep 2021 15:44:55 -0400 Subject: [PATCH 5/6] sparse-index: make non-mixed `reset` sparse-aware This change allows non-mixed resets to no longer require that the full index is expanded. This is accomplished by updating the `prime_cache_tree_rec` function to have it reconstruct the cache tree "aware" of whether a directory is sparse in the index or not. This check must be done by verifying the contents of the cache itself (rather than checking whether a directory is inside of the sparse checkout cone) as entries may have been added to the sparse index outside of the checkout cone. Signed-off-by: Victoria Dye --- cache-tree.c | 37 +++++++++++++++++++++--- t/t1092-sparse-checkout-compatibility.sh | 15 ++++++++-- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 2742d136bf759e..6c95cdfb9e8adb 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -777,13 +777,31 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, static void prime_cache_tree_rec(struct repository *r, struct cache_tree *it, - struct tree *tree) + struct tree *tree, + struct strbuf *tree_path) { + struct strbuf subtree_path = STRBUF_INIT; struct tree_desc desc; struct name_entry entry; int cnt; oidcpy(&it->oid, &tree->object.oid); + + /* + * If this entry is outside the sparse-checkout cone, then it might be + * a sparse directory entry. Check the index to ensure it is by looking + * for an entry with the exact same name as the tree. If no matching sparse + * entry is found, a staged or conflicted entry is preventing this + * directory from collapsing to a sparse directory entry, so the cache + * tree expansion should continue. + */ + if (r->index->sparse_index && + !path_in_cone_modesparse_checkout(tree_path->buf, r->index) && + index_name_pos(r->index, tree_path->buf, tree_path->len) >= 0) { + it->entry_count = 1; + return; + } + init_tree_desc(&desc, tree->buffer, tree->size); cnt = 0; while (tree_entry(&desc, &entry)) { @@ -792,27 +810,38 @@ static void prime_cache_tree_rec(struct repository *r, else { struct cache_tree_sub *sub; struct tree *subtree = lookup_tree(r, &entry.oid); + if (!subtree->object.parsed) parse_tree(subtree); sub = cache_tree_sub(it, entry.path); sub->cache_tree = cache_tree(); - prime_cache_tree_rec(r, sub->cache_tree, subtree); + strbuf_reset(&subtree_path); + strbuf_grow(&subtree_path, tree_path->len + entry.pathlen + 1); + strbuf_addbuf(&subtree_path, tree_path); + strbuf_add(&subtree_path, entry.path, entry.pathlen); + strbuf_addch(&subtree_path, '/'); + + prime_cache_tree_rec(r, sub->cache_tree, subtree, &subtree_path); cnt += sub->cache_tree->entry_count; } } it->entry_count = cnt; + + strbuf_release(&subtree_path); } void prime_cache_tree(struct repository *r, struct index_state *istate, struct tree *tree) { + struct strbuf tree_path = STRBUF_INIT; + trace2_region_enter("cache-tree", "prime_cache_tree", r); cache_tree_free(&istate->cache_tree); istate->cache_tree = cache_tree(); - ensure_full_index(istate); - prime_cache_tree_rec(r, istate->cache_tree, tree); + prime_cache_tree_rec(r, istate->cache_tree, tree, &tree_path); + strbuf_release(&tree_path); istate->cache_changed |= CACHE_TREE_CHANGED; trace2_region_leave("cache-tree", "prime_cache_tree", r); } diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index ab4a69302c5040..45da5470bf85a6 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -787,9 +787,9 @@ test_expect_success 'sparse-index is not expanded' ' ensure_not_expanded checkout - && ensure_not_expanded switch rename-out-to-out && ensure_not_expanded switch - && - git -C sparse-index reset --hard && + ensure_not_expanded reset --hard && ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 && - git -C sparse-index reset --hard && + ensure_not_expanded reset --hard && ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 && echo >>sparse-index/README.md && @@ -799,6 +799,17 @@ test_expect_success 'sparse-index is not expanded' ' echo >>sparse-index/untracked.txt && ensure_not_expanded add . && + for ref in update-deep update-folder1 update-folder2 update-deep + do + echo >>sparse-index/README.md && + ensure_not_expanded reset --hard $ref + done && + + ensure_not_expanded reset --hard update-deep && + ensure_not_expanded reset --keep base && + ensure_not_expanded reset --merge update-deep && + ensure_not_expanded reset --hard && + ensure_not_expanded checkout -f update-deep && ( sane_unset GIT_TEST_MERGE_ALGORITHM && From e9331c4c3fb11a3306612ae351bd1861cb8da969 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 31 Aug 2021 15:41:26 -0400 Subject: [PATCH 6/6] sparse-index: make mixed `reset` sparse-aware Due to the introduction of sparse directory entries in the `diff_cache` execution (used internally by `reset --mixed`), explicit `change` and `add_remove` functions are specified for merging of the interior contents of those sparse directories. Additionally, to handle cases in which `reset --mixed` must merge files nested multiple levels deep in directories outside the sparse checkout cone, the `recursive` diff option is added. Signed-off-by: Victoria Dye --- builtin/reset.c | 30 +++++++++++++++++++++++- t/t1092-sparse-checkout-compatibility.sh | 15 ++++++++++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index a456d1e5a3d7e4..f63f5c440158c4 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -195,6 +195,8 @@ static int read_from_tree(const struct pathspec *pathspec, int intent_to_add) { struct diff_options opt; + unsigned int i; + char *skip_worktree_seen = NULL; memset(&opt, 0, sizeof(opt)); copy_pathspec(&opt.pathspec, pathspec); @@ -202,9 +204,35 @@ static int read_from_tree(const struct pathspec *pathspec, opt.format_callback = update_index_from_diff; opt.format_callback_data = &intent_to_add; opt.flags.override_submodule_config = 1; + opt.flags.recursive = 1; opt.repo = the_repository; + opt.change = diff_change; + opt.add_remove = diff_addremove; + + /* + * When pathspec is given for resetting a cone-mode sparse checkout, it may + * identify entries that are nested in sparse directories, in which case the + * index should be expanded. For the sake of efficiency, this check is + * overly-cautious: anything with a wildcard or a magic prefix requires + * expansion, as well as literal paths that aren't in the sparse checkout + * definition AND don't match any directory in the index. + */ + if (pathspec->nr && the_index.sparse_index) { + if (pathspec->magic || pathspec->has_wildcard) { + ensure_full_index(&the_index); + } else { + for (i = 0; i < pathspec->nr; i++) { + if (!path_in_cone_modesparse_checkout(pathspec->items[i].original, &the_index) && + !matches_skip_worktree(pathspec, i, &skip_worktree_seen)) { + ensure_full_index(&the_index); + break; + } + } + } + } + + free(skip_worktree_seen); - ensure_full_index(&the_index); if (do_diff_cache(tree_oid, &opt)) return 1; diffcore_std(&opt); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 45da5470bf85a6..6c8386b1d75396 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -569,7 +569,7 @@ test_expect_success 'reset with sparse directory pathspec outside definition' ' test_all_match git status --porcelain=v2 ' -test_expect_success 'reset with file pathspec outside sparse definition' ' +test_expect_success 'reset with pathspec match in sparse directory' ' init_repos && test_all_match git checkout -b reset-test update-deep && @@ -802,14 +802,25 @@ test_expect_success 'sparse-index is not expanded' ' for ref in update-deep update-folder1 update-folder2 update-deep do echo >>sparse-index/README.md && + ensure_not_expanded reset --mixed $ref ensure_not_expanded reset --hard $ref done && ensure_not_expanded reset --hard update-deep && ensure_not_expanded reset --keep base && ensure_not_expanded reset --merge update-deep && - ensure_not_expanded reset --hard && + ensure_not_expanded reset base -- deep/a && + ensure_not_expanded reset base -- nonexistent-file && + ensure_not_expanded reset deepest -- deep && + + # Although folder1 is outside the sparse definition, it exists as a + # directory entry in the index, so it will be reset without needing to + # expand the full index. + ensure_not_expanded reset --hard update-folder1 && + ensure_not_expanded reset base -- folder1 && + + ensure_not_expanded reset --hard update-deep && ensure_not_expanded checkout -f update-deep && ( sane_unset GIT_TEST_MERGE_ALGORITHM &&