{"thread":{"id":"52410","subject":"[PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","startedAt":"2019-12-08T18:06:20Z","lastAt":"2019-12-16T16:13:38Z","messageCount":6,"participants":["otalpster@gmail.com","Derrick Stolee","Matheus Tavares Bernardino","Junio C Hamano","otalpster"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"387719","messageId":"20191208180439.19018-1-otalpster@gmail.com","threadId":"52410","inReplyTo":null,"subject":"[PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","fromName":"","fromEmail":"otalpster@gmail.com","sentAt":"2019-12-08T18:04:39Z","receivedAt":"2019-12-08T18:06:20Z","isPatch":true,"sender":{"key":"otalpster@gmail.com","avatar":null},"body":"From: Plato <otalpster@gmail.com>\n\nReplace usage of opendir/readdir/closedir API to traverse directories\nrecursively, at remove_subtree() function, by the dir-iterator API. This\nsimplifies the code and avoids recursive calls to remove_subtree().\n\nSigned-off-by: Plato <otalpster@gmail.com>\n---\nHello,\n\nThis is my first patch.\nI hope I cc'd the correct people and didn't mess up.\n\nThe changes pass the test suite t/ and Travis CI.\nPlease point out any mistakes.\n\nThanks for your time! :)\n\n entry.c | 32 +++++++++++++++-----------------\n 1 file changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 53380bb614..e7f4881d3b 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -2,6 +2,8 @@\n #include \"blob.h\"\n #include \"object-store.h\"\n #include \"dir.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n #include \"streaming.h\"\n #include \"submodule.h\"\n #include \"progress.h\"\n@@ -50,29 +52,25 @@ 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+\tint ok;\n+\tunsigned int flags = DIR_ITERATOR_PEDANTIC;\n+\tstruct dir_iterator *iter = dir_iterator_begin(path->buf, flags);\n \n-\tif (!dir)\n+\tif (!iter)\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+\twhile ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n+\t\tif (is_dot_or_dotdot(iter->path.buf))\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-\t\t\tdie_errno(\"cannot unlink '%s'\", path->buf);\n-\t\tstrbuf_setlen(path, origlen);\n+\t\tif (unlink(iter->path.buf)) {\n+\t\t\tdie_errno(\"cannot unlink '%s'\", iter->path.buf);\n+\t\t}\n \t}\n-\tclosedir(dir);\n+\n+\tif (ok != ITER_DONE)\n+\t\tdie(_(\"failed to iterate over '%s'\"), path->buf);\n+\n \tif (rmdir(path->buf))\n \t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n }\n-- \n2.24.0\n\n"},{"id":"387786","messageId":"06024905-b00e-0fd6-f985-64a360caac2a@gmail.com","threadId":"52410","inReplyTo":"20191208180439.19018-1-otalpster@gmail.com","subject":"Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-09T15:21:32Z","receivedAt":"2019-12-09T15:21:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/8/2019 1:04 PM, otalpster@gmail.com wrote:\n> From: Plato <otalpster@gmail.com>\n> \n> Replace usage of opendir/readdir/closedir API to traverse directories\n> recursively, at remove_subtree() function, by the dir-iterator API. This\n> simplifies the code and avoids recursive calls to remove_subtree().\n> \n> Signed-off-by: Plato <otalpster@gmail.com>\n> ---\n> Hello,\n> \n> This is my first patch.\n> I hope I cc'd the correct people and didn't mess up.\n\nWelcome, Plato!\n\n> The changes pass the test suite t/ and Travis CI.\n> Please point out any mistakes.\n> \n> Thanks for your time! :)\n> \n>  entry.c | 32 +++++++++++++++-----------------\n>  1 file changed, 15 insertions(+), 17 deletions(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 53380bb614..e7f4881d3b 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -2,6 +2,8 @@\n>  #include \"blob.h\"\n>  #include \"object-store.h\"\n>  #include \"dir.h\"\n> +#include \"iterator.h\"\n> +#include \"dir-iterator.h\"\n>  #include \"streaming.h\"\n>  #include \"submodule.h\"\n>  #include \"progress.h\"\n> @@ -50,29 +52,25 @@ 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> +\tint ok;\n> +\tunsigned int flags = DIR_ITERATOR_PEDANTIC;\n> +\tstruct dir_iterator *iter = dir_iterator_begin(path->buf, flags);\n>  \n> -\tif (!dir)\n> +\tif (!iter)\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> +\twhile ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n> +\t\tif (is_dot_or_dotdot(iter->path.buf))\n>  \t\t\tcontinue;\n\nThe documentation in dir-iterator.h seems to skip . and .. for you.\nThis 'if' shouldn't be needed.\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\nIt took me reading dir-iterator.h to realize that the iterator\nis recursive, because I'm new to this API. This is a nice cleanup!\n\n> -\t\telse if (unlink(path->buf))\n> -\t\t\tdie_errno(\"cannot unlink '%s'\", path->buf);\n> -\t\tstrbuf_setlen(path, origlen);\n> +\t\tif (unlink(iter->path.buf)) {\n> +\t\t\tdie_errno(\"cannot unlink '%s'\", iter->path.buf);\n> +\t\t}\n\nnit: don't use braces for a single-line block.\n\n>  \t}\n> -\tclosedir(dir);\n> +\n> +\tif (ok != ITER_DONE)\n> +\t\tdie(_(\"failed to iterate over '%s'\"), path->buf);\n> +\n>  \tif (rmdir(path->buf))\n>  \t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n>  }\n\nThanks,\n-Stolee\n\n"},{"id":"387809","messageId":"CAHd-oW4MzH9Wrb2zNPF0JBN8+FRB4NtDX0166Qt2Kbqs0mDgcw@mail.gmail.com","threadId":"52410","inReplyTo":"20191208180439.19018-1-otalpster@gmail.com","subject":"Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-12-09T17:41:31Z","receivedAt":"2019-12-09T17:41:46Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sun, Dec 8, 2019 at 3:06 PM <otalpster@gmail.com> wrote:\n>\n> From: Plato <otalpster@gmail.com>\n>\n> Replace usage of opendir/readdir/closedir API to traverse directories\n> recursively, at remove_subtree() function, by the dir-iterator API. This\n> simplifies the code and avoids recursive calls to remove_subtree().\n>\n> Signed-off-by: Plato <otalpster@gmail.com>\n> ---\n> Hello,\n>\n> This is my first patch.\n\nHello Plato, and welcome!\nThanks for working on this.\n\n> I hope I cc'd the correct people and didn't mess up.\n>\n> The changes pass the test suite t/ and Travis CI.\n> Please point out any mistakes.\n>\n> Thanks for your time! :)\n>\n>  entry.c | 32 +++++++++++++++-----------------\n>  1 file changed, 15 insertions(+), 17 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index 53380bb614..e7f4881d3b 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -2,6 +2,8 @@\n>  #include \"blob.h\"\n>  #include \"object-store.h\"\n>  #include \"dir.h\"\n> +#include \"iterator.h\"\n> +#include \"dir-iterator.h\"\n>  #include \"streaming.h\"\n>  #include \"submodule.h\"\n>  #include \"progress.h\"\n> @@ -50,29 +52,25 @@ 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> +       int ok;\n> +       unsigned int flags = DIR_ITERATOR_PEDANTIC;\n> +       struct dir_iterator *iter = dir_iterator_begin(path->buf, flags);\n>\n> -       if (!dir)\n> +       if (!iter)\n>                 die_errno(\"cannot opendir '%s'\", path->buf);\n\nNitpick: since dir_iterator_begin() might fail for reasons other than\nan opendir() error, I think the error message here could be more\ngeneric. Maybe \"failed to start iterator over %s\"?\n\n> -       while ((de = readdir(dir)) != NULL) {\n> -               struct stat st;\n>\n> -               if (is_dot_or_dotdot(de->d_name))\n> +       while ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n> +               if (is_dot_or_dotdot(iter->path.buf))\n\nThis check is already done by dir-iterator internally, so you may\nremove it here.\n\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> -                       die_errno(\"cannot unlink '%s'\", path->buf);\n> -               strbuf_setlen(path, origlen);\n> +               if (unlink(iter->path.buf)) {\n\nunlink()-ing a directory in Linux will return a EISDIR error. So I\nthink you still need to use S_ISDIR() to check if iter->path.buf is a\ndirectory and call rmdir(), in this case.\n\nHowever, note that the dir-iterator API gives entries in pre-order.\nI.e. a directory appears before its subentries. In the use case of\nremove_subtree(), though, we need to traverse in post-order, since we\nhave to remove the subentries before removing the directory where they\nreside. My suggestion is that you add a preliminary patch,\nimplementing a new DIR_ITERATOR_POST_ORDER flag to dir-iterator.h, and\nthen use it in this patch.\n\nYou may also want to check this[1] series, which worked towards the\nsame goal of converting remove_subtree(). It ended up not getting\nmerged, back them, but some of the patches were re-used in this[2]\nseries which got merged. I think you could also re-use some of the\ncode from [1] that implements the post-order traversing and a test[3]\nfor remove_subtree().\n\nThanks,\nMatheus\n\n[1]: https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/\n[2]: https://public-inbox.org/git/cover.1562801254.git.matheus.bernardino@usp.br/\n[3]: https://public-inbox.org/git/1493226219-33423-3-git-send-email-bnmvco@gmail.com/\n"},{"id":"387845","messageId":"xmqqv9qptcyz.fsf@gitster-ct.c.googlers.com","threadId":"52410","inReplyTo":"CAHd-oW4MzH9Wrb2zNPF0JBN8+FRB4NtDX0166Qt2Kbqs0mDgcw@mail.gmail.com","subject":"Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T21:18:12Z","receivedAt":"2019-12-09T21:18:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n\n>> The changes pass the test suite t/ and Travis CI.\n>> Please point out any mistakes.\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>> -                       die_errno(\"cannot unlink '%s'\", path->buf);\n>> -               strbuf_setlen(path, origlen);\n>> +               if (unlink(iter->path.buf)) {\n>\n> unlink()-ing a directory in Linux will return a EISDIR error. So I\n> think you still need to use S_ISDIR() to check if iter->path.buf is a\n> directory and call rmdir(), in this case.\n>\n> However, note that the dir-iterator API gives entries in pre-order.\n> I.e. a directory appears before its subentries. In the use case of\n> remove_subtree(), though, we need to traverse in post-order, since we\n> have to remove the subentries before removing the directory where they\n> reside. My suggestion is that you add a preliminary patch,\n> implementing a new DIR_ITERATOR_POST_ORDER flag to dir-iterator.h, and\n> then use it in this patch.\n\nThanks for a review and a few hints to nudge a new contributor in\nthe right direction.  Very much appreciated.\n\nI wonder why the bugs in this patch weren't caught by self test we\nalready have, by the way.  We need a bit better test coverage,\nperhaps?\n\n"},{"id":"387870","messageId":"CAHd-oW5tVNP=yNZqPANkk4JLWKJyPkb4g4AeUerrd1O=xdLKXQ@mail.gmail.com","threadId":"52410","inReplyTo":"xmqqv9qptcyz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-12-10T04:38:09Z","receivedAt":"2019-12-10T04:38:25Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Dec 9, 2019 at 6:18 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n>\n> >> The changes pass the test suite t/ and Travis CI.\n> >> Please point out any mistakes.\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> >> -                       die_errno(\"cannot unlink '%s'\", path->buf);\n> >> -               strbuf_setlen(path, origlen);\n> >> +               if (unlink(iter->path.buf)) {\n> >\n> > unlink()-ing a directory in Linux will return a EISDIR error. So I\n> > think you still need to use S_ISDIR() to check if iter->path.buf is a\n> > directory and call rmdir(), in this case.\n> >\n> > However, note that the dir-iterator API gives entries in pre-order.\n> > I.e. a directory appears before its subentries. In the use case of\n> > remove_subtree(), though, we need to traverse in post-order, since we\n> > have to remove the subentries before removing the directory where they\n> > reside. My suggestion is that you add a preliminary patch,\n> > implementing a new DIR_ITERATOR_POST_ORDER flag to dir-iterator.h, and\n> > then use it in this patch.\n>\n> Thanks for a review and a few hints to nudge a new contributor in\n> the right direction.  Very much appreciated.\n>\n> I wonder why the bugs in this patch weren't caught by self test we\n> already have, by the way.  We need a bit better test coverage,\n> perhaps?\n\nI think there is no current test that covers remove_subtree() being\ncalled with nested directories. But we could use the test proposed by\nDaniel[1], which does fail when this current patch is applied. So,\nmaybe, Plato could also include this test in v2, before performing the\ndir-iterator convertion.\n\n[1]: https://public-inbox.org/git/1493226219-33423-3-git-send-email-bnmvco@gmail.com/\n"},{"id":"388284","messageId":"CAEKpa1QqrxyS1jeAUoSEAM2bC5h+trPJc_Mm1M18NVA73sCWKQ@mail.gmail.com","threadId":"52410","inReplyTo":"CAHd-oW5tVNP=yNZqPANkk4JLWKJyPkb4g4AeUerrd1O=xdLKXQ@mail.gmail.com","subject":"Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal","fromName":"otalpster","fromEmail":"otalpster@gmail.com","sentAt":"2019-12-16T16:12:12Z","receivedAt":"2019-12-16T16:13:38Z","isPatch":true,"sender":{"key":"otalpster@gmail.com","avatar":null},"body":"On Tue, Dec 10, 2019 at 6:38 AM Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n> On Mon, Dec 9, 2019 at 6:18 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n> >\n> > >> The changes pass the test suite t/ and Travis CI.\n> > >> Please point out any mistakes.\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> > >> -                       die_errno(\"cannot unlink '%s'\", path->buf);\n> > >> -               strbuf_setlen(path, origlen);\n> > >> +               if (unlink(iter->path.buf)) {\n> > >\n> > > unlink()-ing a directory in Linux will return a EISDIR error. So I\n> > > think you still need to use S_ISDIR() to check if iter->path.buf is a\n> > > directory and call rmdir(), in this case.\n> > >\n> > > However, note that the dir-iterator API gives entries in pre-order.\n> > > I.e. a directory appears before its subentries. In the use case of\n> > > remove_subtree(), though, we need to traverse in post-order, since we\n> > > have to remove the subentries before removing the directory where they\n> > > reside. My suggestion is that you add a preliminary patch,\n> > > implementing a new DIR_ITERATOR_POST_ORDER flag to dir-iterator.h, and\n> > > then use it in this patch.\n> >\n> > Thanks for a review and a few hints to nudge a new contributor in\n> > the right direction.  Very much appreciated.\n> >\n> > I wonder why the bugs in this patch weren't caught by self test we\n> > already have, by the way.  We need a bit better test coverage,\n> > perhaps?\n>\n> I think there is no current test that covers remove_subtree() being\n> called with nested directories. But we could use the test proposed by\n> Daniel[1], which does fail when this current patch is applied. So,\n> maybe, Plato could also include this test in v2, before performing the\n> dir-iterator convertion.\n>\n> [1]: https://public-inbox.org/git/1493226219-33423-3-git-send-email-bnmvco@gmail.com/\n\nHello!\nThanks for the warm welcome and sorry for the late reply.\nYour review and hints you provided were very helpful and appreciated.\n\nYes, I can work and include the test in v2, as well as performing the\ndir-iterator conversion. The required process and the necessary\nchanges I need to make were very clear.\n\nThis week I'm very busy. I'll work on this in great detail from next week.\n\nThanks,\nPlato\n"}]}