{"thread":{"id":"58389","subject":"[PATCH] unpack-trees: fix sparse directory recursion check","startedAt":"2022-09-01T21:02:53Z","lastAt":"2022-09-02T19:03:26Z","messageCount":5,"participants":["Victoria Dye via GitGitGadget","Johannes Schindelin","Derrick Stolee","Junio C Hamano","Shaoxuan Yuan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"462492","messageId":"pull.1344.git.1662066153644.gitgitgadget@gmail.com","threadId":"58389","inReplyTo":null,"subject":"[PATCH] unpack-trees: fix sparse directory recursion check","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T21:02:33Z","receivedAt":"2022-09-01T21:02:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nEnsure 'is_sparse_directory_entry()' receives a valid 'name_entry *' if one\nexists in the list of tree(s) being unpacked in 'unpack_callback()'.\n\nCurrently, 'is_sparse_directory_entry()' is called with the first\n'name_entry' in the 'names' list of entries on 'unpack_callback()'. However,\nthis entry may be empty even when other elements of 'names' are not (such as\nwhen switching from an orphan branch back to a \"normal\" branch). As a\nresult, 'is_sparse_directory_entry()' could incorrectly indicate that a\nsparse directory is *not* actually sparse because the name of the index\nentry does not match the (empty) 'name_entry' path.\n\nFix the issue by using the existing 'name_entry *p' value in\n'unpack_callback()', which points to the first non-empty entry in 'names'.\nBecause 'p' is 'const', also update 'is_sparse_directory_entry()'s\n'name_entry *' argument to be 'const'.\n\nFinally, add a regression test case.\n\nReported-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    unpack-trees: fix sparse directory recursion check\n    \n    This issue was found when the updates from v2.37.3 introduced a test\n    failure in a downstream test suite.\n    \n    The issue stems from the fact that, before v2.37.3, 'unpack_callback()'\n    could previously \"assume\" that 'names[0]' was non-empty if a cache entry\n    was unpacked as a sparse index. When b15207b8cf (unpack-trees: unpack\n    new trees as sparse directories, 2022-08-08)) was introduced, it\n    invalidated that assumption by allowing sparse directories to be\n    unpacked based on the contents of other 'names' entries, rather than\n    unnecessarily recursing into them and unpacking files individually. As a\n    result, certain scenarios could cause a sparse directory to be unpacked\n    then also recursively unpacked via 'traverse_trees_recursive()',\n    creating duplicate index entries.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1344%2Fvdye%2Fbugfix%2Fsparse-index-orphan-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1344/vdye/bugfix/sparse-index-orphan-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1344\n\n t/t1092-sparse-checkout-compatibility.sh | 9 +++++++++\n unpack-trees.c                           | 4 ++--\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 0302e36fd66..b9350c075c2 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -380,6 +380,15 @@ test_expect_success 'checkout with modified sparse directory' '\n \ttest_all_match git checkout base\n '\n \n+test_expect_success 'checkout orphan then non-orphan' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout --orphan test-orphan &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git checkout base &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n test_expect_success 'add outside sparse cone' '\n \tinit_repos &&\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 90b92114be8..bae812156c4 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1423,7 +1423,7 @@ static void debug_unpack_callback(int n,\n  * from the tree walk at the given traverse_info.\n  */\n static int is_sparse_directory_entry(struct cache_entry *ce,\n-\t\t\t\t     struct name_entry *name,\n+\t\t\t\t     const struct name_entry *name,\n \t\t\t\t     struct traverse_info *info)\n {\n \tif (!ce || !name || !S_ISSPARSEDIR(ce->ce_mode))\n@@ -1562,7 +1562,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t\t}\n \t\t}\n \n-\t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n+\t\tif (!is_sparse_directory_entry(src[0], p, info) &&\n \t\t    !is_new_sparse_dir &&\n \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n \t\t\t\t\t\t    names, info) < 0) {\n\nbase-commit: d42b38dfb5edf1a7fddd9542d722f91038407819\n-- \ngitgitgadget\n"},{"id":"462517","messageId":"70537543-n1ns-657s-245n-p4n563q9rsrs@tzk.qr","threadId":"58389","inReplyTo":"pull.1344.git.1662066153644.gitgitgadget@gmail.com","subject":"Re: [PATCH] unpack-trees: fix sparse directory recursion check","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-02T09:03:44Z","receivedAt":"2022-09-02T09:03:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Victoria,\n\nOn Thu, 1 Sep 2022, Victoria Dye via GitGitGadget wrote:\n\n> From: Victoria Dye <vdye@github.com>\n>\n> Ensure 'is_sparse_directory_entry()' receives a valid 'name_entry *' if one\n> exists in the list of tree(s) being unpacked in 'unpack_callback()'.\n>\n> Currently, 'is_sparse_directory_entry()' is called with the first\n> 'name_entry' in the 'names' list of entries on 'unpack_callback()'. However,\n> this entry may be empty even when other elements of 'names' are not (such as\n> when switching from an orphan branch back to a \"normal\" branch). As a\n> result, 'is_sparse_directory_entry()' could incorrectly indicate that a\n> sparse directory is *not* actually sparse because the name of the index\n> entry does not match the (empty) 'name_entry' path.\n>\n> Fix the issue by using the existing 'name_entry *p' value in\n> 'unpack_callback()', which points to the first non-empty entry in 'names'.\n> Because 'p' is 'const', also update 'is_sparse_directory_entry()'s\n> 'name_entry *' argument to be 'const'.\n>\n> Finally, add a regression test case.\n>\n> Reported-by: Derrick Stolee <derrickstolee@github.com>\n> Signed-off-by: Victoria Dye <vdye@github.com>\n\nA delightful commit message. Thank you!\n\nThe explanation makes perfect sense to me, and with that explanation the\ndiff becomes obvious, too.\n\nThanks,\nDscho\n\n> ---\n>     unpack-trees: fix sparse directory recursion check\n>\n>     This issue was found when the updates from v2.37.3 introduced a test\n>     failure in a downstream test suite.\n>\n>     The issue stems from the fact that, before v2.37.3, 'unpack_callback()'\n>     could previously \"assume\" that 'names[0]' was non-empty if a cache entry\n>     was unpacked as a sparse index. When b15207b8cf (unpack-trees: unpack\n>     new trees as sparse directories, 2022-08-08)) was introduced, it\n>     invalidated that assumption by allowing sparse directories to be\n>     unpacked based on the contents of other 'names' entries, rather than\n>     unnecessarily recursing into them and unpacking files individually. As a\n>     result, certain scenarios could cause a sparse directory to be unpacked\n>     then also recursively unpacked via 'traverse_trees_recursive()',\n>     creating duplicate index entries.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1344%2Fvdye%2Fbugfix%2Fsparse-index-orphan-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1344/vdye/bugfix/sparse-index-orphan-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1344\n>\n>  t/t1092-sparse-checkout-compatibility.sh | 9 +++++++++\n>  unpack-trees.c                           | 4 ++--\n>  2 files changed, 11 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 0302e36fd66..b9350c075c2 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -380,6 +380,15 @@ test_expect_success 'checkout with modified sparse directory' '\n>  \ttest_all_match git checkout base\n>  '\n>\n> +test_expect_success 'checkout orphan then non-orphan' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git checkout --orphan test-orphan &&\n> +\ttest_all_match git status --porcelain=v2 &&\n> +\ttest_all_match git checkout base &&\n> +\ttest_all_match git status --porcelain=v2\n> +'\n> +\n>  test_expect_success 'add outside sparse cone' '\n>  \tinit_repos &&\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 90b92114be8..bae812156c4 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1423,7 +1423,7 @@ static void debug_unpack_callback(int n,\n>   * from the tree walk at the given traverse_info.\n>   */\n>  static int is_sparse_directory_entry(struct cache_entry *ce,\n> -\t\t\t\t     struct name_entry *name,\n> +\t\t\t\t     const struct name_entry *name,\n>  \t\t\t\t     struct traverse_info *info)\n>  {\n>  \tif (!ce || !name || !S_ISSPARSEDIR(ce->ce_mode))\n> @@ -1562,7 +1562,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n>  \t\t\t}\n>  \t\t}\n>\n> -\t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n> +\t\tif (!is_sparse_directory_entry(src[0], p, info) &&\n>  \t\t    !is_new_sparse_dir &&\n>  \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n>  \t\t\t\t\t\t    names, info) < 0) {\n>\n> base-commit: d42b38dfb5edf1a7fddd9542d722f91038407819\n> --\n> gitgitgadget\n>\n"},{"id":"462526","messageId":"8dea9ffe-d03b-f838-6cc5-db0de5024d9f@github.com","threadId":"58389","inReplyTo":"pull.1344.git.1662066153644.gitgitgadget@gmail.com","subject":"Re: [PATCH] unpack-trees: fix sparse directory recursion check","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-02T13:32:01Z","receivedAt":"2022-09-02T14:02:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/1/2022 5:02 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n> \n> Ensure 'is_sparse_directory_entry()' receives a valid 'name_entry *' if one\n> exists in the list of tree(s) being unpacked in 'unpack_callback()'.\n> \n> Currently, 'is_sparse_directory_entry()' is called with the first\n> 'name_entry' in the 'names' list of entries on 'unpack_callback()'. However,\n> this entry may be empty even when other elements of 'names' are not (such as\n> when switching from an orphan branch back to a \"normal\" branch). As a\n> result, 'is_sparse_directory_entry()' could incorrectly indicate that a\n> sparse directory is *not* actually sparse because the name of the index\n> entry does not match the (empty) 'name_entry' path.\n\nThank you for finding the root cause here. It was very non-obvious!\n\n> +test_expect_success 'checkout orphan then non-orphan' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git checkout --orphan test-orphan &&\n> +\ttest_all_match git status --porcelain=v2 &&\n> +\ttest_all_match git checkout base &&\n> +\ttest_all_match git status --porcelain=v2\n> +'\n> +\n\nThis test demonstrates how specific the case needs to be for this to\nactually happen.\n\nCould it also happen if we are going from a commit without that\nsparse directory and then to a commit with that sparse directory? I\nthink that would be a more common case, but I was unable to\nmanipulate the test repo in t1092 to trigger this bug in the existing\ntest cases.\n\nThis makes me think that this bug _is_ extremely rare, so we don't\nneed to rush this into a 2.37.4 or anything. It would be good to\nbring it into 2.38.0-rc0, though.\n\nThanks!\n-Stolee\n"},{"id":"462543","messageId":"xmqqr10t90jb.fsf@gitster.g","threadId":"58389","inReplyTo":"8dea9ffe-d03b-f838-6cc5-db0de5024d9f@github.com","subject":"Re: [PATCH] unpack-trees: fix sparse directory recursion check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T16:57:44Z","receivedAt":"2022-09-02T16:58:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> Could it also happen if we are going from a commit without that\n> sparse directory and then to a commit with that sparse directory? I\n> think that would be a more common case, but I was unable to\n> manipulate the test repo in t1092 to trigger this bug in the existing\n> test cases.\n>\n> This makes me think that this bug _is_ extremely rare, so we don't\n> need to rush this into a 2.37.4 or anything. It would be good to\n> bring it into 2.38.0-rc0, though.\n\nWhen the \"test\" part of the patch is applied directly on top of the\nvd/sparse-reset-checkout-fixes topic without its \"fix\" part, the\nnewly added test fails, and with the \"fix\" part applied, everything\ngoes peachy.  I think applying it on top of the topic as a fix to it\nwould be the most sensible.  Any motivated distro packager can grab\nthe topic and merge to their long-term maintenance track that way\nmore easily.\n\nThanks all.\n"},{"id":"462555","messageId":"b3516420-872f-e556-dcea-d2dc5317f8dc@gmail.com","threadId":"58389","inReplyTo":"pull.1344.git.1662066153644.gitgitgadget@gmail.com","subject":"Re: [PATCH] unpack-trees: fix sparse directory recursion check","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-09-02T19:03:19Z","receivedAt":"2022-09-02T19:03:26Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On 9/1/2022 2:02 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n> \n> Ensure 'is_sparse_directory_entry()' receives a valid 'name_entry *' if one\n> exists in the list of tree(s) being unpacked in 'unpack_callback()'.\n> \n> Currently, 'is_sparse_directory_entry()' is called with the first\n> 'name_entry' in the 'names' list of entries on 'unpack_callback()'. However,\n> this entry may be empty even when other elements of 'names' are not (such as\n> when switching from an orphan branch back to a \"normal\" branch). As a\n> result, 'is_sparse_directory_entry()' could incorrectly indicate that a\n> sparse directory is *not* actually sparse because the name of the index\n> entry does not match the (empty) 'name_entry' path.\n> \n> Fix the issue by using the existing 'name_entry *p' value in\n> 'unpack_callback()', which points to the first non-empty entry in 'names'.\n> Because 'p' is 'const', also update 'is_sparse_directory_entry()'s\n> 'name_entry *' argument to be 'const'.\n> \n> Finally, add a regression test case.\n> \n> Reported-by: Derrick Stolee <derrickstolee@github.com>\n> Signed-off-by: Victoria Dye <vdye@github.com>\n\nThe issue is well explained: even I don't have prior knowledge but I\nstill get the gist out of it! The added tests in t1092 reflect the\nchanges brought by the diff!\n\nThanks,\nShaoxuan\n\n> ---\n>     unpack-trees: fix sparse directory recursion check\n>     \n>     This issue was found when the updates from v2.37.3 introduced a test\n>     failure in a downstream test suite.\n>     \n>     The issue stems from the fact that, before v2.37.3, 'unpack_callback()'\n>     could previously \"assume\" that 'names[0]' was non-empty if a cache entry\n>     was unpacked as a sparse index. When b15207b8cf (unpack-trees: unpack\n>     new trees as sparse directories, 2022-08-08)) was introduced, it\n>     invalidated that assumption by allowing sparse directories to be\n>     unpacked based on the contents of other 'names' entries, rather than\n>     unnecessarily recursing into them and unpacking files individually. As a\n>     result, certain scenarios could cause a sparse directory to be unpacked\n>     then also recursively unpacked via 'traverse_trees_recursive()',\n>     creating duplicate index entries.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1344%2Fvdye%2Fbugfix%2Fsparse-index-orphan-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1344/vdye/bugfix/sparse-index-orphan-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1344\n> \n>  t/t1092-sparse-checkout-compatibility.sh | 9 +++++++++\n>  unpack-trees.c                           | 4 ++--\n>  2 files changed, 11 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 0302e36fd66..b9350c075c2 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -380,6 +380,15 @@ test_expect_success 'checkout with modified sparse directory' '\n>  \ttest_all_match git checkout base\n>  '\n>  \n> +test_expect_success 'checkout orphan then non-orphan' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git checkout --orphan test-orphan &&\n> +\ttest_all_match git status --porcelain=v2 &&\n> +\ttest_all_match git checkout base &&\n> +\ttest_all_match git status --porcelain=v2\n> +'\n> +\n>  test_expect_success 'add outside sparse cone' '\n>  \tinit_repos &&\n>  \n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 90b92114be8..bae812156c4 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1423,7 +1423,7 @@ static void debug_unpack_callback(int n,\n>   * from the tree walk at the given traverse_info.\n>   */\n>  static int is_sparse_directory_entry(struct cache_entry *ce,\n> -\t\t\t\t     struct name_entry *name,\n> +\t\t\t\t     const struct name_entry *name,\n>  \t\t\t\t     struct traverse_info *info)\n>  {\n>  \tif (!ce || !name || !S_ISSPARSEDIR(ce->ce_mode))\n> @@ -1562,7 +1562,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n>  \t\t\t}\n>  \t\t}\n>  \n> -\t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n> +\t\tif (!is_sparse_directory_entry(src[0], p, info) &&\n>  \t\t    !is_new_sparse_dir &&\n>  \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n>  \t\t\t\t\t\t    names, info) < 0) {\n> \n> base-commit: d42b38dfb5edf1a7fddd9542d722f91038407819\n\n"}]}