[PATCH 0/7] Sparse Index: integrate with reset

This series integrates the sparse index with git reset and provides
miscellaneous fixes and improvements to the command in sparse checkouts.
This includes:

 1. tests added to t1092 and p2000 to establish the baseline functionality
    of the command
 2. repository settings to enable the sparse index with ensure_full_index
    guarding any code paths that break tests without other compatibility
    updates.
 3. modifications to remove or reduce the scope in which ensure_full_index
    must be called.

The sparse index updates are predicated on a fix originating from the
microsoft/git fork [1], correcting how git reset --mixed handles resetting
entries outside the sparse checkout definition. Additionally, a performance
"bug" in next_cache_entry with sparse index is corrected, preventing
repeatedly looping over already-searched entries.

The p2000 tests demonstrate an overall ~70% execution time reduction across
all tested usages of git reset using a sparse index:

Test                                               before   after       
------------------------------------------------------------------------
2000.22: git reset (full-v3)                       0.48     0.51 +6.3% 
2000.23: git reset (full-v4)                       0.47     0.50 +6.4% 
2000.24: git reset (sparse-v3)                     0.93     0.30 -67.7%
2000.25: git reset (sparse-v4)                     0.94     0.29 -69.1%
2000.26: git reset --hard (full-v3)                0.69     0.68 -1.4% 
2000.27: git reset --hard (full-v4)                0.75     0.68 -9.3% 
2000.28: git reset --hard (sparse-v3)              1.29     0.34 -73.6%
2000.29: git reset --hard (sparse-v4)              1.31     0.34 -74.0%
2000.30: git reset -- does-not-exist (full-v3)     0.54     0.51 -5.6% 
2000.31: git reset -- does-not-exist (full-v4)     0.54     0.52 -3.7% 
2000.32: git reset -- does-not-exist (sparse-v3)   1.02     0.31 -69.6%
2000.33: git reset -- does-not-exist (sparse-v4)   1.07     0.30 -72.0%


[1] microsoft@6b8a074

Thanks! -Victoria

Kevin Willford (1):
  reset: behave correctly with sparse-checkout

Victoria Dye (6):
  sparse-index: update command for expand/collapse test
  reset: expand test coverage for sparse checkouts
  reset: integrate with sparse index
  reset: make sparse-aware (except --mixed)
  reset: make --mixed sparse-aware
  unpack-trees: improve performance of next_cache_entry

 builtin/reset.c                          |  77 ++++++++++++-
 cache-tree.c                             |  43 ++++++-
 cache.h                                  |  10 ++
 read-cache.c                             |  22 ++--
 t/perf/p2000-sparse-operations.sh        |   3 +
 t/t1092-sparse-checkout-compatibility.sh | 139 ++++++++++++++++++++++-
 t/t7114-reset-sparse-checkout.sh         |  61 ++++++++++
 unpack-trees.c                           |  23 +++-
 8 files changed, 353 insertions(+), 25 deletions(-)
 create mode 100755 t/t7114-reset-sparse-checkout.sh


base-commit: cefe983a320c03d7843ac78e73bd513a27806845
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1048%2Fvdye%2Fvdye%2Fsparse-index-part1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1048/vdye/vdye/sparse-index-part1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1048
-- 
gitgitgadget

From: Kevin Willford <redacted>

When using the sparse checkout feature, 'git reset' will add entries to
the index that will have the skip-worktree bit off but will leave the
working directory empty. File data is lost because the index version of
the files has been changed but there is nothing that is in the working
directory. This will cause the next 'git status' call to show either
deleted for files modified or deleting or nothing for files added. The
added files should be shown as untracked and modified files should be
shown as modified.

To fix this when the reset is running if there is not a file in the
working directory and if it will be missing with the new index entry or
was not missing in the previous version, we create the previous index
version of the file in the working directory so that status will report
correctly and the files will be availble for the user to deal with.

This fixes a documented failure from t1092 that was created in 19a0acc
(t1092: test interesting sparse-checkout scenarios, 2021-01-23).

Signed-off-by: Kevin Willford <redacted>
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Victoria Dye <redacted>
---
 builtin/reset.c                          | 39 +++++++++++++++
 t/t1092-sparse-checkout-compatibility.sh |  4 +-
 t/t7114-reset-sparse-checkout.sh         | 61 ++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 3 deletions(-)
 create mode 100755 t/t7114-reset-sparse-checkout.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index 51c9e2f43ff..8ffcd713720 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -25,6 +25,8 @@
 #include "cache-tree.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "dir.h"
+#include "entry.h"
 
 #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
 

@@ -127,12 +129,49 @@ 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++) {
 		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);
 		struct cache_entry *ce;
