{"thread":{"id":"50518","subject":"[GSoC][PATCH 0/2] clone: convert explicit traversal to","startedAt":"2019-02-15T15:49:24Z","lastAt":"2019-02-22T00:21:29Z","messageCount":14,"participants":["Matheus Tavares","Christian Couder","Thomas Gummerer","Matheus Tavares Bernardino","Daniel Ferreira (theiostream)"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"369386","messageId":"20190215154913.18800-1-matheus.bernardino@usp.br","threadId":"50518","inReplyTo":null,"subject":"[GSoC][PATCH 0/2] clone: convert explicit traversal to","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-15T15:49:11Z","receivedAt":"2019-02-15T15:49:24Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Convert explicit recursive dir traversal at copy_or_link_directory of\nclone.c to use dir-iterator API.\n\nThis is my microproject for GSoC 2019. Idea taken from\nhttps://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal\n\nMatheus Tavares (2):\n clone: extract function from copy_or_link_directory\n clone: use dir-iterator to avoid explicit dir traversal\n\n builtin/clone.c | 64 ++++++++++++++++++++++++++++---------------------\n 1 file changed, 37 insertions(+), 27 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"369387","messageId":"20190215154913.18800-2-matheus.bernardino@usp.br","threadId":"50518","inReplyTo":"20190215154913.18800-1-matheus.bernardino@usp.br","subject":"[GSoC][PATCH 1/2] clone: extract function from copy_or_link_directory","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-15T15:49:12Z","receivedAt":"2019-02-15T15:49:26Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Extract dir creation code snippet from copy_or_link_directory to its own\nfunction named mkdir_if_missing. This change will help removing\ncopy_or_link_directory's explicit recursion, which will be done in patch\n\"clone: use dir-iterator to avoid explicit dir traversal\". Also makes\ncode more readable.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/clone.c | 27 +++++++++++++++++++--------\n 1 file changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..2a1cc4dab9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -392,6 +392,24 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n \tfclose(in);\n }\n \n+static void mkdir_if_missing(const char *pathname, mode_t mode)\n+{\n+\t/*\n+\t * Create a dir at pathname unless there's already one.\n+\t */\n+\tstruct stat buf;\n+\n+\tif (mkdir(pathname, mode)) {\n+\t\tif (errno != EEXIST)\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\"), 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@@ -404,14 +422,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \tif (!dir)\n \t\tdie_errno(_(\"failed to open '%s'\"), src->buf);\n \n-\tif (mkdir(dest->buf, 0777)) {\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\telse if (!S_ISDIR(buf.st_mode))\n-\t\t\tdie(_(\"%s exists and is not a directory\"), dest->buf);\n-\t}\n+\tmkdir_if_missing(dest->buf, 0777);\n \n \tstrbuf_addch(src, '/');\n \tsrc_len = src->len;\n-- \n2.20.1\n\n"},{"id":"369388","messageId":"20190215154913.18800-3-matheus.bernardino@usp.br","threadId":"50518","inReplyTo":"20190215154913.18800-1-matheus.bernardino@usp.br","subject":"[GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-15T15:49:13Z","receivedAt":"2019-02-15T15:49:30Z","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---\n builtin/clone.c | 39 +++++++++++++++++++--------------------\n 1 file changed, 19 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 2a1cc4dab9..66ae347f79 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@@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\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-\tstruct dirent *de;\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+\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@@ -463,7 +458,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":"369429","messageId":"CAP8UFD1pFeV6wUPpqNHkYB_3m=HQYFssJzTSrhfsyg0p+svDgw@mail.gmail.com","threadId":"50518","inReplyTo":"20190215154913.18800-2-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 1/2] clone: extract function from copy_or_link_directory","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-16T06:23:37Z","receivedAt":"2019-02-16T06:23:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares\n wrote:\n>\n> Extract dir creation code snippet from copy_or_link_directory to its own\n> function named mkdir_if_missing. This change will help removing\n> copy_or_link_directory's explicit recursion, which will be done in patch\n> \"clone: use dir-iterator to avoid explicit dir traversal\".\n\n\"which will be done in a following patch\" is enough and perhaps even\nbetter as the following patch can then be changed independently of\nthis one.\n\n> Also makes\n> code more readable.\n>\n> Signed-off-by: Matheus Tavares \n\n[...]\n\n> +static void mkdir_if_missing(const char *pathname, mode_t mode)\n> +{\n> + /*\n> + * Create a dir at pathname unless there's already one.\n> + */\n> + struct stat buf;\n\nI know that the variable was already called \"buf\" in\ncopy_or_link_directory() and that there are a few other places in the\ncode where a 'struct stat' is called \"buf\", but in most places the\n'struct stat' variables are called \"st\":\n\n$ git grep 'struct stat ' '*.c' | perl -ne 'print \"$1\\n\" if (m/struct\nstat ([\\w_]+)/);' | sort | uniq -c | sort -nr\n 129 st\n 6 sb\n 3 buf\n 2 statbuf\n 1 st_stdin\n 1 st_git\n 1 statbuffer\n 1 st2\n 1 st1\n 1 nst\n 1 loginfo\n 1 cwd_stat\n 1 argstat\n\nSo I wonder if we should use \"st\" here instead of \"buf\". We also often\nuse \"buf\" for 'struct strbuf' variables which can be confusing.\n\nOtherwise the patch looks good to me.\n"},{"id":"369430","messageId":"CAP8UFD3BvcdfbmYTsAprx0Au_AN2VUbuQd3LA0i9TBz0N4kfYQ@mail.gmail.com","threadId":"50518","inReplyTo":"20190215154913.18800-3-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-16T06:41:38Z","receivedAt":"2019-02-16T06:41:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 15, 2019 at 5:39 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\nYou may want to add that this simplifies the code and avoid recursive\ncalls of copy_or_link_directory().\n\nOtherwise the patch looks good to me. Thanks.\n"},{"id":"369468","messageId":"20190216143824.GJ6085@hank.intra.tgummerer.com","threadId":"50518","inReplyTo":"20190215154913.18800-3-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-16T14:38:24Z","receivedAt":"2019-02-16T14:44:05Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/15, Matheus Tavares wrote:\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> builtin/clone.c | 39 +++++++++++++++++++--------------------\n> 1 file changed, 19 insertions(+), 20 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 2a1cc4dab9..66ae347f79 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> @@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\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\nThe src_baselen parameter is now unused, and should be removed. Our\ncodebase currently doesn't compile with -Wunused-parameter, so this is\nnot something the compiler can catch at the moment unfortunately.\nHowever there is some work going on towards removing unused parameter\nfrom the codebase, so it would be nice to not make things worse here.\n\n*1*: https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net\n\n> -\tstruct dirent *de;\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> +\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\nWhy was this warning removed? I don't see a corresponding warning in\nthe iterator API. The one thing the iterator API is doing is that it\ndoes an lstat on the path to check if it exists. However that does\nnot follow symlinks, and ignores possible other errors such as\npermission errors.\n\nIf there is a good reason to remove the warning, that would be useful\nto describe in the commit message.\n\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> @@ -463,7 +458,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\nInterestingly enough, this is not something that can currently\nhappen if I read the dir-iterator code correctly. Even though the\ndir_iterator_advance function says it can return ITER_ERROR, it never\nactually does. The only way the iter_status can be not ITER_DONE at\nthis point is if we would 'break' out of the loop.\n\nI don't think it hurts to be defensive here in case someone decides to\nbreak out of the loop in the future, just something odd I noticed\nwhile reviewing the code.\n\n> }\n> \n> static void clone_local(const char *src_repo, const char *dest_repo)\n> -- \n> 2.20.1\n> \n"},{"id":"369566","messageId":"CAHd-oW60a+zz9J+u0HiRuTy-FKYN4s95fCcR3mgJz0hUokhTCQ@mail.gmail.com","threadId":"50518","inReplyTo":"20190216143824.GJ6085@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-18T21:13:16Z","receivedAt":"2019-02-18T21:13:32Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Feb 16, 2019 at 12:38 PM Thomas Gummerer wrote:\n>\n> On 02/15, Matheus Tavares wrote:\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> > builtin/clone.c | 39 +++++++++++++++++++--------------------\n> > 1 file changed, 19 insertions(+), 20 deletions(-)\n> >\n> > diff --git a/builtin/clone.c b/builtin/clone.c\n> > index 2a1cc4dab9..66ae347f79 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> > @@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\n> > static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> > const char *src_repo, int src_baselen)\n> > {\n>\n\nHi, Thomas. Thanks for the review.\n\n> The src_baselen parameter is now unused, and should be removed. Our\n> codebase currently doesn't compile with -Wunused-parameter, so this is\n> not something the compiler can catch at the moment unfortunately.\n> However there is some work going on towards removing unused parameter\n> from the codebase, so it would be nice to not make things worse here.\n>\n\nNice! I will fix that in v2.\n\n> *1*: https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net\n>\n> > - struct dirent *de;\n> > - struct stat buf;\n> > int src_len, dest_len;\n> > - DIR *dir;\n> > -\n> > - dir = opendir(src->buf);\n> > - if (!dir)\n> > - die_errno(_(\"failed to open '%s'\"), src->buf);\n> > + struct dir_iterator *iter;\n> > + int iter_status;\n> >\n> > mkdir_if_missing(dest->buf, 0777);\n> >\n> > + iter = dir_iterator_begin(src->buf);\n> > +\n> > strbuf_addch(src, '/');\n> > src_len = src->len;\n> > strbuf_addch(dest, '/');\n> > dest_len = dest->len;\n> >\n> > - while ((de = readdir(dir)) != NULL) {\n> > + while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {\n> > strbuf_setlen(src, src_len);\n> > - strbuf_addstr(src, de->d_name);\n> > + strbuf_addstr(src, iter->relative_path);\n> > strbuf_setlen(dest, dest_len);\n> > - strbuf_addstr(dest, de->d_name);\n> > - if (stat(src->buf, &buf)) {\n> > - warning (_(\"failed to stat %s\\n\"), src->buf);\n> > - continue;\n> > - }\n>\n> Why was this warning removed? I don't see a corresponding warning in\n> the iterator API. The one thing the iterator API is doing is that it\n> does an lstat on the path to check if it exists. However that does\n> not follow symlinks, and ignores possible other errors such as\n> permission errors.\n>\n\nYou are right. I didn't know the differences from lstat and stat. And\nreflecting on this now, I realize that the problem is even deeper:\ncopy_or_link_directory follows symlinks but dir-iterator don't, so I\ncannot use dir-iterator without falling back to recursion on\ncopy_or_link_directory. Because of that, I thought off adding an\noption for dir-iterator to follow symlinks. Does this seem like a good\nidea?\n\nAlso, I just noticed that dir-iterator follows hidden paths while\ncopy_or_link_directory don't. Maybe another option to add for\ndir-iterator?\n\n> If there is a good reason to remove the warning, that would be useful\n> to describe in the commit message.\n>\n> > - if (S_ISDIR(buf.st_mode)) {\n> > - if (de->d_name[0] != '.')\n> > - copy_or_link_directory(src, dest,\n> > - src_repo, src_baselen);\n> > + strbuf_addstr(dest, iter->relative_path);\n> > +\n> > + if (S_ISDIR(iter->st.st_mode)) {\n> > + if (iter->basename[0] != '.')\n> > + mkdir_if_missing(dest->buf, 0777);\n> > continue;\n> > }\n> >\n> > /* Files that cannot be copied bit-for-bit... */\n> > - if (!strcmp(src->buf + src_baselen, \"/info/alternates\")) {\n> > + if (!strcmp(iter->relative_path, \"info/alternates\")) {\n> > copy_alternates(src, dest, src_repo);\n> > continue;\n> > }\n> > @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> > if (copy_file_with_time(dest->buf, src->buf, 0666))\n> > die_errno(_(\"failed to copy file to '%s'\"), dest->buf);\n> > }\n> > - closedir(dir);\n> > +\n> > + if (iter_status != ITER_DONE) {\n> > + strbuf_setlen(src, src_len);\n> > + die(_(\"failed to iterate over '%s'\"), src->buf);\n> > + }\n>\n> Interestingly enough, this is not something that can currently\n> happen if I read the dir-iterator code correctly. Even though the\n> dir_iterator_advance function says it can return ITER_ERROR, it never\n> actually does. The only way the iter_status can be not ITER_DONE at\n> this point is if we would 'break' out of the loop.\n>\n> I don't think it hurts to be defensive here in case someone decides to\n> break out of the loop in the future, just something odd I noticed\n> while reviewing the code.\n>\n\nYes, I also noticed that. But I thought it would be nice to make this\ncheck so that this code stays consistent to the documentation at\ndir-iterator.h (although implementation is different).\n\nSomething I just noticed now is that copy_or_link_directory dies upon\nan opendir error, but dir-iterator just prints a warning and keeps\ngoing (looking for another file/dir to return for the caller). Is this\nok? Or should I, perhaps, add a \"pedantic\" option to dir-iterator so\nthat, when enabled, it immediately returns ITER_ERROR when an error\noccurs instead of keep going?\n\nI'm proposing some new options to dir-iterator because as the patch\nthat adds it says \"There are obviously a lot of features that could\neasily be added to this class\", and maybe those would be useful for\nother dir-iterator users. But I don't know if that would be the best\nway of doing it, so any feedback on this will be much appreciated.\nAlso, I saw on public-inbox[1] a patchset from 2017 proposing new\nfeatures/options for dir-iterator, but I don't really know if (and\nwhy) it was rejected. (I couldn't find the patches on master/pu log)\n\n[1] https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/\n\nThanks,\nMatheus Tavares\n\n\n> > }\n> >\n> > static void clone_local(const char *src_repo, const char *dest_repo)\n> > --\n> > 2.20.1\n> >\n"},{"id":"369568","messageId":"CAHd-oW7rocXECsYdFwVefD58hxoRocw=2UZBJGSPyWjDE53zXQ@mail.gmail.com","threadId":"50518","inReplyTo":"CAP8UFD1pFeV6wUPpqNHkYB_3m=HQYFssJzTSrhfsyg0p+svDgw@mail.gmail.com","subject":"Re: [GSoC][PATCH 1/2] clone: extract function from copy_or_link_directory","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-18T21:28:06Z","receivedAt":"2019-02-18T21:28:20Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Feb 16, 2019 at 4:23 AM Christian Couder\n wrote:\n>\n> On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares\n> wrote:\n> >\n> > Extract dir creation code snippet from copy_or_link_directory to its own\n> > function named mkdir_if_missing. This change will help removing\n> > copy_or_link_directory's explicit recursion, which will be done in patch\n> > \"clone: use dir-iterator to avoid explicit dir traversal\".\n>\n> \"which will be done in a following patch\" is enough and perhaps even\n> better as the following patch can then be changed independently of\n> this one.\n>\n\nOk! I will fix that in v2.\n\n> > Also makes\n> > code more readable.\n> >\n> > Signed-off-by: Matheus Tavares \n>\n> [...]\n>\n> > +static void mkdir_if_missing(const char *pathname, mode_t mode)\n> > +{\n> > + /*\n> > + * Create a dir at pathname unless there's already one.\n> > + */\n> > + struct stat buf;\n>\n> I know that the variable was already called \"buf\" in\n> copy_or_link_directory() and that there are a few other places in the\n> code where a 'struct stat' is called \"buf\", but in most places the\n> 'struct stat' variables are called \"st\":\n>\n> $ git grep 'struct stat ' '*.c' | perl -ne 'print \"$1\\n\" if (m/struct\n> stat ([\\w_]+)/);' | sort | uniq -c | sort -nr\n> 129 st\n> 6 sb\n> 3 buf\n> 2 statbuf\n> 1 st_stdin\n> 1 st_git\n> 1 statbuffer\n> 1 st2\n> 1 st1\n> 1 nst\n> 1 loginfo\n> 1 cwd_stat\n> 1 argstat\n>\n> So I wonder if we should use \"st\" here instead of \"buf\". We also often\n> use \"buf\" for 'struct strbuf' variables which can be confusing.\n>\n\nRight! I'm changing that in v2 too.\n\nThanks,\nMatheus\n\n> Otherwise the patch looks good to me.\n"},{"id":"369572","messageId":"20190218233541.GK6085@hank.intra.tgummerer.com","threadId":"50518","inReplyTo":"CAHd-oW60a+zz9J+u0HiRuTy-FKYN4s95fCcR3mgJz0hUokhTCQ@mail.gmail.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-18T23:35:41Z","receivedAt":"2019-02-18T23:35:48Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/18, Matheus Tavares Bernardino wrote:\n> On Sat, Feb 16, 2019 at 12:38 PM Thomas Gummerer wrote:\n> >\n> > On 02/15, Matheus Tavares wrote:\n> > > - struct dirent *de;\n> > > - struct stat buf;\n> > > int src_len, dest_len;\n> > > - DIR *dir;\n> > > -\n> > > - dir = opendir(src->buf);\n> > > - if (!dir)\n> > > - die_errno(_(\"failed to open '%s'\"), src->buf);\n> > > + struct dir_iterator *iter;\n> > > + int iter_status;\n> > >\n> > > mkdir_if_missing(dest->buf, 0777);\n> > >\n> > > + iter = dir_iterator_begin(src->buf);\n> > > +\n> > > strbuf_addch(src, '/');\n> > > src_len = src->len;\n> > > strbuf_addch(dest, '/');\n> > > dest_len = dest->len;\n> > >\n> > > - while ((de = readdir(dir)) != NULL) {\n> > > + while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {\n> > > strbuf_setlen(src, src_len);\n> > > - strbuf_addstr(src, de->d_name);\n> > > + strbuf_addstr(src, iter->relative_path);\n> > > strbuf_setlen(dest, dest_len);\n> > > - strbuf_addstr(dest, de->d_name);\n> > > - if (stat(src->buf, &buf)) {\n> > > - warning (_(\"failed to stat %s\\n\"), src->buf);\n> > > - continue;\n> > > - }\n> >\n> > Why was this warning removed? I don't see a corresponding warning in\n> > the iterator API. The one thing the iterator API is doing is that it\n> > does an lstat on the path to check if it exists. However that does\n> > not follow symlinks, and ignores possible other errors such as\n> > permission errors.\n> >\n> \n> You are right. I didn't know the differences from lstat and stat. And\n> reflecting on this now, I realize that the problem is even deeper:\n> copy_or_link_directory follows symlinks but dir-iterator don't, so I\n> cannot use dir-iterator without falling back to recursion on\n> copy_or_link_directory. Because of that, I thought off adding an\n> option for dir-iterator to follow symlinks. Does this seem like a good\n> idea?\n\nHmm looking at how this function is actually called, I don't think we\nneed to worry about symlinks too much. It only copies the\n.git/objects directory, which normally shouldn't contain any\nsymlinks. I think it's still good to preserve the warning, in case\nsomeone gave some object directories bad permissions, but following\nsymlinks in there may be overly paranoid, dunno.\n\nAdding an option to dir-iterator to follow symlinks may be a good\nidea, but it raises a few questions I think, e.g. how are we going to\ndeal with symlinks that point to a parent directory. The answer for\nthis function is that we currently don't and just keep recursing,\nuntil 'stat()' fails, but that is probably not the behaviour we'd want\nif we were to add this functionality to dir-iterator.\n\nAt this point I'm almost convinced that not following symlinks may be\nthe better solution here. That is a change in behaviour though, and\nit should be described in the commit message why that change is a good\nchange.\n\n> Also, I just noticed that dir-iterator follows hidden paths while\n> copy_or_link_directory don't. Maybe another option to add for\n> dir-iterator?\n\nThat feels like quite a special case in 'copy_or_link_directory()'. I\ndon't think it's worth adding a feature like that for now, at least\nnot until we find another use case for it. It's easy enough to skip\nover hidden directories here.\n\n> > If there is a good reason to remove the warning, that would be useful\n> > to describe in the commit message.\n> >\n> > > - if (S_ISDIR(buf.st_mode)) {\n> > > - if (de->d_name[0] != '.')\n> > > - copy_or_link_directory(src, dest,\n> > > - src_repo, src_baselen);\n> > > + strbuf_addstr(dest, iter->relative_path);\n> > > +\n> > > + if (S_ISDIR(iter->st.st_mode)) {\n> > > + if (iter->basename[0] != '.')\n> > > + mkdir_if_missing(dest->buf, 0777);\n> > > continue;\n> > > }\n> > >\n> > > /* Files that cannot be copied bit-for-bit... */\n> > > - if (!strcmp(src->buf + src_baselen, \"/info/alternates\")) {\n> > > + if (!strcmp(iter->relative_path, \"info/alternates\")) {\n> > > copy_alternates(src, dest, src_repo);\n> > > continue;\n> > > }\n> > > @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> > > if (copy_file_with_time(dest->buf, src->buf, 0666))\n> > > die_errno(_(\"failed to copy file to '%s'\"), dest->buf);\n> > > }\n> > > - closedir(dir);\n> > > +\n> > > + if (iter_status != ITER_DONE) {\n> > > + strbuf_setlen(src, src_len);\n> > > + die(_(\"failed to iterate over '%s'\"), src->buf);\n> > > + }\n> >\n> > Interestingly enough, this is not something that can currently\n> > happen if I read the dir-iterator code correctly. Even though the\n> > dir_iterator_advance function says it can return ITER_ERROR, it never\n> > actually does. The only way the iter_status can be not ITER_DONE at\n> > this point is if we would 'break' out of the loop.\n> >\n> > I don't think it hurts to be defensive here in case someone decides to\n> > break out of the loop in the future, just something odd I noticed\n> > while reviewing the code.\n> >\n> \n> Yes, I also noticed that. But I thought it would be nice to make this\n> check so that this code stays consistent to the documentation at\n> dir-iterator.h (although implementation is different).\n\nYeah, I'm not opposed to keep this check to be more defensive against\nfuture changes of the API and the code here.\n\n> Something I just noticed now is that copy_or_link_directory dies upon\n> an opendir error, but dir-iterator just prints a warning and keeps\n> going (looking for another file/dir to return for the caller). Is this\n> ok? Or should I, perhaps, add a \"pedantic\" option to dir-iterator so\n> that, when enabled, it immediately returns ITER_ERROR when an error\n> occurs instead of keep going?\n\nGood point, I failed to notice that during my earlier review. I do\nthink a \"pedantic\" option would be a good idea here, to preserve the\nprevious behaviour.\n\n> I'm proposing some new options to dir-iterator because as the patch\n> that adds it says \"There are obviously a lot of features that could\n> easily be added to this class\", and maybe those would be useful for\n> other dir-iterator users. But I don't know if that would be the best\n> way of doing it, so any feedback on this will be much appreciated.\n> Also, I saw on public-inbox[1] a patchset from 2017 proposing new\n> features/options for dir-iterator, but I don't really know if (and\n> why) it was rejected. (I couldn't find the patches on master/pu log)\n\nI don't think we should necessarily add new options because the\noriginal patch said we should do so. Instead the best way to\nintroduce new options is to introduce them when they are actually\ngoing to be needed in subsequent patches. So it would be nice to\nimplement features that are actually needed by your patches, but I\ndon't think it's worth trying to introduce new features that could\npotentially be useful in the future, but where we don't know yet\nwhether they really will be or not.\n\nI can't recall those patches, and from a quick look at the history I\ncan't tell why they were never merged. The what's cooking emails from\nJunio around that time may have a bit more background, but I don't\nhave time to dig them up now.\n\n> [1] https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/\n> \n> Thanks,\n> Matheus Tavares\n> \n> \n> > > }\n> > >\n> > > static void clone_local(const char *src_repo, const char *dest_repo)\n> > > --\n> > > 2.20.1\n> > >\n"},{"id":"369650","messageId":"CAHd-oW6m6JSgxwdE6U5vBw=DcPigK+P8eODoie0_Mag3Lg_eYw@mail.gmail.com","threadId":"50518","inReplyTo":"20190218233541.GK6085@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-19T16:23:58Z","receivedAt":"2019-02-19T16:24:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Feb 18, 2019 at 8:35 PM Thomas Gummerer wrote:\n>\n> >\n> > You are right. I didn't know the differences from lstat and stat. And\n> > reflecting on this now, I realize that the problem is even deeper:\n> > copy_or_link_directory follows symlinks but dir-iterator don't, so I\n> > cannot use dir-iterator without falling back to recursion on\n> > copy_or_link_directory. Because of that, I thought off adding an\n> > option for dir-iterator to follow symlinks. Does this seem like a good\n> > idea?\n>\n> Hmm looking at how this function is actually called, I don't think we\n> need to worry about symlinks too much. It only copies the\n> .git/objects directory, which normally shouldn't contain any\n> symlinks. I think it's still good to preserve the warning, in case\n> someone gave some object directories bad permissions, but following\n> symlinks in there may be overly paranoid, dunno.\n>\n> Adding an option to dir-iterator to follow symlinks may be a good\n> idea, but it raises a few questions I think, e.g. how are we going to\n> deal with symlinks that point to a parent directory. The answer for\n> this function is that we currently don't and just keep recursing,\n> until 'stat()' fails, but that is probably not the behaviour we'd want\n> if we were to add this functionality to dir-iterator.\n>\n> At this point I'm almost convinced that not following symlinks may be\n> the better solution here. That is a change in behaviour though, and\n> it should be described in the commit message why that change is a good\n> change.\n>\n\nOk, so I will add this note at v2's commit message.\n\n> > Also, I just noticed that dir-iterator follows hidden paths while\n> > copy_or_link_directory don't. Maybe another option to add for\n> > dir-iterator?\n>\n> That feels like quite a special case in 'copy_or_link_directory()'. I\n> don't think it's worth adding a feature like that for now, at least\n> not until we find another use case for it. It's easy enough to skip\n> over hidden directories here.\n>\n\nOk, I agree. I noticed copy_or_link_directory only skips hidden\ndirectories, not hidden files. And I couldn't think why we would want\nto skip one but not the other... Could that be unintentional? I mean,\nmaybe the idea was to skip just \".\" and \"..\"? If that is the case, I\ndon't even need to make an if check at copy_or_link_directory, since\ndir-iterator already skips \".\" and \"..\". What do you think about that?\n\n> > > If there is a good reason to remove the warning, that would be useful\n> > > to describe in the commit message.\n> > >\n> > > > - if (S_ISDIR(buf.st_mode)) {\n> > > > - if (de->d_name[0] != '.')\n> > > > - copy_or_link_directory(src, dest,\n> > > > - src_repo, src_baselen);\n> > > > + strbuf_addstr(dest, iter->relative_path);\n> > > > +\n> > > > + if (S_ISDIR(iter->st.st_mode)) {\n> > > > + if (iter->basename[0] != '.')\n> > > > + mkdir_if_missing(dest->buf, 0777);\n> > > > continue;\n> > > > }\n> > > >\n> > > > /* Files that cannot be copied bit-for-bit... */\n> > > > - if (!strcmp(src->buf + src_baselen, \"/info/alternates\")) {\n> > > > + if (!strcmp(iter->relative_path, \"info/alternates\")) {\n> > > > copy_alternates(src, dest, src_repo);\n> > > > continue;\n> > > > }\n> > > > @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> > > > if (copy_file_with_time(dest->buf, src->buf, 0666))\n> > > > die_errno(_(\"failed to copy file to '%s'\"), dest->buf);\n> > > > }\n> > > > - closedir(dir);\n> > > > +\n> > > > + if (iter_status != ITER_DONE) {\n> > > > + strbuf_setlen(src, src_len);\n> > > > + die(_(\"failed to iterate over '%s'\"), src->buf);\n> > > > + }\n> > >\n> > > Interestingly enough, this is not something that can currently\n> > > happen if I read the dir-iterator code correctly. Even though the\n> > > dir_iterator_advance function says it can return ITER_ERROR, it never\n> > > actually does. The only way the iter_status can be not ITER_DONE at\n> > > this point is if we would 'break' out of the loop.\n> > >\n> > > I don't think it hurts to be defensive here in case someone decides to\n> > > break out of the loop in the future, just something odd I noticed\n> > > while reviewing the code.\n> > >\n> >\n> > Yes, I also noticed that. But I thought it would be nice to make this\n> > check so that this code stays consistent to the documentation at\n> > dir-iterator.h (although implementation is different).\n>\n> Yeah, I'm not opposed to keep this check to be more defensive against\n> future changes of the API and the code here.\n>\n> > Something I just noticed now is that copy_or_link_directory dies upon\n> > an opendir error, but dir-iterator just prints a warning and keeps\n> > going (looking for another file/dir to return for the caller). Is this\n> > ok? Or should I, perhaps, add a \"pedantic\" option to dir-iterator so\n> > that, when enabled, it immediately returns ITER_ERROR when an error\n> > occurs instead of keep going?\n>\n> Good point, I failed to notice that during my earlier review. I do\n> think a \"pedantic\" option would be a good idea here, to preserve the\n> previous behaviour.\n>\n\nOk, I'm going to work on that. Thanks.\n\n> > I'm proposing some new options to dir-iterator because as the patch\n> > that adds it says \"There are obviously a lot of features that could\n> > easily be added to this class\", and maybe those would be useful for\n> > other dir-iterator users. But I don't know if that would be the best\n> > way of doing it, so any feedback on this will be much appreciated.\n> > Also, I saw on public-inbox[1] a patchset from 2017 proposing new\n> > features/options for dir-iterator, but I don't really know if (and\n> > why) it was rejected. (I couldn't find the patches on master/pu log)\n>\n> I don't think we should necessarily add new options because the\n> original patch said we should do so. Instead the best way to\n> introduce new options is to introduce them when they are actually\n> going to be needed in subsequent patches. So it would be nice to\n> implement features that are actually needed by your patches, but I\n> don't think it's worth trying to introduce new features that could\n> potentially be useful in the future, but where we don't know yet\n> whether they really will be or not.\n>\n\nYes, I agree with you. What I meant is that there is some space for\nnew features at dir-iterator and maybe those that I suggested could be\nnice not just for my patch, on clone.c, but for others. But I see your\npoint. We really should try to keep it simpler and just add the\nfeature when (and if) there is a more expressive usage for it.\n\nThanks again for all the help and feedback.\n"},{"id":"369680","messageId":"20190219234506.GL6085@hank.intra.tgummerer.com","threadId":"50518","inReplyTo":"CAHd-oW6m6JSgxwdE6U5vBw=DcPigK+P8eODoie0_Mag3Lg_eYw@mail.gmail.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-19T23:45:06Z","receivedAt":"2019-02-19T23:45:12Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/19, Matheus Tavares Bernardino wrote:\n> On Mon, Feb 18, 2019 at 8:35 PM Thomas Gummerer wrote:\n> > > Also, I just noticed that dir-iterator follows hidden paths while\n> > > copy_or_link_directory don't. Maybe another option to add for\n> > > dir-iterator?\n> >\n> > That feels like quite a special case in 'copy_or_link_directory()'. I\n> > don't think it's worth adding a feature like that for now, at least\n> > not until we find another use case for it. It's easy enough to skip\n> > over hidden directories here.\n> >\n> \n> Ok, I agree. I noticed copy_or_link_directory only skips hidden\n> directories, not hidden files. And I couldn't think why we would want\n> to skip one but not the other... Could that be unintentional? I mean,\n> maybe the idea was to skip just \".\" and \"..\"? If that is the case, I\n> don't even need to make an if check at copy_or_link_directory, since\n> dir-iterator already skips \".\" and \"..\". What do you think about that?\n\nI don't know. The best way to look at these things is usually to use\n'git blame' and have a look at the commit that introduced whatever you\nare looking at, in this case why we are skipping anything starting\nwith a '.'. But looking at that commit it's just the translation from\nshell script to builtin. It's also more than 10 years old by now, so\nit's unlikely that the original author would still remember why\nexactly they made this choice. (Note that I didn't take the time to\nactually dig into the original shell script).\n\nI feel like you are probably right in that it could be unintentional,\nand I don't think anyone should be messing with the objects\ndirectory. However that is no guarantee that changing this wouldn't\nbreak *something*.\n\nFor the purpose of this change I would try to keep the same behaviour\nas we currently have where possible, to not increase the scope too\nmuch.\n\n> > > I'm proposing some new options to dir-iterator because as the patch\n> > > that adds it says \"There are obviously a lot of features that could\n> > > easily be added to this class\", and maybe those would be useful for\n> > > other dir-iterator users. But I don't know if that would be the best\n> > > way of doing it, so any feedback on this will be much appreciated.\n> > > Also, I saw on public-inbox[1] a patchset from 2017 proposing new\n> > > features/options for dir-iterator, but I don't really know if (and\n> > > why) it was rejected. (I couldn't find the patches on master/pu log)\n> >\n> > I don't think we should necessarily add new options because the\n> > original patch said we should do so. Instead the best way to\n> > introduce new options is to introduce them when they are actually\n> > going to be needed in subsequent patches. So it would be nice to\n> > implement features that are actually needed by your patches, but I\n> > don't think it's worth trying to introduce new features that could\n> > potentially be useful in the future, but where we don't know yet\n> > whether they really will be or not.\n> >\n> \n> Yes, I agree with you. What I meant is that there is some space for\n> new features at dir-iterator and maybe those that I suggested could be\n> nice not just for my patch, on clone.c, but for others. But I see your\n> point. We really should try to keep it simpler and just add the\n> feature when (and if) there is a more expressive usage for it.\n\nRight, if you wanted to pursue this, that could be a separate patch\nseries. However generally patch series that just introduce something\nthat might be used later, but doesn't add much value on its own tend\nto be much more likely to be rejected.\n\n> Thanks again for all the help and feedback.\n\nThanks for working on this!\n"},{"id":"369758","messageId":"CAHd-oW6VPrHMo1QjAXn6G8P_gmJOQ91LVO+UDK1a8T7uhuM9uQ@mail.gmail.com","threadId":"50518","inReplyTo":"20190219234506.GL6085@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-21T04:46:30Z","receivedAt":"2019-02-21T04:46:44Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Ok, I think I'm almost there and I should be able to send a v2 on the\nweekend. But again, a few questions arose while I'm coding v2. Please,\nsee inline.\n\nOn Tue, Feb 19, 2019 at 8:45 PM Thomas Gummerer wrote:\n>\n> On 02/19, Matheus Tavares Bernardino wrote:\n> > Ok, I agree. I noticed copy_or_link_directory only skips hidden\n> > directories, not hidden files. And I couldn't think why we would want\n> > to skip one but not the other... Could that be unintentional? I mean,\n> > maybe the idea was to skip just \".\" and \"..\"? If that is the case, I\n> > don't even need to make an if check at copy_or_link_directory, since\n> > dir-iterator already skips \".\" and \"..\". What do you think about that?\n\n[...]\n\n> I feel like you are probably right in that it could be unintentional,\n> and I don't think anyone should be messing with the objects\n> directory. However that is no guarantee that changing this wouldn't\n> break *something*.\n>\n> For the purpose of this change I would try to keep the same behaviour\n> as we currently have where possible, to not increase the scope too\n> much.\n>\n\nI understand your point, but to make copy_or_link_directory() skip\njust hidden directories using dir-iterator seems, now, a little\ntricker than I though before. The \"if (iter->basename[0] != '.')\"\nstatement in the patch I sent for v1 only skips the hidden directories\ncreation, but it does not avoid the attempt to copy the hidden\ndirectory contents (which would obviously fail). If we were to do\nthat, we would need to check every directory in the relative path\nstring of each iteration entry looking for a hidden directory. That\nseems a little too much, IMHO. Because of that and the intuition that\nskipping over hidden dirs was an unintentional operation, I think we\ncould modify copy_or_link_directory() to skip '.' and '..', only.\nAlso, hidden dirs/files are not expected to be at .git/objects anyway,\nare they?\n\nAnd to have copy_or_link_directory()'s behaviour changed as little as\npossible, I could add the option to not follow hidden paths (dirs and\nregular files) at dir-iterator.c and use it at\ncopy_or_link_directory() (now that I'm adding the 'pedantic' option,\nthis won't be so difficult, anyway). Would these suggestions be a good\nplan?\n\nThanks,\nMatheus Tavares\n"},{"id":"369853","messageId":"20190221212522.GO6085@hank.intra.tgummerer.com","threadId":"50518","inReplyTo":"CAHd-oW6VPrHMo1QjAXn6G8P_gmJOQ91LVO+UDK1a8T7uhuM9uQ@mail.gmail.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-21T21:25:22Z","receivedAt":"2019-02-21T21:25:28Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/21, Matheus Tavares Bernardino wrote:\n> Ok, I think I'm almost there and I should be able to send a v2 on the\n> weekend. But again, a few questions arose while I'm coding v2. Please,\n> see inline.\n> \n> On Tue, Feb 19, 2019 at 8:45 PM Thomas Gummerer wrote:\n> >\n> > On 02/19, Matheus Tavares Bernardino wrote:\n> > > Ok, I agree. I noticed copy_or_link_directory only skips hidden\n> > > directories, not hidden files. And I couldn't think why we would want\n> > > to skip one but not the other... Could that be unintentional? I mean,\n> > > maybe the idea was to skip just \".\" and \"..\"? If that is the case, I\n> > > don't even need to make an if check at copy_or_link_directory, since\n> > > dir-iterator already skips \".\" and \"..\". What do you think about that?\n> \n> [...]\n> \n> > I feel like you are probably right in that it could be unintentional,\n> > and I don't think anyone should be messing with the objects\n> > directory. However that is no guarantee that changing this wouldn't\n> > break *something*.\n> >\n> > For the purpose of this change I would try to keep the same behaviour\n> > as we currently have where possible, to not increase the scope too\n> > much.\n> >\n> \n> I understand your point, but to make copy_or_link_directory() skip\n> just hidden directories using dir-iterator seems, now, a little\n> tricker than I though before. The \"if (iter->basename[0] != '.')\"\n> statement in the patch I sent for v1 only skips the hidden directories\n> creation, but it does not avoid the attempt to copy the hidden\n> directory contents (which would obviously fail). If we were to do\n> that, we would need to check every directory in the relative path\n> string of each iteration entry looking for a hidden directory. That\n> seems a little too much, IMHO. Because of that and the intuition that\n> skipping over hidden dirs was an unintentional operation, I think we\n> could modify copy_or_link_directory() to skip '.' and '..', only.\n> Also, hidden dirs/files are not expected to be at .git/objects anyway,\n> are they?\n\nNo, as far as I know hidden dirs/files should not be in .git/objects.\nThe only reason they would be there is if somebody would create them\nmanually. I didn't think of hidden child directories, which as you\nnoticed should be excluded as well to keep current behaviour.\n\nWith that in mind, I think I agree with the change of only excluding\n'.' and '..', for which we don't need to do anything in your\ndir-iterator patches. Also thinking of what the worst case that could\nhappen is, it's that while cloning a repository locally a few more\ndirectories could be copied, which is not all that bad.\n\nSo I think I was overly paranoid here and am on board with skipping\n'.' and '..' only. I even think that this can be just described in\ndetail in the commit message, rather than being done in a separate\npatch, though others may disagree with that. \n\n> And to have copy_or_link_directory()'s behaviour changed as little as\n> possible, I could add the option to not follow hidden paths (dirs and\n> regular files) at dir-iterator.c and use it at\n> copy_or_link_directory() (now that I'm adding the 'pedantic' option,\n> this won't be so difficult, anyway). Would these suggestions be a good\n> plan?\n\nI think this would be a valid plan as well, however thinking this\nthrough again I don't feel like it is necessary. I'm happy with\nwhichever way you prefer here.\n\n> Thanks,\n> Matheus Tavares\n"},{"id":"369876","messageId":"CAEA2_RL2ZxfjW5fU9sWeAovEHExptNDxOeQAiAtDqjLdz0bBuw@mail.gmail.com","threadId":"50518","inReplyTo":"CAMy9T_GzgPPMFLrPNbtf4zaYtyCoUDjXQbd2z8JeXFYogvfrVQ@mail.gmail.com","subject":"Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2019-02-22T00:22:39Z","receivedAt":"2019-02-22T00:21:29Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Hi there! I'm happy to see this series has gotten renewed attention.\n\nThe series was fine per se, and it even got through to the pu branch.\nBut then it got ejected since it conflicted non-trivially with another\npatch series that was added along the way (see\nhttps://lwn.net/Articles/724166/). At that point I did not keep\npushing it forward.\n\nI'd be happy to help anyone willing to make some progress on this! (Or\neven ultimately solve the conflict myself, if it serves a greater goal\nthan just being a GSoC microproject.)\n\nBest,\n-- Daniel.\n"}]}