From 95aa0fd0405b2862735d8a2712369bcce33c0cd4 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 6 Oct 2021 13:13:50 -0400 Subject: [PATCH 1/4] 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 --- builtin/reset.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 7b0d3169cf0424..428f2ffc858963 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -144,8 +144,8 @@ 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; struct diff_filespec *two = q->queue[i]->two; - int is_missing = !(one->mode && !is_null_oid(&one->oid)); int was_missing = !two->mode && is_null_oid(&two->oid); + int is_in_reset_tree = one->mode && !is_null_oid(&one->oid); struct cache_entry *ce; struct cache_entry *ceBefore; struct checkout state = CHECKOUT_INIT; @@ -163,7 +163,7 @@ static void update_index_from_diff(struct diff_queue_struct *q, if (core_apply_sparse_checkout && !file_exists(two->path)) { pos = cache_name_pos(two->path, strlen(two->path)); - if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) && (is_missing || !was_missing)) + if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) && (is_in_reset_tree || !was_missing)) { state.force = 1; state.refresh_cache = 1; @@ -178,7 +178,7 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } - if (is_missing && !intent_to_add) { + if (!is_in_reset_tree && !intent_to_add) { remove_file_from_cache(one->path); continue; } @@ -188,7 +188,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); } From 74ac47e2989685ba10ca8c3e02bc153739658741 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 11 Oct 2021 15:53:13 -0400 Subject: [PATCH 2/4] 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 --- builtin/reset.c | 49 ++++++-------------- t/t1092-sparse-checkout-compatibility.sh | 16 ++----- t/t7102-reset.sh | 17 +++++++ t/t7114-reset-sparse-checkout.sh | 58 ------------------------ 4 files changed, 36 insertions(+), 104 deletions(-) delete mode 100755 t/t7114-reset-sparse-checkout.sh diff --git a/builtin/reset.c b/builtin/reset.c index 428f2ffc858963..4f9d23550e8acc 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -28,7 +28,6 @@ #include "strbuf.h" #include "quote.h" #include "dir.h" -#include "entry.h" #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) @@ -138,45 +137,13 @@ static void update_index_from_diff(struct diff_queue_struct *q, struct diff_options *opt, void *data) { int i; - int pos; int intent_to_add = *(int *)data; for (i = 0; i < q->nr; i++) { + int pos; struct diff_filespec *one = q->queue[i]->one; - struct diff_filespec *two = q->queue[i]->two; - int was_missing = !two->mode && is_null_oid(&two->oid); int is_in_reset_tree = one->mode && !is_null_oid(&one->oid); struct cache_entry *ce; - struct cache_entry *ceBefore; - struct checkout state = CHECKOUT_INIT; - - /* - * When using the sparse-checkout feature the cache entries that are - * added here will not have the skip-worktree bit set. - * Without this code there is data that is lost because the files that - * would normally be in the working directory are not there and show as - * deleted for the next status or in the case of added files just disappear. - * We need to create the previous version of the files in the working - * directory so that they will have the right content and the next - * status call will show modified or untracked files correctly. - */ - if (core_apply_sparse_checkout && !file_exists(two->path)) - { - pos = cache_name_pos(two->path, strlen(two->path)); - if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) && (is_in_reset_tree || !was_missing)) - { - state.force = 1; - state.refresh_cache = 1; - state.istate = &the_index; - ceBefore = make_cache_entry(&the_index, two->mode, &two->oid, two->path, - 0, 0); - if (!ceBefore) - die(_("make_cache_entry failed for path '%s'"), - two->path); - - checkout_entry(ceBefore, &state, NULL, NULL); - } - } if (!is_in_reset_tree && !intent_to_add) { remove_file_from_cache(one->path); @@ -185,6 +152,20 @@ 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. 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])) || + (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 f37e75ca34b538..b81c574acc3954 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -528,25 +528,17 @@ test_expect_success 'blame with pathspec outside sparse definition' ' test_sparse_match test_must_fail git blame deep/deeper2/deepest/a ' -# TODO: This behaves correctly in microsoft/git. Why? 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 full-checkout that are not present + # in sparse-checkout or sparse-index. test_sparse_match git reset update-folder1 && - test_sparse_match git reset update-folder2 + run_on_sparse test_path_is_missing folder1 ' # NEEDSWORK: with mixed reset, files with differences between HEAD and diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 601b2bf97f0e7f..d05426062ec29d 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) && diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh deleted file mode 100755 index c46cbdb64e4ebc..00000000000000 --- a/t/t7114-reset-sparse-checkout.sh +++ /dev/null @@ -1,58 +0,0 @@ -#!/bin/sh - -test_description='reset when using a sparse-checkout' - -. ./test-lib.sh - -# reset using a sparse-checkout file - -test_expect_success 'setup' ' - test_tick && - echo "checkout file" >c && - echo "modify file" >m && - echo "delete file" >d && - git add . && - git commit -m "initial commit" && - echo "added file" >a && - echo "modification of a file" >m && - git rm d && - git add . && - git commit -m "second commit" && - git checkout -b endCommit -' - -test_expect_success 'reset when there is a sparse-checkout' ' - echo "/c" >.git/info/sparse-checkout && - test_config core.sparsecheckout true && - git checkout -B resetBranch && - test_path_is_missing m && - test_path_is_missing a && - test_path_is_missing d && - git reset HEAD~1 && - test "checkout file" = "$(cat c)" && - test "modification of a file" = "$(cat m)" && - test "added file" = "$(cat a)" && - test_path_is_missing d -' - -test_expect_success 'reset after deleting file without skip-worktree bit' ' - git checkout -f endCommit && - git clean -xdf && - echo "/c -/m" >.git/info/sparse-checkout && - test_config core.sparsecheckout true && - git checkout -B resetAfterDelete && - test_path_is_file m && - test_path_is_missing a && - test_path_is_missing d && - rm -f m && - git reset HEAD~1 && - test "checkout file" = "$(cat c)" && - test "added file" = "$(cat a)" && - test_path_is_missing m && - test_path_is_missing d -' - - - -test_done From 4b145fbd4431db96c2366fba5b2f3764200d0d66 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 27 Sep 2021 15:39:43 -0400 Subject: [PATCH 3/4] fixup! reset: expand test coverage for sparse checkouts Add new tests for `--merge` and `--keep` modes, as well as mixed reset with pathspecs. New performance test cases exercise various execution paths for `reset`. Co-authored-by: Derrick Stolee Signed-off-by: Derrick Stolee Signed-off-by: Victoria Dye --- t/t1092-sparse-checkout-compatibility.sh | 59 ++++++++---------------- 1 file changed, 18 insertions(+), 41 deletions(-) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index b81c574acc3954..83b64483363616 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -541,32 +541,6 @@ test_expect_success 'checkout and reset (mixed)' ' run_on_sparse test_path_is_missing folder1 ' -# 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 'checkout and reset (merge)' ' init_repos && @@ -621,31 +595,34 @@ test_expect_success 'reset with pathspecs inside sparse definition' ' test_all_match git status --porcelain=v2 ' -test_expect_success 'reset with sparse directory pathspec outside definition' ' +# Although the working tree differs between full and sparse checkouts after +# reset, the state of the index is the same. +test_expect_success 'reset with pathspecs outside sparse definition' ' init_repos && + test_all_match git checkout -b reset-test base && - 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 pathspec match in sparse directory' ' - init_repos && + 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 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_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_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_all_match git status --porcelain=v2 && + test_all_match git rev-parse HEAD:folder1/a && + + test_all_match git reset base -- folder\* && + test_all_match git status --porcelain=v2 && + test_all_match git rev-parse HEAD:folder2 ' # NEEDSWORK: although update-index executes without error on files outside From 26d9bfffb46eaf51ef9780d5cb48cd444a8a3d7b Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 11 Oct 2021 15:18:35 -0400 Subject: [PATCH 4/4] sparse-index: fix index.sparse inline usage Update behavior when the `index.sparse` config setting is provided inline to a command. Previously would run command on sparse index (unless otherwise expanded), then write out as sparse. Now, the index will be expanded in the process of reading it in. Co-authored-by: Derrick Stolee Signed-off-by: Victoria Dye --- read-cache.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/read-cache.c b/read-cache.c index 9aaba14073672c..ebe88cd039dd1c 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2405,7 +2405,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) if (!istate->repo) istate->repo = the_repository; prepare_repo_settings(istate->repo); - if (istate->repo->settings.command_requires_full_index) + if (!istate->repo->settings.sparse_index || + istate->repo->settings.command_requires_full_index) ensure_full_index(istate); return istate->cache_nr;