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

[Victoria Dye <vdye@github.com>]

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