Re: [PATCH v4 2/8] reset: preserve skip-worktree bit in mixed reset

Emily Shaffer wrote:

On Mon, Oct 11, 2021 at 08:30:16PM +0000, Victoria Dye via GitGitGadget wrote:

quoted

diff --git a/builtin/reset.c b/builtin/reset.c
index d3695ce43c4..e441b6601b9 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)
 

@@ -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;

To put it another way and check my understanding (because I'm not
familiar with the sparse-index yet): if the file exists in the index but
we didn't care about the worktree anyway, then skip it; if the file
doesn't exist in the index but it also isn't in the sparse-checkout
cone, then also skip it, because we don't care about the file anyway.

I was going to ask if we could check ce_skip_worktree() without checking
pos first, but I suppose a negative pos would make the array deref
pretty unhappy. Ok.

Exactly! Generally the current skip-worktree flag is the "source of truth"
on whether to add the flag to the new entry, but if the file isn't in the
index pre-reset, the sparse checkout patterns are used to determine if it
should have skip-worktree applied.

quoted

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

@@ -459,26 +459,17 @@ 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)' '

Ooh ooh, we can start using these tests :) Always exciting.

quoted

 	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.

This doesn't really have anything to do with your patch. But I'm having
a very hard time understanding what each branch you're switching between
and basing on is for; this entire test suite is a little miserly with
comments. 

The branches used in this test are:

* `base` is the initial branch; all branches in the list here contain one
  commit on top of `base`
* `update-deep` modifies a file inside the sparse checkout cone (`deep/a`) 
* `deepest` modifies a file deep inside the sparse checkout cone
  (`deep/deeper1/deepest/a`)
* `update-folder1` and `update-folder2` each modify a file outside the
  sparse checkout cone (`folder1/a` and `folder2/a`, respectively)

There are other branches used throughout the file, but they deal with more
complicated conflict scenarios not used in this particular test case. 

I *think* your comment is saying that you're not bothering to
check test_all_match because you know that the full-checkout tree won't
match? But I also don't see that being asserted; test_sparse_match looks
to compare sparse-checkout and sparse-index trees but doesn't say
anything at all about the full-checkout tree, right?

Your understanding is correct (it's attempting to explain why we're not
using `test_all_match`, unlike earlier assertions in the test). That said,
it probably _should_ assert on the differences from `full-checkout`, namely
that "M	folder1/a" would appear in `full-checkout` but not in
`sparse-checkout`/`sparse-index`. I can add that in my next version.

quoted

 	test_sparse_match git reset update-folder1 &&
-	test_sparse_match git reset update-folder2
+	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 601b2bf97f0..d05426062ec 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 &&

file2 is just in the worktree...

quoted

+	git add file2 &&

...and now it's in the index...

quoted

+	git update-index --skip-worktree file2 &&

...and now we're asking Git to ignore worktree changes to file2...

quoted

+	git reset --mixed HEAD >output &&

But now I'm a little confused, maybe because of 'git reset' syntax. I'd
expect this to say "ah yes, the index is different from HEAD, it's got
this file2 thingie" and still reset the index; I'm surprised that
--skip-worktree, which sounds like it's saying only "don't consider
what's going on in the worktree". So I would expect this to still delete
file2 from the index. But instead I guess it is keeping file2 in the
index with the "who cares what happened in the wt" marker?

Yes - `git update-index --skip-worktree` sets the skip-worktree flag on the
index entry of the specified file(s) (in this case, `file2`). So, because
`file2` is in the index but ignored in the worktree, the file isn't
identified as "modified" after `git reset --mixed HEAD`. Once the
skip-worktree flag is removed (with `git update-index --no-skip-worktree`),
the reset results in it showing up as "modified".

quoted

+	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) &&
-- 
gitgitgadget