{"thread":{"id":"50497","subject":"[GSoC][RFC PATCH] clone: use dir-iterator to avoid explicit dir traversal","startedAt":"2019-02-13T20:56:07Z","lastAt":"2019-02-15T09:00:13Z","messageCount":4,"participants":["Matheus Tavares","Christian Couder","Matheus Tavares Bernardino"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"369252","messageId":"20190213205554.4086-1-matheus.bernardino@usp.br","threadId":"50497","inReplyTo":null,"subject":"[GSoC][RFC PATCH] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-13T20:55:54Z","receivedAt":"2019-02-13T20:56:07Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Replace usage of opendir/readdir/closedir API to traverse directories\nrecursively, at copy_or_link_directory function, by the dir-iterator\nAPI.\n\nSigned-off-by: Matheus Tavares \n---\nThis is my microproject for GSoC 2019. It's still a RFC because I have \nsome questions. Any help will be much appreciated. \n \nThere're three places inside copy_or_link_directory's loop where \ndie_errno() is called. Should I call dir_iterator_abort, at these \nplaces, before die_errno() is called (to free resources)? \n \nAnd if so, should I check dir_iterator_abort's return code? It's said at\ndir-iterator.h that dir_iterator_abort returns ITER_ERROR on error, but \nby the code, we can see that it only possibly returns ITER_DONE. \n \nFinally, if this call and check both needs to be done, there'll be code \nreplication in this three places. Should I add a goto and do all this \nstuff at the function end? (But then, I would have to store die_errno's \nerror messages and errno code in temporary variables. Is this approach \ngood?) \n\n builtin/clone.c | 65 +++++++++++++++++++++++++++++--------------------\n 1 file changed, 38 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..f5208ad9ca 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -23,6 +23,8 @@\n #include \"transport.h\"\n #include \"strbuf.h\"\n #include \"dir.h\"\n+#include \"dir-iterator.h\"\n+#include \"iterator.h\"\n #include \"sigchain.h\"\n #include \"branch.h\"\n #include \"remote.h\"\n@@ -392,50 +394,55 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n \tfclose(in);\n }\n \n-static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n-\t\t\t\t const char *src_repo, int src_baselen)\n+static void mkdir_if_missing(const char *pathname, mode_t mode)\n {\n-\tstruct dirent *de;\n+\t/*\n+\t * Tries to create a dir at pathname. If pathname already exists and\n+\t * is a dir, do nothing.\n+\t */\n \tstruct stat buf;\n-\tint src_len, dest_len;\n-\tDIR *dir;\n \n-\tdir = opendir(src->buf);\n-\tif (!dir)\n-\t\tdie_errno(_(\"failed to open '%s'\"), src->buf);\n-\n-\tif (mkdir(dest->buf, 0777)) {\n+\tif (mkdir(pathname, mode)) {\n \t\tif (errno != EEXIST)\n-\t\t\tdie_errno(_(\"failed to create directory '%s'\"), dest->buf);\n-\t\telse if (stat(dest->buf, &buf))\n-\t\t\tdie_errno(_(\"failed to stat '%s'\"), dest->buf);\n+\t\t\tdie_errno(_(\"failed to create directory '%s'\"),\n+\t\t\t\t pathname);\n+\t\telse if (stat(pathname, &buf))\n+\t\t\tdie_errno(_(\"failed to stat '%s'\"), pathname);\n \t\telse if (!S_ISDIR(buf.st_mode))\n-\t\t\tdie(_(\"%s exists and is not a directory\"), dest->buf);\n+\t\t\tdie(_(\"%s exists and is not a directory\"), pathname);\n \t}\n+}\n+\n+static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n+\t\t\t\t const char *src_repo, int src_baselen)\n+{\n+\tint src_len, dest_len;\n+\tstruct dir_iterator *iter;\n+\tint iter_status;\n+\n+\tmkdir_if_missing(dest->buf, 0777);\n+\n+\titer = dir_iterator_begin(src->buf);\n \n \tstrbuf_addch(src, '/');\n \tsrc_len = src->len;\n \tstrbuf_addch(dest, '/');\n \tdest_len = dest->len;\n \n-\twhile ((de = readdir(dir)) != NULL) {\n+\twhile ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {\n \t\tstrbuf_setlen(src, src_len);\n-\t\tstrbuf_addstr(src, de->d_name);\n+\t\tstrbuf_addstr(src, iter->relative_path);\n \t\tstrbuf_setlen(dest, dest_len);\n-\t\tstrbuf_addstr(dest, de->d_name);\n-\t\tif (stat(src->buf, &buf)) {\n-\t\t\twarning (_(\"failed to stat %s\\n\"), src->buf);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (S_ISDIR(buf.st_mode)) {\n-\t\t\tif (de->d_name[0] != '.')\n-\t\t\t\tcopy_or_link_directory(src, dest,\n-\t\t\t\t\t\t src_repo, src_baselen);\n+\t\tstrbuf_addstr(dest, iter->relative_path);\n+\n+\t\tif (S_ISDIR(iter->st.st_mode)) {\n+\t\t\tif (iter->basename[0] != '.')\n+\t\t\t\tmkdir_if_missing(dest->buf, 0777);\n \t\t\tcontinue;\n \t\t}\n \n \t\t/* Files that cannot be copied bit-for-bit... */\n-\t\tif (!strcmp(src->buf + src_baselen, \"/info/alternates\")) {\n+\t\tif (!strcmp(iter->relative_path, \"info/alternates\")) {\n \t\t\tcopy_alternates(src, dest, src_repo);\n \t\t\tcontinue;\n \t\t}\n@@ -452,7 +459,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\tif (copy_file_with_time(dest->buf, src->buf, 0666))\n \t\t\tdie_errno(_(\"failed to copy file to '%s'\"), dest->buf);\n \t}\n-\tclosedir(dir);\n+\n+\tif (iter_status != ITER_DONE) {\n+\t\tstrbuf_setlen(src, src_len);\n+\t\tdie(_(\"failed to iterate over '%s'\"), src->buf);\n+\t}\n }\n \n static void clone_local(const char *src_repo, const char *dest_repo)\n-- \n2.20.1\n\n"},{"id":"369344","messageId":"CAP8UFD1aBn8=KOTp-D=CmO1iEnWUBS1-jR-aqfg3MZ7HZRU7Pg@mail.gmail.com","threadId":"50497","inReplyTo":"20190213205554.4086-1-matheus.bernardino@usp.br","subject":"Re: [GSoC][RFC PATCH] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-14T21:16:11Z","receivedAt":"2019-02-14T21:16:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 14, 2019 at 1:16 PM Matheus Tavares\n wrote:\n>\n> Replace usage of opendir/readdir/closedir API to traverse directories\n> recursively, at copy_or_link_directory function, by the dir-iterator\n> API.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> This is my microproject for GSoC 2019. It's still a RFC because I have\n> some questions. Any help will be much appreciated.\n\nThanks for working on a microproject!\n\n> There're three places inside copy_or_link_directory's loop where\n> die_errno() is called. Should I call dir_iterator_abort, at these\n> places, before die_errno() is called (to free resources)?\n\nI don't think it's necessary. We care about freeing resources when we\nreport errors (for example by returning an error code from inside a\nfunction), but not when we are exiting.\n\n> -static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> - const char *src_repo, int src_baselen)\n> +static void mkdir_if_missing(const char *pathname, mode_t mode)\n\nIt looks like your patch is both splitting copy_or_link_directory()\ninto 2 functions and converting it to use the dir-iterator API. Maybe\nit would be better to have 2 patches instead, one for splitting it and\none for converting it.\n\n> {\n> - struct dirent *de;\n> + /*\n> + * Tries to create a dir at pathname. If pathname already exists and\n> + * is a dir, do nothing.\n> + */\n\nI think we usually put such comments just before the function. And\nmaybe it could be shortened to \"Create a dir at pathname unless\nthere's already one\"\n"},{"id":"369351","messageId":"CAHd-oW6uHKfa_P+fZNZxG4+pme=SH_Wi+SJkhxwOtfR+L=0JBA@mail.gmail.com","threadId":"50497","inReplyTo":"CAP8UFD1aBn8=KOTp-D=CmO1iEnWUBS1-jR-aqfg3MZ7HZRU7Pg@mail.gmail.com","subject":"Re: [GSoC][RFC PATCH] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-14T22:03:55Z","receivedAt":"2019-02-14T22:04:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Feb 14, 2019 at 7:16 PM Christian Couder\n wrote:\n>\n> On Thu, Feb 14, 2019 at 1:16 PM Matheus Tavares\n> wrote:\n> >\n> > Replace usage of opendir/readdir/closedir API to traverse directories\n> > recursively, at copy_or_link_directory function, by the dir-iterator\n> > API.\n> >\n> > Signed-off-by: Matheus Tavares \n> > ---\n> > This is my microproject for GSoC 2019. It's still a RFC because I have\n> > some questions. Any help will be much appreciated.\n>\n> Thanks for working on a microproject!\n>\n\nHi, Christian. Thank you for the review and comments.\n\n> > There're three places inside copy_or_link_directory's loop where\n> > die_errno() is called. Should I call dir_iterator_abort, at these\n> > places, before die_errno() is called (to free resources)?\n>\n> I don't think it's necessary. We care about freeing resources when we\n> report errors (for example by returning an error code from inside a\n> function), but not when we are exiting.\n>\n\nOk, thanks!\n\n> > -static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> > - const char *src_repo, int src_baselen)\n> > +static void mkdir_if_missing(const char *pathname, mode_t mode)\n>\n> It looks like your patch is both splitting copy_or_link_directory()\n> into 2 functions and converting it to use the dir-iterator API. Maybe\n> it would be better to have 2 patches instead, one for splitting it and\n> one for converting it.\n>\n\nGot it. As the justification for splitting the function was to use the\nextracted part in the section that was previously recursive, I thought\nboth changes should be in the same patch. But I really was in doubt\nabout that. Should I split it into two patches and mention that\njustification at the first one? Or just split?\n\n> > {\n> > - struct dirent *de;\n> > + /*\n> > + * Tries to create a dir at pathname. If pathname already exists and\n> > + * is a dir, do nothing.\n> > + */\n>\n> I think we usually put such comments just before the function. And\n> maybe it could be shortened to \"Create a dir at pathname unless\n> there's already one\"\n\nRight, the shortened version does sounds much better, thanks! About\nthe comment placement, I followed what I saw in other functions from\nthe same file (\"copy_alternates\", for example). But also, I couldn't\nfind any instruction about that at Documentation/CodingGuidelines. So\nshould I move it as you suggested?\n"},{"id":"369376","messageId":"CAP8UFD0c7oPdaPkU55mus-3xgYcMmUSa_dsuGcGRF0ijn6r8iw@mail.gmail.com","threadId":"50497","inReplyTo":"CAHd-oW6uHKfa_P+fZNZxG4+pme=SH_Wi+SJkhxwOtfR+L=0JBA@mail.gmail.com","subject":"Re: [GSoC][RFC PATCH] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-15T08:59:56Z","receivedAt":"2019-02-15T09:00:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 14, 2019 at 11:04 PM Matheus Tavares Bernardino\n wrote:\n>\n> On Thu, Feb 14, 2019 at 7:16 PM Christian Couder\n> wrote:\n\n> > > -static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> > > - const char *src_repo, int src_baselen)\n> > > +static void mkdir_if_missing(const char *pathname, mode_t mode)\n> >\n> > It looks like your patch is both splitting copy_or_link_directory()\n> > into 2 functions and converting it to use the dir-iterator API. Maybe\n> > it would be better to have 2 patches instead, one for splitting it and\n> > one for converting it.\n> >\n>\n> Got it. As the justification for splitting the function was to use the\n> extracted part in the section that was previously recursive, I thought\n> both changes should be in the same patch. But I really was in doubt\n> about that. Should I split it into two patches and mention that\n> justification at the first one? Or just split?\n\nIf 2 patches instead of 1 makes it easier to review and understand\nwhat's going on, then I think you should indeed send 2 patches and\nmention the justification for splitting the function in the commit\nmessage of the first patch.\n\n> > I think we usually put such comments just before the function. And\n> > maybe it could be shortened to \"Create a dir at pathname unless\n> > there's already one\"\n>\n> Right, the shortened version does sounds much better, thanks! About\n> the comment placement, I followed what I saw in other functions from\n> the same file (\"copy_alternates\", for example).\n\nThen it's ok to place it like you did. Sorry about the noise.\n\n> But also, I couldn't\n> find any instruction about that at Documentation/CodingGuidelines. So\n> should I move it as you suggested?\n\nI think we have not been very consistent over the years. Recently I\nthink we have tried to add or move API documentation inside header\nfiles, and in general before functions in the code, but yeah it might\nnot have been documented.\n"}]}