{"thread":{"id":"57561","subject":"[RFC PATCH 0/1] mv: integrate with sparse-index","startedAt":"2022-03-15T10:02:19Z","lastAt":"2022-03-28T13:32:44Z","messageCount":22,"participants":["Shaoxuan Yuan","Victoria Dye","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"451379","messageId":"20220315100145.214054-1-shaoxuan.yuan02@gmail.com","threadId":"57561","inReplyTo":null,"subject":"[RFC PATCH 0/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-15T10:01:44Z","receivedAt":"2022-03-15T10:02:19Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Integrate `git mv` with sparse-index.\nThe performance tests and ensure_not_expanded tests are not added yet,\ncause I want to see if the added tests in this patch are on the right\ntrack.\n\nShaoxuan Yuan (1):\n mv: integrate with sparse-index\n\n builtin/mv.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n 2 files changed, 37 insertions(+)\n\n\nbase-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\n-- \n2.35.1\n\n"},{"id":"451380","messageId":"20220315100145.214054-2-shaoxuan.yuan02@gmail.com","threadId":"57561","inReplyTo":"20220315100145.214054-1-shaoxuan.yuan02@gmail.com","subject":"[RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-15T10:01:45Z","receivedAt":"2022-03-15T10:02:20Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Signed-off-by: Shaoxuan Yuan \n---\n builtin/mv.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n 2 files changed, 37 insertions(+)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 83a465ba83..111360ebf5 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -138,6 +138,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \targc = parse_options(argc, argv, prefix, builtin_mv_options,\n \t\t\t builtin_mv_usage, 0);\n \tif (--argc < 1)\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 2a04b532f9..0a8164c5f6 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1521,4 +1521,38 @@ test_expect_success 'checkout behaves oddly with df-conflict-2' '\n \ttest_cmp full-checkout-err sparse-index-err\n '\n \n+test_expect_success 'mv' '\n+\tinit_repos &&\n+\n+\t# test first form \n+\ttest_all_match git mv deep/a deep/a_mod &&\n+\ttest_all_match git mv deep/deeper1 deep/deeper1_mod &&\n+\ttest_all_match git mv deep/deeper2/deeper1/deepest2/a \\\n+\tdeep/deeper2/deeper1/deepest2/a_mod &&\n+\n+\trun_on_all git reset --hard &&\n+\n+\ttest_all_match git mv -f deep/a deep/before/a &&\n+\ttest_all_match git mv -f deep/before/a deep/a &&\n+\n+\trun_on_all git reset --hard &&\n+\n+\ttest_all_match git mv -k deep/a deep/before/a &&\n+\ttest_all_match git mv -k deep/before/a deep/a &&\n+\n+\trun_on_all git reset --hard &&\n+\n+\ttest_all_match git mv -v deep/a deep/a_mod &&\n+\ttest_all_match git mv -v deep/deeper1 deep/deeper1_mod &&\n+\ttest_all_match git mv -v deep/deeper2/deeper1/deepest2/a \\\n+\tdeep/deeper2/deeper1/deepest2/a_mod &&\n+\n+\t# test second form ... \n+\trun_on_all git reset --hard &&\n+\trun_on_all mkdir deep/folder &&\n+\ttest_all_match git mv deep/a deep/folder &&\n+\ttest_all_match git mv -v deep/deeper1 deep/folder &&\n+\ttest_all_match git mv -f deep/deeper2/deeper1/deepest2/a deep/folder\n+'\n+\n test_done\n-- \n2.35.1\n\n"},{"id":"451397","messageId":"1ab24e4b-1feb-e1bc-4ae4-c28a69f77e05@github.com","threadId":"57561","inReplyTo":"20220315100145.214054-2-shaoxuan.yuan02@gmail.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-15T16:07:35Z","receivedAt":"2022-03-15T16:07:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shaoxuan Yuan wrote:\n> Signed-off-by: Shaoxuan Yuan \n> ---\n> builtin/mv.c | 3 +++\n> t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n> 2 files changed, 37 insertions(+)\n> \n\nHi Shaoxuan! \n\nI'll answer your question \"are the tests on the right track?\" [1] inline\nwith the tests here. \n\n[1] https://lore.kernel.org/git/20220315100145.214054-1-shaoxuan.yuan02@gmail.com/\n\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 83a465ba83..111360ebf5 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -138,6 +138,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \n> \tgit_config(git_default_config, NULL);\n> \n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n> +\n> \targc = parse_options(argc, argv, prefix, builtin_mv_options,\n> \t\t\t builtin_mv_usage, 0);\n> \tif (--argc < 1)\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 2a04b532f9..0a8164c5f6 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1521,4 +1521,38 @@ test_expect_success 'checkout behaves oddly with df-conflict-2' '\n> \ttest_cmp full-checkout-err sparse-index-err\n> '\n> \n> +test_expect_success 'mv' '\n> +\tinit_repos &&\n> +\n\nIn 't1092', I've tried to write test cases around some of the\ncharacteristics relevant to sparse checkout/sparse index. For example:\n\n- files inside vs. outside of sparse cone (e.g., 'deep/a' vs 'folder1/a')\n- \"normal\" directories vs. sparse directories (e.g., 'deep/' vs. 'folder1/')\n- directories inside a sparse directory vs. \"toplevel\" sparse directories\n (e.g., 'folder1/0/' vs. 'folder1/')\n- options that follow different code paths, especially if those code paths\n interact with the index differently (e.g., 'git reset --hard' vs 'git\n reset --mixed')\n- (probably not relevant for 'git mv') files with vs. without staged changes\n in the index\n\nI've found that exercising these characteristics provides good baseline\ncoverage for a sparse index integration, not leaving any major gaps. I'll\nalso typically add cases specific to any workarounds I need to add to a\ncommand (like for 'git read-tree --prefix' [2]).\n\nAlso, if some of the information about the test repos (e.g., what's inside\nvs. outside cone, or what's in the repos in the first place) isn't clear,\nI'm happy to give a deeper dive into how they're set up.\n\nWith all of that in mind, let's go over the cases you have so far.\n\n[2] https://lore.kernel.org/git/90ebcb7b8ff4b4f1ba09abcbe636d639fa597e74.1646166271.git.gitgitgadget@gmail.com/\n\n> +\t# test first form \n> +\ttest_all_match git mv deep/a deep/a_mod &&\n> +\ttest_all_match git mv deep/deeper1 deep/deeper1_mod &&\n> +\ttest_all_match git mv deep/deeper2/deeper1/deepest2/a \\\n> +\tdeep/deeper2/deeper1/deepest2/a_mod &&\n> +\n\nThis is a good basis for \"inside cone\" to \"inside cone\" moves. That said, I\ndon't think you need all three (since they're all testing effectively the\nsame inside-to-inside cone move). I'd suggest instead adding cases like: \n\n\ttest_all_match git mv &&\n\ttest_all_match git mv &&\n\ttest_all_match git mv &&\n\nto see how the sparse index behaves when files are moved in and out of\nsparse directories. Similarly, you may want to try 'git mv ' to see if that triggers any unintended\nbehavior.\n\nAdditionally, I don't *think* 'git mv' prints out the state of the index, so\nyou'll probably want to follow these cases with:\n\n\ttest_all_match git status --porcelain=v2 &&\n\nwhich prints the status info in a machine-readable format.\n\n> +\trun_on_all git reset --hard &&\n> +\n> +\ttest_all_match git mv -f deep/a deep/before/a &&\n> +\ttest_all_match git mv -f deep/before/a deep/a &&\n> +\n\nGood! The '-f' option will allow one file to overwrite another in the index,\nwhich is definitely interesting in a sparse index. Same as above, though,\nyou should verify 'git status --porcelain=v2'.\n\n> +\trun_on_all git reset --hard &&\n> +\n> +\ttest_all_match git mv -k deep/a deep/before/a &&\n> +\ttest_all_match git mv -k deep/before/a deep/a &&\n> +\n\nThe '-k' option might be interesting in the context of the index, since it\npushes past errors that would normally make it exit early. However, if it\njust skips things that fail rather than exiting with an error, it probably\nisn't testing anything more than the 'git mv' cases. \n\n> +\trun_on_all git reset --hard &&\n> +\n> +\ttest_all_match git mv -v deep/a deep/a_mod &&\n> +\ttest_all_match git mv -v deep/deeper1 deep/deeper1_mod &&\n> +\ttest_all_match git mv -v deep/deeper2/deeper1/deepest2/a \\\n> +\tdeep/deeper2/deeper1/deepest2/a_mod &&\n> +\n\nLooking at 'builtin/mv.c', the '-v' \"verbose\" option only controls whether\nsome verbose printouts are emitted. This might be relevant if the printouts\nwere printing index information that didn't match between 'full-checkout',\n'sparse-checkout' and 'sparse-index', but if you haven't seen that, I'd\nleave these cases out.\n\n> +\t# test second form ... \n> +\trun_on_all git reset --hard &&\n> +\trun_on_all mkdir deep/folder &&\n> +\ttest_all_match git mv deep/a deep/folder &&\n> +\ttest_all_match git mv -v deep/deeper1 deep/folder &&\n> +\ttest_all_match git mv -f deep/deeper2/deeper1/deepest2/a deep/folder\n\nThis is a good variation on the standard \"inside cone\" to \"inside cone\", and\nI'd like to see something similar done inside sparse directories. And,\nsimilar to above, I don't think '-v' needs to be tested.\n\n> +'\n> +\n> test_done\n\nOverall, this is a great start! You've got a good pattern set up (it's very\nclear to follow), I think it mainly needs some more variety to the test\ncases. Also, if you find that this test gets way too large after adding more\ncases, feel free to split it into multiple named tests if one gets too long\n(e.g. \"mv\", \"mv -f\"). \n\nMy recommendations:\n\n- add tests covering outside-of-sparse-cone 'mv' arguments\n- add tests covering 'mv' attempting to move directories (in-cone and\n sparse)\n- add some \"test_must_fail\" tests to see what happens when you do something\n \"wrong\", e.g. to try to overwrite a file without '-f' (I've found some\n really interesting issues in the past where you expect something to fail\n and it doesn't)\n- add 'git status --porcelain=v2' checks to confirm that the 'mv' worked the\n same across the different checkouts\n- remove multiples of test cases that test the same general behavior (e.g.,\n 'git mv ' only needs to be done once)\n- double-check whether '-v' and '-k' have the ability to affect\n full-checkout/sparse-checkout/sparse-index differently - if not, you\n probably don't need to test them\n\nThanks for working on this, and I hope this helps!\n"},{"id":"451401","messageId":"20ffd93d-e3dd-4df6-5ec7-d3577cac910d@github.com","threadId":"57561","inReplyTo":"1ab24e4b-1feb-e1bc-4ae4-c28a69f77e05@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-15T17:14:28Z","receivedAt":"2022-03-15T17:14:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/15/2022 12:07 PM, Victoria Dye wrote:\n> Shaoxuan Yuan wrote:\n>> Signed-off-by: Shaoxuan Yuan \n>> ---\n>> builtin/mv.c | 3 +++\n>> t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n>> 2 files changed, 37 insertions(+)\n>>\n> \n> Hi Shaoxuan! \n\nHello!\n \n> I'll answer your question \"are the tests on the right track?\" [1] inline\n> with the tests here. \n> In 't1092', I've tried to write test cases around some of the\n> characteristics relevant to sparse checkout/sparse index. For example:\n> \n> - files inside vs. outside of sparse cone (e.g., 'deep/a' vs 'folder1/a')\n> - \"normal\" directories vs. sparse directories (e.g., 'deep/' vs. 'folder1/')\n> - directories inside a sparse directory vs. \"toplevel\" sparse directories\n> (e.g., 'folder1/0/' vs. 'folder1/')\n> - options that follow different code paths, especially if those code paths\n> interact with the index differently (e.g., 'git reset --hard' vs 'git\n> reset --mixed')\n> - (probably not relevant for 'git mv') files with vs. without staged changes\n> in the index\n> \n> I've found that exercising these characteristics provides good baseline\n> coverage for a sparse index integration, not leaving any major gaps. I'll\n> also typically add cases specific to any workarounds I need to add to a\n> command (like for 'git read-tree --prefix' [2]).\n\nThis, and other advice that Victoria mentions, are really\ngood points to keep in mind.\n\n> My recommendations:\n> \n> - add tests covering outside-of-sparse-cone 'mv' arguments\n> - add tests covering 'mv' attempting to move directories (in-cone and\n> sparse)\n> - add some \"test_must_fail\" tests to see what happens when you do something\n> \"wrong\", e.g. to try to overwrite a file without '-f' (I've found some\n> really interesting issues in the past where you expect something to fail\n> and it doesn't)\n> - add 'git status --porcelain=v2' checks to confirm that the 'mv' worked the\n> same across the different checkouts\n> - remove multiples of test cases that test the same general behavior (e.g.,\n> 'git mv ' only needs to be done once)\n> - double-check whether '-v' and '-k' have the ability to affect\n> full-checkout/sparse-checkout/sparse-index differently - if not, you\n> probably don't need to test them\n> \n> Thanks for working on this, and I hope this helps!\n\nYou mention in your cover letter that the ensure_not_expanded tests\nare not added yet (same with performance tests). Now that you've\ngotten feedback on this version of the patch, I might recommend the\norganization you might want for a full series:\n\n1. Add these 'mv' tests to t1092 _without_ the code change. These\n tests should work when the index is expanded, and making the\n code change to not expand the index shouldn't change the\n behavior.\n\n2. Add the performance test so we have a baseline to measure how\n well 'mv' does in the normal case (and how it is slower when\n expanding the index).\n\n3. Make the code change and add the ensure_not_expanded test,\n since the functionality from the tests added in (1) will not\n change and we can report the results from the perf tests\n added in (2). The only thing to test is the new, internal\n behavior that the index is not expanded when doing these\n actions. (Keep in mind that we expect the index to be\n expanded for out-of-cone moves, but it's the in-cone moves\n that we expect to not expand.)\n\nThanks!\n-Stolee\n"},{"id":"451403","messageId":"xmqq8rtbf7uz.fsf@gitster.g","threadId":"57561","inReplyTo":"20220315100145.214054-2-shaoxuan.yuan02@gmail.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-15T17:23:00Z","receivedAt":"2022-03-15T17:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> Signed-off-by: Shaoxuan Yuan \n> ---\n> builtin/mv.c | 3 +++\n> t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n> 2 files changed, 37 insertions(+)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 83a465ba83..111360ebf5 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -138,6 +138,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \n> \tgit_config(git_default_config, NULL);\n> \n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n> +\n\nThe command used to be marked as one of the commands that require\nfull index to work correctly. Why did it suddenly become not to\nrequire it, especially without any other changes to make it so?\n\nThis patch needs a lot more explaining to do in itse proposed log\nmessage.\n\nThanks.\n"},{"id":"451426","messageId":"f634f89c-b4b8-6736-d519-a4c554bac959@github.com","threadId":"57561","inReplyTo":"xmqq8rtbf7uz.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-15T20:00:48Z","receivedAt":"2022-03-15T20:00:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/15/2022 1:23 PM, Junio C Hamano wrote:\n> Shaoxuan Yuan writes:\n> \n>> Signed-off-by: Shaoxuan Yuan \n>> ---\n>> builtin/mv.c | 3 +++\n>> t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n>> 2 files changed, 37 insertions(+)\n>>\n>> diff --git a/builtin/mv.c b/builtin/mv.c\n>> index 83a465ba83..111360ebf5 100644\n>> --- a/builtin/mv.c\n>> +++ b/builtin/mv.c\n>> @@ -138,6 +138,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n>> \n>> \tgit_config(git_default_config, NULL);\n>> \n>> +\tprepare_repo_settings(the_repository);\n>> +\tthe_repository->settings.command_requires_full_index = 0;\n>> +\n> \n> The command used to be marked as one of the commands that require\n> full index to work correctly. Why did it suddenly become not to\n> require it, especially without any other changes to make it so?\n> \n> This patch needs a lot more explaining to do in itse proposed log\n> message.\n\nRight. Some builtins already work safely with the sparse index,\nbut we just were not sure without creating the proper tests for\nit. In this case, I expect 'git mv' uses index_name_pos() to find\nthe locations of a given index entry, which can cause the index\nto expand naturally.\n\nI can definitely imagine a bug where index_name_pos() fixes the\nlocation of the in-cone path within the sparse index, then the\nindex_name_pos() for the out-of-cone path expands the index and\ncauses the position of the in-cone path to no longer be correct.\n\nTesting with a variety of in-cone and out-of-cone paths will\nhelp here.\n\nWhile I was writing this reply, I realized that our default cone\nin `t1092` doesn't have a sparse directory before the typical in-cone\npath of \"deep/a\". I set out to make one.\n\nThis diff _should_ apply to `t1092` without causing any failures:\n\n--- >8 ---\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 2a04b532f91..e9533832aab 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -16,7 +16,9 @@ test_expect_success 'setup' '\n \t\techo \"after deep\" >e &&\n \t\techo \"after folder1\" >g &&\n \t\techo \"after x\" >z &&\n-\t\tmkdir folder1 folder2 deep x &&\n+\t\tmkdir folder1 folder2 deep before x &&\n+\t\techo \"before deep\" >before/a &&\n+\t\techo \"before deep again\" >before/b &&\n \t\tmkdir deep/deeper1 deep/deeper2 deep/before deep/later &&\n \t\tmkdir deep/deeper1/deepest &&\n \t\tmkdir deep/deeper1/deepest2 &&\n@@ -1311,6 +1313,7 @@ test_expect_success 'ls-files' '\n \n \tcat >expect <<-\\EOF &&\n \ta\n+\tbefore/\n \tdeep/\n \te\n \tfolder1-\n@@ -1358,6 +1361,7 @@ test_expect_success 'ls-files' '\n \n \tcat >expect <<-\\EOF &&\n \ta\n+\tbefore/\n \tdeep/\n \te\n \tfolder1-\n\n--- >8 ---\n\nHowever, it causes failures in these tests:\n\n1. `8 - add outside sparse cone`\n2. `10 - status/add: outside sparse cone`\n3. `21 - reset with pathspecs inside sparse definition`\n\nAfter talking with @vdye about this, it seems that they are all\nfailing based on a common issue regarding an index-based diff.\nSomehow the diff is not finding a version of the paths so is\nreporting them as added.\n\nFor example, at the point of failure in `8 - add outside sparse\ncone`, we have these results for some Git commands:\n\n$ git diff\ndiff --git a/folder1/a b/folder1/a\nindex 7898192..8e27be7 100644\n--- a/folder1/a\n+++ b/folder1/a\n@@ -1 +1 @@\n-a\n+text\n\n$ git diff --staged\n\n$ git diff --staged -- folder1/a\ndiff --git a/folder1/a b/folder1/a\nnew file mode 100644\nindex 0000000..7898192\n--- /dev/null\n+++ b/folder1/a\n@@ -0,0 +1 @@\n+a\n\n$ git diff --staged -- deep\ndiff --git a/deep/later/a b/deep/later/a\nnew file mode 100644\nindex 0000000..7898192\n--- /dev/null\n+++ b/deep/later/a\n@@ -0,0 +1 @@\n+a\n```\n\nThe impact must be pretty low and specific to these prefixed diffs\n(the reset test also uses prefixed resets, so pathspecs are somehow\ninvolved) which are rare for users to actually use. Still, we\nshould fix this and strengthen our tests.\n\nAfter trying for an hour to fix this myself, I have failed to find\nthe root cause of this issue. I'm about to head out on vacation, so\nI won't have time to look into this again until Monday. I wanted to\nshare this information so it doesn't cause Shaoxuan too much pain\nwhile working on this 'git mv' change.\n\nThanks,\n-Stolee\n"},{"id":"451453","messageId":"CAJyCBOQXnKC9oGc5RDH-uRtc679RVnb-8bKAJ5QTpYnOc0D4Lw@mail.gmail.com","threadId":"57561","inReplyTo":"1ab24e4b-1feb-e1bc-4ae4-c28a69f77e05@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-16T03:18:46Z","receivedAt":"2022-03-16T03:19:03Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Wed, Mar 16, 2022 at 12:07 AM Victoria Dye wrote:\n>\n> Shaoxuan Yuan wrote:\n> > Signed-off-by: Shaoxuan Yuan \n> > ---\n> > builtin/mv.c | 3 +++\n> > t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n> > 2 files changed, 37 insertions(+)\n> >\n>\n> Hi Shaoxuan!\n\nHi Victoria!\n\n> Thanks for working on this, and I hope this helps!\n\nThanks for all the feedback, they are really helpful! I'm still researching and\nexperimenting with various documents and examples regarding your\nfeedback. And I will try to submit a more refined patch in less than a\nfew days :)\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"451454","messageId":"CAJyCBOTATLNyhE8A7_M9HnuS_QM0dR0H2yQ2_4VRP=XgEKP7ow@mail.gmail.com","threadId":"57561","inReplyTo":"20ffd93d-e3dd-4df6-5ec7-d3577cac910d@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-16T03:29:14Z","receivedAt":"2022-03-16T03:29:30Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Wed, Mar 16, 2022 at 1:14 AM Derrick Stolee wrote:\n>\n> Hello!\n\nHi Derrick,\n\n>\n> > I'll answer your question \"are the tests on the right track?\" [1] inline\n> > with the tests here.\n> > In 't1092', I've tried to write test cases around some of the\n> > characteristics relevant to sparse checkout/sparse index. For example:\n> >\n> > - files inside vs. outside of sparse cone (e.g., 'deep/a' vs 'folder1/a')\n> > - \"normal\" directories vs. sparse directories (e.g., 'deep/' vs. 'folder1/')\n> > - directories inside a sparse directory vs. \"toplevel\" sparse directories\n> > (e.g., 'folder1/0/' vs. 'folder1/')\n> > - options that follow different code paths, especially if those code paths\n> > interact with the index differently (e.g., 'git reset --hard' vs 'git\n> > reset --mixed')\n> > - (probably not relevant for 'git mv') files with vs. without staged changes\n> > in the index\n> >\n> > I've found that exercising these characteristics provides good baseline\n> > coverage for a sparse index integration, not leaving any major gaps. I'll\n> > also typically add cases specific to any workarounds I need to add to a\n> > command (like for 'git read-tree --prefix' [2]).\n>\n> This, and other advice that Victoria mentions, are really\n> good points to keep in mind.\n>\n> > My recommendations:\n> >\n> > - add tests covering outside-of-sparse-cone 'mv' arguments\n> > - add tests covering 'mv' attempting to move directories (in-cone and\n> > sparse)\n> > - add some \"test_must_fail\" tests to see what happens when you do something\n> > \"wrong\", e.g. to try to overwrite a file without '-f' (I've found some\n> > really interesting issues in the past where you expect something to fail\n> > and it doesn't)\n> > - add 'git status --porcelain=v2' checks to confirm that the 'mv' worked the\n> > same across the different checkouts\n> > - remove multiples of test cases that test the same general behavior (e.g.,\n> > 'git mv ' only needs to be done once)\n> > - double-check whether '-v' and '-k' have the ability to affect\n> > full-checkout/sparse-checkout/sparse-index differently - if not, you\n> > probably don't need to test them\n> >\n> > Thanks for working on this, and I hope this helps!\n>\n> You mention in your cover letter that the ensure_not_expanded tests\n> are not added yet (same with performance tests). Now that you've\n> gotten feedback on this version of the patch, I might recommend the\n> organization you might want for a full series:\n>\n> 1. Add these 'mv' tests to t1092 _without_ the code change. These\n> tests should work when the index is expanded, and making the\n> code change to not expand the index shouldn't change the\n> behavior.\n>\n> 2. Add the performance test so we have a baseline to measure how\n> well 'mv' does in the normal case (and how it is slower when\n> expanding the index).\n>\n> 3. Make the code change and add the ensure_not_expanded test,\n> since the functionality from the tests added in (1) will not\n> change and we can report the results from the perf tests\n> added in (2). The only thing to test is the new, internal\n> behavior that the index is not expanded when doing these\n> actions. (Keep in mind that we expect the index to be\n> expanded for out-of-cone moves, but it's the in-cone moves\n> that we expect to not expand.)\n\nThanks for the recommendations, they are really helpful! I will try to\naddress them in the next patch :)\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"451463","messageId":"CAJyCBORDOJUwTzOC+hYwGGPUBCXST0_mBdwRLh2N+cA=5k0d4A@mail.gmail.com","threadId":"57561","inReplyTo":"1ab24e4b-1feb-e1bc-4ae4-c28a69f77e05@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-16T10:45:16Z","receivedAt":"2022-03-16T10:45:31Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Hi Victoria,\n\nJust found an interesting (probably) behavior.\n\nIn the `sparse-checkout` directory created in `init_repos()` in t1092, if I\nsay:\n\n$ mkdir folder3\n$ touch folder3/a\n$ git mv folder3/a deep\n\nand git will prompt:\n\n\"fatal: not under version control, source=folder3/a, destination=deep/a\"\n\nAnd if I say:\n\n$ git mv folder3 deep\n\ngit will prompt:\n\n\"fatal: source directory is empty, source=folder3, destination=deep/folder3\"\n\nWhat I am wondering is that file `folder3/a` is outside of sparse-checkout cone,\nshould `git mv` instead prompts with `advise_on_updating_sparse_paths()` or this\n\"not under version control\" alarm is acceptable?\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"451469","messageId":"675c7681-c495-727d-1262-ee8c6a5c8ce5@github.com","threadId":"57561","inReplyTo":"CAJyCBORDOJUwTzOC+hYwGGPUBCXST0_mBdwRLh2N+cA=5k0d4A@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-16T13:34:22Z","receivedAt":"2022-03-16T13:34:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/16/2022 6:45 AM, Shaoxuan Yuan wrote:\n> Hi Victoria,\n> \n> Just found an interesting (probably) behavior.\n> \n> In the `sparse-checkout` directory created in `init_repos()` in t1092, if I\n> say:\n> \n> $ mkdir folder3\n> $ touch folder3/a\n\nThe issue here is that this file is \"untracked\", not just outside\nof the sparse-checkout cone.\n\n> $ git mv folder3/a deep\n> \n> and git will prompt:\n> \n> \"fatal: not under version control, source=folder3/a, destination=deep/a\"\n> \n> And if I say:\n> \n> $ git mv folder3 deep\n> \n> git will prompt:\n> \n> \"fatal: source directory is empty, source=folder3, destination=deep/folder3\"\n> \n> What I am wondering is that file `folder3/a` is outside of sparse-checkout cone,\n> should `git mv` instead prompts with `advise_on_updating_sparse_paths()` or this\n> \"not under version control\" alarm is acceptable?\n\nInstead, what about\n\n\tgit mv folder2/a deep/new\n\nsince folder2/a is a tracked file, just not in the working tree\nsince it is outside the sparse-checkout cone.\n\n(If it fails, then it should fail the same with and without the\nsparse index, which is what \"test_sparse_match\" is for.)\n\nThanks,\n-Stolee\n"},{"id":"451475","messageId":"CAJyCBORfAV_TV6DrOxgim4KtU9T-uTibOaQCsJZsi5_FQfci1Q@mail.gmail.com","threadId":"57561","inReplyTo":"675c7681-c495-727d-1262-ee8c6a5c8ce5@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-16T14:46:46Z","receivedAt":"2022-03-16T14:47:05Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Hi Derrick,\n\nOn Wed, Mar 16, 2022 at 9:34 PM Derrick Stolee wrote:\n> The issue here is that this file is \"untracked\", not just outside\n> of the sparse-checkout cone.\n\nThanks for the succinct explanation, it makes much more sense now :)\n\n> Instead, what about\n>\n> git mv folder2/a deep/new\n>\n> since folder2/a is a tracked file, just not in the working tree\n> since it is outside the sparse-checkout cone.\n>\n> (If it fails, then it should fail the same with and without the\n> sparse index, which is what \"test_sparse_match\" is for.)\n\nI tested this and it fails as expected with:\n\"fatal: bad source, source=folder2/a, destination=deep/new\"\n\n> Thanks,\n> -Stolee\n\nThanks for the reply above!\n\nOther than that, I also have found another issue (probably), with\n\n$ mkdir folder2\n$ git mv folder2 deep\n\nAfter these I do:\n\n$ git status\n\nAnd the output indicates that the index is updated with the following changes:\n\n renamed: folder2/0/0/0 -> deep/folder2/0/0/0\n renamed: folder2/0/1 -> deep/folder2/0/1\n renamed: folder2/a -> deep/folder2/a\n\nNothing fails, which is not what I expected. What I expect is `git mv` will\nfail because it is being told to update a sparse-directory (which as I read the\nblogs and sparse-index.txt is taken as a sparse-directory entry) outside of the\nsparse-checkout cone. Unless `git mv` is supplied with `--sparse`, the command\nwill do nothing but fail, no?\n\nWhat confuses me more is that the `folder2`, which is present in the index but\nnot in the working tree (due to sparse-checkout cone), seems to be \"unlocked\"\nand re-picked up by Git after `mkdir folder2` and move `folder2` into\nthe cone area.\nAnd still, the files under `deep/folder2` are not present in the\nworking tree (might\nbe relevant to the previous context).\n\nI haven't run the gdb to see into the process, I just get somehow confused by\nthese discrepancies (seemingly to me). I think I should gdb into it though,\ngetting some info here from people can also be really helpful :)\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"451537","messageId":"CAJyCBOTfaaeqhiRS6xFzQHpf-H35ATygKJqWYDijfPDJOGcShQ@mail.gmail.com","threadId":"57561","inReplyTo":"20ffd93d-e3dd-4df6-5ec7-d3577cac910d@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-17T08:37:52Z","receivedAt":"2022-03-17T08:38:13Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Hi Derrick and Victoria.\n\nOn Wed, Mar 16, 2022 at 1:14 AM Derrick Stolee wrote:\n> You mention in your cover letter that the ensure_not_expanded tests\n> are not added yet (same with performance tests). Now that you've\n> gotten feedback on this version of the patch, I might recommend the\n> organization you might want for a full series:\n>\n> 1. Add these 'mv' tests to t1092 _without_ the code change. These\n> tests should work when the index is expanded, and making the\n> code change to not expand the index shouldn't change the\n> behavior.\n>\n> 2. Add the performance test so we have a baseline to measure how\n> well 'mv' does in the normal case (and how it is slower when\n> expanding the index).\n\nI'm a bit caught up here.\n\nDo I just do a before-code-change test and after-code-change test, and\nbenchmark the after against the before?\n\nOr do you mean I should also perf test out-of-cone arguments with 'mv' so\nthat the index could be expanded? According to my understanding, the\nsparse-index could be required to expand when out-of-cone actions\nhappen and the 'ensure_full_index()' is called. And do a 3-way comparison\namong before-code-change, after-code-change, and after-code-change-\nindex-expanded, no?\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"451598","messageId":"97a665fe-07c9-c4f6-4ab6-b6c0e1397c31@github.com","threadId":"57561","inReplyTo":"CAJyCBORfAV_TV6DrOxgim4KtU9T-uTibOaQCsJZsi5_FQfci1Q@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-17T21:57:32Z","receivedAt":"2022-03-17T21:57:37Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shaoxuan Yuan wrote:\n> Hi Derrick,\n> \n> On Wed, Mar 16, 2022 at 9:34 PM Derrick Stolee wrote:\n>> The issue here is that this file is \"untracked\", not just outside\n>> of the sparse-checkout cone.\n> \n> Thanks for the succinct explanation, it makes much more sense now :)\n> \n>> Instead, what about\n>>\n>> git mv folder2/a deep/new\n>>\n>> since folder2/a is a tracked file, just not in the working tree\n>> since it is outside the sparse-checkout cone.\n>>\n>> (If it fails, then it should fail the same with and without the\n>> sparse index, which is what \"test_sparse_match\" is for.)\n> \n> I tested this and it fails as expected with:\n> \"fatal: bad source, source=folder2/a, destination=deep/new\"\n> \n\nGreat! This should then probably be turned into a \"test_expect_fail\" test in\n't1092' - that'll make sure we get both the right behavior and right error\nmessage with sparse index after it's enabled.\n\nHowever, I also get the same result when I add the '--sparse' option. I\nwould expect the behavior to be \"move 'folder2/a' to 'deep/new' and check it\nout in the worktree\" - this may be a good candidate for improving the\nexisting integration with sparse *checkout* before enabling sparse *index*\n(e.g., like when 'git add' was updated to not add sparse files by default\n[1]).\n\n[1] https://lore.kernel.org/git/2c5c834bc9fb42aeaff7befbba477aec727184c0.1632497954.git.gitgitgadget@gmail.com/\n\n>> Thanks,\n>> -Stolee\n> \n> Thanks for the reply above!\n> \n> Other than that, I also have found another issue (probably), with\n> \n> $ mkdir folder2\n> $ git mv folder2 deep\n> \n> After these I do:\n> \n> $ git status\n> \n> And the output indicates that the index is updated with the following changes:\n> \n> renamed: folder2/0/0/0 -> deep/folder2/0/0/0\n> renamed: folder2/0/1 -> deep/folder2/0/1\n> renamed: folder2/a -> deep/folder2/a\n> \n> Nothing fails, which is not what I expected. What I expect is `git mv` will\n> fail because it is being told to update a sparse-directory (which as I read the\n> blogs and sparse-index.txt is taken as a sparse-directory entry) outside of the\n> sparse-checkout cone. Unless `git mv` is supplied with `--sparse`, the command\n> will do nothing but fail, no?\n> \n\nI think you're right that this is a bug. This appears to come from the fact\nthat 'mv' decides whether a directory is sparse only *after* it sees that it\ndoesn't exist on-disk. \n\n> What confuses me more is that the `folder2`, which is present in the index but\n> not in the working tree (due to sparse-checkout cone), seems to be \"unlocked\"\n> and re-picked up by Git after `mkdir folder2` and move `folder2` into\n> the cone area.\n> And still, the files under `deep/folder2` are not present in the\n> working tree (might\n> be relevant to the previous context).\n> \nThis is a consequence of how sparse-checkout is implemented. The files in\n'folder2/' aren't on-disk (and aren't correspondingly shown as \"deleted\" in\n'git status') because the index entries of 'folder2/' files are marked with\nthe \"SKIP_WORKTREE\" flag. This flag basically indicates to git \"this file\nshouldn't be in the worktree (on-disk), but it is part of the repository (in\nthe index)\". In other words, it's what makes a file \"sparse\", and\nsparse-checkout (when you run 'set' or 'add') assigns that flag based on the\nuser-specified patterns. However, the flag isn't constantly being\nre-evaluated - only certain commands change SKIP_WORKTREE, and only because\nthey do so explicitly (e.g., 'git reset --mixed' [2]) - leading to\nsituations like what you're seeing, where 'folder2/' files (which have\nSKIP_WORKTREE enabled) are moved into the sparse cone, but still have\nSKIP_WORKTREE enabled.\n\nSo I think there are three potential things to fix here: \n\n1. When empty folder2/ is on-disk, 'git mv' (without '--sparse') doesn't\n fail with \"bad source\", even though it should.\n2. When you try to move a sparse file with 'git mv --sparse', it still\n fails.\n3. SKIP_WORKTREE is not removed from out-of-cone files moved into the sparse\n cone.\n\nOn a related note, there is precedent for needing to make fixes like this\nbefore integrating with sparse index. For example: in addition to the\nearlier examples in 'add' and 'reset', 'checkout-index' was changed to no\nlonger checkout SKIP_WORKTREE files by default [3]. It's a somewhat expected\npart of this process because sparse-checkout is still \"experimental\", and\none of our secondary goals with this sparse index work is to improve the\nbehavior of sparse-checkout in the commands we integrate. All of this\ncombined will, ideally, make the experience of using sparse-checkout much\nnicer for users (both from usability and performance perspectives).\n\n[2] https://lore.kernel.org/git/b221b00b7e06a3b135b9f68ce87cffaa7d782581.1638201164.git.gitgitgadget@gmail.com/\n[3] https://lore.kernel.org/git/601888606d1cf7d7752844dbdbc7fac20d4be8c4.1641924306.git.gitgitgadget@gmail.com/\n\n> I haven't run the gdb to see into the process, I just get somehow confused by\n> these discrepancies (seemingly to me). I think I should gdb into it though,\n> getting some info here from people can also be really helpful :)\n> \n\nAnother tool that may help you here is 'git ls-files --sparse -t'. It lists\nthe files in the index and their \"tags\" ('H' is \"normal\" tracked files, 'S'\nis SKIP_WORKTREE, etc. [4]), which can help identify when a file you'd\nexpect to be SKIP_WORKTREE is not and vice versa.\n\n[4] https://git-scm.com/docs/git-ls-files#Documentation/git-ls-files.txt--t \n"},{"id":"451612","messageId":"xmqqo824cbxl.fsf@gitster.g","threadId":"57561","inReplyTo":"97a665fe-07c9-c4f6-4ab6-b6c0e1397c31@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T01:00:06Z","receivedAt":"2022-03-18T01:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n>> I tested this and it fails as expected with:\n>> \"fatal: bad source, source=folder2/a, destination=deep/new\"\n>\n> Great! This should then probably be turned into a \"test_expect_fail\" test in\n> 't1092' - that'll make sure we get both the right behavior and right error\n> message with sparse index after it's enabled.\n>\n> However, I also get the same result when I add the '--sparse' option. I\n> would expect the behavior to be \"move 'folder2/a' to 'deep/new' and check it\n> out in the worktree\" - this may be a good candidate for improving the\n> existing integration with sparse *checkout* before enabling sparse *index*\n> (e.g., like when 'git add' was updated to not add sparse files by default\n> [1]).\n> ...\n> I think you're right that this is a bug. This appears to come from the fact\n> that 'mv' decides whether a directory is sparse only *after* it sees that it\n> doesn't exist on-disk. \n> ...\n> So I think there are three potential things to fix here: \n>\n> 1. When empty folder2/ is on-disk, 'git mv' (without '--sparse') doesn't\n> fail with \"bad source\", even though it should.\n> 2. When you try to move a sparse file with 'git mv --sparse', it still\n> fails.\n> 3. SKIP_WORKTREE is not removed from out-of-cone files moved into the sparse\n> cone.\n>\n> On a related note, there is precedent for needing to make fixes like this\n> before integrating with sparse index. For example: in addition to the\n> earlier examples in 'add' and 'reset', 'checkout-index' was changed to no\n> longer checkout SKIP_WORKTREE files by default [3]. It's a somewhat expected\n> part of this process ...\n> ...\n> Another tool that may help you here is 'git ls-files --sparse -t'. It lists\n> the files in the index and their \"tags\" ('H' is \"normal\" tracked files, 'S'\n> is SKIP_WORKTREE, etc. [4]), which can help identify when a file you'd\n> expect to be SKIP_WORKTREE is not and vice versa.\n\nWonderful.\n\nQuite honestly, because the code will most likely compile correctly\nif you just remove the unconditional \"we first expand the in-core\nindex fully\" code, and because the \"sparse index\" makes the existing\nindex walking code fail in unexpected and surprising ways, I\nconsider it unsuitably harder for people who are not yet familiar\nwith the system. Without a good test coverage (which is hard to\ngive unless you are familiar with the code being tested X-<), one\ncan easily get confused and lost.\n\nThanks for guiding a new contributor with the usual process of\nloosening \"require-full-index\".\n"},{"id":"451730","messageId":"e127dadb-7b44-55f8-16ea-9fcf94905db8@github.com","threadId":"57561","inReplyTo":"xmqqo824cbxl.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-21T15:20:08Z","receivedAt":"2022-03-21T15:20:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/17/2022 9:00 PM, Junio C Hamano wrote:\n> Victoria Dye writes:\n> \n>>> I tested this and it fails as expected with:\n>>> \"fatal: bad source, source=folder2/a, destination=deep/new\"\n>>\n>> Great! This should then probably be turned into a \"test_expect_fail\" test in\n>> 't1092' - that'll make sure we get both the right behavior and right error\n>> message with sparse index after it's enabled.\n>>\n>> However, I also get the same result when I add the '--sparse' option. I\n>> would expect the behavior to be \"move 'folder2/a' to 'deep/new' and check it\n>> out in the worktree\" - this may be a good candidate for improving the\n>> existing integration with sparse *checkout* before enabling sparse *index*\n>> (e.g., like when 'git add' was updated to not add sparse files by default\n>> [1]).\n>> ...\n>> I think you're right that this is a bug. This appears to come from the fact\n>> that 'mv' decides whether a directory is sparse only *after* it sees that it\n>> doesn't exist on-disk. \n>> ...\n>> So I think there are three potential things to fix here: \n>>\n>> 1. When empty folder2/ is on-disk, 'git mv' (without '--sparse') doesn't\n>> fail with \"bad source\", even though it should.\n>> 2. When you try to move a sparse file with 'git mv --sparse', it still\n>> fails.\n>> 3. SKIP_WORKTREE is not removed from out-of-cone files moved into the sparse\n>> cone.\n>>\n>> On a related note, there is precedent for needing to make fixes like this\n>> before integrating with sparse index. For example: in addition to the\n>> earlier examples in 'add' and 'reset', 'checkout-index' was changed to no\n>> longer checkout SKIP_WORKTREE files by default [3]. It's a somewhat expected\n>> part of this process ...\n>> ...\n>> Another tool that may help you here is 'git ls-files --sparse -t'. It lists\n>> the files in the index and their \"tags\" ('H' is \"normal\" tracked files, 'S'\n>> is SKIP_WORKTREE, etc. [4]), which can help identify when a file you'd\n>> expect to be SKIP_WORKTREE is not and vice versa.\n> \n> Wonderful.\n> \n> Quite honestly, because the code will most likely compile correctly\n> if you just remove the unconditional \"we first expand the in-core\n> index fully\" code, and because the \"sparse index\" makes the existing\n> index walking code fail in unexpected and surprising ways, I\n> consider it unsuitably harder for people who are not yet familiar\n> with the system. Without a good test coverage (which is hard to\n> give unless you are familiar with the code being tested X-<), one\n> can easily get confused and lost.\n\nCertainly, 'git mv' is looking to be harder than expected, but there\nis a lot of interesting exploration happening in the process.\n\nThanks for persisting on this one, Shaoxuan!\n\nThanks,\n-Stolee\n\n"},{"id":"451752","messageId":"xmqq8rt3xgmb.fsf@gitster.g","threadId":"57561","inReplyTo":"e127dadb-7b44-55f8-16ea-9fcf94905db8@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-21T19:14:36Z","receivedAt":"2022-03-21T19:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>>> Another tool that may help you here is 'git ls-files --sparse -t'. It lists\n>>> the files in the index and their \"tags\" ('H' is \"normal\" tracked files, 'S'\n>>> is SKIP_WORKTREE, etc. [4]), which can help identify when a file you'd\n>>> expect to be SKIP_WORKTREE is not and vice versa.\n>> \n>> Wonderful.\n>> \n>> Quite honestly, because the code will most likely compile correctly\n>> if you just remove the unconditional \"we first expand the in-core\n>> index fully\" code, and because the \"sparse index\" makes the existing\n>> index walking code fail in unexpected and surprising ways, I\n>> consider it unsuitably harder for people who are not yet familiar\n>> with the system. Without a good test coverage (which is hard to\n>> give unless you are familiar with the code being tested X-<), one\n>> can easily get confused and lost.\n>\n> Certainly, 'git mv' is looking to be harder than expected, but there\n> is a lot of interesting exploration happening in the process.\n\nYeah, I know.\n\nI am suprised that it is harder than expected *to* *you*, though.\nAfter having seen a few other topics, I thought that you should know\nhow deceptively easy to lose \"require-full\" and how hard to audit\nthe code that may expect \"a flat list of paths\" in the in-core index\n;-).\n\n> Thanks for persisting on this one, Shaoxuan!\n\nYes, thanks. And thanks for mentoring Shaoxuan.\n"},{"id":"451754","messageId":"b64c1805-dff9-3fd3-1e5e-84bd68d4b058@github.com","threadId":"57561","inReplyTo":"xmqq8rt3xgmb.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-21T19:45:11Z","receivedAt":"2022-03-21T19:45:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/21/2022 3:14 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>>>> Another tool that may help you here is 'git ls-files --sparse -t'. It lists\n>>>> the files in the index and their \"tags\" ('H' is \"normal\" tracked files, 'S'\n>>>> is SKIP_WORKTREE, etc. [4]), which can help identify when a file you'd\n>>>> expect to be SKIP_WORKTREE is not and vice versa.\n>>>\n>>> Wonderful.\n>>>\n>>> Quite honestly, because the code will most likely compile correctly\n>>> if you just remove the unconditional \"we first expand the in-core\n>>> index fully\" code, and because the \"sparse index\" makes the existing\n>>> index walking code fail in unexpected and surprising ways, I\n>>> consider it unsuitably harder for people who are not yet familiar\n>>> with the system. Without a good test coverage (which is hard to\n>>> give unless you are familiar with the code being tested X-<), one\n>>> can easily get confused and lost.\n>>\n>> Certainly, 'git mv' is looking to be harder than expected, but there\n>> is a lot of interesting exploration happening in the process.\n> \n> Yeah, I know.\n> \n> I am suprised that it is harder than expected *to* *you*, though.\n> After having seen a few other topics, I thought that you should know\n> how deceptively easy to lose \"require-full\" and how hard to audit\n> the code that may expect \"a flat list of paths\" in the in-core index\n> ;-).\n\nI'm particularly surprised in how much 'git mv' doesn't work very\nwell in the sparse-checkout environment already, which makes things\nmore difficult than \"just\" doing the normal sparse index things.\n\nIt's good that we are discovering them and working to fix them.\n\nThanks,\n-Stolee\n"},{"id":"451831","messageId":"CAJyCBORkauHAdDiHjQ2Agj3bNhLNPtKk-VW5=bNmBfNuQtv7hA@mail.gmail.com","threadId":"57561","inReplyTo":"b64c1805-dff9-3fd3-1e5e-84bd68d4b058@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-22T08:38:18Z","receivedAt":"2022-03-22T08:38:39Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Hi all,\n\nOn Tue, Mar 22, 2022 at 3:45 AM Derrick Stolee wrote:\n> I'm particularly surprised in how much 'git mv' doesn't work very\n> well in the sparse-checkout environment already, which makes things\n> more difficult than \"just\" doing the normal sparse index things.\n>\n> It's good that we are discovering them and working to fix them.\n>\n> Thanks,\n> -Stolee\n\nReally appreciate the mentoring and tips here, I'm trying to make some progress\nnow. The problems facing here certainly push me to explore more and know\nbetter about the codebase. Appreciate all the help :-)\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"451977","messageId":"e61303b8-10ad-5b5b-d48b-cac89ac53d29@github.com","threadId":"57561","inReplyTo":"CAJyCBORkauHAdDiHjQ2Agj3bNhLNPtKk-VW5=bNmBfNuQtv7hA@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-23T13:10:52Z","receivedAt":"2022-03-23T13:11:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/22/2022 4:38 AM, Shaoxuan Yuan wrote:\n> Hi all,\n> \n> On Tue, Mar 22, 2022 at 3:45 AM Derrick Stolee wrote:\n>> I'm particularly surprised in how much 'git mv' doesn't work very\n>> well in the sparse-checkout environment already, which makes things\n>> more difficult than \"just\" doing the normal sparse index things.\n>>\n>> It's good that we are discovering them and working to fix them.\n>>\n>> Thanks,\n>> -Stolee\n> \n> Really appreciate the mentoring and tips here, I'm trying to make some progress\n> now. The problems facing here certainly push me to explore more and know\n> better about the codebase. Appreciate all the help :-)\n\nA thought occurred to me while thinking about these difficulties:\nperhaps it is better to start with 'git rm' since that does only\nhalf of what 'git mv' does. It should be a smaller lift as a first\ncontribution. There is even a clear loop that is marked with \"TODO:\naudit for interaction with sparse-index.\"\n\nAs we've discovered in this thread, the direction for integrating\na builtin with the sparse index should follow this outline:\n\n0. Test the builtin in t1092 with interactions inside and outside\n of the sparse-checkout cone.*\n\n1. Add command_requires_full_index = 0 line to the builtin.\n\n2. Check for failures and diagnose them.\n\n3. Check for index expansion and remove them as necessary.\n (Go back to 2.)\n\n4. Run performance tests.\n\n(*) This step is the one we failed to focus enough on previously.\n\nOf course, if you've already gotten really far on 'mv' and don't\nwant to switch context, then keep at it.\n\nThanks,\n-Stolee\n"},{"id":"452070","messageId":"xmqqh77oqrpo.fsf@gitster.g","threadId":"57561","inReplyTo":"e61303b8-10ad-5b5b-d48b-cac89ac53d29@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T21:33:39Z","receivedAt":"2022-03-23T21:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 3/22/2022 4:38 AM, Shaoxuan Yuan wrote:\n>> Hi all,\n>> \n>> On Tue, Mar 22, 2022 at 3:45 AM Derrick Stolee wrote:\n>>> I'm particularly surprised in how much 'git mv' doesn't work very\n>>> well in the sparse-checkout environment already, which makes things\n>>> more difficult than \"just\" doing the normal sparse index things.\n>>>\n>>> It's good that we are discovering them and working to fix them.\n>>>\n>>> Thanks,\n>>> -Stolee\n>> \n>> Really appreciate the mentoring and tips here, I'm trying to make some progress\n>> now. The problems facing here certainly push me to explore more and know\n>> better about the codebase. Appreciate all the help :-)\n>\n> A thought occurred to me while thinking about these difficulties:\n> perhaps it is better to start with 'git rm' since that does only\n> half of what 'git mv' does. It should be a smaller lift as a first\n> contribution. There is even a clear loop that is marked with \"TODO:\n> audit for interaction with sparse-index.\"\n>\n> As we've discovered in this thread, the direction for integrating\n> a builtin with the sparse index should follow this outline:\n>\n> 0. Test the builtin in t1092 with interactions inside and outside\n> of the sparse-checkout cone.*\n>\n> 1. Add command_requires_full_index = 0 line to the builtin.\n>\n> 2. Check for failures and diagnose them.\n>\n> 3. Check for index expansion and remove them as necessary.\n> (Go back to 2.)\n>\n> 4. Run performance tests.\n>\n> (*) This step is the one we failed to focus enough on previously.\n\nJonathan, I thought you had a radically different approach that\nought to be much safer than the above---do you want to bring it up\nfor discussion?\n\n> Of course, if you've already gotten really far on 'mv' and don't\n> want to switch context, then keep at it.\n>\n> Thanks,\n> -Stolee\n"},{"id":"452424","messageId":"CAJyCBOQT1TwkNX_be9B3uKsv4Buf_ojfZoqfTAUqQ22Na7dY=g@mail.gmail.com","threadId":"57561","inReplyTo":"97a665fe-07c9-c4f6-4ab6-b6c0e1397c31@github.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-03-27T03:48:43Z","receivedAt":"2022-03-27T03:49:11Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Fri, Mar 18, 2022 at 5:57 AM Victoria Dye wrote:\n\nHi all,\n\nIt's been a busy week, I'm sorry that did not have much time to respond.\n\n=================================\nA brief summary of my latest investigations:\n\nI think the 'git mv' command still has some questionable aspects, especially\nwith the 'git sparse-checkout' command. And I feel we have to sort things out\nbetween 'git mv' and sparse-checkout first, then proceed to the issues with\nsparse-index.\n===========\n\nI'm trying to fix the first 2 of the 3 potential things mentioned earlier.\n\n> 1. When empty folder2/ is on-disk, 'git mv' (without '--sparse') doesn't\n> fail with \"bad source\", even though it should.\n\nIn this case, 'git mv' does not fail with \"bad source\" is something expected,\nbecause this error is related to the existence of an on-disk file, not\na directory.\nThe closest thing that it should fail with, in my opinion, is by\ncalling the advise\nfunction 'advise_on_updating_sparse_paths'.\n\nWith that being said, I now raise the first question: should we change\nthe sparse-\ncheckout cone check to be placed at the very beginning of the checking process,\nor keep it at the end as a very final check (where it is right now).\nMy preference is\nto place it at the very beginning, since the user should always be\ncautious about\ntouching contents outside of sparse-checkout cone, no matter what.\n\nIf a certain move touches out-of-cone stuff, and at the same time it will fail\nwith the, for example, \"destination exists\" or \"conflicted\" error, I\nthink these errors\nshould come second, after being supplied with the \"--sparse\" flag.\n\n> 2. When you try to move a sparse file with 'git mv --sparse', it still\n> fails.\n\nThis is also related to the first question, because in this case,\n\"folder2/a\" is not\non-disk, then 'git mv' will fail fast at the first check and ignore\nthe \"--sparse\" flag.\nEven if we modify the code to place the out-of-cone check at the very beginning,\nand supply a \"--sparse\" flag, we have to make 'git mv' look into the\nindex to find\na sparse file. And here I raise my second question: by moving a sparse\nfile, which\nis normally not on-disk, we have to alter the original 'git mv' logic\nto make it grope\ninto the index for the missing sparse file (for now it does not care\nabout the index, except\nwhen it receives a directory as ); can we make this change?\n\n--\nThanks & Regards,\nShaoxuan\n"},{"id":"452467","messageId":"b709435b-d5f7-d7f8-a22a-0fc78e106928@github.com","threadId":"57561","inReplyTo":"CAJyCBOQT1TwkNX_be9B3uKsv4Buf_ojfZoqfTAUqQ22Na7dY=g@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] mv: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-28T13:32:40Z","receivedAt":"2022-03-28T13:32:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/26/2022 11:48 PM, Shaoxuan Yuan wrote:\n> On Fri, Mar 18, 2022 at 5:57 AM Victoria Dye wrote:\n> \n> Hi all,\n> \n> It's been a busy week, I'm sorry that did not have much time to respond.\n> \n> =================================\n> A brief summary of my latest investigations:\n> \n> I think the 'git mv' command still has some questionable aspects, especially\n> with the 'git sparse-checkout' command. And I feel we have to sort things out\n> between 'git mv' and sparse-checkout first, then proceed to the issues with\n> sparse-index.\n> ===========\n> \n> I'm trying to fix the first 2 of the 3 potential things mentioned earlier.\n> \n>> 1. When empty folder2/ is on-disk, 'git mv' (without '--sparse') doesn't\n>> fail with \"bad source\", even though it should.\n\n>> 2. When you try to move a sparse file with 'git mv --sparse', it still\n>> fails.\n\nI think that these conditions are all related to the perspective as\ndescribed in the documentation of 'git mv':\n\n In the first form, it renames , which **must exist** and be\n either a file, symlink or directory, to . In the second\n form, the last argument has to be **an existing** directory; the given\n sources will be moved into this directory.\n\n The index is updated after successful completion, but the change must\n still be committed.\n\n(**emphasis mine**)\n\nSo, this documents the perspective that files must exist in the worktree\n(and destination directories must exist in the worktree), and after all\nof those checks, then the move is staged.\n\nI think part of our issues here is that in the case of a sparse-checkout,\nwe can have index entries that don't exist in the worktree. The expected\nbehavior we are considering is that 'git mv' should stage the movement\nof the file in the index, ignoring the worktree for paths outside of the\nsparse-checkout definition.\n\nOne way to do this would be to flip the implementation's direction:\nperform the index operation of moving the cache entry, then update the\nworktree to reflect that change (if necessary).\n\nThe case that I can think about being a bit strange is if the user has\nan unstaged deletion of the source file, then runs 'git mv'. Since the\nworktree is missing the file, then we cannot do the equivalent 'mv'\noperation in the worktree.\n\nOne other thing to keep in mind is that 'git mv '\ncan act like 'mv && git add ' if\n is an untracked path. So, 'git mv' can succeed even if the\nsource is not in the index!\n\nSo the change here is to ignore a non-existing path when the same path\nexists as a cache entry with the SKIP_WORKTREE bit. That bit does say\n\"ignore the worktree\" so 'git mv' isn't doing the right thing already.\n\n---\n\nIn conclusion, there might be multiple ways forward here:\n\n 1. Keep the expectation that is in the worktree as given,\n and let a tracked outside of the sparse-checkout cone\n result in a failure (as it currently does). Consider adding an\n advice message if is a tracked, sparse path.\n\n 2. Change the expectation to be that must either be a\n file in the worktree _or_ a tracked, sparse path (or both).\n\nThe nice thing here is that we can do (1) and then (2) later. The\nkey things to investigate for the sparse index, then are what\nhappens when is outside of the sparse-checkout cone?\nI imagine it would be fine if the moved file still appears in the\nworktree and is cleaned up by a later 'git sparse-checkout reapply'.\n\nI'm eager to here alternate opinions and options on this topic.\n\nThanks,\n-Stolee\n"}]}