Emily Shaffer wrote: > On Mon, Oct 11, 2021 at 08:30:16PM +0000, Victoria Dye via GitGitGadget wrote: >> 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. >> 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. > >> 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. >> 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... > >> + git add file2 && > > ...and now it's in the index... > >> + git update-index --skip-worktree file2 && > > ...and now we're asking Git to ignore worktree changes to file2... > >> + 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". >> + 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 >>
