On 3/15/2022 1:23 PM, Junio C Hamano wrote: > Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> writes: > >> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> >> --- >> 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