+		struct cache_entry *ce_before;
+		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.
+		 * In the case of added files, they 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_missing || !was_missing)) {
+				state.force = 1;
+				state.refresh_cache = 1;
+				state.istate = &the_index;
+
+				ce_before = make_cache_entry(&the_index, two->mode,
+							     &two->oid, two->path,
+							     0, 0);
+				if (!ce_before)
+					die(_("make_cache_entry failed for path '%s'"),
+						two->path);
+
+				checkout_entry(ce_before, &state, NULL, NULL);
+			}
+		}
 
 		if (is_missing && !intent_to_add) {
 			remove_file_from_cache(one->path);

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 886e78715fe..c5977152661 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -459,9 +459,7 @@ 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 &&

diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
new file mode 100755
index 00000000000..a8029707fb1
--- /dev/null
+++ b/t/t7114-reset-sparse-checkout.sh

@@ -0,0 +1,61 @@
+#!/bin/sh
+
+test_description='reset when using a sparse-checkout'
+
+. ./test-lib.sh
+
+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 &&
+	echo "checkout file" >expect &&
+	test_cmp expect c &&
+	echo "added file" >expect &&
+	test_cmp expect a &&
+	echo "modification of a file" >expect &&
+	test_cmp expect m &&
+	test_path_is_missing d
+'
+
+test_expect_success 'reset after deleting file without skip-worktree bit' '
+	git checkout -f endCommit &&
+	git clean -xdf &&
+	cat >.git/info/sparse-checkout <<-\EOF &&
+	/c
+	/m
+	EOF
+	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 &&
+	echo "checkout file" >expect &&
+	test_cmp expect c &&
+	echo "added file" >expect &&
+	test_cmp expect a &&
+	test_path_is_missing m &&
+	test_path_is_missing d
+'
+
+test_done

-- 
gitgitgadget

From: Victoria Dye <redacted>

In anticipation of multiple commands being fully integrated with sparse
index, update the test for index expand and collapse for non-sparse index
integrated commands to use `mv`.

Signed-off-by: Victoria Dye <redacted>
---
 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 c5977152661..aed8683e629 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -642,7 +642,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="" mv a b &&
 	test_region index convert_to_sparse trace2.txt &&
 	test_region index ensure_full_index trace2.txt
 '

-- 
gitgitgadget

On Thu, Sep 30, 2021 at 02:50:56PM +0000, Victoria Dye via GitGitGadget wrote:

Double-checking my understanding as somebody who is not so familiar with
t1092: this test is to ensure that commands which don't yet understand
the sparse index can temporarily expand it in order to do their work?

If so, makes sense to me. And renaming 'a' to 'b' is arbitrary and fine
to do since we end up recreating the sparse-index repository each time
via init_repos.

Looks good to me.

Thanks,
Taylor

Taylor Blau wrote:

On Thu, Sep 30, 2021 at 02:50:56PM +0000, Victoria Dye via GitGitGadget wrote:

quoted

From: Victoria Dye <redacted>

In anticipation of multiple commands being fully integrated with sparse
index, update the test for index expand and collapse for non-sparse index
integrated commands to use `mv`.

Signed-off-by: Victoria Dye <redacted>
---
 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 c5977152661..aed8683e629 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -642,7 +642,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="" mv a b &&

Double-checking my understanding as somebody who is not so familiar with
t1092: this test is to ensure that commands which don't yet understand
the sparse index can temporarily expand it in order to do their work?

Exactly - if a command doesn't explicitly enable use of the sparse index by
setting `command_requires_full_index` to 0, the index is expanded if/when it
is first read during the command's execution and collapsed if/when it is
written to disk. This test makes sure that mechanism works as intended.

-Victoria

On 30/09/21 21.50, Victoria Dye via GitGitGadget wrote:

From: Victoria Dye <redacted>

In anticipation of multiple commands being fully integrated with sparse
index, update the test for index expand and collapse for non-sparse index
integrated commands to use `mv`.

We can say "use git sparse-index mv instead of git sparse-index reset".

Why is mv used for this case?

-- 
An old man doll... just what I always wanted! - Clara

From: Victoria Dye <redacted>

Add new tests for `--merge` and `--keep` modes, as well as mixed reset with
pathspecs both inside and outside of the sparse checkout definition. New
performance test cases exercise various execution paths for `reset`.

Co-authored-by: Derrick Stolee [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Victoria Dye <redacted>
---
 t/perf/p2000-sparse-operations.sh        |   3 +
 t/t1092-sparse-checkout-compatibility.sh | 107 +++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 597626276fb..bfd332120c8 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh

@@ -110,5 +110,8 @@ test_perf_on_all git add -A
 test_perf_on_all git add .
 test_perf_on_all git commit -a -m A
 test_perf_on_all git checkout -f -
+test_perf_on_all git reset
+test_perf_on_all git reset --hard
+test_perf_on_all git reset -- does-not-exist
 
 test_done

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index aed8683e629..e36fb18098d 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -479,6 +479,113 @@ 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 <commit>
+# 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 &&
+
+	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 pathspec match in sparse directory' '
+	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 &&
 

-- 
gitgitgadget

From: Victoria Dye <redacted>

`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 resetting.

Signed-off-by: Victoria Dye <redacted>
---
 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 8ffcd713720..92b9a3815c7 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -205,6 +205,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);

@@ -282,9 +283,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),

@@ -430,6 +428,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 90919f9e345..9be19c85b66 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);

-- 
gitgitgadget

From: Victoria Dye <redacted>

In order to accurately reconstruct the cache tree in `prime_cache_tree_rec`,
the function must determine whether the currently-processing directory in
the tree is sparse or not. If it is not sparse, the tree is parsed and
subtree recursively constructed. If it is sparse, no subtrees are added to
the tree and the entry count is set to 1 (representing the sparse directory
itself).

Signed-off-by: Victoria Dye <redacted>
---
 cache-tree.c                             | 44 +++++++++++++++++++++---
 cache.h                                  | 10 ++++++
 read-cache.c                             | 22 ++++++++----
 t/t1092-sparse-checkout-compatibility.sh | 15 ++++++--
 4 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 9be19c85b66..9021669d682 100644
--- a/cache-tree.c
+++ b/cache-tree.c

@@ -740,15 +740,29 @@ out:
 	return ret;
 }
 
+static void prime_cache_tree_sparse_dir(struct repository *r,
+					struct cache_tree *it,
+					struct tree *tree,
+					struct strbuf *tree_path)
+{
+
+	oidcpy(&it->oid, &tree->object.oid);
+	it->entry_count = 1;
+	return;
+}
+
 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);
+
 	init_tree_desc(&desc, tree->buffer, tree->size);
 	cnt = 0;
 	while (tree_entry(&desc, &entry)) {

@@ -757,27 +771,49 @@ 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, '/');
+
+			/*
+			 * If a sparse index is in use, the directory being processed may be
+			 * sparse. To confirm that, we can check whether an entry with that
+			 * exact name exists in the index. If it does, the created subtree
+			 * should be sparse. Otherwise, cache tree expansion should continue
+			 * as normal.
+			 */
+			if (r->index->sparse_index &&
+			    index_entry_exists(r->index, subtree_path.buf, subtree_path.len))
+				prime_cache_tree_sparse_dir(r, sub->cache_tree, subtree, &subtree_path);
+			else
+				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", the_repository);
 	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", the_repository);
 }

diff --git a/cache.h b/cache.h
index f6295f3b048..1d3e4665562 100644
--- a/cache.h
+++ b/cache.h

@@ -816,6 +816,16 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na
  */
 int index_name_pos(struct index_state *, const char *name, int namelen);
 
+/*
+ * Determines whether an entry with the given name exists within the
+ * given index. The return value is 1 if an exact match is found, otherwise
+ * it is 0. Note that, unlike index_name_pos, this function does not expand
+ * the index if it is sparse. If an item exists within the full index but it
+ * is contained within a sparse directory (and not in the sparse index), 0 is
+ * returned.
+ */
+int index_entry_exists(struct index_state *, const char *name, int namelen);
+
 /*
  * Some functions return the negative complement of an insert position when a
  * precise match was not found but a position was found where the entry would

diff --git a/read-cache.c b/read-cache.c
index f5d4385c408..ea1166895f8 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -551,7 +551,10 @@ int cache_name_stage_compare(const char *name1, int len1, int stage1, const char
 	return 0;
 }
 
-static int index_name_stage_pos(struct index_state *istate, const char *name, int namelen, int stage)
+static int index_name_stage_pos(struct index_state *istate,
+				const char *name, int namelen,
+				int stage,
+				int search_sparse)
 {
 	int first, last;
 

@@ -570,7 +573,7 @@ static int index_name_stage_pos(struct index_state *istate, const char *name, in
 		first = next+1;
 	}
 
-	if (istate->sparse_index &&
+	if (search_sparse && istate->sparse_index &&
 	    first > 0) {
 		/* Note: first <= istate->cache_nr */
 		struct cache_entry *ce = istate->cache[first - 1];

@@ -586,7 +589,7 @@ static int index_name_stage_pos(struct index_state *istate, const char *name, in
 		    ce_namelen(ce) < namelen &&
 		    !strncmp(name, ce->name, ce_namelen(ce))) {
 			ensure_full_index(istate);
-			return index_name_stage_pos(istate, name, namelen, stage);
+			return index_name_stage_pos(istate, name, namelen, stage, search_sparse);
 		}
 	}
 

@@ -595,7 +598,12 @@ static int index_name_stage_pos(struct index_state *istate, const char *name, in
 
 int index_name_pos(struct index_state *istate, const char *name, int namelen)
 {
-	return index_name_stage_pos(istate, name, namelen, 0);
+	return index_name_stage_pos(istate, name, namelen, 0, 1);
+}
+
+int index_entry_exists(struct index_state *istate, const char *name, int namelen)
+{
+	return index_name_stage_pos(istate, name, namelen, 0, 0) >= 0;
 }
 
 int remove_index_entry_at(struct index_state *istate, int pos)

@@ -1222,7 +1230,7 @@ static int has_dir_name(struct index_state *istate,
 			 */
 		}
 
-		pos = index_name_stage_pos(istate, name, len, stage);
+		pos = index_name_stage_pos(istate, name, len, stage, 1);
 		if (pos >= 0) {
 			/*
 			 * Found one, but not so fast.  This could

@@ -1322,7 +1330,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
 		pos = index_pos_to_insert_pos(istate->cache_nr);
 	else
-		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
+		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), 1);
 
 	/* existing match? Just replace it. */
 	if (pos >= 0) {

@@ -1357,7 +1365,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 		if (!ok_to_replace)
 			return error(_("'%s' appears as both a file and as a directory"),
 				     ce->name);
-		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
+		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), 1);
 		pos = -pos-1;
 	}
 	return pos + 1;

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index e36fb18098d..0b6ff0de17d 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -786,9 +786,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 &&

@@ -798,6 +798,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 || return 1
+	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 &&
 	test_config -C sparse-index pull.twohead ort &&
 	(

-- 
gitgitgadget

From: Victoria Dye <redacted>

Sparse directory entries are "diffed" as trees in `diff_cache` (used
internally by `reset --mixed`), following a code path separate from
individual file handling. The use of `diff_tree_oid` there requires setting
explicit `change` and `add_remove` functions to process the internal
contents of a sparse directory.

Additionally, the `recursive` diff option handles cases in which `reset
--mixed` must diff/merge files that are nested multiple levels deep in a
sparse directory.

Signed-off-by: Victoria Dye <redacted>
---
 builtin/reset.c                          | 30 +++++++++++++++++++++++-
 t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++-
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 92b9a3815c7..2d95ce76f20 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -196,6 +196,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);

@@ -203,9 +205,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_mode_sparse_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 0b6ff0de17d..c9b9ef4992c 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -801,14 +801,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 || return 1
 	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 &&
 	test_config -C sparse-index pull.twohead ort &&
 	(

-- 
gitgitgadget

Victoria Dye via GitGitGadget wrote:

This is a bug - it's missing `&&` at the end of the line (and adding it will
cause the test to fail). The index is expanded if a mixed reset modifies an
entry outside the sparse cone, so I'll update the test in V2 to verify reset
between two refs with only in-cone files changed between them. 

From: Victoria Dye <redacted>

To find the first non-unpacked cache entry, `next_cache_entry` iterates
through index, starting at `cache_bottom`. The performance of this in full
indexes is helped by `cache_bottom` advancing with each invocation of
`mark_ce_used` (called by `unpack_index_entry`). However, the presence of
sparse directories can prevent the `cache_bottom` from advancing in a sparse
index case, effectively forcing `next_cache_entry` to search from the
beginning of the index each time it is called.

The `cache_bottom` must be preserved for the sparse index (see 17a1bb570b
(unpack-trees: preserve cache_bottom, 2021-07-14)).  Therefore, to retain
the benefit `cache_bottom` provides in non-sparse index cases, a separate
`hint` position indicates the first position `next_cache_entry` should
search, updated each execution with a new position.  The performance of `git
reset -- does-not-exist` (testing the "worst case" in which all entries in
the index are unpacked with `next_cache_entry`) is significantly improved
for the sparse index case:

Test          before            after
------------------------------------------------------
(full-v3)     0.79(0.38+0.30)   0.91(0.43+0.34) +15.2%
(full-v4)     0.80(0.38+0.29)   0.85(0.40+0.35) +6.2%
(sparse-v3)   0.76(0.43+0.69)   0.44(0.08+0.67) -42.1%
(sparse-v4)   0.71(0.40+0.65)   0.41(0.09+0.65) -42.3%

Signed-off-by: Victoria Dye <redacted>
---
 unpack-trees.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 8ea0a542da8..b94733de6be 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -645,17 +645,24 @@ static void mark_ce_used_same_name(struct cache_entry *ce,
 	}
 }
 
-static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
+static struct cache_entry *next_cache_entry(struct unpack_trees_options *o, int *hint)
 {
 	const struct index_state *index = o->src_index;
 	int pos = o->cache_bottom;
 
+	if (*hint > pos)
+		pos = *hint;
+
 	while (pos < index->cache_nr) {
 		struct cache_entry *ce = index->cache[pos];
-		if (!(ce->ce_flags & CE_UNPACKED))
+		if (!(ce->ce_flags & CE_UNPACKED)) {
+			*hint = pos + 1;
 			return ce;
+		}
 		pos++;
 	}
+
+	*hint = pos;
 	return NULL;
 }
 

@@ -1365,12 +1372,13 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 
 	/* Are we supposed to look at the index too? */
 	if (o->merge) {
+		int hint = -1;
 		while (1) {
 			int cmp;
 			struct cache_entry *ce;
 
 			if (o->diff_index_cached)
-				ce = next_cache_entry(o);
+				ce = next_cache_entry(o, &hint);
 			else
 				ce = find_cache_entry(info, p);
 

@@ -1690,7 +1698,7 @@ static int verify_absent(const struct cache_entry *,
 int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
 {
 	struct repository *repo = the_repository;
-	int i, ret;
+	int i, hint, ret;
 	static struct cache_entry *dfc;
 	struct pattern_list pl;
 	int free_pattern_list = 0;

@@ -1763,13 +1771,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		info.pathspec = o->pathspec;
 
 		if (o->prefix) {
+			hint = -1;
+
 			/*
 			 * Unpack existing index entries that sort before the
 			 * prefix the tree is spliced into.  Note that o->merge
 			 * is always true in this case.
 			 */
 			while (1) {
-				struct cache_entry *ce = next_cache_entry(o);
+				struct cache_entry *ce = next_cache_entry(o, &hint);
 				if (!ce)
 					break;
 				if (ce_in_traverse_path(ce, &info))

@@ -1790,8 +1800,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 
 	/* Any left-over entries in the index? */
 	if (o->merge) {
+		hint = -1;
 		while (1) {
-			struct cache_entry *ce = next_cache_entry(o);
+			struct cache_entry *ce = next_cache_entry(o, &hint);
 			if (!ce)
 				break;
 			if (unpack_index_entry(ce, o) < 0)

-- 
gitgitgadget

This series integrates the sparse index with git reset and provides
miscellaneous fixes and improvements to the command in sparse checkouts.
This includes:

 1. tests added to t1092 and p2000 to establish the baseline functionality
    of the command
 2. repository settings to enable the sparse index with ensure_full_index
    guarding any code paths that break tests without other compatibility
    updates.
 3. modifications to remove or reduce the scope in which ensure_full_index
    must be called.

The sparse index updates are predicated on a fix originating from the
microsoft/git fork [1], correcting how git reset --mixed handles resetting
entries outside the sparse checkout definition. Additionally, a performance
"bug" in next_cache_entry with sparse index is corrected, preventing
repeatedly looping over already-searched entries.

The p2000 tests demonstrate an overall ~70% execution time reduction across
all tested usages of git reset using a sparse index:

Test                                               before   after       
------------------------------------------------------------------------
2000.22: git reset (full-v3)                       0.48     0.51 +6.3% 
2000.23: git reset (full-v4)                       0.47     0.50 +6.4% 
2000.24: git reset (sparse-v3)                     0.93     0.30 -67.7%
2000.25: git reset (sparse-v4)                     0.94     0.29 -69.1%
2000.26: git reset --hard (full-v3)                0.69     0.68 -1.4% 
2000.27: git reset --hard (full-v4)                0.75     0.68 -9.3% 
2000.28: git reset --hard (sparse-v3)              1.29     0.34 -73.6%
2000.29: git reset --hard (sparse-v4)              1.31     0.34 -74.0%
2000.30: git reset -- does-not-exist (full-v3)     0.54     0.51 -5.6% 
2000.31: git reset -- does-not-exist (full-v4)     0.54     0.52 -3.7% 
2000.32: git reset -- does-not-exist (sparse-v3)   1.02     0.31 -69.6%
2000.33: git reset -- does-not-exist (sparse-v4)   1.07     0.30 -72.0%



Changes since V1
================

 * Add --force-full-index option to update-index. The option is used
   circumvent changing command_requires_full_index from its default value -
   right now this is effectively a no-op, but will change once update-index
   is integrated with sparse index. By using this option in the t1092
   expand/collapse test, the command used to test will not need to be
   updated with subsequent sparse index integrations.
 * Update implementation of mixed reset for entries outside sparse checkout
   definition. The condition in which a file should be checked out before
   index reset is simplified to "if it has skip-worktree enabled and a reset
   would change the file, check it out".
   * After checking the behavior of update_index_from_diff with renames,
     found that the diff used by reset does not produce diff queue entries
     with different pathnames for one and two. Because of this, and that
     nothing in the implementation seems to rely on identical path names, no
     BUG check is added.
 * Correct a bug in the sparse index is not expanded tests in t1092 where
   failure of a git reset --mixed test was not being reported. Test now
   verifies an appropriate scenario with corrected failure-checking.

Thanks! -Victoria

[1] microsoft@6b8a074

Kevin Willford (1):
  reset: behave correctly with sparse-checkout

Victoria Dye (6):
  update-index: add --force-full-index option for expand/collapse test
  reset: expand test coverage for sparse checkouts
  reset: integrate with sparse index
  reset: make sparse-aware (except --mixed)
  reset: make --mixed sparse-aware
  unpack-trees: improve performance of next_cache_entry

 Documentation/git-update-index.txt       |   5 +
 builtin/reset.c                          |  62 +++++++++-
 builtin/update-index.c                   |  11 ++
 cache-tree.c                             |  43 ++++++-
 cache.h                                  |  10 ++
 read-cache.c                             |  22 ++--
 t/perf/p2000-sparse-operations.sh        |   3 +
 t/t1092-sparse-checkout-compatibility.sh | 139 ++++++++++++++++++++++-
 t/t7114-reset-sparse-checkout.sh         |  61 ++++++++++
 unpack-trees.c                           |  23 +++-
 10 files changed, 351 insertions(+), 28 deletions(-)
 create mode 100755 t/t7114-reset-sparse-checkout.sh


base-commit: cefe983a320c03d7843ac78e73bd513a27806845
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1048%2Fvdye%2Fvdye%2Fsparse-index-part1-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1048/vdye/vdye/sparse-index-part1-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1048

Range-diff vs v1:

 1:  65905bf4e00 ! 1:  22c69bc6030 reset: behave correctly with sparse-checkout
     @@ builtin/reset.c
       #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
       
      @@ builtin/reset.c: 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;
     +-		int is_missing = !(one->mode && !is_null_oid(&one->oid));
      +		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 *ce_before;
     -+		struct checkout state = CHECKOUT_INIT;
     -+
     + 
     +-		if (is_missing && !intent_to_add) {
      +		/*
     -+		 * 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.
     -+		 * In the case of added files, they 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 the file being reset has `skip-worktree` enabled, we need
     ++		 * to check it out to prevent the file from being hard reset.
      +		 */
     -+		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)) {
     -+				state.force = 1;
     -+				state.refresh_cache = 1;
     -+				state.istate = &the_index;
     -+
     -+				ce_before = make_cache_entry(&the_index, two->mode,
     -+							     &two->oid, two->path,
     -+							     0, 0);
     -+				if (!ce_before)
     -+					die(_("make_cache_entry failed for path '%s'"),
     -+						two->path);
     ++		pos = cache_name_pos(two->path, strlen(two->path));
     ++		if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
     ++			struct checkout state = CHECKOUT_INIT;
     ++			state.force = 1;
     ++			state.refresh_cache = 1;
     ++			state.istate = &the_index;
      +
     -+				checkout_entry(ce_before, &state, NULL, NULL);
     -+			}
     ++			checkout_entry(active_cache[pos], &state, NULL, NULL);
      +		}
     - 
     - 		if (is_missing && !intent_to_add) {
     ++
     ++		if (!is_in_reset_tree && !intent_to_add) {
       			remove_file_from_cache(one->path);
     + 			continue;
     + 		}
     +@@ builtin/reset.c: 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);
     + 		}
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'blame with pathspec outside sparse definition' '
 2:  a1fa7c080ae ! 2:  f7cb9013d46 sparse-index: update command for expand/collapse test
     @@ Metadata
      Author: Victoria Dye [off-list ref]
      
       ## Commit message ##
     -    sparse-index: update command for expand/collapse test
     +    update-index: add --force-full-index option for expand/collapse test
      
     -    In anticipation of multiple commands being fully integrated with sparse
     -    index, update the test for index expand and collapse for non-sparse index
     -    integrated commands to use `mv`.
     +    Add a new `--force-full-index` option to `git update-index`, which skips
     +    explicitly setting `command_requires_full_index`. This lets `git
     +    update-index --force-full-index` run as a command without sparse index
     +    compatibility implemented, even after it receives sparse index compatibility
     +    updates.
     +
     +    By using `git update-index --force-full-index` in the `t1092` test
     +    `sparse-index is expanded and converted back`, commands can continue to
     +    integrate with the sparse index without the need to keep modifying the
     +    command used in the test.
      
          Signed-off-by: Victoria Dye [off-list ref]
      
     + ## Documentation/git-update-index.txt ##
     +@@ Documentation/git-update-index.txt: SYNOPSIS
     + 	     [--[no-]fsmonitor]
     + 	     [--really-refresh] [--unresolve] [--again | -g]
     + 	     [--info-only] [--index-info]
     ++	     [--force-full-index]
     + 	     [-z] [--stdin] [--index-version <n>]
     + 	     [--verbose]
     + 	     [--] [<file>...]
     +@@ Documentation/git-update-index.txt: time. Version 4 is relatively young (first released in 1.8.0 in
     + October 2012). Other Git implementations such as JGit and libgit2
     + may not support it yet.
     + 
     ++--force-full-index::
     ++	Force the command to operate on a full index, expanding a sparse
     ++	index if necessary.
     ++
     + -z::
     + 	Only meaningful with `--stdin` or `--index-info`; paths are
     + 	separated with NUL character instead of LF.
     +
     + ## builtin/update-index.c ##
     +@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
     + 	int split_index = -1;
     + 	int force_write = 0;
     + 	int fsmonitor = -1;
     ++	int use_default_full_index = 0;
     + 	struct lock_file lock_file = LOCK_INIT;
     + 	struct parse_opt_ctx_t ctx;
     + 	strbuf_getline_fn getline_fn;
     +@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
     + 		{OPTION_SET_INT, 0, "no-fsmonitor-valid", &mark_fsmonitor_only, NULL,
     + 			N_("clear fsmonitor valid bit"),
     + 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
     ++		OPT_SET_INT(0, "force-full-index", &use_default_full_index,
     ++			N_("run with full index explicitly required"), 1),
     + 		OPT_END()
     + 	};
     + 
     +@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
     + 	if (newfd < 0)
     + 		lock_error = errno;
     + 
     ++	/*
     ++	 * If --force-full-index is set, the command should skip manually
     ++	 * setting `command_requires_full_index`.
     ++	 */
     ++	prepare_repo_settings(r);
     ++	if (!use_default_full_index)
     ++		r->settings.command_requires_full_index = 1;
     ++
     + 	entries = read_cache();
     + 	if (entries < 0)
     + 		die("cache corrupted");
     +
       ## t/t1092-sparse-checkout-compatibility.sh ##
      @@ t/t1092-sparse-checkout-compatibility.sh: 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="" mv a b &&
     ++		git -C sparse-index -c core.fsmonitor="" update-index --force-full-index &&
       	test_region index convert_to_sparse trace2.txt &&
       	test_region index ensure_full_index trace2.txt
       '
 3:  d033c5e365f = 3:  c7e9d9f4e03 reset: expand test coverage for sparse checkouts
 4:  2d63a250637 = 4:  49813c8d9ed reset: integrate with sparse index
 5:  e919e6d3270 = 5:  78cd85d8dcc reset: make sparse-aware (except --mixed)
 6:  e7cda32efb6 ! 6:  5eaae0825af reset: make --mixed sparse-aware
     @@ builtin/reset.c: static int read_from_tree(const struct pathspec *pathspec,
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
      @@ t/t1092-sparse-checkout-compatibility.sh: 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 || 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 &&
 7:  8637ec1660e = 7:  aa963eefae7 unpack-trees: improve performance of next_cache_entry

-- 
gitgitgadget

From: Kevin Willford <redacted>

When using the sparse checkout feature, 'git reset' will add entries to
the index that will have the skip-worktree bit off but will leave the
working directory empty. File data is lost because the index version of
the files has been changed but there is nothing that is in the working
directory. This will cause the next 'git status' call to show either
deleted for files modified or deleting or nothing for files added. The
added files should be shown as untracked and modified files should be
shown as modified.

To fix this when the reset is running if there is not a file in the
working directory and if it will be missing with the new index entry or
was not missing in the previous version, we create the previous index
version of the file in the working directory so that status will report
correctly and the files will be availble for the user to deal with.

This fixes a documented failure from t1092 that was created in 19a0acc
(t1092: test interesting sparse-checkout scenarios, 2021-01-23).

Signed-off-by: Kevin Willford <redacted>
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Victoria Dye <redacted>
---
 builtin/reset.c                          | 24 ++++++++--
 t/t1092-sparse-checkout-compatibility.sh |  4 +-
 t/t7114-reset-sparse-checkout.sh         | 61 ++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 6 deletions(-)
 create mode 100755 t/t7114-reset-sparse-checkout.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index 51c9e2f43ff..3b75d3b2f20 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -25,6 +25,8 @@
 #include "cache-tree.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "dir.h"
+#include "entry.h"
 
 #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
 

@@ -130,11 +132,27 @@ 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_missing = !(one->mode && !is_null_oid(&one->oid));
+		struct diff_filespec *two = q->queue[i]->two;
+		int is_in_reset_tree = one->mode && !is_null_oid(&one->oid);
 		struct cache_entry *ce;
 
-		if (is_missing && !intent_to_add) {
+		/*
+		 * If the file being reset has `skip-worktree` enabled, we need
+		 * to check it out to prevent the file from being hard reset.
+		 */
+		pos = cache_name_pos(two->path, strlen(two->path));
+		if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
+			struct checkout state = CHECKOUT_INIT;
+			state.force = 1;
+			state.refresh_cache = 1;
+			state.istate = &the_index;
+
+			checkout_entry(active_cache[pos], &state, NULL, NULL);
+		}
+
+		if (!is_in_reset_tree && !intent_to_add) {
 			remove_file_from_cache(one->path);
 			continue;
 		}

@@ -144,7 +162,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);
 		}

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 886e78715fe..c5977152661 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -459,9 +459,7 @@ 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 &&

diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
new file mode 100755
index 00000000000..a8029707fb1
--- /dev/null
+++ b/t/t7114-reset-sparse-checkout.sh

@@ -0,0 +1,61 @@
+#!/bin/sh
+
+test_description='reset when using a sparse-checkout'
+
+. ./test-lib.sh
+
+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 &&
+	echo "checkout file" >expect &&
+	test_cmp expect c &&
+	echo "added file" >expect &&
+	test_cmp expect a &&
+	echo "modification of a file" >expect &&
+	test_cmp expect m &&
+	test_path_is_missing d
+'
+
+test_expect_success 'reset after deleting file without skip-worktree bit' '
+	git checkout -f endCommit &&
+	git clean -xdf &&
+	cat >.git/info/sparse-checkout <<-\EOF &&
+	/c
+	/m
+	EOF
+	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 &&
+	echo "checkout file" >expect &&
+	test_cmp expect c &&
+	echo "added file" >expect &&
+	test_cmp expect a &&
+	test_path_is_missing m &&
+	test_path_is_missing d
+'
+
+test_done

-- 
gitgitgadget

Hi!

It appears Junio has already commented on this patch and in more
detail, but since I already typed up some comments I'll send them
along in case they are useful.

On Tue, Oct 5, 2021 at 6:20 AM Kevin Willford via GitGitGadget
[off-list ref] wrote:

From: Kevin Willford <redacted>

When using the sparse checkout feature, 'git reset' will add entries to
the index that will have the skip-worktree bit off but will leave the
working directory empty.

Yes, that seems like a problem.

File data is lost because the index version of
the files has been changed but there is nothing that is in the working
directory. This will cause the next 'git status' call to show either
deleted for files modified or deleting or nothing for files added. The
added files should be shown as untracked and modified files should be
shown as modified.

Why is the solution to add the files to the working tree rather than
to make sure the files have the skip-worktree bit set?  That's not at
all what I would have expected.

To fix this when the reset is running if there is not a file in the
working directory and if it will be missing with the new index entry or
was not missing in the previous version, we create the previous index
version of the file in the working directory so that status will report
correctly and the files will be availble for the user to deal with.

s/availble/available/

Isn't !is_null_oid(&one->oid) redundant to checking one->mode?  When
does the diff machinery ever give you a non-zero mode with a null oid?

Also, is_in_reset_tree == !is_missing; I'll note that below.

                struct cache_entry *ce;

+               /*
+                * If the file being reset has `skip-worktree` enabled, we need
+                * to check it out to prevent the file from being hard reset.

I don't understand this comment.  If the file wasn't originally in the
index (is_missing), and is being added to it, and is correctly marked
as skip_worktree, and the file isn't in the working tree, then it
sounds like everything is already in a good state.  Files outside the
sparse checkout are meant to have the skip_worktree bit set and be
missing from the working tree.

Also, I don't know what you mean by 'hard reset' here.

+                */
+               pos = cache_name_pos(two->path, strlen(two->path));
+               if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
+                       struct checkout state = CHECKOUT_INIT;
+                       state.force = 1;
+                       state.refresh_cache = 1;
+                       state.istate = &the_index;
+
+                       checkout_entry(active_cache[pos], &state, NULL, NULL);

Does this introduce an error in the opposite direction from the one
stated in the commit message?  Namely we have two things that should
be in sync: the skip_worktree flag stating whether the file should be
present in the working directory (skip_worktree), and the question of
whether the file is actually in the working directory.  In the commit
message, you pointed out a case where the y were out of sync one way:
the skip_worktree flag was not set but the file was missing.  Here you
say the skip_worktree flag is set, but you add it to the working tree
anyway.

Or am I misunderstanding the code?

+               }
+

[I did some slight editing to the diff to make the next two parts
appear next to each other]

-               if (is_missing && !intent_to_add) {
+               if (!is_in_reset_tree && !intent_to_add) {

I thought this was some subtle bugfix or something, and spent a while
trying to figure it out, before realizing that is_in_reset_tree was
simply defined as !is_missing (for some reason I was assuming it was
dealing with two->mode while is_missing was looking at one->mode).  So
this is a simple variable renaming, which I think is probably good,
but I'd prefer if this was separated into a different patch to make it
easier to review.

same note as above; the variable rename is good, but should be a separate patch.

Elijah Newren wrote:

quoted

-               int is_missing = !(one->mode && !is_null_oid(&one->oid));
+               struct diff_filespec *two = q->queue[i]->two;
+               int is_in_reset_tree = one->mode && !is_null_oid(&one->oid);

Isn't !is_null_oid(&one->oid) redundant to checking one->mode?  When
does the diff machinery ever give you a non-zero mode with a null oid?

It looks like this originally only checked the mode, and the extra OID check
was introduced in ff00b682f2 (reset [<commit>] paths...: do not mishandle
unmerged paths, 2011-07-13). I was able to remove `!is_null_oid(&one->oid)`
from the condition and run the `t71*` tests without any failures, but I'm
hesitant to remove it on the off chance that this handles a case I'm not
thinking of.

Also, is_in_reset_tree == !is_missing; I'll note that below.

quoted

                struct cache_entry *ce;

+               /*
+                * If the file being reset has `skip-worktree` enabled, we need
+                * to check it out to prevent the file from being hard reset.

I don't understand this comment.  If the file wasn't originally in the
index (is_missing), and is being added to it, and is correctly marked
as skip_worktree, and the file isn't in the working tree, then it
sounds like everything is already in a good state.  Files outside the
sparse checkout are meant to have the skip_worktree bit set and be
missing from the working tree.

Also, I don't know what you mean by 'hard reset' here.

quoted

+                */
+               pos = cache_name_pos(two->path, strlen(two->path));
+               if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
+                       struct checkout state = CHECKOUT_INIT;
+                       state.force = 1;
+                       state.refresh_cache = 1;
+                       state.istate = &the_index;
+
+                       checkout_entry(active_cache[pos], &state, NULL, NULL);

Does this introduce an error in the opposite direction from the one
stated in the commit message?  Namely we have two things that should
be in sync: the skip_worktree flag stating whether the file should be
present in the working directory (skip_worktree), and the question of
whether the file is actually in the working directory.  In the commit
message, you pointed out a case where the y were out of sync one way:
the skip_worktree flag was not set but the file was missing.  Here you
say the skip_worktree flag is set, but you add it to the working tree
anyway.

Or am I misunderstanding the code?

Most of this is addressed in [1], and you're right that what's in this 
patch isn't the right fix for the problem. This patch tried to solve the
issue of "skip-worktree is being ignored and reset files are showing up 
deleted" by continuing to ignore `skip-worktree`, but now checking out the
`skip-worktree` files based on their pre-reset state in the index (unless
they, for some reason, were already present in the worktree). However, that
completely disregards the reasoning for having `skip-worktree` in the first
place (the user wants the file *ignored* in the worktree) and violates the
premise of `git reset --mixed` not modifying the worktree, so the better
solution is to set `skip-worktree` in the resulting index entry and not
check out anything.

[1] https://lore.kernel.org/git/9b99e856-24cc-03fd-7871-de92dc6e39b6@github.com/

quoted

+               }
+

[I did some slight editing to the diff to make the next two parts
appear next to each other]

quoted

-               if (is_missing && !intent_to_add) {
+               if (!is_in_reset_tree && !intent_to_add) {

I thought this was some subtle bugfix or something, and spent a while
trying to figure it out, before realizing that is_in_reset_tree was
simply defined as !is_missing (for some reason I was assuming it was
dealing with two->mode while is_missing was looking at one->mode).  So
this is a simple variable renaming, which I think is probably good,
but I'd prefer if this was separated into a different patch to make it
easier to review.

Good call, I'll include this in V3.

On 05/10/21 20.20, Kevin Willford via GitGitGadget wrote:

When using the sparse checkout feature, 'git reset' will add entries to
the index that will have the skip-worktree bit off but will leave the
working directory empty. File data is lost because the index version of
the files has been changed but there is nothing that is in the working
directory. This will cause the next 'git status' call to show either
deleted for files modified or deleting or nothing for files added. The
added files should be shown as untracked and modified files should be
shown as modified.

Better say `... but there is nothing in the working directory`.

To fix this when the reset is running if there is not a file in the
working directory and if it will be missing with the new index entry or
was not missing in the previous version, we create the previous index
version of the file in the working directory so that status will report
correctly and the files will be availble for the user to deal with.

s/availble/available

-- 
An old man doll... just what I always wanted! - Clara

From: Victoria Dye <redacted>

Add new tests for `--merge` and `--keep` modes, as well as mixed reset with
pathspecs both inside and outside of the sparse checkout definition. New
performance test cases exercise various execution paths for `reset`.

Co-authored-by: Derrick Stolee [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Victoria Dye <redacted>
---
 t/perf/p2000-sparse-operations.sh        |   3 +
 t/t1092-sparse-checkout-compatibility.sh | 107 +++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 597626276fb..bfd332120c8 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh

@@ -110,5 +110,8 @@ test_perf_on_all git add -A
 test_perf_on_all git add .
 test_perf_on_all git commit -a -m A
 test_perf_on_all git checkout -f -
+test_perf_on_all git reset
+test_perf_on_all git reset --hard
+test_perf_on_all git reset -- does-not-exist
 
 test_done

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index b3c0d3b98ee..f0723a6ac97 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -479,6 +479,113 @@ 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 <commit>
+# 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 &&
+
+	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 pathspec match in sparse directory' '
+	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 &&