{"thread":{"id":"50572","subject":"[GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","startedAt":"2019-02-23T19:03:25Z","lastAt":"2019-02-26T11:33:24Z","messageCount":35,"participants":["Matheus Tavares","Matheus Tavares Bernardino","Ævar Arnfjörð Bjarmason","Thomas Gummerer","Christian Couder","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"370030","messageId":"20190223190309.6728-1-matheus.bernardino@usp.br","threadId":"50572","inReplyTo":null,"subject":"[GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-23T19:03:06Z","receivedAt":"2019-02-23T19:03:25Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add pedantic option to dir_iterator_begin at dir-iterator.c and convert\nexplicit recursive directory traversal at copy_or_link_directory\n(builtin/clone.c) to the 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\nBuild: https://travis-ci.org/MatheusBernardino/git/builds/497512561\n\nMatheus Tavares (3):\n dir-iterator: add pedantic option to dir_iterator_begin\n clone: extract function from copy_or_link_directory\n clone: use dir-iterator to avoid explicit dir traversal\n\n builtin/clone.c | 72 ++++++++++++++++++++++++++++----------------\n dir-iterator.c | 23 ++++++++++++--\n dir-iterator.h | 16 ++++++++--\n refs/files-backend.c | 2 +-\n 4 files changed, 81 insertions(+), 32 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"370031","messageId":"20190223190309.6728-2-matheus.bernardino@usp.br","threadId":"50572","inReplyTo":"20190223190309.6728-1-matheus.bernardino@usp.br","subject":"[GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-23T19:03:07Z","receivedAt":"2019-02-23T19:03:26Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add the pedantic option to dir-iterator's initialization function,\ndir_iterator_begin. When this option is set to true,\ndir_iterator_advance will immediately return ITER_ERROR when failing to\nfetch the next entry. When set to false, dir_iterator_advance will emit\na warning and keep looking for the next entry.\n\nAlso adjust refs/files-backend.c to the new dir_iterator_begin\nsignature.\n\nSigned-off-by: Matheus Tavares \n---\nChanges in v2:\n - Added in v2\n\n dir-iterator.c | 23 +++++++++++++++++++++--\n dir-iterator.h | 16 +++++++++++++---\n refs/files-backend.c | 2 +-\n 3 files changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex f2dcd82fde..070a656555 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -48,6 +48,13 @@ struct dir_iterator_int {\n \t * that will be included in this iteration.\n \t */\n \tstruct dir_iterator_level *levels;\n+\n+\t/*\n+\t * Boolean value to define dir-iterator's behaviour when failing to\n+\t * fetch next entry. See comments on dir_iterator_begin at\n+\t * dir-iterator.h\n+\t */\n+\tint pedantic;\n };\n \n int dir_iterator_advance(struct dir_iterator *dir_iterator)\n@@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \n \t\t\tlevel->dir = opendir(iter->base.path.buf);\n \t\t\tif (!level->dir && errno != ENOENT) {\n+\t\t\t\tif (iter->pedantic)\n+\t\t\t\t\tgoto error_out;\n \t\t\t\twarning(\"error opening directory %s: %s\",\n \t\t\t\t\titer->base.path.buf, strerror(errno));\n \t\t\t\t/* Popping the level is handled below */\n@@ -122,6 +131,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\tif (!de) {\n \t\t\t\t/* This level is exhausted; pop up a level. */\n \t\t\t\tif (errno) {\n+\t\t\t\t\tif (iter->pedantic)\n+\t\t\t\t\t\tgoto error_out;\n \t\t\t\t\twarning(\"error reading directory %s: %s\",\n \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n \t\t\t\t} else if (closedir(level->dir))\n@@ -139,10 +150,13 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \n \t\t\tstrbuf_addstr(&iter->base.path, de->d_name);\n \t\t\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n-\t\t\t\tif (errno != ENOENT)\n+\t\t\t\tif (errno != ENOENT) {\n+\t\t\t\t\tif (iter->pedantic)\n+\t\t\t\t\t\tgoto error_out;\n \t\t\t\t\twarning(\"error reading path '%s': %s\",\n \t\t\t\t\t\titer->base.path.buf,\n \t\t\t\t\t\tstrerror(errno));\n+\t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n \n@@ -159,6 +173,10 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\treturn ITER_OK;\n \t\t}\n \t}\n+\n+error_out:\n+\tdir_iterator_abort(dir_iterator);\n+\treturn ITER_ERROR;\n }\n \n int dir_iterator_abort(struct dir_iterator *dir_iterator)\n@@ -182,7 +200,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n \treturn ITER_DONE;\n }\n \n-struct dir_iterator *dir_iterator_begin(const char *path)\n+struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)\n {\n \tstruct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));\n \tstruct dir_iterator *dir_iterator = &iter->base;\n@@ -195,6 +213,7 @@ struct dir_iterator *dir_iterator_begin(const char *path)\n \n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n \n+\titer->pedantic = pedantic;\n \titer->levels_nr = 1;\n \titer->levels[0].initialized = 0;\n \ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 970793d07a..50ca8e1a27 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -19,7 +19,7 @@\n * A typical iteration looks like this:\n *\n * int ok;\n- * struct iterator *iter = dir_iterator_begin(path);\n+ * struct iterator *iter = dir_iterator_begin(path, 0);\n *\n * while ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n * if (want_to_stop_iteration()) {\n@@ -65,9 +65,15 @@ struct dir_iterator {\n * The iteration includes all paths under path, not including path\n * itself and not including \".\" or \"..\" entries.\n *\n- * path is the starting directory. An internal copy will be made.\n+ * Parameters are:\n+ * - path is the starting directory. An internal copy will be made.\n+ * - pedantic is a boolean value. If true, dir-iterator will free\n+ * resources and return ITER_ERROR immediately, in case of an error\n+ * while trying to fetch the next entry in dir_iterator_advance. If\n+ * false, it will just emit a warning and keep looking for the next\n+ * entry.\n */\n-struct dir_iterator *dir_iterator_begin(const char *path);\n+struct dir_iterator *dir_iterator_begin(const char *path, int pedantic);\n \n /*\n * Advance the iterator to the first or next item and return ITER_OK.\n@@ -76,6 +82,10 @@ struct dir_iterator *dir_iterator_begin(const char *path);\n * dir_iterator and associated resources and return ITER_ERROR. It is\n * a bug to use iterator or call this function again after it has\n * returned ITER_DONE or ITER_ERROR.\n+ *\n+ * Note that whether dir_iterator_advance will return ITER_ERROR when\n+ * failing to fetch the next entry or keep going is defined by the\n+ * 'pedantic' option at dir-iterator's initialization.\n */\n int dir_iterator_advance(struct dir_iterator *iterator);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex dd8abe9185..c3d3b6c454 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2143,7 +2143,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \n \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n-\titer->dir_iterator = dir_iterator_begin(sb.buf);\n+\titer->dir_iterator = dir_iterator_begin(sb.buf, 0);\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n \n-- \n2.20.1\n\n"},{"id":"370032","messageId":"20190223190309.6728-3-matheus.bernardino@usp.br","threadId":"50572","inReplyTo":"20190223190309.6728-1-matheus.bernardino@usp.br","subject":"[GSoC][PATCH 2/3] clone: extract function from copy_or_link_directory","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-23T19:03:08Z","receivedAt":"2019-02-23T19:03:31Z","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 a\nfollowing patch. Also makes code more readable.\n\nSigned-off-by: Matheus Tavares \n---\nChanges in v2:\n - Replaced explicit reference to a following patch at patch message to\n a more generic reference.\n - Changed struct stat variable name to st, which is the most common\n name for such variables in git's code.\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..862d2ea69c 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 st;\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, &st))\n+\t\t\tdie_errno(_(\"failed to stat '%s'\"), pathname);\n+\t\telse if (!S_ISDIR(st.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":"370033","messageId":"20190223190309.6728-4-matheus.bernardino@usp.br","threadId":"50572","inReplyTo":"20190223190309.6728-1-matheus.bernardino@usp.br","subject":"[GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-23T19:03:09Z","receivedAt":"2019-02-23T19:03:31Z","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. This simplifies the code and avoid recursive calls to\ncopy_or_link_directory.\n\nThis process also brings some safe behaviour changes to\ncopy_or_link_directory:\n - It will no longer follows symbolic links. This is not a problem,\n since the function is only used to copy .git/objects directory, and\n symbolic links are not expected there.\n - Hidden directories won't be skipped anymore. In fact, it is odd that\n the function currently skip hidden directories but not hidden files.\n The reason for that could be unintentional: probably the intention\n was to skip '.' and '..' only, but it ended up accidentally skipping\n all directories starting with '.'. Again, it must not be a problem\n not to skip hidden dirs since hidden dirs/files are not expected at\n .git/objects.\n - Now, copy_or_link_directory will call die() in case of an error on\n openddir, readdir or lstat, inside dir_iterator_advance. That means\n it will abort in case of an error trying to fetch any iteration\n entry.\n\nSigned-off-by: Matheus Tavares \n---\nChanges in v2:\n - Improved patch message\n - Removed a now unused variable\n - Put warning on stat error back\n - Added pedantic option to dir-iterator initialization\n - Modified copy_or_link_directory not to skip hidden paths\n\n builtin/clone.c | 47 ++++++++++++++++++++++++++++-------------------\n 1 file changed, 28 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 862d2ea69c..515dc91d63 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@@ -411,42 +413,45 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\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+\t\t\t\t const char *src_repo)\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+\tstruct stat st;\n \n \tmkdir_if_missing(dest->buf, 0777);\n \n+\titer = dir_iterator_begin(src->buf, 1);\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\tstrbuf_addstr(dest, iter->relative_path);\n+\n+\t\t/*\n+\t\t * dir_iterator_advance already calls lstat to populate iter->st\n+\t\t * but, unlike stat, lstat does not checks for permissions on\n+\t\t * the given path.\n+\t\t */\n+\t\tif (stat(src->buf, &st)) {\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+\n+\t\tif (S_ISDIR(iter->st.st_mode)) {\n+\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 +468,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@@ -481,7 +490,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n \t\tget_common_dir(&dest, dest_repo);\n \t\tstrbuf_addstr(&src, \"/objects\");\n \t\tstrbuf_addstr(&dest, \"/objects\");\n-\t\tcopy_or_link_directory(&src, &dest, src_repo, src.len);\n+\t\tcopy_or_link_directory(&src, &dest, src_repo);\n \t\tstrbuf_release(&src);\n \t\tstrbuf_release(&dest);\n \t}\n-- \n2.20.1\n\n"},{"id":"370034","messageId":"CAHd-oW7iLZq+fE-q2gGmPBPHSqAyz=YyecsdHkcYAoACxGfY1A@mail.gmail.com","threadId":"50572","inReplyTo":"20190223190309.6728-1-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-23T19:07:34Z","receivedAt":"2019-02-23T19:07:48Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Feb 23, 2019 at 4:03 PM Matheus Tavares\n wrote:\n>\n> Add pedantic option to dir_iterator_begin at dir-iterator.c and convert\n> explicit recursive directory traversal at copy_or_link_directory\n> (builtin/clone.c) to the dir-iterator API.\n>\n> This is my microproject for GSoC 2019. Idea taken from\n> https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal\n>\n> Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561\n>\n> Matheus Tavares (3):\n> dir-iterator: add pedantic option to dir_iterator_begin\n> clone: extract function from copy_or_link_directory\n> clone: use dir-iterator to avoid explicit dir traversal\n>\n> builtin/clone.c | 72 ++++++++++++++++++++++++++++----------------\n> dir-iterator.c | 23 ++++++++++++--\n> dir-iterator.h | 16 ++++++++--\n> refs/files-backend.c | 2 +-\n> 4 files changed, 81 insertions(+), 32 deletions(-)\n>\n\nSadly, I forgot the v2 tag, but this is a v2. Should I resent the\nseries with [PATCH v2 ...] or is it ok?\n\nMatheus Tavares\n\n> --\n> 2.20.1\n>\n"},{"id":"370035","messageId":"87wolq4666.fsf@evledraar.gmail.com","threadId":"50572","inReplyTo":"CAHd-oW7iLZq+fE-q2gGmPBPHSqAyz=YyecsdHkcYAoACxGfY1A@mail.gmail.com","subject":"Re: [GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-23T20:10:41Z","receivedAt":"2019-02-23T20:10:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Feb 23 2019, Matheus Tavares Bernardino wrote:\n\n> On Sat, Feb 23, 2019 at 4:03 PM Matheus Tavares\n> wrote:\n>>\n>> Add pedantic option to dir_iterator_begin at dir-iterator.c and convert\n>> explicit recursive directory traversal at copy_or_link_directory\n>> (builtin/clone.c) to the dir-iterator API.\n>>\n>> This is my microproject for GSoC 2019. Idea taken from\n>> https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal\n>>\n>> Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561\n>>\n>> Matheus Tavares (3):\n>> dir-iterator: add pedantic option to dir_iterator_begin\n>> clone: extract function from copy_or_link_directory\n>> clone: use dir-iterator to avoid explicit dir traversal\n>>\n>> builtin/clone.c | 72 ++++++++++++++++++++++++++++----------------\n>> dir-iterator.c | 23 ++++++++++++--\n>> dir-iterator.h | 16 ++++++++--\n>> refs/files-backend.c | 2 +-\n>> 4 files changed, 81 insertions(+), 32 deletions(-)\n>>\n>\n> Sadly, I forgot the v2 tag, but this is a v2. Should I resent the\n> series with [PATCH v2 ...] or is it ok?\n\nIt's fine. No need to resend.\n"},{"id":"370036","messageId":"20190223213527.GP6085@hank.intra.tgummerer.com","threadId":"50572","inReplyTo":"20190223190309.6728-2-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-23T21:35:27Z","receivedAt":"2019-02-23T21:35:35Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/23, Matheus Tavares wrote:\n> Add the pedantic option to dir-iterator's initialization function,\n> dir_iterator_begin. When this option is set to true,\n> dir_iterator_advance will immediately return ITER_ERROR when failing to\n> fetch the next entry. When set to false, dir_iterator_advance will emit\n> a warning and keep looking for the next entry.\n> \n> Also adjust refs/files-backend.c to the new dir_iterator_begin\n> signature.\n\nThanks, this makes sense to me. This commit message describes what we\nare doing in this patch, but not why we are doing it. From previously\nreviewing this series, I know this is going to be used in a subsequent\npatch, but it is nice to give reviewers and future readers of this\npatch that context as well. Just something like \"This behaviour is\ngoing to be used in a subsequent patch.\" should be enough here.\n\nA few comments inline.\n\n> Signed-off-by: Matheus Tavares \n> ---\n> Changes in v2:\n> - Added in v2\n> \n> dir-iterator.c | 23 +++++++++++++++++++++--\n> dir-iterator.h | 16 +++++++++++++---\n> refs/files-backend.c | 2 +-\n> 3 files changed, 35 insertions(+), 6 deletions(-)\n> \n> diff --git a/dir-iterator.c b/dir-iterator.c\n> index f2dcd82fde..070a656555 100644\n> --- a/dir-iterator.c\n> +++ b/dir-iterator.c\n> @@ -48,6 +48,13 @@ struct dir_iterator_int {\n> \t * that will be included in this iteration.\n> \t */\n> \tstruct dir_iterator_level *levels;\n> +\n> +\t/*\n> +\t * Boolean value to define dir-iterator's behaviour when failing to\n> +\t * fetch next entry. See comments on dir_iterator_begin at\n> +\t * dir-iterator.h\n> +\t */\n> +\tint pedantic;\n> };\n> \n> int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> \n> \t\t\tlevel->dir = opendir(iter->base.path.buf);\n> \t\t\tif (!level->dir && errno != ENOENT) {\n> +\t\t\t\tif (iter->pedantic)\n> +\t\t\t\t\tgoto error_out;\n\nI think we should also print an error here. The caller doesn't have\nany context on what went wrong, and will probably just 'die()' if an\nerror is encountered. I think it would make sense to call\n'error(...)' here before 'goto error_out;' to give a useful error\nmessage here.\n\n> \t\t\t\twarning(\"error opening directory %s: %s\",\n> \t\t\t\t\titer->base.path.buf, strerror(errno));\n> \t\t\t\t/* Popping the level is handled below */\n> @@ -122,6 +131,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> \t\t\tif (!de) {\n> \t\t\t\t/* This level is exhausted; pop up a level. */\n> \t\t\t\tif (errno) {\n> +\t\t\t\t\tif (iter->pedantic)\n> +\t\t\t\t\t\tgoto error_out;\n> \t\t\t\t\twarning(\"error reading directory %s: %s\",\n> \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n> \t\t\t\t} else if (closedir(level->dir))\n> @@ -139,10 +150,13 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> \n> \t\t\tstrbuf_addstr(&iter->base.path, de->d_name);\n> \t\t\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n> -\t\t\t\tif (errno != ENOENT)\n> +\t\t\t\tif (errno != ENOENT) {\n> +\t\t\t\t\tif (iter->pedantic)\n> +\t\t\t\t\t\tgoto error_out;\n> \t\t\t\t\twarning(\"error reading path '%s': %s\",\n> \t\t\t\t\t\titer->base.path.buf,\n> \t\t\t\t\t\tstrerror(errno));\n> +\t\t\t\t}\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \n> @@ -159,6 +173,10 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> \t\t\treturn ITER_OK;\n> \t\t}\n> \t}\n> +\n> +error_out:\n> +\tdir_iterator_abort(dir_iterator);\n> +\treturn ITER_ERROR;\n> }\n> \n> int dir_iterator_abort(struct dir_iterator *dir_iterator)\n> @@ -182,7 +200,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n> \treturn ITER_DONE;\n> }\n> \n> -struct dir_iterator *dir_iterator_begin(const char *path)\n> +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)\n\nThinking about the future evolution of this interface, it might make\nmore sense to have that second parameter be a \"struct\ndir_iterator_opts\". For now it would just have one member \"pedantic\",\nbut in the future we could add additional options there instead of\nadding additional parameters.\n\nFor now there are not many callers of the dir iterator API, so adding\na parameter is not a huge problem. However we will most likely add\nmore callers in the future, so changing all of them becomes more and\nmore costly.\n\nIt also becomes a problem for integrating these patches, in the case\nwhere a topic in flight adds a new use of this API. In that case the\nbreakage wouldn't be noticed on merges, as there would be no merge\nconflicts, however it would break the build, which makes the\nmaintainers job harder.\n\nWe could of course add new functions in the future, rather than\nchanging dir_iterator_begin, however just having one function, that\nallows combining different options sounds like the better choice to me\nhere.\n\n> {\n> \tstruct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));\n> \tstruct dir_iterator *dir_iterator = &iter->base;\n> @@ -195,6 +213,7 @@ struct dir_iterator *dir_iterator_begin(const char *path)\n> \n> \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n> \n> +\titer->pedantic = pedantic;\n> \titer->levels_nr = 1;\n> \titer->levels[0].initialized = 0;\n> \n> diff --git a/dir-iterator.h b/dir-iterator.h\n> index 970793d07a..50ca8e1a27 100644\n> --- a/dir-iterator.h\n> +++ b/dir-iterator.h\n> @@ -19,7 +19,7 @@\n> * A typical iteration looks like this:\n> *\n> * int ok;\n> - * struct iterator *iter = dir_iterator_begin(path);\n> + * struct iterator *iter = dir_iterator_begin(path, 0);\n> *\n> * while ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n> * if (want_to_stop_iteration()) {\n> @@ -65,9 +65,15 @@ struct dir_iterator {\n> * The iteration includes all paths under path, not including path\n> * itself and not including \".\" or \"..\" entries.\n> *\n> - * path is the starting directory. An internal copy will be made.\n> + * Parameters are:\n> + * - path is the starting directory. An internal copy will be made.\n> + * - pedantic is a boolean value. If true, dir-iterator will free\n> + * resources and return ITER_ERROR immediately, in case of an error\n> + * while trying to fetch the next entry in dir_iterator_advance. If\n> + * false, it will just emit a warning and keep looking for the next\n> + * entry.\n> */\n> -struct dir_iterator *dir_iterator_begin(const char *path);\n> +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic);\n> \n> /*\n> * Advance the iterator to the first or next item and return ITER_OK.\n> @@ -76,6 +82,10 @@ struct dir_iterator *dir_iterator_begin(const char *path);\n> * dir_iterator and associated resources and return ITER_ERROR. It is\n> * a bug to use iterator or call this function again after it has\n> * returned ITER_DONE or ITER_ERROR.\n> + *\n> + * Note that whether dir_iterator_advance will return ITER_ERROR when\n> + * failing to fetch the next entry or keep going is defined by the\n> + * 'pedantic' option at dir-iterator's initialization.\n> */\n> int dir_iterator_advance(struct dir_iterator *iterator);\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index dd8abe9185..c3d3b6c454 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2143,7 +2143,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n> \n> \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n> \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n> -\titer->dir_iterator = dir_iterator_begin(sb.buf);\n> +\titer->dir_iterator = dir_iterator_begin(sb.buf, 0);\n> \titer->ref_store = ref_store;\n> \tstrbuf_release(&sb);\n> \n> -- \n> 2.20.1\n> \n"},{"id":"370037","messageId":"20190223214856.GQ6085@hank.intra.tgummerer.com","threadId":"50572","inReplyTo":"20190223190309.6728-4-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-23T21:48:56Z","receivedAt":"2019-02-23T21:49:02Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/23, 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. This simplifies the code and avoid recursive calls to\n> copy_or_link_directory.\n> \n> This process also brings some safe behaviour changes to\n> copy_or_link_directory:\n> - It will no longer follows symbolic links. This is not a problem,\n> since the function is only used to copy .git/objects directory, and\n> symbolic links are not expected there.\n> - Hidden directories won't be skipped anymore. In fact, it is odd that\n> the function currently skip hidden directories but not hidden files.\n> The reason for that could be unintentional: probably the intention\n> was to skip '.' and '..' only, but it ended up accidentally skipping\n> all directories starting with '.'. Again, it must not be a problem\n> not to skip hidden dirs since hidden dirs/files are not expected at\n> .git/objects.\n> - Now, copy_or_link_directory will call die() in case of an error on\n> openddir, readdir or lstat, inside dir_iterator_advance. That means\n> it will abort in case of an error trying to fetch any iteration\n> entry.\n> \n> Signed-off-by: Matheus Tavares \n> ---\n> Changes in v2:\n> - Improved patch message\n> - Removed a now unused variable\n\ns/variable/parameter/ I believe?\n\n> - Put warning on stat error back\n> - Added pedantic option to dir-iterator initialization\n> - Modified copy_or_link_directory not to skip hidden paths\n\nThanks, these descriptions are very useful for reviewers that had a\nlook at previous rounds.\n\n> builtin/clone.c | 47 ++++++++++++++++++++++++++++-------------------\n> 1 file changed, 28 insertions(+), 19 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 862d2ea69c..515dc91d63 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> @@ -411,42 +413,45 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\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> +\t\t\t\t const char *src_repo)\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> +\tstruct stat st;\n> \n> \tmkdir_if_missing(dest->buf, 0777);\n> \n> +\titer = dir_iterator_begin(src->buf, 1);\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\tstrbuf_addstr(dest, iter->relative_path);\n> +\n> +\t\t/*\n> +\t\t * dir_iterator_advance already calls lstat to populate iter->st\n> +\t\t * but, unlike stat, lstat does not checks for permissions on\n> +\t\t * the given path.\n> +\t\t */\n\nHmm, lstat does check the permissions on the path, it just doesn't\nfollow symlinks. I think I actually mislead you in my previous review\nhere, and was reading the code in dir-iterator.c all wrong.\n\nI thought it said \"if (errno == ENOENT) warning(...)\", however the\ncondition is \"errno != ENOENT\", which is why I thought we were loosing\nwarnings when errno == EACCES for example.\n\nAs we decided that we would no longer follow symlinks now, I think we\ncan actually get rid of the stat call here. Sorry about the confusion.\n\n> +\t\tif (stat(src->buf, &st)) {\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> +\n> +\t\tif (S_ISDIR(iter->st.st_mode)) {\n> +\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 +468,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> @@ -481,7 +490,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n> \t\tget_common_dir(&dest, dest_repo);\n> \t\tstrbuf_addstr(&src, \"/objects\");\n> \t\tstrbuf_addstr(&dest, \"/objects\");\n> -\t\tcopy_or_link_directory(&src, &dest, src_repo, src.len);\n> +\t\tcopy_or_link_directory(&src, &dest, src_repo);\n> \t\tstrbuf_release(&src);\n> \t\tstrbuf_release(&dest);\n> \t}\n> -- \n> 2.20.1\n> \n"},{"id":"370038","messageId":"20190223215906.GR6085@hank.intra.tgummerer.com","threadId":"50572","inReplyTo":"20190223190309.6728-1-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-23T21:59:06Z","receivedAt":"2019-02-23T21:59:11Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/23, Matheus Tavares wrote:\n> Add pedantic option to dir_iterator_begin at dir-iterator.c and convert\n> explicit recursive directory traversal at copy_or_link_directory\n> (builtin/clone.c) to the dir-iterator API.\n\nThanks for another iteration of this.\n\nTo make life easier for reviewers, please include a link (or the\nmessage-ID) to all previous iteration of the series. We often use\nlinks to the public-inbox mirror for this,\ne.g. https://public-inbox.org/git/20190215154913.18800-1-matheus.bernardino@usp.br/.\n\nThis helps reviewers go back quickly to previous iterations of the\nseries, and refresh their memory on the comments that were left\nthere.\n\nYou can also use the --in-reply-to option in 'git send-email' to chain\nthe threads, which also makes life easier for reviewers.\n\nAn additional way to help reviewers is to include a 'range-diff'\nbetween the previous iteration of the series, and the current\niteration. See the 'git range-diff' command or the '--range-diff'\noption to 'git format-patch' for that. That helps reviewers to\nquickly see what changed between iterations, so that they don't have\nto re-review everything, if they can still remember the last round\nwell enough.\n\nI also added Christian Couder back to the Cc list, as he commented on\nthe RFC. It's good to keep people that commented on the series in Cc,\nas they have shown some interest in the series, so keeping them in the\nCc list helps highlight those emails for them, and makes it more\nlikely that the patches get reviewed quickly.\n\n> This is my microproject for GSoC 2019. Idea taken from\n> https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal\n> \n> Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561\n> \n> Matheus Tavares (3):\n> dir-iterator: add pedantic option to dir_iterator_begin\n> clone: extract function from copy_or_link_directory\n> clone: use dir-iterator to avoid explicit dir traversal\n> \n> builtin/clone.c | 72 ++++++++++++++++++++++++++++----------------\n> dir-iterator.c | 23 ++++++++++++--\n> dir-iterator.h | 16 ++++++++--\n> refs/files-backend.c | 2 +-\n> 4 files changed, 81 insertions(+), 32 deletions(-)\n> \n> -- \n> 2.20.1\n> \n"},{"id":"370039","messageId":"87va1a3z8e.fsf@evledraar.gmail.com","threadId":"50572","inReplyTo":"20190223190309.6728-4-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-23T22:40:33Z","receivedAt":"2019-02-23T22:40:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Feb 23 2019, Matheus Tavares 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. This simplifies the code and avoid recursive calls to\n> copy_or_link_directory.\n\nSounds good in principle.\n\n> This process also brings some safe behaviour changes to\n> copy_or_link_directory:\n\nI ad-hoc tested some of these, and could spot behavior changes. We\nshould have tests for these.\n\n> - It will no longer follows symbolic links. This is not a problem,\n> since the function is only used to copy .git/objects directory, and\n> symbolic links are not expected there.\n\nI don't think we should make that assumption, and I don't know of\nanything else in git that does.\n\nI've certainly symlinked individual objects or packs into a repo for\ndebugging / recovery, and it would be unexpected to clone that and miss\nsomething.\n\nSo in the general case we should be strict in what we generate, but\npermissive in what we accept. We don't want a \"clone\" of an existing\nrepo to fail, or \"fsck\" to fail after clone...\n\nWhen trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,\nand a specific object in objects/4d/ a symlink to /tmp too.\n\nWithout this patch the individual object is still a symlink, but the\nobject under the directory gets resolved, and \"un-symlinked\", also with\n--dissociate, which seems like an existing bug.\n\nWith your patch that symlink structure is copied as-is. That's more\nfaithful under --local, but a regression for --dissociate (which didn't\nwork fully to begin with...).\n\nI was paranoid that \"no longer follows symbolic links\" could also mean\n\"will ignore those objects\", but it seems to more faithfully copy things\nas-is for *that* case.\n\nBut then I try with --no-hardlinks and stock git dereferences my symlink\nstructure, but with your patches fails completely:\n\n Cloning into bare repository 'repo2'...\n error: copy-fd: read returned: Is a directory\n fatal: failed to copy file to 'repo2/objects/c4': Is a directory\n fatal: the remote end hung up unexpectedly\n fatal: cannot change to 'repo2': No such file or directory\n\nSo there's at least one case in a few minutes of prodding this where we\ncan't clone a working repo now, however obscure the setup.\n\n> - Hidden directories won't be skipped anymore. In fact, it is odd that\n> the function currently skip hidden directories but not hidden files.\n> The reason for that could be unintentional: probably the intention\n> was to skip '.' and '..' only, but it ended up accidentally skipping\n> all directories starting with '.'. Again, it must not be a problem\n> not to skip hidden dirs since hidden dirs/files are not expected at\n> .git/objects.\n\nI reproduce this with --local. A \".foo\" isn't copied before, now it\nis. Good, I guess. We'd have already copied a \"foo\".\n\n> - Now, copy_or_link_directory will call die() in case of an error on\n> openddir, readdir or lstat, inside dir_iterator_advance. That means\n> it will abort in case of an error trying to fetch any iteration\n> entry.\n\nGood, but really IMNSHO this series is tweaking some critical core code\nand desperately needs tests.\n\nUnfortunately, in this as in so many edge case we have no existing\ntests.\n\nThis would be much easier to review and would give reviewers more\nconfidence if the parts of this that changed behavior started with a\npatch or patches that just manually objects/ dirs with various\ncombinations of symlinks, hardlinks etc., and asserted that the various\noptions did exactly what they're doing now, and made sure the\nsource/target repos were the same after/both passed \"fsck\".\n\nThen followed by some version of this patch which changes the behavior,\nand would be forced to tweak those tests. To make it clear e.g. that\nsome cases where we have a working \"clone\" are now a hard error.\n\nThanks for working on this!\n"},{"id":"370052","messageId":"CAP8UFD0uZbzG7pjqLAQ5374_0vBTbaQ89u-rhiY7c7GXSKb8tw@mail.gmail.com","threadId":"50572","inReplyTo":"20190223213527.GP6085@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-24T08:35:46Z","receivedAt":"2019-02-24T08:36:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer wrote:\n>\n> On 02/23, Matheus Tavares wrote:\n> > Add the pedantic option to dir-iterator's initialization function,\n> > dir_iterator_begin. When this option is set to true,\n> > dir_iterator_advance will immediately return ITER_ERROR when failing to\n> > fetch the next entry. When set to false, dir_iterator_advance will emit\n> > a warning and keep looking for the next entry.\n> >\n> > Also adjust refs/files-backend.c to the new dir_iterator_begin\n> > signature.\n>\n> Thanks, this makes sense to me. This commit message describes what we\n> are doing in this patch, but not why we are doing it. From previously\n> reviewing this series, I know this is going to be used in a subsequent\n> patch, but it is nice to give reviewers and future readers of this\n> patch that context as well. Just something like \"This behaviour is\n> going to be used in a subsequent patch.\" should be enough here.\n\nI agree that it's a good idea to add just that.\n\n> > int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> >\n> > level->dir = opendir(iter->base.path.buf);\n> > if (!level->dir && errno != ENOENT) {\n> > + if (iter->pedantic)\n> > + goto error_out;\n>\n> I think we should also print an error here. The caller doesn't have\n> any context on what went wrong, and will probably just 'die()' if an\n> error is encountered. I think it would make sense to call\n> 'error(...)' here before 'goto error_out;' to give a useful error\n> message here.\n\nIf we start to give error messages, then we might as well give error\nmessages all the times when we error out. This will avoid the callers\nwondering if they need to give an error message or not.\n\nI am not sure it's necessary here though. And I think if it's useful,\nit can be added in another patch or another patch series.\n\n> > warning(\"error opening directory %s: %s\",\n> > iter->base.path.buf, strerror(errno));\n> > /* Popping the level is handled below */\n\n> > -struct dir_iterator *dir_iterator_begin(const char *path)\n> > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)\n>\n> Thinking about the future evolution of this interface, it might make\n> more sense to have that second parameter be a \"struct\n> dir_iterator_opts\". For now it would just have one member \"pedantic\",\n> but in the future we could add additional options there instead of\n> adding additional parameters.\n\nI think it's ok with `int pedantic` for now as improvements can be\ndone when they are really needed. And we will perhaps find out that\nit's better to just change `int pedantic` to `unsigned flags` instead\nof `struct dir_iterator_opts`.\n\nThanks,\nChristian.\n"},{"id":"370053","messageId":"CAP8UFD0cDMr4jnjmveb10PK+xAoEEhKUgcaqbeDEktG2QuQoqQ@mail.gmail.com","threadId":"50572","inReplyTo":"20190223190309.6728-3-matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH 2/3] clone: extract function from copy_or_link_directory","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-24T08:38:37Z","receivedAt":"2019-02-24T08:38:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Feb 23, 2019 at 8:06 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 a\n> following patch. Also makes code more readable.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> Changes in v2:\n> - Replaced explicit reference to a following patch at patch message to\n> a more generic reference.\n> - Changed struct stat variable name to st, which is the most common\n> name for such variables in git's code.\n\nNice!\n"},{"id":"370054","messageId":"CAP8UFD2LLL+V54k3XmHzmg5t1zanpAvY_=7GqXL43vLPTTmuww@mail.gmail.com","threadId":"50572","inReplyTo":"87va1a3z8e.fsf@evledraar.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-24T09:41:04Z","receivedAt":"2019-02-24T09:41:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Sat, Feb 23 2019, Matheus Tavares 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. This simplifies the code and avoid recursive calls to\n> > copy_or_link_directory.\n>\n> Sounds good in principle.\n>\n> > This process also brings some safe behaviour changes to\n> > copy_or_link_directory:\n>\n> I ad-hoc tested some of these, and could spot behavior changes. We\n> should have tests for these.\n\nI agree that ideally we should have a few tests for these, but this is\na grey area (see below) and there are areas that are not grey for\nwhich we don't have any test...\n\nAnd then adding tests would make this series become larger than a\ntypical GSoC micro-project...\n\n> > - It will no longer follows symbolic links. This is not a problem,\n> > since the function is only used to copy .git/objects directory, and\n> > symbolic links are not expected there.\n>\n> I don't think we should make that assumption, and I don't know of\n> anything else in git that does.\n\nI think Git itself doesn't create symlinks in \".git/objects/\" and we\ndon't recommend people manually tweaking what's inside \".git/\". That's\nwhy I think it's a grey area.\n\n> I've certainly symlinked individual objects or packs into a repo for\n> debugging / recovery, and it would be unexpected to clone that and miss\n> something.\n\nIf people tweak what's inside \".git\" by hand, they are expected to\nknow what they doing and be able to debug it.\n\n> So in the general case we should be strict in what we generate, but\n> permissive in what we accept. We don't want a \"clone\" of an existing\n> repo to fail, or \"fsck\" to fail after clone...\n\nYeah, but realistically I don't think we are going to foolproof Git\nfrom everything that someone could do by tweaking random things\nmanually in \".git/\".\n\nI am not saying that it should be ok to make things much worse than\nthey are now in case some things have been tweaked in \".git/\", but if\nthings in general don't look worse in this grey area, and a patch\notherwise improves things, then I think the patch should be ok.\n\n> When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,\n> and a specific object in objects/4d/ a symlink to /tmp too.\n>\n> Without this patch the individual object is still a symlink, but the\n> object under the directory gets resolved, and \"un-symlinked\", also with\n> --dissociate, which seems like an existing bug.\n>\n> With your patch that symlink structure is copied as-is. That's more\n> faithful under --local, but a regression for --dissociate (which didn't\n> work fully to begin with...).\n\nI think that I use --local (which is the default if the repository is\nspecified as a local path) much more often than --dissociate, so for\nme the patch would be very positive, especially since --dissociate is\nalready buggy anyway in this case.\n\n> I was paranoid that \"no longer follows symbolic links\" could also mean\n> \"will ignore those objects\", but it seems to more faithfully copy things\n> as-is for *that* case.\n\nNice!\n\n> But then I try with --no-hardlinks and stock git dereferences my symlink\n> structure, but with your patches fails completely:\n>\n> Cloning into bare repository 'repo2'...\n> error: copy-fd: read returned: Is a directory\n> fatal: failed to copy file to 'repo2/objects/c4': Is a directory\n> fatal: the remote end hung up unexpectedly\n> fatal: cannot change to 'repo2': No such file or directory\n\nMaybe this could be fixed. Anyway I don't use --no-hardlinks very\noften, so I still think the patch is a positive even with this\nfailure.\n\n> So there's at least one case in a few minutes of prodding this where we\n> can't clone a working repo now, however obscure the setup.\n>\n> > - Hidden directories won't be skipped anymore. In fact, it is odd that\n> > the function currently skip hidden directories but not hidden files.\n> > The reason for that could be unintentional: probably the intention\n> > was to skip '.' and '..' only, but it ended up accidentally skipping\n> > all directories starting with '.'. Again, it must not be a problem\n> > not to skip hidden dirs since hidden dirs/files are not expected at\n> > .git/objects.\n>\n> I reproduce this with --local. A \".foo\" isn't copied before, now it\n> is. Good, I guess. We'd have already copied a \"foo\".\n>\n> > - Now, copy_or_link_directory will call die() in case of an error on\n> > openddir, readdir or lstat, inside dir_iterator_advance. That means\n> > it will abort in case of an error trying to fetch any iteration\n> > entry.\n\nIt would be nice if the above paragraph in the commit message would\nsay what was the previous behavior and why it's better to die() .\n\n> Good, but really IMNSHO this series is tweaking some critical core code\n> and desperately needs tests.\n\nIt's critical that this code works well in the usual case, yes. (And\nthere are already a lot of tests that test that.) But when people have\nmanually tweaked things in their \".git/objects/\", it's not critical\nwhat happens. Many systems have \"undefined behaviors\" at some point\nand that's ok.\n\nAnd no, I am not saying that we should consider it completely\n\"undefined behavior\" as soon as something is manually tweaked in\n\".git/\", and yes, tests would be nice, and your comments and manual\ntests on this are very much appreciated. It's just that I don't think\nwe should require too much when a patch, especially from a first time\ncontributor, is already improving things, though it also changes a few\nthings in a grey area.\n\n> Unfortunately, in this as in so many edge case we have no existing\n> tests.\n>\n> This would be much easier to review and would give reviewers more\n> confidence if the parts of this that changed behavior started with a\n> patch or patches that just manually objects/ dirs with various\n\nI think \"created\" is missing between \"manually\" and \"objects/\" in the\nabove sentence.\n\n> combinations of symlinks, hardlinks etc., and asserted that the various\n> options did exactly what they're doing now, and made sure the\n> source/target repos were the same after/both passed \"fsck\".\n>\n> Then followed by some version of this patch which changes the behavior,\n> and would be forced to tweak those tests. To make it clear e.g. that\n> some cases where we have a working \"clone\" are now a hard error.\n\nUnfortunately this would be a lot of work and not appropriate for a\nGSoC micro-project.\n\nThanks,\nChristian.\n"},{"id":"370088","messageId":"87tvgt454u.fsf@evledraar.gmail.com","threadId":"50572","inReplyTo":"CAP8UFD2LLL+V54k3XmHzmg5t1zanpAvY_=7GqXL43vLPTTmuww@mail.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-24T14:45:21Z","receivedAt":"2019-02-24T14:47:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 24 2019, Christian Couder wrote:\n\n> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Sat, Feb 23 2019, Matheus Tavares 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. This simplifies the code and avoid recursive calls to\n>> > copy_or_link_directory.\n>>\n>> Sounds good in principle.\n>>\n>> > This process also brings some safe behaviour changes to\n>> > copy_or_link_directory:\n>>\n>> I ad-hoc tested some of these, and could spot behavior changes. We\n>> should have tests for these.\n>\n> I agree that ideally we should have a few tests for these, but this is\n> a grey area (see below) and there are areas that are not grey for\n> which we don't have any test...\n>\n> And then adding tests would make this series become larger than a\n> typical GSoC micro-project...\n>\n>> > - It will no longer follows symbolic links. This is not a problem,\n>> > since the function is only used to copy .git/objects directory, and\n>> > symbolic links are not expected there.\n>>\n>> I don't think we should make that assumption, and I don't know of\n>> anything else in git that does.\n>\n> I think Git itself doesn't create symlinks in \".git/objects/\" and we\n> don't recommend people manually tweaking what's inside \".git/\". That's\n> why I think it's a grey area.\n>\n>> I've certainly symlinked individual objects or packs into a repo for\n>> debugging / recovery, and it would be unexpected to clone that and miss\n>> something.\n>\n> If people tweak what's inside \".git\" by hand, they are expected to\n> know what they doing and be able to debug it.\n>\n>> So in the general case we should be strict in what we generate, but\n>> permissive in what we accept. We don't want a \"clone\" of an existing\n>> repo to fail, or \"fsck\" to fail after clone...\n>\n> Yeah, but realistically I don't think we are going to foolproof Git\n> from everything that someone could do by tweaking random things\n> manually in \".git/\".\n>\n> I am not saying that it should be ok to make things much worse than\n> they are now in case some things have been tweaked in \".git/\", but if\n> things in general don't look worse in this grey area, and a patch\n> otherwise improves things, then I think the patch should be ok.\n>\n>> When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,\n>> and a specific object in objects/4d/ a symlink to /tmp too.\n>>\n>> Without this patch the individual object is still a symlink, but the\n>> object under the directory gets resolved, and \"un-symlinked\", also with\n>> --dissociate, which seems like an existing bug.\n>>\n>> With your patch that symlink structure is copied as-is. That's more\n>> faithful under --local, but a regression for --dissociate (which didn't\n>> work fully to begin with...).\n>\n> I think that I use --local (which is the default if the repository is\n> specified as a local path) much more often than --dissociate, so for\n> me the patch would be very positive, especially since --dissociate is\n> already buggy anyway in this case.\n>\n>> I was paranoid that \"no longer follows symbolic links\" could also mean\n>> \"will ignore those objects\", but it seems to more faithfully copy things\n>> as-is for *that* case.\n>\n> Nice!\n>\n>> But then I try with --no-hardlinks and stock git dereferences my symlink\n>> structure, but with your patches fails completely:\n>>\n>> Cloning into bare repository 'repo2'...\n>> error: copy-fd: read returned: Is a directory\n>> fatal: failed to copy file to 'repo2/objects/c4': Is a directory\n>> fatal: the remote end hung up unexpectedly\n>> fatal: cannot change to 'repo2': No such file or directory\n>\n> Maybe this could be fixed. Anyway I don't use --no-hardlinks very\n> often, so I still think the patch is a positive even with this\n> failure.\n>\n>> So there's at least one case in a few minutes of prodding this where we\n>> can't clone a working repo now, however obscure the setup.\n>>\n>> > - Hidden directories won't be skipped anymore. In fact, it is odd that\n>> > the function currently skip hidden directories but not hidden files.\n>> > The reason for that could be unintentional: probably the intention\n>> > was to skip '.' and '..' only, but it ended up accidentally skipping\n>> > all directories starting with '.'. Again, it must not be a problem\n>> > not to skip hidden dirs since hidden dirs/files are not expected at\n>> > .git/objects.\n>>\n>> I reproduce this with --local. A \".foo\" isn't copied before, now it\n>> is. Good, I guess. We'd have already copied a \"foo\".\n>>\n>> > - Now, copy_or_link_directory will call die() in case of an error on\n>> > openddir, readdir or lstat, inside dir_iterator_advance. That means\n>> > it will abort in case of an error trying to fetch any iteration\n>> > entry.\n>\n> It would be nice if the above paragraph in the commit message would\n> say what was the previous behavior and why it's better to die() .\n>\n>> Good, but really IMNSHO this series is tweaking some critical core code\n>> and desperately needs tests.\n>\n> It's critical that this code works well in the usual case, yes. (And\n> there are already a lot of tests that test that.) But when people have\n> manually tweaked things in their \".git/objects/\", it's not critical\n> what happens. Many systems have \"undefined behaviors\" at some point\n> and that's ok.\n>\n> And no, I am not saying that we should consider it completely\n> \"undefined behavior\" as soon as something is manually tweaked in\n> \".git/\", and yes, tests would be nice, and your comments and manual\n> tests on this are very much appreciated. It's just that I don't think\n> we should require too much when a patch, especially from a first time\n> contributor, is already improving things, though it also changes a few\n> things in a grey area.\n>\n>> Unfortunately, in this as in so many edge case we have no existing\n>> tests.\n>>\n>> This would be much easier to review and would give reviewers more\n>> confidence if the parts of this that changed behavior started with a\n>> patch or patches that just manually objects/ dirs with various\n>\n> I think \"created\" is missing between \"manually\" and \"objects/\" in the\n> above sentence.\n>\n>> combinations of symlinks, hardlinks etc., and asserted that the various\n>> options did exactly what they're doing now, and made sure the\n>> source/target repos were the same after/both passed \"fsck\".\n>>\n>> Then followed by some version of this patch which changes the behavior,\n>> and would be forced to tweak those tests. To make it clear e.g. that\n>> some cases where we have a working \"clone\" are now a hard error.\n>\n> Unfortunately this would be a lot of work and not appropriate for a\n> GSoC micro-project.\n>\n> Thanks,\n> Christian.\n\nHere's a test that works before 3/3 and fails after, can be used with my SOB:\n\n diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\n index 4320082b1b..80c0a4a19b 100755\n --- a/t/t5604-clone-reference.sh\n +++ b/t/t5604-clone-reference.sh\n @@ -221,4 +221,37 @@ test_expect_success 'clone, dissociate from alternates' '\n \t( cd C && git fsck )\n '\n\n +test_expect_success SHA1,SYMLINKS 'setup repo with manually symlinked objects/*' '\n +\tgit init S &&\n +\t(\n +\t\tcd S &&\n +\t\ttest_commit A &&\n +\t\tgit gc &&\n +\t\ttest_commit B &&\n +\t\t(\n +\t\t\tcd .git/objects &&\n +\t\t\tmv 22/3b7836fb19fdf64ba2d3cd6173c6a283141f78 . &&\n +\t\t\tln -s ../3b7836fb19fdf64ba2d3cd6173c6a283141f78 22/ &&\n +\t\t\tmv 40 forty &&\n +\t\t\tln -s forty 40 &&\n +\t\t\tmv pack packs &&\n +\t\t\tln -s packs pack\n +\t\t)\n +\t)\n +'\n +\n +test_expect_success SHA1,SYMLINKS 'clone repo with manually symlinked objects/*' '\n +\tfor option in --local --no-hardlinks --shared --dissociate\n +\tdo\n +\t\tgit clone $option S S$option || return 1 &&\n +\t\tgit -C S$option fsck || return 1\n +\tdone &&\n +\tfind S-* -type l | sort >actual &&\n +\tcat >expected <<-EOF &&\n +\tS--dissociate/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78\n +\tS--local/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78\n +\tEOF\n +\ttest_cmp expected actual\n +'\n +\n test_done\n\nObviously not ideal, there's a way to do this without relying on the\nSHA1 prereq, but in this case I don't see much point. Also similar to\nthe test_cmp there it would make senses to create\n.git/objects/{foo,.foo} and see if they show up in the clone.\n\nThe GSOC project is basically \"replace existing thing with identical\nutility function\", but software can always be more/less complex when it\ngets to writing the code, so now it's become \"we no longer uniformly\naccept the same repository formats across fsck/clone as a side-effect\".\n\nTo me that's not a grey area. Yes we don't produce that ourselves, but\nit's the on-disk format we've always supported, and can expect e.g. that\nsomeone's symlinking objects/?? to different partitions etc. in some\nadvanced setups.\n\nCan't the utility function we're moving to just be made to be\nbug-compatible with what we're doing now with symlinks?\n"},{"id":"370093","messageId":"CAHd-oW5zQQkJ7WCcKpz0V7btNAbwuwCn0fvkcvAmXE1U9BhMxA@mail.gmail.com","threadId":"50572","inReplyTo":"20190223215906.GR6085@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-24T16:34:51Z","receivedAt":"2019-02-24T16:35:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Feb 23, 2019 at 6:59 PM Thomas Gummerer wrote:\n>\n> On 02/23, Matheus Tavares wrote:\n> > Add pedantic option to dir_iterator_begin at dir-iterator.c and convert\n> > explicit recursive directory traversal at copy_or_link_directory\n> > (builtin/clone.c) to the dir-iterator API.\n>\n> Thanks for another iteration of this.\n>\n> To make life easier for reviewers, please include a link (or the\n> message-ID) to all previous iteration of the series. We often use\n> links to the public-inbox mirror for this,\n> e.g. https://public-inbox.org/git/20190215154913.18800-1-matheus.bernardino@usp.br/.\n>\n> This helps reviewers go back quickly to previous iterations of the\n> series, and refresh their memory on the comments that were left\n> there.\n>\n> You can also use the --in-reply-to option in 'git send-email' to chain\n> the threads, which also makes life easier for reviewers.\n>\n> An additional way to help reviewers is to include a 'range-diff'\n> between the previous iteration of the series, and the current\n> iteration. See the 'git range-diff' command or the '--range-diff'\n> option to 'git format-patch' for that. That helps reviewers to\n> quickly see what changed between iterations, so that they don't have\n> to re-review everything, if they can still remember the last round\n> well enough.\n>\n> I also added Christian Couder back to the Cc list, as he commented on\n> the RFC. It's good to keep people that commented on the series in Cc,\n> as they have shown some interest in the series, so keeping them in the\n> Cc list helps highlight those emails for them, and makes it more\n> likely that the patches get reviewed quickly.\n>\n\nOk, thanks for all the tips, Thomas! Now I started to notice the\npublic-inbox references for previous iterations in other people's\npatches, too.\n\nI am part of a FLOSS group here at USP called FLUSP\n(https://flusp.ime.usp.br/), and I plan to write some posts on our\nwebsite about what I am learning in the git community so that other\npeople in the group can have as a base if they decide to start\ncontributing to git too. So all this tips and explanations are of\ngreat value, not only for me but for others here! Thanks a lot.\n\n> > This is my microproject for GSoC 2019. Idea taken from\n> > https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal\n> >\n> > Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561\n> >\n> > Matheus Tavares (3):\n> > dir-iterator: add pedantic option to dir_iterator_begin\n> > clone: extract function from copy_or_link_directory\n> > clone: use dir-iterator to avoid explicit dir traversal\n> >\n> > builtin/clone.c | 72 ++++++++++++++++++++++++++++----------------\n> > dir-iterator.c | 23 ++++++++++++--\n> > dir-iterator.h | 16 ++++++++--\n> > refs/files-backend.c | 2 +-\n> > 4 files changed, 81 insertions(+), 32 deletions(-)\n> >\n> > --\n> > 2.20.1\n> >\n"},{"id":"370094","messageId":"CAHd-oW78Sj_qyFZgUfYgJcB5w5_BC_F-wbiLOU7VzJhFt9919A@mail.gmail.com","threadId":"50572","inReplyTo":"CAP8UFD0uZbzG7pjqLAQ5374_0vBTbaQ89u-rhiY7c7GXSKb8tw@mail.gmail.com","subject":"Re: [GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-24T17:43:50Z","receivedAt":"2019-02-24T17:44:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sun, Feb 24, 2019 at 5:35 AM Christian Couder\n wrote:\n>\n> On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer wrote:\n> >\n> > On 02/23, Matheus Tavares wrote:\n> > > Add the pedantic option to dir-iterator's initialization function,\n> > > dir_iterator_begin. When this option is set to true,\n> > > dir_iterator_advance will immediately return ITER_ERROR when failing to\n> > > fetch the next entry. When set to false, dir_iterator_advance will emit\n> > > a warning and keep looking for the next entry.\n> > >\n> > > Also adjust refs/files-backend.c to the new dir_iterator_begin\n> > > signature.\n> >\n> > Thanks, this makes sense to me. This commit message describes what we\n> > are doing in this patch, but not why we are doing it. From previously\n> > reviewing this series, I know this is going to be used in a subsequent\n> > patch, but it is nice to give reviewers and future readers of this\n> > patch that context as well. Just something like \"This behaviour is\n> > going to be used in a subsequent patch.\" should be enough here.\n>\n> I agree that it's a good idea to add just that.\n>\n\nIndeed! Thank you Thomas and Christian. I will be adding this in v3.\n\n> > > int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> > > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> > >\n> > > level->dir = opendir(iter->base.path.buf);\n> > > if (!level->dir && errno != ENOENT) {\n> > > + if (iter->pedantic)\n> > > + goto error_out;\n> >\n> > I think we should also print an error here. The caller doesn't have\n> > any context on what went wrong, and will probably just 'die()' if an\n> > error is encountered.\n\nTo correctly handle the error, I assumed that the caller wouldn't need\nto know which exact function returned an error, as long as it knew it\nwas a \"failure to fetch the next entry\" kind of error, which is the\n\"category\" of errors caught with the 'pedantic' option. (currently, it\nincludes errors on lstat, opendir and readdir). Is this assumption\nvalid?\n\n> > I think it would make sense to call\n> > 'error(...)' here before 'goto error_out;' to give a useful error\n> > message here.\n>\n> If we start to give error messages, then we might as well give error\n> messages all the times when we error out. This will avoid the callers\n> wondering if they need to give an error message or not.\n>\n> I am not sure it's necessary here though. And I think if it's useful,\n> it can be added in another patch or another patch series.\n>\n\nI could just copy the warning messages bellow each 'goto error_out'\nand use then at an 'error(...)' call before the goto. But as Christian\npointed out, I think this would confuse callers wether they should\nprint error messages or not. On the other side, it may just be\ndifferent 'layers' of errors... I don't have any strong opinion about\nthis.\n\n> > > warning(\"error opening directory %s: %s\",\n> > > iter->base.path.buf, strerror(errno));\n> > > /* Popping the level is handled below */\n>\n> > > -struct dir_iterator *dir_iterator_begin(const char *path)\n> > > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)\n> >\n> > Thinking about the future evolution of this interface, it might make\n> > more sense to have that second parameter be a \"struct\n> > dir_iterator_opts\". For now it would just have one member \"pedantic\",\n> > but in the future we could add additional options there instead of\n> > adding additional parameters.\n>\n> I think it's ok with `int pedantic` for now as improvements can be\n> done when they are really needed. And we will perhaps find out that\n> it's better to just change `int pedantic` to `unsigned flags` instead\n> of `struct dir_iterator_opts`.\n>\n\nI did thought about using `unsigned flags` instead of `int pedantic`\nfor the same reasons Thomas pointed out, but as there would be just\none flag for now, it seemed to me that `int pedantic` would make more\nsense (following the 'YAGNI' principle). But if it is already known\nthat more flags (or options) are coming in a very near future, I may\nchange this to `unsigned flags` or `struct dir_iterator_opts` in v3 if\nyou think it is needed. Just let me know, please.\n\n> Thanks,\n> Christian.\n"},{"id":"370097","messageId":"CAHd-oW71y7nadq96c_4WDgs=xuTy-Kd98arJxv7eMwvZ3LkPXg@mail.gmail.com","threadId":"50572","inReplyTo":"20190223214856.GQ6085@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-24T18:19:46Z","receivedAt":"2019-02-24T18:20:00Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Feb 23, 2019 at 6:48 PM Thomas Gummerer wrote:\n>\n> On 02/23, Matheus Tavares wrote:\n> > ---\n> > Changes in v2:\n> > - Improved patch message\n> > - Removed a now unused variable\n>\n> s/variable/parameter/ I believe?\n\nYes, you are right!\n\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> > + strbuf_addstr(dest, iter->relative_path);\n> > +\n> > + /*\n> > + * dir_iterator_advance already calls lstat to populate iter->st\n> > + * but, unlike stat, lstat does not checks for permissions on\n> > + * the given path.\n> > + */\n>\n> Hmm, lstat does check the permissions on the path, it just doesn't\n> follow symlinks. I think I actually mislead you in my previous review\n> here, and was reading the code in dir-iterator.c all wrong.\n>\n> I thought it said \"if (errno == ENOENT) warning(...)\", however the\n> condition is \"errno != ENOENT\", which is why I thought we were loosing\n> warnings when errno == EACCES for example.\n>\n> As we decided that we would no longer follow symlinks now, I think we\n> can actually get rid of the stat call here. Sorry about the confusion.\n>\n\nOk, I also read the lstat man page wrongly and though it didn't check\nfor permissions. Thanks for noticing that. I will remove the lstat\ncall in v3.\n"},{"id":"370098","messageId":"20190224210605.GA27711@hank.intra.tgummerer.com","threadId":"50572","inReplyTo":"CAHd-oW78Sj_qyFZgUfYgJcB5w5_BC_F-wbiLOU7VzJhFt9919A@mail.gmail.com","subject":"Re: [GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-24T21:06:05Z","receivedAt":"2019-02-24T21:06:12Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/24, Matheus Tavares Bernardino wrote:\n> On Sun, Feb 24, 2019 at 5:35 AM Christian Couder\n> wrote:\n> > On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer wrote:\n> > > > int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> > > > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> > > >\n> > > > level->dir = opendir(iter->base.path.buf);\n> > > > if (!level->dir && errno != ENOENT) {\n> > > > + if (iter->pedantic)\n> > > > + goto error_out;\n> > >\n> > > I think we should also print an error here. The caller doesn't have\n> > > any context on what went wrong, and will probably just 'die()' if an\n> > > error is encountered.\n> \n> To correctly handle the error, I assumed that the caller wouldn't need\n> to know which exact function returned an error, as long as it knew it\n> was a \"failure to fetch the next entry\" kind of error, which is the\n> \"category\" of errors caught with the 'pedantic' option. (currently, it\n> includes errors on lstat, opendir and readdir). Is this assumption\n> valid?\n>\n> > > I think it would make sense to call\n> > > 'error(...)' here before 'goto error_out;' to give a useful error\n> > > message here.\n> >\n> > If we start to give error messages, then we might as well give error\n> > messages all the times when we error out. This will avoid the callers\n> > wondering if they need to give an error message or not.\n> >\n> > I am not sure it's necessary here though. And I think if it's useful,\n> > it can be added in another patch or another patch series.\n> >\n>\n> I could just copy the warning messages bellow each 'goto error_out'\n> and use then at an 'error(...)' call before the goto. But as Christian\n> pointed out, I think this would confuse callers wether they should\n> print error messages or not. On the other side, it may just be\n> different 'layers' of errors... I don't have any strong opinion about\n> this.\n\nRight, I think it just comes down to which amount of detail we want to\ncommunicate back to the user. I thought a bit more detail could be\nhelpful, but just giving a more generic error should also be okay.\n\n> > > > warning(\"error opening directory %s: %s\",\n> > > > iter->base.path.buf, strerror(errno));\n> > > > /* Popping the level is handled below */\n> >\n> > > > -struct dir_iterator *dir_iterator_begin(const char *path)\n> > > > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)\n> > >\n> > > Thinking about the future evolution of this interface, it might make\n> > > more sense to have that second parameter be a \"struct\n> > > dir_iterator_opts\". For now it would just have one member \"pedantic\",\n> > > but in the future we could add additional options there instead of\n> > > adding additional parameters.\n> >\n> > I think it's ok with `int pedantic` for now as improvements can be\n> > done when they are really needed. And we will perhaps find out that\n> > it's better to just change `int pedantic` to `unsigned flags` instead\n> > of `struct dir_iterator_opts`.\n> >\n> \n> I did thought about using `unsigned flags` instead of `int pedantic`\n> for the same reasons Thomas pointed out, but as there would be just\n> one flag for now, it seemed to me that `int pedantic` would make more\n> sense (following the 'YAGNI' principle). But if it is already known\n> that more flags (or options) are coming in a very near future, I may\n> change this to `unsigned flags` or `struct dir_iterator_opts` in v3 if\n> you think it is needed. Just let me know, please.\n\nLooking at the potential improvements that were suggested in the\ninitial commit adding dir-iterator, 'unsigned flags' would not be\nenough to be able to pass all those options. That's where my\nsuggestion for 'struct dir_iterator_opts' comes in.\n\nBut I don't feel too strongly about it, and am okay with just an 'int\npedantic' option for now, until we see different usages, if others\nfeel like that's the better option for now.\n\n> > Thanks,\n> > Christian.\n"},{"id":"370099","messageId":"20190224210742.GB27711@hank.intra.tgummerer.com","threadId":"50572","inReplyTo":"CAHd-oW5zQQkJ7WCcKpz0V7btNAbwuwCn0fvkcvAmXE1U9BhMxA@mail.gmail.com","subject":"Re: [GSoC][PATCH 0/3] clone: convert explicit dir traversal to dir-iterator","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-02-24T21:07:42Z","receivedAt":"2019-02-24T21:07:47Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/24, Matheus Tavares Bernardino wrote:\n> I am part of a FLOSS group here at USP called FLUSP\n> (https://flusp.ime.usp.br/), and I plan to write some posts on our\n> website about what I am learning in the git community so that other\n> people in the group can have as a base if they decide to start\n> contributing to git too. So all this tips and explanations are of\n> great value, not only for me but for others here! Thanks a lot.\n\nGreat to hear! Please do post links to those posts on the mailing\nlist as well, I'm interested in reading them.\n"},{"id":"370103","messageId":"CAHd-oW4P-aAKH0_kKrOJeprWmvPCUfa++X3pSNYSQisAV-25HA@mail.gmail.com","threadId":"50572","inReplyTo":"CAP8UFD2LLL+V54k3XmHzmg5t1zanpAvY_=7GqXL43vLPTTmuww@mail.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-25T02:31:25Z","receivedAt":"2019-02-25T02:31:40Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Christian and Ævar\n\nFirst of all, thanks for the fast and attentive reviews.\n\nI am a little confused about what I should do next. How should I\nproceed with this series?\n\nBy what was said, I understood that the series:\n1) Is indeed an improvement under --local, because it won't deference\nsymlinks in this case.\n2) Don't make --dissociate any better but as it is already buggy that\nwould be some work for another patchset.\n3) Makes git-clone copy hidden paths which is a good behaviour.\n4) Breaks --no-hardlinks when there are symlinks at the repo's objects\ndirectory.\n\nI understood that even though git itself does not create symlinks in\n.git/objects, we should take care of the case where the user manually\ncreates them, right? But what would be the appropriate behaviour: to\nfollow (i.e. deference) symlinks (the way it is done now) or just copy\nthe link file itself (the way my series currently do)? And shouldn't\nwe document this decision somewhere?\n\nAbout the failure with --no-hardlinks having symlinks at .dir/objects,\nit's probably because copy_or_link_directory() is trying to copy a\nfile which is a symlink to a dir and the copy function used is trying\nto copy the dir not the link itself. A possible fix is to change\ncopy.c to copy the link file, but I haven't studied yet how that could\nbe accomplished.\n\nAnother possible fix is to make copy_or_link_directory() deference\nsymlink structures when --no-hardlinks is given. But because the\nfunction falls back to no-hardlinks when failing to hardlink, I don't\nthink it would be easy to accomplish this without making the function\n*always* deference symlinks. And that would make the series lose the\nitem 1), which I understand you liked.\n\nSo, what would be the best here?\n\nBest,\nMatheus Tavares\n\nOn Sun, Feb 24, 2019 at 6:41 AM Christian Couder\n wrote:\n>\n> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason\n> wrote:\n> >\n> >\n> > On Sat, Feb 23 2019, Matheus Tavares 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. This simplifies the code and avoid recursive calls to\n> > > copy_or_link_directory.\n> >\n> > Sounds good in principle.\n> >\n> > > This process also brings some safe behaviour changes to\n> > > copy_or_link_directory:\n> >\n> > I ad-hoc tested some of these, and could spot behavior changes. We\n> > should have tests for these.\n>\n> I agree that ideally we should have a few tests for these, but this is\n> a grey area (see below) and there are areas that are not grey for\n> which we don't have any test...\n>\n> And then adding tests would make this series become larger than a\n> typical GSoC micro-project...\n>\n> > > - It will no longer follows symbolic links. This is not a problem,\n> > > since the function is only used to copy .git/objects directory, and\n> > > symbolic links are not expected there.\n> >\n> > I don't think we should make that assumption, and I don't know of\n> > anything else in git that does.\n>\n> I think Git itself doesn't create symlinks in \".git/objects/\" and we\n> don't recommend people manually tweaking what's inside \".git/\". That's\n> why I think it's a grey area.\n>\n> > I've certainly symlinked individual objects or packs into a repo for\n> > debugging / recovery, and it would be unexpected to clone that and miss\n> > something.\n>\n> If people tweak what's inside \".git\" by hand, they are expected to\n> know what they doing and be able to debug it.\n>\n> > So in the general case we should be strict in what we generate, but\n> > permissive in what we accept. We don't want a \"clone\" of an existing\n> > repo to fail, or \"fsck\" to fail after clone...\n>\n> Yeah, but realistically I don't think we are going to foolproof Git\n> from everything that someone could do by tweaking random things\n> manually in \".git/\".\n>\n> I am not saying that it should be ok to make things much worse than\n> they are now in case some things have been tweaked in \".git/\", but if\n> things in general don't look worse in this grey area, and a patch\n> otherwise improves things, then I think the patch should be ok.\n>\n> > When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,\n> > and a specific object in objects/4d/ a symlink to /tmp too.\n> >\n> > Without this patch the individual object is still a symlink, but the\n> > object under the directory gets resolved, and \"un-symlinked\", also with\n> > --dissociate, which seems like an existing bug.\n> >\n> > With your patch that symlink structure is copied as-is. That's more\n> > faithful under --local, but a regression for --dissociate (which didn't\n> > work fully to begin with...).\n>\n> I think that I use --local (which is the default if the repository is\n> specified as a local path) much more often than --dissociate, so for\n> me the patch would be very positive, especially since --dissociate is\n> already buggy anyway in this case.\n>\n> > I was paranoid that \"no longer follows symbolic links\" could also mean\n> > \"will ignore those objects\", but it seems to more faithfully copy things\n> > as-is for *that* case.\n>\n> Nice!\n>\n> > But then I try with --no-hardlinks and stock git dereferences my symlink\n> > structure, but with your patches fails completely:\n> >\n> > Cloning into bare repository 'repo2'...\n> > error: copy-fd: read returned: Is a directory\n> > fatal: failed to copy file to 'repo2/objects/c4': Is a directory\n> > fatal: the remote end hung up unexpectedly\n> > fatal: cannot change to 'repo2': No such file or directory\n>\n> Maybe this could be fixed. Anyway I don't use --no-hardlinks very\n> often, so I still think the patch is a positive even with this\n> failure.\n>\n> > So there's at least one case in a few minutes of prodding this where we\n> > can't clone a working repo now, however obscure the setup.\n> >\n> > > - Hidden directories won't be skipped anymore. In fact, it is odd that\n> > > the function currently skip hidden directories but not hidden files.\n> > > The reason for that could be unintentional: probably the intention\n> > > was to skip '.' and '..' only, but it ended up accidentally skipping\n> > > all directories starting with '.'. Again, it must not be a problem\n> > > not to skip hidden dirs since hidden dirs/files are not expected at\n> > > .git/objects.\n> >\n> > I reproduce this with --local. A \".foo\" isn't copied before, now it\n> > is. Good, I guess. We'd have already copied a \"foo\".\n> >\n> > > - Now, copy_or_link_directory will call die() in case of an error on\n> > > openddir, readdir or lstat, inside dir_iterator_advance. That means\n> > > it will abort in case of an error trying to fetch any iteration\n> > > entry.\n>\n> It would be nice if the above paragraph in the commit message would\n> say what was the previous behavior and why it's better to die() .\n>\n> > Good, but really IMNSHO this series is tweaking some critical core code\n> > and desperately needs tests.\n>\n> It's critical that this code works well in the usual case, yes. (And\n> there are already a lot of tests that test that.) But when people have\n> manually tweaked things in their \".git/objects/\", it's not critical\n> what happens. Many systems have \"undefined behaviors\" at some point\n> and that's ok.\n>\n> And no, I am not saying that we should consider it completely\n> \"undefined behavior\" as soon as something is manually tweaked in\n> \".git/\", and yes, tests would be nice, and your comments and manual\n> tests on this are very much appreciated. It's just that I don't think\n> we should require too much when a patch, especially from a first time\n> contributor, is already improving things, though it also changes a few\n> things in a grey area.\n>\n> > Unfortunately, in this as in so many edge case we have no existing\n> > tests.\n> >\n> > This would be much easier to review and would give reviewers more\n> > confidence if the parts of this that changed behavior started with a\n> > patch or patches that just manually objects/ dirs with various\n>\n> I think \"created\" is missing between \"manually\" and \"objects/\" in the\n> above sentence.\n>\n> > combinations of symlinks, hardlinks etc., and asserted that the various\n> > options did exactly what they're doing now, and made sure the\n> > source/target repos were the same after/both passed \"fsck\".\n> >\n> > Then followed by some version of this patch which changes the behavior,\n> > and would be forced to tweak those tests. To make it clear e.g. that\n> > some cases where we have a working \"clone\" are now a hard error.\n>\n> Unfortunately this would be a lot of work and not appropriate for a\n> GSoC micro-project.\n>\n> Thanks,\n> Christian.\n"},{"id":"370109","messageId":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","threadId":"50572","inReplyTo":"87tvgt454u.fsf@evledraar.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-25T09:45:23Z","receivedAt":"2019-02-25T09:45:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 24, 2019 at 9:45 PM Ævar Arnfjörð Bjarmason\n wrote:\n> ..\n> Can't the utility function we're moving to just be made to be\n> bug-compatible with what we're doing now with symlinks?\n\nI haven't really followed closely this thread. But I think the first\nstep should be bug-compatible (it really depends if you count the\ncurrent behavior \"buggy\"). Then add improvements on top if there's\nstill time or strong consensus. It's easier to bisect that way at\nleast.\n-- \nDuy\n"},{"id":"370110","messageId":"87zhqk5fnf.fsf@evledraar.gmail.com","threadId":"50572","inReplyTo":"CAHd-oW4P-aAKH0_kKrOJeprWmvPCUfa++X3pSNYSQisAV-25HA@mail.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-25T10:25:08Z","receivedAt":"2019-02-25T10:25:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 25 2019, Matheus Tavares Bernardino wrote:\n\n> Hi, Christian and Ævar\n>\n> First of all, thanks for the fast and attentive reviews.\n>\n> I am a little confused about what I should do next. How should I\n> proceed with this series?\n>\n> By what was said, I understood that the series:\n> 1) Is indeed an improvement under --local, because it won't deference\n> symlinks in this case.\n> 2) Don't make --dissociate any better but as it is already buggy that\n> would be some work for another patchset.\n> 3) Makes git-clone copy hidden paths which is a good behaviour.\n> 4) Breaks --no-hardlinks when there are symlinks at the repo's objects\n> directory.\n>\n> I understood that even though git itself does not create symlinks in\n> .git/objects, we should take care of the case where the user manually\n> creates them, right? But what would be the appropriate behaviour: to\n> follow (i.e. deference) symlinks (the way it is done now) or just copy\n> the link file itself (the way my series currently do)? And shouldn't\n> we document this decision somewhere?\n>\n> About the failure with --no-hardlinks having symlinks at .dir/objects,\n> it's probably because copy_or_link_directory() is trying to copy a\n> file which is a symlink to a dir and the copy function used is trying\n> to copy the dir not the link itself. A possible fix is to change\n> copy.c to copy the link file, but I haven't studied yet how that could\n> be accomplished.\n>\n> Another possible fix is to make copy_or_link_directory() deference\n> symlink structures when --no-hardlinks is given. But because the\n> function falls back to no-hardlinks when failing to hardlink, I don't\n> think it would be easy to accomplish this without making the function\n> *always* deference symlinks. And that would make the series lose the\n> item 1), which I understand you liked.\n\n\nI don't really have formed opinions one way or the other about what\nthese specific flags should do in combination with such a repository,\ne.g. should --dissociate copy data rather than point to the same\nsymlinks?\n\nI'm inclined to think so, but I've only thought about it for a couple of\nminutes. Maybe if someone starts digging they'll rightly come to a\ndifferent conclusion.\n\nRather, my comment is on the process. Clone behavior is too important to\nleave to prose in a commit message. I already found a case where we hard\nerror not explicitly called out, are there other edge cases I didn't\nthink of?\n\nSo having this e.g. be a 4-part series where 3/4 is introducing tests in\nthe direction I posted upthread (but needs more work), with 4/4 going\nthrough/justifying each one.\n\n\n> On Sun, Feb 24, 2019 at 6:41 AM Christian Couder\n> wrote:\n>>\n>> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason\n>> wrote:\n>> >\n>> >\n>> > On Sat, Feb 23 2019, Matheus Tavares 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. This simplifies the code and avoid recursive calls to\n>> > > copy_or_link_directory.\n>> >\n>> > Sounds good in principle.\n>> >\n>> > > This process also brings some safe behaviour changes to\n>> > > copy_or_link_directory:\n>> >\n>> > I ad-hoc tested some of these, and could spot behavior changes. We\n>> > should have tests for these.\n>>\n>> I agree that ideally we should have a few tests for these, but this is\n>> a grey area (see below) and there are areas that are not grey for\n>> which we don't have any test...\n>>\n>> And then adding tests would make this series become larger than a\n>> typical GSoC micro-project...\n>>\n>> > > - It will no longer follows symbolic links. This is not a problem,\n>> > > since the function is only used to copy .git/objects directory, and\n>> > > symbolic links are not expected there.\n>> >\n>> > I don't think we should make that assumption, and I don't know of\n>> > anything else in git that does.\n>>\n>> I think Git itself doesn't create symlinks in \".git/objects/\" and we\n>> don't recommend people manually tweaking what's inside \".git/\". That's\n>> why I think it's a grey area.\n>>\n>> > I've certainly symlinked individual objects or packs into a repo for\n>> > debugging / recovery, and it would be unexpected to clone that and miss\n>> > something.\n>>\n>> If people tweak what's inside \".git\" by hand, they are expected to\n>> know what they doing and be able to debug it.\n>>\n>> > So in the general case we should be strict in what we generate, but\n>> > permissive in what we accept. We don't want a \"clone\" of an existing\n>> > repo to fail, or \"fsck\" to fail after clone...\n>>\n>> Yeah, but realistically I don't think we are going to foolproof Git\n>> from everything that someone could do by tweaking random things\n>> manually in \".git/\".\n>>\n>> I am not saying that it should be ok to make things much worse than\n>> they are now in case some things have been tweaked in \".git/\", but if\n>> things in general don't look worse in this grey area, and a patch\n>> otherwise improves things, then I think the patch should be ok.\n>>\n>> > When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,\n>> > and a specific object in objects/4d/ a symlink to /tmp too.\n>> >\n>> > Without this patch the individual object is still a symlink, but the\n>> > object under the directory gets resolved, and \"un-symlinked\", also with\n>> > --dissociate, which seems like an existing bug.\n>> >\n>> > With your patch that symlink structure is copied as-is. That's more\n>> > faithful under --local, but a regression for --dissociate (which didn't\n>> > work fully to begin with...).\n>>\n>> I think that I use --local (which is the default if the repository is\n>> specified as a local path) much more often than --dissociate, so for\n>> me the patch would be very positive, especially since --dissociate is\n>> already buggy anyway in this case.\n>>\n>> > I was paranoid that \"no longer follows symbolic links\" could also mean\n>> > \"will ignore those objects\", but it seems to more faithfully copy things\n>> > as-is for *that* case.\n>>\n>> Nice!\n>>\n>> > But then I try with --no-hardlinks and stock git dereferences my symlink\n>> > structure, but with your patches fails completely:\n>> >\n>> > Cloning into bare repository 'repo2'...\n>> > error: copy-fd: read returned: Is a directory\n>> > fatal: failed to copy file to 'repo2/objects/c4': Is a directory\n>> > fatal: the remote end hung up unexpectedly\n>> > fatal: cannot change to 'repo2': No such file or directory\n>>\n>> Maybe this could be fixed. Anyway I don't use --no-hardlinks very\n>> often, so I still think the patch is a positive even with this\n>> failure.\n>>\n>> > So there's at least one case in a few minutes of prodding this where we\n>> > can't clone a working repo now, however obscure the setup.\n>> >\n>> > > - Hidden directories won't be skipped anymore. In fact, it is odd that\n>> > > the function currently skip hidden directories but not hidden files.\n>> > > The reason for that could be unintentional: probably the intention\n>> > > was to skip '.' and '..' only, but it ended up accidentally skipping\n>> > > all directories starting with '.'. Again, it must not be a problem\n>> > > not to skip hidden dirs since hidden dirs/files are not expected at\n>> > > .git/objects.\n>> >\n>> > I reproduce this with --local. A \".foo\" isn't copied before, now it\n>> > is. Good, I guess. We'd have already copied a \"foo\".\n>> >\n>> > > - Now, copy_or_link_directory will call die() in case of an error on\n>> > > openddir, readdir or lstat, inside dir_iterator_advance. That means\n>> > > it will abort in case of an error trying to fetch any iteration\n>> > > entry.\n>>\n>> It would be nice if the above paragraph in the commit message would\n>> say what was the previous behavior and why it's better to die() .\n>>\n>> > Good, but really IMNSHO this series is tweaking some critical core code\n>> > and desperately needs tests.\n>>\n>> It's critical that this code works well in the usual case, yes. (And\n>> there are already a lot of tests that test that.) But when people have\n>> manually tweaked things in their \".git/objects/\", it's not critical\n>> what happens. Many systems have \"undefined behaviors\" at some point\n>> and that's ok.\n>>\n>> And no, I am not saying that we should consider it completely\n>> \"undefined behavior\" as soon as something is manually tweaked in\n>> \".git/\", and yes, tests would be nice, and your comments and manual\n>> tests on this are very much appreciated. It's just that I don't think\n>> we should require too much when a patch, especially from a first time\n>> contributor, is already improving things, though it also changes a few\n>> things in a grey area.\n>>\n>> > Unfortunately, in this as in so many edge case we have no existing\n>> > tests.\n>> >\n>> > This would be much easier to review and would give reviewers more\n>> > confidence if the parts of this that changed behavior started with a\n>> > patch or patches that just manually objects/ dirs with various\n>>\n>> I think \"created\" is missing between \"manually\" and \"objects/\" in the\n>> above sentence.\n>>\n>> > combinations of symlinks, hardlinks etc., and asserted that the various\n>> > options did exactly what they're doing now, and made sure the\n>> > source/target repos were the same after/both passed \"fsck\".\n>> >\n>> > Then followed by some version of this patch which changes the behavior,\n>> > and would be forced to tweak those tests. To make it clear e.g. that\n>> > some cases where we have a working \"clone\" are now a hard error.\n>>\n>> Unfortunately this would be a lot of work and not appropriate for a\n>> GSoC micro-project.\n>>\n>> Thanks,\n>> Christian.\n"},{"id":"370137","messageId":"CAP8UFD2xrfMHNxcmeYf8G+d53SL26N07FFAoDP+e0h3r-tvKQw@mail.gmail.com","threadId":"50572","inReplyTo":"87zhqk5fnf.fsf@evledraar.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-25T20:40:40Z","receivedAt":"2019-02-25T20:40:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 25, 2019 at 11:25 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Mon, Feb 25 2019, Matheus Tavares Bernardino wrote:\n>\n> > Hi, Christian and Ævar\n> >\n> > First of all, thanks for the fast and attentive reviews.\n> >\n> > I am a little confused about what I should do next. How should I\n> > proceed with this series?\n> >\n> > By what was said, I understood that the series:\n> > 1) Is indeed an improvement under --local, because it won't deference\n> > symlinks in this case.\n> > 2) Don't make --dissociate any better but as it is already buggy that\n> > would be some work for another patchset.\n> > 3) Makes git-clone copy hidden paths which is a good behaviour.\n> > 4) Breaks --no-hardlinks when there are symlinks at the repo's objects\n> > directory.\n> >\n> > I understood that even though git itself does not create symlinks in\n> > .git/objects, we should take care of the case where the user manually\n> > creates them, right? But what would be the appropriate behaviour: to\n> > follow (i.e. deference) symlinks (the way it is done now) or just copy\n> > the link file itself (the way my series currently do)? And shouldn't\n> > we document this decision somewhere?\n> >\n> > About the failure with --no-hardlinks having symlinks at .dir/objects,\n> > it's probably because copy_or_link_directory() is trying to copy a\n> > file which is a symlink to a dir and the copy function used is trying\n> > to copy the dir not the link itself. A possible fix is to change\n> > copy.c to copy the link file, but I haven't studied yet how that could\n> > be accomplished.\n> >\n> > Another possible fix is to make copy_or_link_directory() deference\n> > symlink structures when --no-hardlinks is given. But because the\n> > function falls back to no-hardlinks when failing to hardlink, I don't\n> > think it would be easy to accomplish this without making the function\n> > *always* deference symlinks. And that would make the series lose the\n> > item 1), which I understand you liked.\n>\n> I don't really have formed opinions one way or the other about what\n> these specific flags should do in combination with such a repository,\n> e.g. should --dissociate copy data rather than point to the same\n> symlinks?\n>\n> I'm inclined to think so, but I've only thought about it for a couple of\n> minutes. Maybe if someone starts digging they'll rightly come to a\n> different conclusion.\n\nAnd maybe one day we will find a very good way to take advantage of\nsymlinks in .git/objects/ when Git is used normally, but that will go\nagainst what we have decided now, though we have no real need at all\nto decide now.\n\nThat's why I think it can actually be a good thing not to decide\nanything now, and to let us free to decide later.\n\nIt's kind of the same as with short options versus long options. It's\na good idea not to use up all your short options too soon (in the name\nof current ease of use), and instead wait until people really want a\nshort option for one option they use really often before attributing\nit to this option.\n\n> Rather, my comment is on the process. Clone behavior is too important to\n> leave to prose in a commit message.\n\nThe same argument could be made to say that what happens in\n.git/objects/ when cloning and there are symlinks is too important a\nstill free design option we have left to give it up right now for no\ngood reason.\n"},{"id":"370187","messageId":"20190226002625.13022-1-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 0/7] clone: dir iterator refactoring with tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:18Z","receivedAt":"2019-02-26T00:26:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Feb 25 2019, Duy Nguyen wrote:\n\n> On Sun, Feb 24, 2019 at 9:45 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>> ..\n>> Can't the utility function we're moving to just be made to be\n>> bug-compatible with what we're doing now with symlinks?\n>\n> I haven't really followed closely this thread. But I think the first\n> step should be bug-compatible (it really depends if you count the\n> current behavior \"buggy\"). Then add improvements on top if there's\n> still time or strong consensus. It's easier to bisect that way at\n> least.\n\nHere's an attempt at doing that. This was hastily done and as seen\nfrom the commit messages I don't really know what the intention with\nsome of this stuff is / didn't have time to dig, e.g. why the API is\nnow using lstat() instead of stat().\n\nBut as far as I can tell this is a series where up to patch 5 we\nretain the current behavior 100%. And in 6/7 and 7/7 introduce the\nbehavior changes in the previous 3/3 which I've been commenting on.\n\nMatheus Tavares (3):\n dir-iterator: add pedantic option to dir_iterator_begin\n clone: extract function from copy_or_link_directory\n clone: use dir-iterator to avoid explicit dir traversal\n\nÆvar Arnfjörð Bjarmason (4):\n dir-iterator: use stat() instead of lstat()\n clone: test for our behavior on odd objects/* content\n clone: stop ignoring dotdirs in --local etc. clone\n clone: break cloning repos that have symlinks in them\n\n builtin/clone.c | 72 ++++++++++++++++++++++++--------------\n dir-iterator.c | 23 ++++++++++--\n dir-iterator.h | 16 +++++++--\n refs/files-backend.c | 2 +-\n t/t5604-clone-reference.sh | 40 +++++++++++++++++++++\n 5 files changed, 121 insertions(+), 32 deletions(-)\n\n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370188","messageId":"20190226002625.13022-2-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 1/7] dir-iterator: add pedantic option to dir_iterator_begin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:19Z","receivedAt":"2019-02-26T00:26:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Matheus Tavares \n\nAdd the pedantic option to dir-iterator's initialization function,\ndir_iterator_begin. When this option is set to true,\ndir_iterator_advance will immediately return ITER_ERROR when failing to\nfetch the next entry. When set to false, dir_iterator_advance will emit\na warning and keep looking for the next entry.\n\nAlso adjust refs/files-backend.c to the new dir_iterator_begin\nsignature.\n\nSigned-off-by: Matheus Tavares \n---\n dir-iterator.c | 23 +++++++++++++++++++++--\n dir-iterator.h | 16 +++++++++++++---\n refs/files-backend.c | 2 +-\n 3 files changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex f2dcd82fde..070a656555 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -48,6 +48,13 @@ struct dir_iterator_int {\n \t * that will be included in this iteration.\n \t */\n \tstruct dir_iterator_level *levels;\n+\n+\t/*\n+\t * Boolean value to define dir-iterator's behaviour when failing to\n+\t * fetch next entry. See comments on dir_iterator_begin at\n+\t * dir-iterator.h\n+\t */\n+\tint pedantic;\n };\n \n int dir_iterator_advance(struct dir_iterator *dir_iterator)\n@@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \n \t\t\tlevel->dir = opendir(iter->base.path.buf);\n \t\t\tif (!level->dir && errno != ENOENT) {\n+\t\t\t\tif (iter->pedantic)\n+\t\t\t\t\tgoto error_out;\n \t\t\t\twarning(\"error opening directory %s: %s\",\n \t\t\t\t\titer->base.path.buf, strerror(errno));\n \t\t\t\t/* Popping the level is handled below */\n@@ -122,6 +131,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\tif (!de) {\n \t\t\t\t/* This level is exhausted; pop up a level. */\n \t\t\t\tif (errno) {\n+\t\t\t\t\tif (iter->pedantic)\n+\t\t\t\t\t\tgoto error_out;\n \t\t\t\t\twarning(\"error reading directory %s: %s\",\n \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n \t\t\t\t} else if (closedir(level->dir))\n@@ -139,10 +150,13 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \n \t\t\tstrbuf_addstr(&iter->base.path, de->d_name);\n \t\t\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n-\t\t\t\tif (errno != ENOENT)\n+\t\t\t\tif (errno != ENOENT) {\n+\t\t\t\t\tif (iter->pedantic)\n+\t\t\t\t\t\tgoto error_out;\n \t\t\t\t\twarning(\"error reading path '%s': %s\",\n \t\t\t\t\t\titer->base.path.buf,\n \t\t\t\t\t\tstrerror(errno));\n+\t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n \n@@ -159,6 +173,10 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\treturn ITER_OK;\n \t\t}\n \t}\n+\n+error_out:\n+\tdir_iterator_abort(dir_iterator);\n+\treturn ITER_ERROR;\n }\n \n int dir_iterator_abort(struct dir_iterator *dir_iterator)\n@@ -182,7 +200,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n \treturn ITER_DONE;\n }\n \n-struct dir_iterator *dir_iterator_begin(const char *path)\n+struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)\n {\n \tstruct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));\n \tstruct dir_iterator *dir_iterator = &iter->base;\n@@ -195,6 +213,7 @@ struct dir_iterator *dir_iterator_begin(const char *path)\n \n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n \n+\titer->pedantic = pedantic;\n \titer->levels_nr = 1;\n \titer->levels[0].initialized = 0;\n \ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 970793d07a..50ca8e1a27 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -19,7 +19,7 @@\n * A typical iteration looks like this:\n *\n * int ok;\n- * struct iterator *iter = dir_iterator_begin(path);\n+ * struct iterator *iter = dir_iterator_begin(path, 0);\n *\n * while ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n * if (want_to_stop_iteration()) {\n@@ -65,9 +65,15 @@ struct dir_iterator {\n * The iteration includes all paths under path, not including path\n * itself and not including \".\" or \"..\" entries.\n *\n- * path is the starting directory. An internal copy will be made.\n+ * Parameters are:\n+ * - path is the starting directory. An internal copy will be made.\n+ * - pedantic is a boolean value. If true, dir-iterator will free\n+ * resources and return ITER_ERROR immediately, in case of an error\n+ * while trying to fetch the next entry in dir_iterator_advance. If\n+ * false, it will just emit a warning and keep looking for the next\n+ * entry.\n */\n-struct dir_iterator *dir_iterator_begin(const char *path);\n+struct dir_iterator *dir_iterator_begin(const char *path, int pedantic);\n \n /*\n * Advance the iterator to the first or next item and return ITER_OK.\n@@ -76,6 +82,10 @@ struct dir_iterator *dir_iterator_begin(const char *path);\n * dir_iterator and associated resources and return ITER_ERROR. It is\n * a bug to use iterator or call this function again after it has\n * returned ITER_DONE or ITER_ERROR.\n+ *\n+ * Note that whether dir_iterator_advance will return ITER_ERROR when\n+ * failing to fetch the next entry or keep going is defined by the\n+ * 'pedantic' option at dir-iterator's initialization.\n */\n int dir_iterator_advance(struct dir_iterator *iterator);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex dd8abe9185..c3d3b6c454 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2143,7 +2143,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \n \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n-\titer->dir_iterator = dir_iterator_begin(sb.buf);\n+\titer->dir_iterator = dir_iterator_begin(sb.buf, 0);\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n \n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370189","messageId":"20190226002625.13022-3-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 2/7] dir-iterator: use stat() instead of lstat()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:20Z","receivedAt":"2019-02-26T00:26:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is surely a horrible idea, but all tests pass with this (not that\nI trust them much). Doing this for later WIP use in clone.c.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n dir-iterator.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 070a656555..6a9c0c4d08 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -149,7 +149,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\tcontinue;\n \n \t\t\tstrbuf_addstr(&iter->base.path, de->d_name);\n-\t\t\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n+\t\t\tif (stat(iter->base.path.buf, &iter->base.st) < 0) {\n \t\t\t\tif (errno != ENOENT) {\n \t\t\t\t\tif (iter->pedantic)\n \t\t\t\t\t\tgoto error_out;\n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370190","messageId":"20190226002625.13022-4-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 3/7] clone: extract function from copy_or_link_directory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:21Z","receivedAt":"2019-02-26T00:26:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Matheus Tavares \n\nExtract 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 a\nfollowing patch. Also makes code 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..862d2ea69c 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 st;\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, &st))\n+\t\t\tdie_errno(_(\"failed to stat '%s'\"), pathname);\n+\t\telse if (!S_ISDIR(st.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.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370191","messageId":"20190226002625.13022-5-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 4/7] clone: test for our behavior on odd objects/* content","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:22Z","receivedAt":"2019-02-26T00:26:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We've implicitly supported .git/objects/* content of symlinks since\napproximately forever, and when we do a copy of the repo we transfer\nthose over, but aren't very consistent about other random stuff we\nfind depending on if it's a \"hidden\" file or not.\n\nLet's add a test for that, which shouldn't read as an endorsement of\nwhat we're doing now, just asserts current behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5604-clone-reference.sh | 60 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 60 insertions(+)\n\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 4320082b1b..6f9c77049e 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -221,4 +221,64 @@ test_expect_success 'clone, dissociate from alternates' '\n \t( cd C && git fsck )\n '\n \n+test_expect_success SHA1,SYMLINKS 'setup repo with manually symlinked objects/*' '\n+\tgit init S &&\n+\t(\n+\t\tcd S &&\n+\t\ttest_commit A &&\n+\t\tgit gc &&\n+\t\ttest_commit B &&\n+\t\t(\n+\t\t\tcd .git/objects &&\n+\t\t\tmv 22/3b7836fb19fdf64ba2d3cd6173c6a283141f78 . &&\n+\t\t\tln -s ../3b7836fb19fdf64ba2d3cd6173c6a283141f78 22/ &&\n+\t\t\tmv 40 forty &&\n+\t\t\tln -s forty 40 &&\n+\t\t\tmv pack packs &&\n+\t\t\tln -s packs pack &&\n+\t\t\t>.some-hidden-file &&\n+\t\t\t>some-file &&\n+\t\t\tmkdir .some-hidden-dir &&\n+\t\t\t>.some-hidden-dir/some-file &&\n+\t\t\t>.some-hidden-dir/.some-dot-file &&\n+\t\t\tmkdir some-dir &&\n+\t\t\t>some-dir/some-file &&\n+\t\t\t>some-dir/.some-dot-file\n+\t\t)\n+\t)\n+'\n+\n+test_expect_success SHA1,SYMLINKS 'clone repo with manually symlinked objects/*' '\n+\tfor option in --local --no-hardlinks --shared --dissociate\n+\tdo\n+\t\tgit clone $option S S$option || return 1 &&\n+\t\tgit -C S$option fsck || return 1\n+\tdone &&\n+\tfind S-* -type l | sort >actual &&\n+\tcat >expected <<-EOF &&\n+\tS--dissociate/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78\n+\tS--local/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78\n+\tEOF\n+\ttest_cmp expected actual &&\n+\tfind S-* -name \"*some*\" | sort >actual &&\n+\tcat >expected <<-EOF &&\n+\tS--dissociate/.git/objects/.some-hidden-file\n+\tS--dissociate/.git/objects/some-dir\n+\tS--dissociate/.git/objects/some-dir/.some-dot-file\n+\tS--dissociate/.git/objects/some-dir/some-file\n+\tS--dissociate/.git/objects/some-file\n+\tS--local/.git/objects/.some-hidden-file\n+\tS--local/.git/objects/some-dir\n+\tS--local/.git/objects/some-dir/.some-dot-file\n+\tS--local/.git/objects/some-dir/some-file\n+\tS--local/.git/objects/some-file\n+\tS--no-hardlinks/.git/objects/.some-hidden-file\n+\tS--no-hardlinks/.git/objects/some-dir\n+\tS--no-hardlinks/.git/objects/some-dir/.some-dot-file\n+\tS--no-hardlinks/.git/objects/some-dir/some-file\n+\tS--no-hardlinks/.git/objects/some-file\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370192","messageId":"20190226002625.13022-6-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 5/7] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:23Z","receivedAt":"2019-02-26T00:26:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Matheus Tavares \n\nReplace usage of opendir/readdir/closedir API to traverse directories\nrecursively, at copy_or_link_directory function, by the dir-iterator\nAPI. This simplifies the code and avoid recursive calls to\ncopy_or_link_directory.\n\n[Ævar: This should be bug-compatible with the existing \"clone\"\nbehavior. The whole bit here with \"iter->relative_path[0] == '.'\" is a\ndirty hack. We don't copy dot-dirs, and then later on just blindly\nignore ENOENT errors as we descend into them. That case really wants\nto be a is_dotdir_or_file_within() test instead]\n\nNow, copy_or_link_directory will call die() in case of an error on\nopenddir, readdir or lstat, inside dir_iterator_advance. That means it\nwill abort in case of an error trying to fetch any iteration entry.\n\nSigned-off-by: Matheus Tavares \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/clone.c | 55 +++++++++++++++++++++++++++++--------------------\n 1 file changed, 33 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 862d2ea69c..c32e9022b3 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@@ -411,42 +413,47 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\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+\t\t\t\t const char *src_repo)\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+\tstruct stat st;\n \n \tmkdir_if_missing(dest->buf, 0777);\n \n+\titer = dir_iterator_begin(src->buf, 1);\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\tstrbuf_addstr(dest, iter->relative_path);\n+\n+\t\t/*\n+\t\t * dir_iterator_advance already calls lstat to populate iter->st\n+\t\t * but, unlike stat, lstat does not checks for permissions on\n+\t\t * the given path.\n+\t\t */\n+\t\tif (stat(src->buf, &st)) {\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+\n+\t\tif (S_ISDIR(iter->st.st_mode)) {\n+\t\t\tif (iter->relative_path[0] == '.')\n+\t\t\t\tcontinue;\n+\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@@ -456,14 +463,18 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\tif (!option_no_hardlinks) {\n \t\t\tif (!link(src->buf, dest->buf))\n \t\t\t\tcontinue;\n-\t\t\tif (option_local > 0)\n-\t\t\t\tdie_errno(_(\"failed to create link '%s'\"), dest->buf);\n+\t\t\tif (option_local > 0 && errno != ENOENT)\n+\t\t\t\twarning_errno(_(\"failed to create link '%s'\"), dest->buf);\n \t\t\toption_no_hardlinks = 1;\n \t\t}\n-\t\tif (copy_file_with_time(dest->buf, src->buf, 0666))\n+\t\tif (copy_file_with_time(dest->buf, src->buf, 0666) && errno != ENOENT)\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@@ -481,7 +492,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n \t\tget_common_dir(&dest, dest_repo);\n \t\tstrbuf_addstr(&src, \"/objects\");\n \t\tstrbuf_addstr(&dest, \"/objects\");\n-\t\tcopy_or_link_directory(&src, &dest, src_repo, src.len);\n+\t\tcopy_or_link_directory(&src, &dest, src_repo);\n \t\tstrbuf_release(&src);\n \t\tstrbuf_release(&dest);\n \t}\n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370193","messageId":"20190226002625.13022-8-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 7/7] clone: break cloning repos that have symlinks in them","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:25Z","receivedAt":"2019-02-26T00:26:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Revert back to the lstat() behavior in the dir_iterator interface\nwe've had since it was added in 0fe5043dad (\"dir_iterator: new API for\niterating over a directory tree\", 2016-06-18).\n\nNo reflog test depends on it, so it's unclear if it's needed. Since\nclone now uses this it changes the longstanding behavior of how we\njust so happened to support objects dirs with symlinks in them.\n\nWhy? I don't know. Just writing this up as an RFC with tests. Insert\nrationale here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n dir-iterator.c | 2 +-\n t/t5604-clone-reference.sh | 45 +++++++-------------------------------\n 2 files changed, 9 insertions(+), 38 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 6a9c0c4d08..070a656555 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -149,7 +149,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\tcontinue;\n \n \t\t\tstrbuf_addstr(&iter->base.path, de->d_name);\n-\t\t\tif (stat(iter->base.path.buf, &iter->base.st) < 0) {\n+\t\t\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n \t\t\t\tif (errno != ENOENT) {\n \t\t\t\t\tif (iter->pedantic)\n \t\t\t\t\t\tgoto error_out;\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex f1a8e74c44..a4cd12643e 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -249,45 +249,16 @@ test_expect_success SHA1,SYMLINKS 'setup repo with manually symlinked objects/*'\n '\n \n test_expect_success SHA1,SYMLINKS 'clone repo with manually symlinked objects/*' '\n-\tfor option in --local --no-hardlinks --shared --dissociate\n+\tfor option in --local --no-hardlinks --dissociate\n \tdo\n-\t\tgit clone $option S S$option || return 1 &&\n-\t\tgit -C S$option fsck || return 1\n+\t\ttest_must_fail git clone $option S S$option 2>err || return 1 &&\n+\t\ttest_i18ngrep \"the remote end hung up\" err || return 1\n \tdone &&\n-\tfind S-* -type l | sort >actual &&\n-\tcat >expected <<-EOF &&\n-\tS--dissociate/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78\n-\tS--local/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78\n-\tEOF\n-\ttest_cmp expected actual &&\n-\tfind S-* -name \"*some*\" | sort >actual &&\n-\tcat >expected <<-EOF &&\n-\tS--dissociate/.git/objects/.some-hidden-dir\n-\tS--dissociate/.git/objects/.some-hidden-dir/.some-dot-file\n-\tS--dissociate/.git/objects/.some-hidden-dir/some-file\n-\tS--dissociate/.git/objects/.some-hidden-file\n-\tS--dissociate/.git/objects/some-dir\n-\tS--dissociate/.git/objects/some-dir/.some-dot-file\n-\tS--dissociate/.git/objects/some-dir/some-file\n-\tS--dissociate/.git/objects/some-file\n-\tS--local/.git/objects/.some-hidden-dir\n-\tS--local/.git/objects/.some-hidden-dir/.some-dot-file\n-\tS--local/.git/objects/.some-hidden-dir/some-file\n-\tS--local/.git/objects/.some-hidden-file\n-\tS--local/.git/objects/some-dir\n-\tS--local/.git/objects/some-dir/.some-dot-file\n-\tS--local/.git/objects/some-dir/some-file\n-\tS--local/.git/objects/some-file\n-\tS--no-hardlinks/.git/objects/.some-hidden-dir\n-\tS--no-hardlinks/.git/objects/.some-hidden-dir/.some-dot-file\n-\tS--no-hardlinks/.git/objects/.some-hidden-dir/some-file\n-\tS--no-hardlinks/.git/objects/.some-hidden-file\n-\tS--no-hardlinks/.git/objects/some-dir\n-\tS--no-hardlinks/.git/objects/some-dir/.some-dot-file\n-\tS--no-hardlinks/.git/objects/some-dir/some-file\n-\tS--no-hardlinks/.git/objects/some-file\n-\tEOF\n-\ttest_cmp expected actual\n+\tgit clone --shared S S--shared &&\n+\tfind S--shared -type l | sort >actual &&\n+\ttest_must_be_empty actual &&\n+\tfind S--shared -name \"*some*\" | sort >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_done\n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370194","messageId":"20190226002625.13022-7-avarab@gmail.com","threadId":"50572","inReplyTo":"CACsJy8Bw-mXLyT7VaKBjYKLe6uSAQqyLG=_AzjDXykYzSQLkcg@mail.gmail.com","subject":"[WIP RFC PATCH 6/7] clone: stop ignoring dotdirs in --local etc. clone","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T00:26:24Z","receivedAt":"2019-02-26T00:27:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This seems to never have been intentional, just a side-effect of how\nthe existing code was written.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/clone.c | 8 +++-----\n t/t5604-clone-reference.sh | 9 +++++++++\n 2 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c32e9022b3..515dc91d63 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -446,8 +446,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\t}\n \n \t\tif (S_ISDIR(iter->st.st_mode)) {\n-\t\t\tif (iter->relative_path[0] == '.')\n-\t\t\t\tcontinue;\n \t\t\tmkdir_if_missing(dest->buf, 0777);\n \t\t\tcontinue;\n \t\t}\n@@ -463,11 +461,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\tif (!option_no_hardlinks) {\n \t\t\tif (!link(src->buf, dest->buf))\n \t\t\t\tcontinue;\n-\t\t\tif (option_local > 0 && errno != ENOENT)\n-\t\t\t\twarning_errno(_(\"failed to create link '%s'\"), dest->buf);\n+\t\t\tif (option_local > 0)\n+\t\t\t\tdie_errno(_(\"failed to create link '%s'\"), dest->buf);\n \t\t\toption_no_hardlinks = 1;\n \t\t}\n-\t\tif (copy_file_with_time(dest->buf, src->buf, 0666) && errno != ENOENT)\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 \ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 6f9c77049e..f1a8e74c44 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -262,16 +262,25 @@ test_expect_success SHA1,SYMLINKS 'clone repo with manually symlinked objects/*'\n \ttest_cmp expected actual &&\n \tfind S-* -name \"*some*\" | sort >actual &&\n \tcat >expected <<-EOF &&\n+\tS--dissociate/.git/objects/.some-hidden-dir\n+\tS--dissociate/.git/objects/.some-hidden-dir/.some-dot-file\n+\tS--dissociate/.git/objects/.some-hidden-dir/some-file\n \tS--dissociate/.git/objects/.some-hidden-file\n \tS--dissociate/.git/objects/some-dir\n \tS--dissociate/.git/objects/some-dir/.some-dot-file\n \tS--dissociate/.git/objects/some-dir/some-file\n \tS--dissociate/.git/objects/some-file\n+\tS--local/.git/objects/.some-hidden-dir\n+\tS--local/.git/objects/.some-hidden-dir/.some-dot-file\n+\tS--local/.git/objects/.some-hidden-dir/some-file\n \tS--local/.git/objects/.some-hidden-file\n \tS--local/.git/objects/some-dir\n \tS--local/.git/objects/some-dir/.some-dot-file\n \tS--local/.git/objects/some-dir/some-file\n \tS--local/.git/objects/some-file\n+\tS--no-hardlinks/.git/objects/.some-hidden-dir\n+\tS--no-hardlinks/.git/objects/.some-hidden-dir/.some-dot-file\n+\tS--no-hardlinks/.git/objects/.some-hidden-dir/some-file\n \tS--no-hardlinks/.git/objects/.some-hidden-file\n \tS--no-hardlinks/.git/objects/some-dir\n \tS--no-hardlinks/.git/objects/some-dir/.some-dot-file\n-- \n2.21.0.rc2.1.g2d5e20a900.dirty\n\n"},{"id":"370196","messageId":"CAHd-oW5=BoP6=Ti7y=_rugrRnQJ2yBrFqwq_-fjP7UfcNDoxzw@mail.gmail.com","threadId":"50572","inReplyTo":"20190226002625.13022-3-avarab@gmail.com","subject":"Re: [WIP RFC PATCH 2/7] dir-iterator: use stat() instead of lstat()","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-26T01:53:30Z","receivedAt":"2019-02-26T01:53:44Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Feb 25, 2019 at 9:26 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> This is surely a horrible idea, but all tests pass with this (not that\n> I trust them much). Doing this for later WIP use in clone.c.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> dir-iterator.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/dir-iterator.c b/dir-iterator.c\n> index 070a656555..6a9c0c4d08 100644\n> --- a/dir-iterator.c\n> +++ b/dir-iterator.c\n> @@ -149,7 +149,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> continue;\n>\n> strbuf_addstr(&iter->base.path, de->d_name);\n> - if (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n> + if (stat(iter->base.path.buf, &iter->base.st) < 0) {\n\nI think this may have side-effects on other sections that uses the\ndir-iterator API, because now it would follow symlinks, right?\n\n> if (errno != ENOENT) {\n> if (iter->pedantic)\n> goto error_out;\n> --\n> 2.21.0.rc2.1.g2d5e20a900.dirty\n>\n"},{"id":"370200","messageId":"CAHd-oW7HvhkORZogjb0nPioe3KWmXDDACF6Tu-Kmayc7=1z7nA@mail.gmail.com","threadId":"50572","inReplyTo":"20190226002625.13022-6-avarab@gmail.com","subject":"Re: [WIP RFC PATCH 5/7] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-02-26T03:48:44Z","receivedAt":"2019-02-26T03:48:59Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Feb 25, 2019 at 9:26 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> From: Matheus Tavares \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. This simplifies the code and avoid recursive calls to\n> copy_or_link_directory.\n>\n> [Ævar: This should be bug-compatible with the existing \"clone\"\n> behavior. The whole bit here with \"iter->relative_path[0] == '.'\" is a\n> dirty hack. We don't copy dot-dirs, and then later on just blindly\n> ignore ENOENT errors as we descend into them. That case really wants\n> to be a is_dotdir_or_file_within() test instead]\n>\n> Now, copy_or_link_directory will call die() in case of an error on\n> openddir, readdir or lstat, inside dir_iterator_advance. That means it\n> will abort in case of an error trying to fetch any iteration entry.\n>\n> Signed-off-by: Matheus Tavares \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/clone.c | 55 +++++++++++++++++++++++++++++--------------------\n> 1 file changed, 33 insertions(+), 22 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 862d2ea69c..c32e9022b3 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> @@ -411,42 +413,47 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\n> }\n>\n> static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> - const char *src_repo, int src_baselen)\n> + const char *src_repo)\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> + struct stat st;\n>\n> mkdir_if_missing(dest->buf, 0777);\n>\n> + iter = dir_iterator_begin(src->buf, 1);\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> + strbuf_addstr(dest, iter->relative_path);\n> +\n> + /*\n> + * dir_iterator_advance already calls lstat to populate iter->st\n> + * but, unlike stat, lstat does not checks for permissions on\n> + * the given path.\n> + */\n> + if (stat(src->buf, &st)) {\n> warning (_(\"failed to stat %s\\n\"), src->buf);\n> continue;\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> +\n> + if (S_ISDIR(iter->st.st_mode)) {\n> + if (iter->relative_path[0] == '.')\n\nI think it should be iter->basename[0] here, instead, right?\n\nI also have a more conceptual question here: This additions (or the\nis_dotdir_of_file_within as suggested) are just to make patch\ncompatible with the current behaviour, but are going to be removed\nsoon after. As this would be kind of a noise, wouldn't it be better to\nhave a patch before this, already correcting copy_or_link_directory's\nbehaviour on hidden dirs and them this?\n\n> + continue;\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> @@ -456,14 +463,18 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> if (!option_no_hardlinks) {\n> if (!link(src->buf, dest->buf))\n> continue;\n> - if (option_local > 0)\n> - die_errno(_(\"failed to create link '%s'\"), dest->buf);\n> + if (option_local > 0 && errno != ENOENT)\n> + warning_errno(_(\"failed to create link '%s'\"), dest->buf);\n> option_no_hardlinks = 1;\n> }\n> - if (copy_file_with_time(dest->buf, src->buf, 0666))\n> + if (copy_file_with_time(dest->buf, src->buf, 0666) && errno != ENOENT)\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>\n> static void clone_local(const char *src_repo, const char *dest_repo)\n> @@ -481,7 +492,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n> get_common_dir(&dest, dest_repo);\n> strbuf_addstr(&src, \"/objects\");\n> strbuf_addstr(&dest, \"/objects\");\n> - copy_or_link_directory(&src, &dest, src_repo, src.len);\n> + copy_or_link_directory(&src, &dest, src_repo);\n> strbuf_release(&src);\n> strbuf_release(&dest);\n> }\n> --\n> 2.21.0.rc2.1.g2d5e20a900.dirty\n>\n"},{"id":"370214","messageId":"CAP8UFD1++Uy=FA-Y6jpH-4DgpqxvPnvQXQ9DxC6QxXKBPkA+5w@mail.gmail.com","threadId":"50572","inReplyTo":"CAHd-oW4P-aAKH0_kKrOJeprWmvPCUfa++X3pSNYSQisAV-25HA@mail.gmail.com","subject":"Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-26T10:33:44Z","receivedAt":"2019-02-26T10:33:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Matheus,\n\nOn Mon, Feb 25, 2019 at 3:31 AM Matheus Tavares Bernardino\n wrote:\n>\n> I am a little confused about what I should do next. How should I\n> proceed with this series?\n\nWhen there are different opinions about what you should do, I would\nsuggest doing only things everyone is ok with, and then resending\nsaying in the cover letter that such and such issues still need to be\ndecided. Then hopefully either Junio will decide or a consensus will\neventually appear about what you should do.\n\nIn any case please don't wait until something is decided before\nresending with improvements (or just pinging us if you have already\nmade all the improvements that were suggested) as that may not happen.\nIf people on the list stop hearing about your patch series, they might\njust forget about it.\n\nThanks,\nChristian.\n"},{"id":"370219","messageId":"87y362vl6p.fsf@evledraar.gmail.com","threadId":"50572","inReplyTo":"CAHd-oW7HvhkORZogjb0nPioe3KWmXDDACF6Tu-Kmayc7=1z7nA@mail.gmail.com","subject":"Re: [WIP RFC PATCH 5/7] clone: use dir-iterator to avoid explicit dir traversal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-26T11:33:18Z","receivedAt":"2019-02-26T11:33:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 26 2019, Matheus Tavares Bernardino wrote:\n\n> On Mon, Feb 25, 2019 at 9:26 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> From: Matheus Tavares \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. This simplifies the code and avoid recursive calls to\n>> copy_or_link_directory.\n>>\n>> [Ævar: This should be bug-compatible with the existing \"clone\"\n>> behavior. The whole bit here with \"iter->relative_path[0] == '.'\" is a\n>> dirty hack. We don't copy dot-dirs, and then later on just blindly\n>> ignore ENOENT errors as we descend into them. That case really wants\n>> to be a is_dotdir_or_file_within() test instead]\n>>\n>> Now, copy_or_link_directory will call die() in case of an error on\n>> openddir, readdir or lstat, inside dir_iterator_advance. That means it\n>> will abort in case of an error trying to fetch any iteration entry.\n>>\n>> Signed-off-by: Matheus Tavares \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/clone.c | 55 +++++++++++++++++++++++++++++--------------------\n>> 1 file changed, 33 insertions(+), 22 deletions(-)\n>>\n>> diff --git a/builtin/clone.c b/builtin/clone.c\n>> index 862d2ea69c..c32e9022b3 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>> @@ -411,42 +413,47 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)\n>> }\n>>\n>> static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n>> - const char *src_repo, int src_baselen)\n>> + const char *src_repo)\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>> + struct stat st;\n>>\n>> mkdir_if_missing(dest->buf, 0777);\n>>\n>> + iter = dir_iterator_begin(src->buf, 1);\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>> + strbuf_addstr(dest, iter->relative_path);\n>> +\n>> + /*\n>> + * dir_iterator_advance already calls lstat to populate iter->st\n>> + * but, unlike stat, lstat does not checks for permissions on\n>> + * the given path.\n>> + */\n>> + if (stat(src->buf, &st)) {\n>> warning (_(\"failed to stat %s\\n\"), src->buf);\n>> continue;\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>> +\n>> + if (S_ISDIR(iter->st.st_mode)) {\n>> + if (iter->relative_path[0] == '.')\n>\n> I think it should be iter->basename[0] here, instead, right?\n\nYeah, sounds better.\n\n> I also have a more conceptual question here: This additions (or the\n> is_dotdir_of_file_within as suggested) are just to make patch\n> compatible with the current behaviour, but are going to be removed\n> soon after.\n\nYes. it's not an endorsement of the current behavior, but for ease of\nreview. I.e. to split changes into smaller logical ones ones that are\nrefactoring on the one hand, and behavior changes on the other.\n\n> As this would be kind of a noise, wouldn't it be better to have a\n> patch before this, already correcting copy_or_link_directory's\n> behaviour on hidden dirs and them this?\n\nYeah, maybe structuring it like that is more readable.\n\n\n>> + continue;\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>> @@ -456,14 +463,18 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n>> if (!option_no_hardlinks) {\n>> if (!link(src->buf, dest->buf))\n>> continue;\n>> - if (option_local > 0)\n>> - die_errno(_(\"failed to create link '%s'\"), dest->buf);\n>> + if (option_local > 0 && errno != ENOENT)\n>> + warning_errno(_(\"failed to create link '%s'\"), dest->buf);\n>> option_no_hardlinks = 1;\n>> }\n>> - if (copy_file_with_time(dest->buf, src->buf, 0666))\n>> + if (copy_file_with_time(dest->buf, src->buf, 0666) && errno != ENOENT)\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>>\n>> static void clone_local(const char *src_repo, const char *dest_repo)\n>> @@ -481,7 +492,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n>> get_common_dir(&dest, dest_repo);\n>> strbuf_addstr(&src, \"/objects\");\n>> strbuf_addstr(&dest, \"/objects\");\n>> - copy_or_link_directory(&src, &dest, src_repo, src.len);\n>> + copy_or_link_directory(&src, &dest, src_repo);\n>> strbuf_release(&src);\n>> strbuf_release(&dest);\n>> }\n>> --\n>> 2.21.0.rc2.1.g2d5e20a900.dirty\n>>\n"}]}