From 1f86b7cb6396ea5f8cd612041938e7d3280e4002 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 7 Oct 2021 21:15:31 +0000 Subject: reset: rename is_missing to !is_in_reset_tree Rename and invert value of `is_missing` to `is_in_reset_tree` to make the variable more descriptive of what it represents. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/reset.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'builtin') diff --git a/builtin/reset.c b/builtin/reset.c index 51c9e2f43f..d3695ce43c 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -131,10 +131,10 @@ static void update_index_from_diff(struct diff_queue_struct *q, for (i = 0; i < q->nr; i++) { struct diff_filespec *one = q->queue[i]->one; - int is_missing = !(one->mode && !is_null_oid(&one->oid)); + int is_in_reset_tree = one->mode && !is_null_oid(&one->oid); struct cache_entry *ce; - if (is_missing && !intent_to_add) { + if (!is_in_reset_tree && !intent_to_add) { remove_file_from_cache(one->path); continue; } @@ -144,7 +144,7 @@ static void update_index_from_diff(struct diff_queue_struct *q, if (!ce) die(_("make_cache_entry failed for path '%s'"), one->path); - if (is_missing) { + if (!is_in_reset_tree) { ce->ce_flags |= CE_INTENT_TO_ADD; set_object_name_for_intent_to_add_entry(ce); } -- cgit v1.3 From 71471b2a7c72415a52e0c84182098e8856ceab7a Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 27 Oct 2021 14:39:17 +0000 Subject: reset: preserve skip-worktree bit in mixed reset Change `update_index_from_diff` to set `skip-worktree` when applicable for new index entries. When `git reset --mixed ` is run, entries in the index with differences between the pre-reset HEAD and reset are identified and handled with `update_index_from_diff`. For each file, a new cache entry in inserted into the index, created from the side of the reset (without changing the working tree). However, the newly-created entry must have `skip-worktree` explicitly set in either of the following scenarios: 1. the file is in the current index and has `skip-worktree` set 2. the file is not in the current index but is outside of a defined sparse checkout definition Not setting the `skip-worktree` bit leads to likely-undesirable results for a user. It causes `skip-worktree` settings to disappear on the "diff"-containing files (but *only* the diff-containing files), leading to those files now showing modifications in `git status`. For example, when running `git reset --mixed` in a sparse checkout, some file entries outside of sparse checkout could show up as deleted, despite the user never deleting anything (and not wanting them on-disk anyway). Additionally, add a test to `t7102` to ensure `skip-worktree` is preserved in a basic `git reset --mixed` scenario and update a failure-documenting test from 19a0acc (t1092: test interesting sparse-checkout scenarios, 2021-01-23) with new expected behavior. Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/reset.c | 14 ++++++++++++++ t/t1092-sparse-checkout-compatibility.sh | 22 ++++++++-------------- t/t7102-reset.sh | 17 +++++++++++++++++ 3 files changed, 39 insertions(+), 14 deletions(-) (limited to 'builtin') diff --git a/builtin/reset.c b/builtin/reset.c index d3695ce43c..e441b6601b 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -25,6 +25,7 @@ #include "cache-tree.h" #include "submodule.h" #include "submodule-config.h" +#include "dir.h" #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) @@ -130,6 +131,7 @@ static void update_index_from_diff(struct diff_queue_struct *q, int intent_to_add = *(int *)data; for (i = 0; i < q->nr; i++) { + int pos; struct diff_filespec *one = q->queue[i]->one; int is_in_reset_tree = one->mode && !is_null_oid(&one->oid); struct cache_entry *ce; @@ -141,6 +143,18 @@ static void update_index_from_diff(struct diff_queue_struct *q, ce = make_cache_entry(&the_index, one->mode, &one->oid, one->path, 0, 0); + + /* + * If the file 1) corresponds to an existing index entry with + * skip-worktree set, or 2) does not exist in the index but is + * outside the sparse checkout definition, add a skip-worktree bit + * to the new index entry. + */ + pos = cache_name_pos(one->path, strlen(one->path)); + if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) || + (pos < 0 && !path_in_sparse_checkout(one->path, &the_index))) + ce->ce_flags |= CE_SKIP_WORKTREE; + if (!ce) die(_("make_cache_entry failed for path '%s'"), one->path); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 886e78715f..c7449afe96 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -459,26 +459,20 @@ test_expect_failure 'blame with pathspec outside sparse definition' ' test_all_match git blame deep/deeper2/deepest/a ' -# NEEDSWORK: a sparse-checkout behaves differently from a full checkout -# in this scenario, but it shouldn't. -test_expect_failure 'checkout and reset (mixed)' ' +test_expect_success 'checkout and reset (mixed)' ' init_repos && test_all_match git checkout -b reset-test update-deep && test_all_match git reset deepest && - test_all_match git reset update-folder1 && - test_all_match git reset update-folder2 -' - -# NEEDSWORK: a sparse-checkout behaves differently from a full checkout -# in this scenario, but it shouldn't. -test_expect_success 'checkout and reset (mixed) [sparse]' ' - init_repos && - test_sparse_match git checkout -b reset-test update-deep && - test_sparse_match git reset deepest && + # Because skip-worktree is preserved, resetting to update-folder1 + # will show worktree changes for folder1/a in full-checkout, but not + # in sparse-checkout or sparse-index. + git -C full-checkout reset update-folder1 >full-checkout-out && test_sparse_match git reset update-folder1 && - test_sparse_match git reset update-folder2 + grep "M folder1/a" full-checkout-out && + ! grep "M folder1/a" sparse-checkout-out && + run_on_sparse test_path_is_missing folder1 ' test_expect_success 'merge, cherry-pick, and rebase' ' diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 601b2bf97f..d05426062e 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -472,6 +472,23 @@ test_expect_success '--mixed refreshes the index' ' test_cmp expect output ' +test_expect_success '--mixed preserves skip-worktree' ' + echo 123 >>file2 && + git add file2 && + git update-index --skip-worktree file2 && + git reset --mixed HEAD >output && + test_must_be_empty output && + + cat >expect <<-\EOF && + Unstaged changes after reset: + M file2 + EOF + git update-index --no-skip-worktree file2 && + git add file2 && + git reset --mixed HEAD >output && + test_cmp expect output +' + test_expect_success 'resetting specific path that is unmerged' ' git rm --cached file2 && F1=$(git rev-parse HEAD:file1) && -- cgit v1.3 From c01b1cbd473c05e29fe0c1a31b8f6539e82c4559 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 29 Nov 2021 15:52:40 +0000 Subject: reset: integrate with sparse index Disable `command_requires_full_index` repo setting and add `ensure_full_index` guards around code paths that cannot yet use sparse directory index entries. `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 expanded to prevent cache tree corruption and invalid variable accesses. 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()` must be run *after* enabling sparse index for the command (so that the index is not expanded unnecessarily) and *before* using the index for reset (so that it is verified as uncorrupted). Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/reset.c | 10 +++++++--- cache-tree.c | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) (limited to 'builtin') diff --git a/builtin/reset.c b/builtin/reset.c index e441b6601b..0ac0de7dc9 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -180,6 +180,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); @@ -257,9 +258,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), @@ -405,6 +403,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 90919f9e34..9be19c85b6 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -776,6 +776,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", the_repository); -- cgit v1.3 From 4d1cfc1351ffec47bba1318e9cd1ed13c5182951 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 29 Nov 2021 15:52:42 +0000 Subject: reset: make --mixed sparse-aware Remove the `ensure_full_index` guard on `read_from_tree` and update `git reset --mixed` to ensure it can use sparse directory index entries wherever possible. Sparse directory entries are reset using `diff_tree_oid`, which requires `change` and `add_remove` functions to process the internal contents of the sparse directory. The `recursive` diff option handles cases in which `reset --mixed` must diff/merge files that are nested multiple levels deep in a sparse directory. The use of pathspecs with `git reset --mixed` introduces scenarios in which internal contents of sparse directories may be matched by the pathspec. In order to reset *all* files in the repo that may match the pathspec, the following conditions on the pathspec require index expansion before performing the reset: * "magic" pathspecs * wildcard pathspecs that do not match only in-cone files or entire sparse directories * literal pathspecs matching something outside the sparse checkout definition Helped-by: Elijah Newren Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/reset.c | 87 +++++++++++++++++++++++++++++++- t/t1092-sparse-checkout-compatibility.sh | 17 +++++++ 2 files changed, 102 insertions(+), 2 deletions(-) (limited to 'builtin') diff --git a/builtin/reset.c b/builtin/reset.c index 0ac0de7dc9..dcb79fb43a 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -148,7 +148,9 @@ static void update_index_from_diff(struct diff_queue_struct *q, * If the file 1) corresponds to an existing index entry with * skip-worktree set, or 2) does not exist in the index but is * outside the sparse checkout definition, add a skip-worktree bit - * to the new index entry. + * to the new index entry. Note that a sparse index will be expanded + * if this entry is outside the sparse cone - this is necessary + * to properly construct the reset sparse directory. */ pos = cache_name_pos(one->path, strlen(one->path)); if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) || @@ -166,6 +168,82 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } +static int pathspec_needs_expanded_index(const struct pathspec *pathspec) +{ + unsigned int i, pos; + int res = 0; + char *skip_worktree_seen = NULL; + + /* + * When using a magic pathspec, assume for the sake of simplicity that + * the index needs to be expanded to match all matchable files. + */ + if (pathspec->magic) + return 1; + + for (i = 0; i < pathspec->nr; i++) { + struct pathspec_item item = pathspec->items[i]; + + /* + * If the pathspec item has a wildcard, the index should be expanded + * if the pathspec has the possibility of matching a subset of entries inside + * of a sparse directory (but not the entire directory). + * + * If the pathspec item is a literal path, the index only needs to be expanded + * if a) the pathspec isn't in the sparse checkout cone (to make sure we don't + * expand for in-cone files) and b) it doesn't match any sparse directories + * (since we can reset whole sparse directories without expanding them). + */ + if (item.nowildcard_len < item.len) { + /* + * 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. + */ + if (path_in_cone_mode_sparse_checkout(item.original, &the_index) && + strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len) + continue; + + for (pos = 0; pos < active_nr; pos++) { + struct cache_entry *ce = active_cache[pos]; + + if (!S_ISSPARSEDIR(ce->ce_mode)) + continue; + + /* + * If the pre-wildcard length is longer than the sparse + * directory name and the sparse directory is the first + * component of the pathspec, need to expand the index. + */ + if (item.nowildcard_len > ce_namelen(ce) && + !strncmp(item.original, ce->name, ce_namelen(ce))) { + res = 1; + break; + } + + /* + * If the pre-wildcard length is shorter than the sparse + * directory and the pathspec does not match the whole + * directory, need to expand the index. + */ + if (!strncmp(item.original, ce->name, item.nowildcard_len) && + wildmatch(item.original, ce->name, 0)) { + res = 1; + break; + } + } + } else if (!path_in_cone_mode_sparse_checkout(item.original, &the_index) && + !matches_skip_worktree(pathspec, i, &skip_worktree_seen)) + res = 1; + + if (res > 0) + break; + } + + free(skip_worktree_seen); + return res; +} + static int read_from_tree(const struct pathspec *pathspec, struct object_id *tree_oid, int intent_to_add) @@ -178,9 +256,14 @@ 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; + + if (pathspec->nr && the_index.sparse_index && pathspec_needs_expanded_index(pathspec)) + ensure_full_index(&the_index); - 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 871cc3fcb8..77e302a0ef 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -795,11 +795,28 @@ test_expect_success 'sparse-index is not expanded' ' ensure_not_expanded reset --hard $ref || return 1 done && + ensure_not_expanded reset --mixed base && 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 the pathspec will not force the + # index to be expanded. + ensure_not_expanded reset deepest -- folder1 && + ensure_not_expanded reset deepest -- folder1/ && + + # Wildcard identifies only in-cone files, no index expansion + ensure_not_expanded reset deepest -- deep/\* && + + # Wildcard identifies only full sparse directories, no index expansion + ensure_not_expanded reset deepest -- folder\* && + ensure_not_expanded checkout -f update-deep && test_config -C sparse-index pull.twohead ort && ( -- cgit v1.3