{"thread":{"id":"45487","subject":"[PATCH] [GSoC] remove_subtree(): reimplement using iterators","startedAt":"2017-03-24T04:07:21Z","lastAt":"2017-03-25T01:02:50Z","messageCount":3,"participants":["Daniel Ferreira","Stefan Beller","Daniel Ferreira (theiostream)"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315224","messageId":"1490328420-75901-1-git-send-email-bnmvco@gmail.com","threadId":"45487","inReplyTo":null,"subject":"[PATCH] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-24T04:07:00Z","receivedAt":"2017-03-24T04:07:21Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Uses dir_iterator to traverse through remove_subtree()'s directory tree,\navoiding the need for recursive calls to readdir() and simplifying code.\n\nSuggested in the GSoC microproject list, as well as:\nhttps://public-inbox.org/git/xmqqk27m4h3h.fsf@gitster.mtv.corp.google.com/\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 <bnmvco@gmail.com>\n---\n\nHey there! This is my microproject for Google Summer of Code on git.\nIt has passed on Travis CI (https://travis-ci.org/theiostream/git),\nalthough I would appreciate any suggestion to improve test coverage\nfor the affected function.\n\nThis is, to my knowledge, one of the few microprojects that have not\nyet been started by someone on this list, but please let me know if\nsomeone else is already on it.\n\nThank you.\n\n entry.c | 24 +++++++-----------------\n 1 file changed, 7 insertions(+), 17 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex c6eea24..a88c219 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,17 @@ 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+\tstruct dir_iterator *diter = dir_iterator_begin(path->buf);\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+\twhile (dir_iterator_advance(diter) == ITER_OK) {\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\tstrbuf_addstr(path, diter->relative_path);\n+\t\tif (unlink(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.7.4 (Apple Git-66)\n\n"},{"id":"315275","messageId":"CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com","threadId":"45487","inReplyTo":"1490328420-75901-1-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T17:02:57Z","receivedAt":"2017-03-24T17:03:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Welcome to the Git community!\n\nOn Thu, Mar 23, 2017 at 9:07 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:\n> Uses dir_iterator to traverse through remove_subtree()'s directory tree,\n> avoiding the need for recursive calls to readdir() and simplifying code.\n\nPlease use a more imperative style. (e.g. s/Uses/Use/ ...\ns/and simplfying/which simplifies/)\n\n>\n> Suggested in the GSoC microproject list, as well as:\n> https://public-inbox.org/git/xmqqk27m4h3h.fsf@gitster.mtv.corp.google.com/\n\nThanks for this link. It gives good context for reviewing the change,\nbut it will not be good context to record as a commit message.\n(When someone looks at a commit message later on, they are usually trying\nto figure out what the author was thinking; if there were any special cases to\nbe thought about. Was performance on the authors mind? etc)\n\nSo I propose to put the link into the more informal section if a\nreroll is needed.\nI cc'd Duy, who came up with this Microproject.\n\n> A conversion similar in purpose was previously done at 46d092a\n> (\"for_each_reflog(): reimplement using iterators\", 2016-05-21).\n\nThanks for pointing at another conversion.\n\n>\n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n> ---\n>\n> Hey there! This is my microproject for Google Summer of Code on git.\n> It has passed on Travis CI (https://travis-ci.org/theiostream/git),\n> although I would appreciate any suggestion to improve test coverage\n> for the affected function.\n\nThis function is deep down in the worktree update mechanism, so any run\nof \"git reset\", \"git checkout\", git cherry-pick\" (and all the others), which\nremove a directory (possibly recursive) covers the functionality.\n\nIf I were to search for test coverage for this function in particular, I'd\nstart by looking at \"(cd t && ls t1*)\".\n\n\n> This is, to my knowledge, one of the few microprojects that have not\n> yet been started by someone on this list, but please let me know if\n> someone else is already on it.\n\ncool. :)\n\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,17 @@ 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> +       struct dir_iterator *diter = dir_iterator_begin(path->buf);\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> +       while (dir_iterator_advance(diter) == ITER_OK) {\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> +               strbuf_addstr(path, diter->relative_path);\n> +               if (unlink(path->buf))\n>                         die_errno(\"cannot unlink '%s'\", path->buf);\n>                 strbuf_setlen(path, origlen);\n\nInstead of constructing the path again here based on relative path\nand the path parameter, I wonder if we could use\n\n    if (unlink(diter->path))\n        ..\n\nhere? Then we would not need the strbuf at all?\nAlso we'd need to handle (empty) directories differently for removal?\nDo we need to check the return code of dir_iterator_advance\nfor ITER_ERROR as well?\n\n>         }\n> -       closedir(dir);\n> +\n>         if (rmdir(path->buf))\n>                 die_errno(\"cannot rmdir '%s'\", path->buf);\n\nThis would remove the \"top level\" directory as given by path.\nWhen reading the dir-iterator code, I am not sure if this is\nalso part of the yield in dir_iterator_advance.\n\nThanks for working on this micro project!\nStefan\n"},{"id":"315371","messageId":"CAEA2_RJ6OjLjS5yGjvd+S0iH+Zv0Zs6StGMv3_9ON6hVzD-85A@mail.gmail.com","threadId":"45487","inReplyTo":"CAEA2_RLZztaRwcppwS45XfXO1n_VKw5547uScOhQON=ktttW8g@mail.gmail.com","subject":"Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-25T01:02:08Z","receivedAt":"2017-03-25T01:02:50Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"> On Fri, Mar 24, 2017 at 2:02 PM, Stefan Beller <sbeller@google.com> wrote:\n\n> Welcome to the Git community!\n\nThank you!\n\n> Please use a more imperative style. (e.g. s/Uses/Use/ ...\n> s/and simplfying/which simplifies/)\n\nThank you. Will do in a second version of this patch.\n\n> Thanks for this link. It gives good context for reviewing the change,\n> but it will not be good context to record as a commit message.\n> (When someone looks at a commit message later on, they are usually trying\n> to figure out what the author was thinking; if there were any special cases to\n> be thought about. Was performance on the authors mind? etc)\n\n> So I propose to put the link into the more informal section if a\n> reroll is needed.\n\nPerfect. I will remove it from the message.\n\n> Instead of constructing the path again here based on relative path\n> and the path parameter, I wonder if we could use\n>\n>     if (unlink(diter->path))\n>         ..\n>\n> here? Then we would not need the strbuf at all?\n\nYes, we can! Thank you for the pointer. Will be in the next version of the\npatch.\n\n> Also we'd need to handle (empty) directories differently for removal?\n\nFrom what I've tested, we do not need to do it.\n\n> Do we need to check the return code of dir_iterator_advance\n> for ITER_ERROR as well?\n\nI believe not – it only tries to perform an operation if we have ITER_OK. Since\nITER_ERROR would end up in a no-op anyway I don't see how a check for it\nwould be useful.\n\n>\n>\n> >         }\n> > -       closedir(dir);\n> > +\n> >         if (rmdir(path->buf))\n> >                 die_errno(\"cannot rmdir '%s'\", path->buf);\n>\n> This would remove the \"top level\" directory as given by path.\n> When reading the dir-iterator code, I am not sure if this is\n> also part of the yield in dir_iterator_advance.\n\nI've tested it, and it does not yield in there.\n\nThank you for the advice, and as stated, will submit a v2 of the patch\nin short notice.\n\nThank you,\nDaniel.\n"}]}