Re: [RFC PATCH 1/1] mv: integrate with sparse-index

[Derrick Stolee <derrickstolee@github.com>]

On 3/15/2022 1:23 PM, Junio C Hamano wrote:
> Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
> 
>> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>> ---
>>  builtin/mv.c                             |  3 +++
>>  t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index 83a465ba83..111360ebf5 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -138,6 +138,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>>  
>>  	git_config(git_default_config, NULL);
>>  
>> +	prepare_repo_settings(the_repository);
>> +	the_repository->settings.command_requires_full_index = 0;
>> +
> 
> The command used to be marked as one of the commands that require
> full index to work correctly.  Why did it suddenly become not to
> require it, especially without any other changes to make it so?
> 
> This patch needs a lot more explaining to do in itse proposed log
> message.

Right. Some builtins already work safely with the sparse index,
but we just were not sure without creating the proper tests for
it. In this case, I expect 'git mv' uses index_name_pos() to find
the locations of a given index entry, which can cause the index
to expand naturally.

I can definitely imagine a bug where index_name_pos() fixes the
location of the in-cone path within the sparse index, then the
index_name_pos() for the out-of-cone path expands the index and
causes the position of the in-cone path to no longer be correct.

Testing with a variety of in-cone and out-of-cone paths will
help here.

While I was writing this reply, I realized that our default cone
in `t1092` doesn't have a sparse directory before the typical in-cone
path of "deep/a". I set out to make one.

This diff _should_ apply to `t1092` without causing any failures:

--- >8 ---

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 2a04b532f91..e9533832aab 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -16,7 +16,9 @@ test_expect_success 'setup' '
 		echo "after deep" >e &&
 		echo "after folder1" >g &&
 		echo "after x" >z &&
-		mkdir folder1 folder2 deep x &&
+		mkdir folder1 folder2 deep before x &&
+		echo "before deep" >before/a &&
+		echo "before deep again" >before/b &&
 		mkdir deep/deeper1 deep/deeper2 deep/before deep/later &&
 		mkdir deep/deeper1/deepest &&
 		mkdir deep/deeper1/deepest2 &&
@@ -1311,6 +1313,7 @@ test_expect_success 'ls-files' '
 
 	cat >expect <<-\EOF &&
 	a
+	before/
 	deep/
 	e
 	folder1-
@@ -1358,6 +1361,7 @@ test_expect_success 'ls-files' '
 
 	cat >expect <<-\EOF &&
 	a
+	before/
 	deep/
 	e
 	folder1-

--- >8 ---

However, it causes failures in these tests:

1. `8 - add outside sparse cone`
2. `10 - status/add: outside sparse cone`
3. `21 - reset with pathspecs inside sparse definition`

After talking with @vdye about this, it seems that they are all
failing based on a common issue regarding an index-based diff.
Somehow the diff is not finding a version of the paths so is
reporting them as added.

For example, at the point of failure in `8 - add outside sparse
cone`, we have these results for some Git commands:

$ git diff
diff --git a/folder1/a b/folder1/a
index 7898192..8e27be7 100644
--- a/folder1/a
+++ b/folder1/a
@@ -1 +1 @@
-a
+text

$ git diff --staged

$ git diff --staged -- folder1/a
diff --git a/folder1/a b/folder1/a
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/folder1/a
@@ -0,0 +1 @@
+a

$ git diff --staged -- deep
diff --git a/deep/later/a b/deep/later/a
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/deep/later/a
@@ -0,0 +1 @@
+a
```

The impact must be pretty low and specific to these prefixed diffs
(the reset test also uses prefixed resets, so pathspecs are somehow
involved) which are rare for users to actually use. Still, we
should fix this and strengthen our tests.

After trying for an hour to fix this myself, I have failed to find
the root cause of this issue. I'm about to head out on vacation, so
I won't have time to look into this again until Monday. I wanted to
share this information so it doesn't cause Shaoxuan too much pain
while working on this 'git mv' change.

Thanks,
-Stolee