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

[Emily Shaffer <emilyshaffer@google.com>]

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.

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

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

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