{"thread":{"id":"45509","subject":"[PATCH v2] [GSoC] remove_subtree(): reimplement using iterators","startedAt":"2017-03-25T11:27:57Z","lastAt":"2017-03-25T12:29:49Z","messageCount":4,"participants":["Daniel Ferreira","Duy Nguyen","Daniel Ferreira (theiostream)"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"315380","messageId":"1490441257-44727-1-git-send-email-bnmvco@gmail.com","threadId":"45509","inReplyTo":null,"subject":"[PATCH v2] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-25T11:27:37Z","receivedAt":"2017-03-25T11:27:57Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Use dir_iterator to traverse through remove_subtree()'s directory tree,\navoiding the need for recursive calls to readdir(). Simplify\nremove_subtree()'s code.\n\nA conversion similar in purpose was previously done at 46d092a\n(\"for_each_reflog(): reimplement using iterators\", 2016-05-21).\n\nSigned-off-by: Daniel Ferreira \n---\n\nThis is a second-version patch of the Google Summer of Code microproject for\nrefactoring recursive readdir() calls to use dir_iterator instead. v1 can be\nfound in:\n\nhttps://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730\n\nAdditionally, for debugging purposes I turned remove_subtree() into a no-op\nand ran git tests. Some failures were at:\n\n* t2000-checkout-cache-clash.sh\n* t2003-checkout-cache-mkdir.sh\n\nIf you guys could check those files out and warn me if any additional tests\nwould be welcome, please let me know.\n\nThanks.\n\n entry.c | 28 +++++++---------------------\n 1 file changed, 7 insertions(+), 21 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex c6eea240b..3cb92592d 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -2,6 +2,8 @@\n #include \"blob.h\"\n #include \"dir.h\"\n #include \"streaming.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n\n static void create_directories(const char *path, int path_len,\n \t\t\t const struct checkout *state)\n@@ -46,29 +48,13 @@ static void create_directories(const char *path, int path_len,\n\n static void remove_subtree(struct strbuf *path)\n {\n-\tDIR *dir = opendir(path->buf);\n-\tstruct dirent *de;\n-\tint origlen = path->len;\n-\n-\tif (!dir)\n-\t\tdie_errno(\"cannot opendir '%s'\", path->buf);\n-\twhile ((de = readdir(dir)) != NULL) {\n-\t\tstruct stat st;\n-\n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n-\n-\t\tstrbuf_addch(path, '/');\n-\t\tstrbuf_addstr(path, de->d_name);\n-\t\tif (lstat(path->buf, &st))\n-\t\t\tdie_errno(\"cannot lstat '%s'\", path->buf);\n-\t\tif (S_ISDIR(st.st_mode))\n-\t\t\tremove_subtree(path);\n-\t\telse if (unlink(path->buf))\n+\tstruct dir_iterator *diter = dir_iterator_begin(path->buf);\n+\n+\twhile (dir_iterator_advance(diter) == ITER_OK) {\n+\t\tif (unlink(diter->path.buf))\n \t\t\tdie_errno(\"cannot unlink '%s'\", path->buf);\n-\t\tstrbuf_setlen(path, origlen);\n \t}\n-\tclosedir(dir);\n+\n \tif (rmdir(path->buf))\n \t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n }\n--\n2.12.1.433.g82305b74f.dirty\n\n"},{"id":"315381","messageId":"CACsJy8Aajf1Lb4mKC=NEUQTNy7QRK99Aont_UCRnHJfho8n+UA@mail.gmail.com","threadId":"45509","inReplyTo":"1490441257-44727-1-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-25T11:51:03Z","receivedAt":"2017-03-25T11:51:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira wrote:\n> Use dir_iterator to traverse through remove_subtree()'s directory tree,\n> avoiding the need for recursive calls to readdir(). Simplify\n> remove_subtree()'s code.\n>\n> A conversion similar in purpose was previously done at 46d092a\n> (\"for_each_reflog(): reimplement using iterators\", 2016-05-21).\n>\n> Signed-off-by: Daniel Ferreira \n> ---\n>\n> This is a second-version patch of the Google Summer of Code microproject for\n> refactoring recursive readdir() calls to use dir_iterator instead. v1 can be\n> found in:\n>\n> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730\n>\n> Additionally, for debugging purposes I turned remove_subtree() into a no-op\n> and ran git tests. Some failures were at:\n>\n> * t2000-checkout-cache-clash.sh\n> * t2003-checkout-cache-mkdir.sh\n>\n> If you guys could check those files out and warn me if any additional tests\n> would be welcome, please let me know.\n>\n> Thanks.\n>\n> entry.c | 28 +++++++---------------------\n> 1 file changed, 7 insertions(+), 21 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index c6eea240b..3cb92592d 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -2,6 +2,8 @@\n> #include \"blob.h\"\n> #include \"dir.h\"\n> #include \"streaming.h\"\n> +#include \"iterator.h\"\n> +#include \"dir-iterator.h\"\n>\n> static void create_directories(const char *path, int path_len,\n> const struct checkout *state)\n> @@ -46,29 +48,13 @@ static void create_directories(const char *path, int path_len,\n>\n> static void remove_subtree(struct strbuf *path)\n> {\n> - DIR *dir = opendir(path->buf);\n> - struct dirent *de;\n> - int origlen = path->len;\n> -\n> - if (!dir)\n> - die_errno(\"cannot opendir '%s'\", path->buf);\n> - while ((de = readdir(dir)) != NULL) {\n> - struct stat st;\n> -\n> - if (is_dot_or_dotdot(de->d_name))\n> - continue;\n> -\n> - strbuf_addch(path, '/');\n> - strbuf_addstr(path, de->d_name);\n> - if (lstat(path->buf, &st))\n> - die_errno(\"cannot lstat '%s'\", path->buf);\n> - if (S_ISDIR(st.st_mode))\n> - remove_subtree(path);\n> - else if (unlink(path->buf))\n> + struct dir_iterator *diter = dir_iterator_begin(path->buf);\n> +\n> + while (dir_iterator_advance(diter) == ITER_OK) {\n> + if (unlink(diter->path.buf))\n> die_errno(\"cannot unlink '%s'\", path->buf);\n> - strbuf_setlen(path, origlen);\n> }\n> - closedir(dir);\n> +\n> if (rmdir(path->buf))\n> die_errno(\"cannot rmdir '%s'\", path->buf);\n\nEven though it's very nice that lots of code is deleted. This is not\nentirely correct, is it? Before this patch, rmdir() is called for\nevery recursive remove_subtree() call. After this patch, it's only\ncalled once (and likely fails unless you have no subdirectories).\n\n> }\n> --\n> 2.12.1.433.g82305b74f.dirty\n>\n\n\n\n-- \nDuy\n"},{"id":"315387","messageId":"CAEA2_RK=zMwKeidPDS9u5Ep1htb3TUeOqJ6Cfd644ira4DeXaA@mail.gmail.com","threadId":"45509","inReplyTo":"CACsJy8Aajf1Lb4mKC=NEUQTNy7QRK99Aont_UCRnHJfho8n+UA@mail.gmail.com","subject":"Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-25T12:13:35Z","receivedAt":"2017-03-25T12:14:16Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"You are correct, which shows that since all tests pass, we need to\ncome up with better cases for this function.\n\nAs for a solution, I believe that the best way to go for it is to\ndir_iterator's implementation to have an \"Option to iterate over\ndirectory paths before vs. after their contents\" (something predicted\nin the commit that created it). If it iterates over directories after\nall of its contents (currently it does so before) we just need to\ncheck if the entry is a directory and if so, rmdir() it. Does that\nmake sense?\n\nOn Sat, Mar 25, 2017 at 8:51 AM, Duy Nguyen wrote:\n> On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira wrote:\n>> Use dir_iterator to traverse through remove_subtree()'s directory tree,\n>> avoiding the need for recursive calls to readdir(). Simplify\n>> remove_subtree()'s code.\n>>\n>> A conversion similar in purpose was previously done at 46d092a\n>> (\"for_each_reflog(): reimplement using iterators\", 2016-05-21).\n>>\n>> Signed-off-by: Daniel Ferreira \n>> ---\n>>\n>> This is a second-version patch of the Google Summer of Code microproject for\n>> refactoring recursive readdir() calls to use dir_iterator instead. v1 can be\n>> found in:\n>>\n>> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730\n>>\n>> Additionally, for debugging purposes I turned remove_subtree() into a no-op\n>> and ran git tests. Some failures were at:\n>>\n>> * t2000-checkout-cache-clash.sh\n>> * t2003-checkout-cache-mkdir.sh\n>>\n>> If you guys could check those files out and warn me if any additional tests\n>> would be welcome, please let me know.\n>>\n>> Thanks.\n>>\n>> entry.c | 28 +++++++---------------------\n>> 1 file changed, 7 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/entry.c b/entry.c\n>> index c6eea240b..3cb92592d 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -2,6 +2,8 @@\n>> #include \"blob.h\"\n>> #include \"dir.h\"\n>> #include \"streaming.h\"\n>> +#include \"iterator.h\"\n>> +#include \"dir-iterator.h\"\n>>\n>> static void create_directories(const char *path, int path_len,\n>> const struct checkout *state)\n>> @@ -46,29 +48,13 @@ static void create_directories(const char *path, int path_len,\n>>\n>> static void remove_subtree(struct strbuf *path)\n>> {\n>> - DIR *dir = opendir(path->buf);\n>> - struct dirent *de;\n>> - int origlen = path->len;\n>> -\n>> - if (!dir)\n>> - die_errno(\"cannot opendir '%s'\", path->buf);\n>> - while ((de = readdir(dir)) != NULL) {\n>> - struct stat st;\n>> -\n>> - if (is_dot_or_dotdot(de->d_name))\n>> - continue;\n>> -\n>> - strbuf_addch(path, '/');\n>> - strbuf_addstr(path, de->d_name);\n>> - if (lstat(path->buf, &st))\n>> - die_errno(\"cannot lstat '%s'\", path->buf);\n>> - if (S_ISDIR(st.st_mode))\n>> - remove_subtree(path);\n>> - else if (unlink(path->buf))\n>> + struct dir_iterator *diter = dir_iterator_begin(path->buf);\n>> +\n>> + while (dir_iterator_advance(diter) == ITER_OK) {\n>> + if (unlink(diter->path.buf))\n>> die_errno(\"cannot unlink '%s'\", path->buf);\n>> - strbuf_setlen(path, origlen);\n>> }\n>> - closedir(dir);\n>> +\n>> if (rmdir(path->buf))\n>> die_errno(\"cannot rmdir '%s'\", path->buf);\n>\n> Even though it's very nice that lots of code is deleted. This is not\n> entirely correct, is it? Before this patch, rmdir() is called for\n> every recursive remove_subtree() call. After this patch, it's only\n> called once (and likely fails unless you have no subdirectories).\n>\n>> }\n>> --\n>> 2.12.1.433.g82305b74f.dirty\n>>\n>\n>\n>\n> --\n> Duy\n"},{"id":"315390","messageId":"CACsJy8Dxh-QPBBLfaFWPAWUsbA9GVXA7x+mXLjEvYKhk1zOpig@mail.gmail.com","threadId":"45509","inReplyTo":"CAEA2_RK=zMwKeidPDS9u5Ep1htb3TUeOqJ6Cfd644ira4DeXaA@mail.gmail.com","subject":"Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-25T12:29:08Z","receivedAt":"2017-03-25T12:29:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 25, 2017 at 7:13 PM, Daniel Ferreira (theiostream)\n wrote:\n> You are correct, which shows that since all tests pass, we need to\n> come up with better cases for this function.\n>\n> As for a solution, I believe that the best way to go for it is to\n> dir_iterator's implementation to have an \"Option to iterate over\n> directory paths before vs. after their contents\" (something predicted\n> in the commit that created it). If it iterates over directories after\n> all of its contents (currently it does so before) we just need to\n> check if the entry is a directory and if so, rmdir() it. Does that\n> make sense?\n\nYes. However since this is a microproject, intended to be done quickly\njust to get you familiarize with the community and the process, I\nsuggest you look for another readdir() place that can be easily\nconverted to using dir-iterator first so you could go through the\nwhole thing. But of course I will not object you improving\ndir-iterator and clean remove_subtree() up ;-)\n-- \nDuy\n"}]}