{"thread":{"id":"45544","subject":"[PATCH v5 0/6] [GSoC] remove_subtree(): reimplement using iterators","startedAt":"2017-03-30T03:32:39Z","lastAt":"2017-04-01T17:16:41Z","messageCount":19,"participants":["Daniel Ferreira","Michael Haggerty","Duy Nguyen","Daniel Ferreira (theiostream)","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":6},"messages":[{"id":"315738","messageId":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":null,"subject":"[PATCH v5 0/6] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:04Z","receivedAt":"2017-03-30T03:32:39Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"This is the fifth version of a patch series that implements the GSoC\nmicroproject of converting a recursive call to readdir() to use dir_iterator.\n\nv1: https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com/T/#t\nv2: https://public-inbox.org/git/CACsJy8Dxh-QPBBLfaFWPAWUsbA9GVXA7x+mXLjEvYKhk1zOpig@mail.gmail.com/T/#t\nv3: https://public-inbox.org/git/CAGZ79kYtpmURSQWPumobA=e3JBFjKhWCdv_LPhKCd71ZRwMovA@mail.gmail.com/T/#t\nv4: https://public-inbox.org/git/1490747533-89143-1-git-send-email-bnmvco@gmail.com/T/#e437a63e0c22c00c69b5d92977c9b438ed2b9fd3a\n\nI would like to really thank Michael for the incredibly thorough review of\nthe last version of this series. I never expected anyone to give that\nlevel of attention to this change, and it's really, really appreciated.\n\nAll of the points he addressed are fixed in this version. As always, more\nfeedback is greatly appreciated.\n\nThanks,\nDaniel.\n\nDaniel Ferreira (6):\n  dir_iterator: add helpers to dir_iterator_advance\n  dir_iterator: refactor state machine model\n  dir_iterator: iterate over dir after its contents\n  dir_iterator: add tests for dir_iterator API\n  remove_subtree(): reimplement using iterators\n  remove_subtree(): test removing nested directories\n\n Makefile                        |   1 +\n dir-iterator.c                  | 123 ++++++++++++++++++++++++++++++----------\n dir-iterator.h                  |  17 ++++--\n entry.c                         |  41 +++++---------\n refs/files-backend.c            |   2 +-\n t/helper/test-dir-iterator.c    |  32 +++++++++++\n t/t0065-dir-iterator.sh         |  45 +++++++++++++++\n t/t2000-checkout-cache-clash.sh |  11 ++++\n 8 files changed, 210 insertions(+), 62 deletions(-)\n create mode 100644 t/helper/test-dir-iterator.c\n create mode 100755 t/t0065-dir-iterator.sh\n\n--\n2.7.4 (Apple Git-66)\n\n"},{"id":"315739","messageId":"1490844730-47634-2-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v5 1/6] dir_iterator: add helpers to dir_iterator_advance","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:05Z","receivedAt":"2017-03-30T03:32:41Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Create inline helpers to dir_iterator_advance(). Make\ndir_iterator_advance()'s code more legible and allow some behavior to\nbe reusable.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n dir-iterator.c | 65 +++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 42 insertions(+), 23 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 34182a9..ce8bf81 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -50,6 +50,43 @@ struct dir_iterator_int {\n \tstruct dir_iterator_level *levels;\n };\n\n+static inline void push_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level)\n+{\n+\tlevel->dir_state = DIR_STATE_RECURSE;\n+\tALLOC_GROW(iter->levels, iter->levels_nr + 1,\n+\t\t   iter->levels_alloc);\n+\tlevel = &iter->levels[iter->levels_nr++];\n+\tlevel->initialized = 0;\n+}\n+\n+static inline int pop_dir_level(struct dir_iterator_int *iter)\n+{\n+\treturn --iter->levels_nr;\n+}\n+\n+static inline int set_iterator_data(struct dir_iterator_int *iter, struct dir_iterator_level *level)\n+{\n+\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarning(\"error reading path '%s': %s\",\n+\t\t\t\titer->base.path.buf,\n+\t\t\t\tstrerror(errno));\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * We have to set these each time because\n+\t * the path strbuf might have been realloc()ed.\n+\t */\n+\titer->base.relative_path =\n+\t\titer->base.path.buf + iter->levels[0].prefix_len;\n+\titer->base.basename =\n+\t\titer->base.path.buf + level->prefix_len;\n+\tlevel->dir_state = DIR_STATE_ITER;\n+\n+\treturn 0;\n+}\n+\n int dir_iterator_advance(struct dir_iterator *dir_iterator)\n {\n \tstruct dir_iterator_int *iter =\n@@ -84,11 +121,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\t * over; now prepare to iterate into\n \t\t\t\t * it.\n \t\t\t\t */\n-\t\t\t\tlevel->dir_state = DIR_STATE_RECURSE;\n-\t\t\t\tALLOC_GROW(iter->levels, iter->levels_nr + 1,\n-\t\t\t\t\t   iter->levels_alloc);\n-\t\t\t\tlevel = &iter->levels[iter->levels_nr++];\n-\t\t\t\tlevel->initialized = 0;\n+\t\t\t\tpush_dir_level(iter, level);\n \t\t\t\tcontinue;\n \t\t\t} else {\n \t\t\t\t/*\n@@ -104,7 +137,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t * This level is exhausted (or wasn't opened\n \t\t\t * successfully); pop up a level.\n \t\t\t */\n-\t\t\tif (--iter->levels_nr == 0)\n+\t\t\tif (pop_dir_level(iter) == 0)\n \t\t\t\treturn dir_iterator_abort(dir_iterator);\n\n \t\t\tcontinue;\n@@ -129,7 +162,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n\n \t\t\t\tlevel->dir = NULL;\n-\t\t\t\tif (--iter->levels_nr == 0)\n+\t\t\t\tif (pop_dir_level(iter) == 0)\n \t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n \t\t\t\tbreak;\n \t\t\t}\n@@ -138,23 +171,9 @@ 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\t\tif (errno != ENOENT)\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\tcontinue;\n-\t\t\t}\n\n-\t\t\t/*\n-\t\t\t * We have to set these each time because\n-\t\t\t * the path strbuf might have been realloc()ed.\n-\t\t\t */\n-\t\t\titer->base.relative_path =\n-\t\t\t\titer->base.path.buf + iter->levels[0].prefix_len;\n-\t\t\titer->base.basename =\n-\t\t\t\titer->base.path.buf + level->prefix_len;\n-\t\t\tlevel->dir_state = DIR_STATE_ITER;\n+\t\t\tif (set_iterator_data(iter, level))\n+\t\t\t\tcontinue;\n\n \t\t\treturn ITER_OK;\n \t\t}\n--\n2.7.4 (Apple Git-66)\n\n"},{"id":"315740","messageId":"1490844730-47634-3-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v5 2/6] dir_iterator: refactor state machine model","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:06Z","receivedAt":"2017-03-30T03:32:42Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Remove the \"initialized\" member of dir_iterator_level. Replace its\nfunctionality with a DIR_STATE_PUSH state in the\ndir_iterator_level.dir_state enum.\n\nThis serves to remove a redundant property in the dir_iterator_level\nstruct and ease comprehension of the state machine's behavior.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n dir-iterator.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex ce8bf81..3ac984b 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -4,8 +4,6 @@\n #include \"dir-iterator.h\"\n\n struct dir_iterator_level {\n-\tint initialized;\n-\n \tDIR *dir;\n\n \t/*\n@@ -20,6 +18,7 @@ struct dir_iterator_level {\n \t * iteration and also iterated into):\n \t */\n \tenum {\n+\t\tDIR_STATE_PUSH,\n \t\tDIR_STATE_ITER,\n \t\tDIR_STATE_RECURSE\n \t} dir_state;\n@@ -55,8 +54,10 @@ static inline void push_dir_level(struct dir_iterator_int *iter, struct dir_iter\n \tlevel->dir_state = DIR_STATE_RECURSE;\n \tALLOC_GROW(iter->levels, iter->levels_nr + 1,\n \t\t   iter->levels_alloc);\n+\n+\t/* Push a new level */\n \tlevel = &iter->levels[iter->levels_nr++];\n-\tlevel->initialized = 0;\n+\tlevel->dir_state = DIR_STATE_PUSH;\n }\n\n static inline int pop_dir_level(struct dir_iterator_int *iter)\n@@ -97,7 +98,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t&iter->levels[iter->levels_nr - 1];\n \t\tstruct dirent *de;\n\n-\t\tif (!level->initialized) {\n+\t\tif (level->dir_state == DIR_STATE_PUSH) {\n \t\t\t/*\n \t\t\t * Note: dir_iterator_begin() ensures that\n \t\t\t * path is not the empty string.\n@@ -112,8 +113,6 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\t\titer->base.path.buf, strerror(errno));\n \t\t\t\t/* Popping the level is handled below */\n \t\t\t}\n-\n-\t\t\tlevel->initialized = 1;\n \t\t} else if (S_ISDIR(iter->base.st.st_mode)) {\n \t\t\tif (level->dir_state == DIR_STATE_ITER) {\n \t\t\t\t/*\n@@ -215,7 +214,7 @@ struct dir_iterator *dir_iterator_begin(const char *path)\n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n\n \titer->levels_nr = 1;\n-\titer->levels[0].initialized = 0;\n+\titer->levels[0].dir_state = DIR_STATE_PUSH;\n\n \treturn dir_iterator;\n }\n--\n2.7.4 (Apple Git-66)\n\n"},{"id":"315741","messageId":"1490844730-47634-4-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v5 3/6] dir_iterator: iterate over dir after its contents","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:07Z","receivedAt":"2017-03-30T03:32:44Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Create an option for the dir_iterator API to iterate over subdirectories\nonly after having iterated through their contents. This feature was\npredicted, although not implemented by 0fe5043 (\"dir_iterator: new API\nfor iterating over a directory tree\", 2016-06-18).\n\nAdd the \"flags\" parameter to dir_iterator_create, allowing for the\naforementioned \"depth-first\" iteration mode to be enabled. Currently,\nthe only acceptable flag is DIR_ITERATOR_DEPTH_FIRST.\n\nThis is useful for recursively removing a directory and calling rmdir()\non a directory only after all of its contents have been wiped.\n\nAmend a call to dir_iterator_begin() to pass the flags parameter\nintroduced.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n dir-iterator.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++----\n dir-iterator.h       | 17 ++++++++++++-----\n refs/files-backend.c |  2 +-\n 3 files changed, 62 insertions(+), 10 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 3ac984b..05d53d2 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -47,6 +47,9 @@ struct dir_iterator_int {\n \t * that will be included in this iteration.\n \t */\n \tstruct dir_iterator_level *levels;\n+\n+\t/* Holds the flags passed to dir_iterator_begin(). */\n+\tunsigned flags;\n };\n \n static inline void push_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level)\n@@ -113,12 +116,14 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\t\titer->base.path.buf, strerror(errno));\n \t\t\t\t/* Popping the level is handled below */\n \t\t\t}\n-\t\t} else if (S_ISDIR(iter->base.st.st_mode)) {\n+\t\t} else if (S_ISDIR(iter->base.st.st_mode) &&\n+\t\t\t!(iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL)) {\n \t\t\tif (level->dir_state == DIR_STATE_ITER) {\n \t\t\t\t/*\n \t\t\t\t * The directory was just iterated\n \t\t\t\t * over; now prepare to iterate into\n-\t\t\t\t * it.\n+\t\t\t\t * it (unless an option is set for us\n+\t\t\t\t * to do otherwise).\n \t\t\t\t */\n \t\t\t\tpush_dir_level(iter, level);\n \t\t\t\tcontinue;\n@@ -152,7 +157,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\tde = readdir(level->dir);\n \n \t\t\tif (!de) {\n-\t\t\t\t/* This level is exhausted; pop up a level. */\n+\t\t\t\t/* This level is exhausted  */\n \t\t\t\tif (errno) {\n \t\t\t\t\twarning(\"error reading directory %s: %s\",\n \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n@@ -160,6 +165,32 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t\t\twarning(\"error closing directory %s: %s\",\n \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n \n+\t\t\t\tif (iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL) {\n+\t\t\t\t\t/* If we are handling dirpaths after their contents,\n+\t\t\t\t\t * we have to iterate over the directory now that we'll\n+\t\t\t\t\t * have finished iterating into it. */\n+\t\t\t\t\tlevel->dir = NULL;\n+\n+\t\t\t\t\tif (pop_dir_level(iter) == 0)\n+\t\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\n+\t\t\t\t\tlevel = &iter->levels[iter->levels_nr - 1];\n+\t\t\t\t\t/* Since we are iterating through the dirpath\n+\t\t\t\t\t * after we have gone through it, we still need\n+\t\t\t\t\t * to get rid of the trailing slash we appended.\n+\t\t\t\t\t *\n+\t\t\t\t\t * This may generate issues if we ever want to\n+\t\t\t\t\t * iterate through the root directory AND have\n+\t\t\t\t\t * post-order traversal enabled.\n+\t\t\t\t\t */\n+\t\t\t\t\tstrbuf_strip_suffix(&iter->base.path, \"/\");\n+\n+\t\t\t\t\tif (set_iterator_data(iter, level))\n+\t\t\t\t\t\tcontinue;\n+\n+\t\t\t\t\treturn ITER_OK;\n+\t\t\t\t}\n+\n \t\t\t\tlevel->dir = NULL;\n \t\t\t\tif (pop_dir_level(iter) == 0)\n \t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n@@ -174,6 +205,18 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\tif (set_iterator_data(iter, level))\n \t\t\t\tcontinue;\n \n+\t\t\t/*\n+\t\t\t * If we want to iterate dirs after files, we shall\n+\t\t\t * begin looking into them *before* we return the dir\n+\t\t\t * itself.\n+\t\t\t */\n+\t\t\tif (S_ISDIR(iter->base.st.st_mode) &&\n+\t\t\t\t(iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL)) {\n+\t\t\t\tpush_dir_level(iter, level);\n+\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n \t\t\treturn ITER_OK;\n \t\t}\n \t}\n@@ -200,7 +243,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, unsigned flags)\n {\n \tstruct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));\n \tstruct dir_iterator *dir_iterator = &iter->base;\n@@ -208,6 +251,8 @@ struct dir_iterator *dir_iterator_begin(const char *path)\n \tif (!path || !*path)\n \t\tdie(\"BUG: empty path passed to dir_iterator_begin()\");\n \n+\titer->flags = flags;\n+\n \tstrbuf_init(&iter->base.path, PATH_MAX);\n \tstrbuf_addstr(&iter->base.path, path);\n \ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 27739e6..649ccf6 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -11,8 +11,7 @@\n  * Every time dir_iterator_advance() is called, update the members of\n  * the dir_iterator structure to reflect the next path in the\n  * iteration. The order that paths are iterated over within a\n- * directory is undefined, but directory paths are always iterated\n- * over before the subdirectory contents.\n+ * directory is undefined.\n  *\n  * A typical iteration looks like this:\n  *\n@@ -38,6 +37,13 @@\n  * dir_iterator_advance() again.\n  */\n \n+/* Possible flags for dir_iterator_begin().\n+ *\n+ * DIR_ITERATOR_POST_ORDER_TRAVERSAL: ensures subdirectories and\n+ * their contents are iterated through before the containing directory.\n+ */\n+#define DIR_ITERATOR_POST_ORDER_TRAVERSAL (1 << 0)\n+\n struct dir_iterator {\n \t/* The current path: */\n \tstruct strbuf path;\n@@ -57,15 +63,16 @@ struct dir_iterator {\n };\n \n /*\n- * Start a directory iteration over path. Return a dir_iterator that\n- * holds the internal state of the iteration.\n+ * Start a directory iteration over path, with options specified in\n+ * 'flags'. Return a dir_iterator that holds the internal state of\n+ * the iteration.\n  *\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  */\n-struct dir_iterator *dir_iterator_begin(const char *path);\n+struct dir_iterator *dir_iterator_begin(const char *path, unsigned flags);\n \n /*\n  * Advance the iterator to the first or next item and return ITER_OK.\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 50188e9..b4bba74 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3346,7 +3346,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \tfiles_downcast(ref_store, 0, \"reflog_iterator_begin\");\n \n \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n-\titer->dir_iterator = dir_iterator_begin(git_path(\"logs\"));\n+\titer->dir_iterator = dir_iterator_begin(git_path(\"logs\"), 0);\n \treturn ref_iterator;\n }\n \n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"315742","messageId":"1490844730-47634-5-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:08Z","receivedAt":"2017-03-30T03:32:46Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Create t/helper/test-dir-iterator.c, which prints relevant information\nabout a directory tree iterated over with dir_iterator.\n\nCreate t/t0065-dir-iterator.sh, which tests that dir_iterator does\niterate through a whole directory tree and that post-order directory\niteration is correctly implemented.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n Makefile                     |  1 +\n t/helper/test-dir-iterator.c | 32 +++++++++++++++++++++++++++++++\n t/t0065-dir-iterator.sh      | 45 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 78 insertions(+)\n create mode 100644 t/helper/test-dir-iterator.c\n create mode 100755 t/t0065-dir-iterator.sh\n\ndiff --git a/Makefile b/Makefile\nindex a5a11e7..d0245f3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -607,6 +607,7 @@ TEST_PROGRAMS_NEED_X += test-ctype\n TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n+TEST_PROGRAMS_NEED_X += test-dir-iterator\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\n TEST_PROGRAMS_NEED_X += test-dump-split-index\n TEST_PROGRAMS_NEED_X += test-dump-untracked-cache\ndiff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\nnew file mode 100644\nindex 0000000..b4a148f\n--- /dev/null\n+++ b/t/helper/test-dir-iterator.c\n@@ -0,0 +1,32 @@\n+#include \"cache.h\"\n+#include \"blob.h\"\n+#include \"dir.h\"\n+#include \"streaming.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n+\n+int cmd_main(int argc, const char **argv) {\n+\tif (argc < 2) {\n+\t\treturn 1;\n+\t}\n+\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstrbuf_add(&path, argv[1], strlen(argv[1]));\n+\n+\tunsigned flag = 0;\n+\tif (argc == 3 && strcmp(argv[2], \"--post-order\") == 0)\n+\t\tflag = DIR_ITERATOR_POST_ORDER_TRAVERSAL;\n+\n+\tstruct dir_iterator *diter = dir_iterator_begin((&path)->buf, flag);\n+\n+\twhile (dir_iterator_advance(diter) == ITER_OK) {\n+\t\tif (S_ISDIR(diter->st.st_mode))\n+\t\t\tprintf(\"[d] \");\n+\t\telse\n+\t\t\tprintf(\"[f] \");\n+\n+\t\tprintf(\"(%s) %s\\n\", diter->relative_path, diter->path.buf);\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh\nnew file mode 100755\nindex 0000000..3c8ea9a\n--- /dev/null\n+++ b/t/t0065-dir-iterator.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+\n+test_description='Test directory iteration.'\n+\n+. ./test-lib.sh\n+\n+ITER_SORTED_OUTPUT='[d] (a) ./dir/a\n+[d] (a/b) ./dir/a/b\n+[d] (a/b/c) ./dir/a/b/c\n+[d] (d) ./dir/d\n+[d] (d/e) ./dir/d/e\n+[d] (d/e/d) ./dir/d/e/d\n+[f] (a/b/c/d) ./dir/a/b/c/d\n+[f] (a/e) ./dir/a/e\n+[f] (b) ./dir/b\n+[f] (c) ./dir/c\n+[f] (d/e/d/a) ./dir/d/e/d/a'\n+\n+test_expect_success 'dir-iterator should iterate through all files' '\n+\tmkdir -p dir &&\n+\tmkdir -p dir/a/b/c/ &&\n+\tdate >dir/b &&\n+\tdate >dir/c &&\n+\tmkdir -p dir/d/e/d/ &&\n+\tdate >dir/a/b/c/d &&\n+\tdate >dir/a/e &&\n+\tdate >dir/d/e/d/a &&\n+\n+\ttest-dir-iterator ./dir >it &&\n+\ttest \"$(sort it)\" == \"$ITER_SORTED_OUTPUT\"\n+'\n+\n+ITER_POST_ORDER_OUTPUT='[f] (a/b/c/d) ./dir2/a/b/c/d\n+[d] (a/b/c) ./dir2/a/b/c\n+[d] (a/b) ./dir2/a/b\n+[d] (a) ./dir2/a'\n+\n+test_expect_success 'dir-iterator should list files properly on post-order mode' '\n+\tmkdir -p dir2/a/b/c/ &&\n+\tdate >dir2/a/b/c/d &&\n+\n+\ttest \"$(test-dir-iterator ./dir2 --post-order)\" == \"$ITER_POST_ORDER_OUTPUT\"\n+'\n+\n+test_done\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"315743","messageId":"1490844730-47634-6-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v5 5/6] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:09Z","receivedAt":"2017-03-30T03:32:47Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"From: Daniel Ferreira <daniel.calibeta@gmail.com>\n\nUse dir_iterator to traverse through remove_subtree()'s directory tree,\navoiding the need for recursive calls to readdir(). Simplify\nremove_subtree()'s code.\n\nA conversion similar in purpose was previously done at 46d092a\n(\"for_each_reflog(): reimplement using iterators\", 2016-05-21).\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n entry.c | 41 +++++++++++++++--------------------------\n 1 file changed, 15 insertions(+), 26 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex c6eea24..30197b2 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -2,6 +2,8 @@\n #include \"blob.h\"\n #include \"dir.h\"\n #include \"streaming.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n\n static void create_directories(const char *path, int path_len,\n \t\t\t       const struct checkout *state)\n@@ -44,33 +46,20 @@ static void create_directories(const char *path, int path_len,\n \tfree(buf);\n }\n\n-static void remove_subtree(struct strbuf *path)\n+static void remove_subtree(const char *path)\n {\n-\tDIR *dir = opendir(path->buf);\n-\tstruct dirent *de;\n-\tint origlen = path->len;\n-\n-\tif (!dir)\n-\t\tdie_errno(\"cannot opendir '%s'\", path->buf);\n-\twhile ((de = readdir(dir)) != NULL) {\n-\t\tstruct stat st;\n-\n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n-\n-\t\tstrbuf_addch(path, '/');\n-\t\tstrbuf_addstr(path, de->d_name);\n-\t\tif (lstat(path->buf, &st))\n-\t\t\tdie_errno(\"cannot lstat '%s'\", path->buf);\n-\t\tif (S_ISDIR(st.st_mode))\n-\t\t\tremove_subtree(path);\n-\t\telse if (unlink(path->buf))\n-\t\t\tdie_errno(\"cannot unlink '%s'\", path->buf);\n-\t\tstrbuf_setlen(path, origlen);\n+\tstruct dir_iterator *diter = dir_iterator_begin(path, DIR_ITERATOR_POST_ORDER_TRAVERSAL);\n+\n+\twhile (dir_iterator_advance(diter) == ITER_OK) {\n+\t\tif (S_ISDIR(diter->st.st_mode)) {\n+\t\t\tif (rmdir(diter->path.buf))\n+\t\t\t\tdie_errno(\"cannot rmdir '%s'\", diter->path.buf);\n+\t\t} else if (unlink(diter->path.buf))\n+\t\t\tdie_errno(\"cannot unlink '%s'\", diter->path.buf);\n \t}\n-\tclosedir(dir);\n-\tif (rmdir(path->buf))\n-\t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n+\n+\tif (rmdir(path))\n+\t\tdie_errno(\"cannot rmdir '%s'\", path);\n }\n\n static int create_file(const char *path, unsigned int mode)\n@@ -282,7 +271,7 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\t\treturn 0;\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path.buf);\n-\t\t\tremove_subtree(&path);\n+\t\t\tremove_subtree(path.buf);\n \t\t} else if (unlink(path.buf))\n \t\t\treturn error_errno(\"unable to unlink old '%s'\", path.buf);\n \t} else if (state->not_new)\n--\n2.7.4 (Apple Git-66)\n\n"},{"id":"315744","messageId":"1490844730-47634-7-git-send-email-bnmvco@gmail.com","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v5 6/6] remove_subtree(): test removing nested directories","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T03:32:10Z","receivedAt":"2017-03-30T03:32:56Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Test removing a nested directory when an attempt is made to restore the\nindex to a state where it does not exist. A similar test could be found\npreviously in t/t2000-checkout-cache-clash.sh, but it did not check for\nnested directories, which could allow a faulty implementation of\nremove_subtree() pass the tests.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n t/t2000-checkout-cache-clash.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t2000-checkout-cache-clash.sh b/t/t2000-checkout-cache-clash.sh\nindex de3edb5..ac10ba3 100755\n--- a/t/t2000-checkout-cache-clash.sh\n+++ b/t/t2000-checkout-cache-clash.sh\n@@ -57,4 +57,15 @@ test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '\n \tgit checkout-index -a -f --prefix=there/\n '\n \n+test_expect_success 'git checkout-index -f should remove nested subtrees' '\n+\techo content >path &&\n+\tgit update-index --add path &&\n+\trm path &&\n+\tmkdir -p path/with/nested/paths &&\n+\techo content >path/file1 &&\n+\techo content >path/with/nested/paths/file2 &&\n+\tgit checkout-index -f -a &&\n+\ttest ! -d path\n+'\n+\n test_done\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"315755","messageId":"ab7a326c-0189-4542-76e8-649eb6e804c9@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-5-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T07:46:41Z","receivedAt":"2017-03-30T07:46:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> Create t/helper/test-dir-iterator.c, which prints relevant information\n> about a directory tree iterated over with dir_iterator.\n> \n> Create t/t0065-dir-iterator.sh, which tests that dir_iterator does\n> iterate through a whole directory tree and that post-order directory\n> iteration is correctly implemented.\n\nThanks for adding these tests (which really I should have added when I\nfirst introduced `dir_iterator`).\n\nAs a general rule, it's best to add tests:\n\n* of pre-existing functionality as soon as possible in a patch\n  series so that the tests can be used to check your own work,\n  commit by commit\n\n* of new functionality in the commit that adds the new functionality,\n  so that it is obvious that the new functionality works\n\n* of fixes to existing bugs *before* the bug fix, as a\n  `test_expect_failure` test, to prove that the bug exists and that\n  your test catches it. Then in a second commit, actually fix the bug\n  and change the test from `test_expect_failure` to\n  `test_expect_success`, to prove that your change fixed the bug.\n\nConcretely, for this patch series, that means that you should add part\nof the helper program and part of t0065 (i.e., everything that applies\nto pre-order iteration) as the first patch of your series. Then you\nshould add the rest in patch \"dir_iterator: iterate over dir after its\ncontents\", at the same time as you add post-order iteration.\n\n> ---\n>  Makefile                     |  1 +\n>  t/helper/test-dir-iterator.c | 32 +++++++++++++++++++++++++++++++\n>  t/t0065-dir-iterator.sh      | 45 ++++++++++++++++++++++++++++++++++++++++++++\n>  3 files changed, 78 insertions(+)\n>  create mode 100644 t/helper/test-dir-iterator.c\n>  create mode 100755 t/t0065-dir-iterator.sh\n> \n> diff --git a/Makefile b/Makefile\n> index a5a11e7..d0245f3 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -607,6 +607,7 @@ TEST_PROGRAMS_NEED_X += test-ctype\n>  TEST_PROGRAMS_NEED_X += test-config\n>  TEST_PROGRAMS_NEED_X += test-date\n>  TEST_PROGRAMS_NEED_X += test-delta\n> +TEST_PROGRAMS_NEED_X += test-dir-iterator\n>  TEST_PROGRAMS_NEED_X += test-dump-cache-tree\n>  TEST_PROGRAMS_NEED_X += test-dump-split-index\n>  TEST_PROGRAMS_NEED_X += test-dump-untracked-cache\n> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\n> new file mode 100644\n> index 0000000..b4a148f\n> --- /dev/null\n> +++ b/t/helper/test-dir-iterator.c\n> @@ -0,0 +1,32 @@\n> +#include \"cache.h\"\n> +#include \"blob.h\"\n> +#include \"dir.h\"\n> +#include \"streaming.h\"\n> +#include \"iterator.h\"\n> +#include \"dir-iterator.h\"\n\nThree of these includes are unneeded and should be removed.\n\n> +\n> +int cmd_main(int argc, const char **argv) {\n> +\tif (argc < 2) {\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstrbuf_add(&path, argv[1], strlen(argv[1]));\n> +\n> +\tunsigned flag = 0;\n> +\tif (argc == 3 && strcmp(argv[2], \"--post-order\") == 0)\n> +\t\tflag = DIR_ITERATOR_POST_ORDER_TRAVERSAL;\n\nIt's conventional to handle options *before* other arguments. Even\nthough this is only a test script, it's preferable to adhere to\nconvention unless there is a strong reason to deviate from it. You might\nlooks at `test-submodule-config.c` for example code.\n\n> +\tstruct dir_iterator *diter = dir_iterator_begin((&path)->buf, flag);\n\n`(&path)->buf` can be shortened to `path.buf`.\n\n> +\n> +\twhile (dir_iterator_advance(diter) == ITER_OK) {\n> +\t\tif (S_ISDIR(diter->st.st_mode))\n> +\t\t\tprintf(\"[d] \");\n> +\t\telse\n> +\t\t\tprintf(\"[f] \");\n> +\n> +\t\tprintf(\"(%s) %s\\n\", diter->relative_path, diter->path.buf);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> diff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh\n> new file mode 100755\n> index 0000000..3c8ea9a\n> --- /dev/null\n> +++ b/t/t0065-dir-iterator.sh\n> @@ -0,0 +1,45 @@\n> +#!/bin/sh\n> +\n> +test_description='Test directory iteration.'\n> +\n> +. ./test-lib.sh\n> +\n> +ITER_SORTED_OUTPUT='[d] (a) ./dir/a\n> +[d] (a/b) ./dir/a/b\n> +[d] (a/b/c) ./dir/a/b/c\n> +[d] (d) ./dir/d\n> +[d] (d/e) ./dir/d/e\n> +[d] (d/e/d) ./dir/d/e/d\n> +[f] (a/b/c/d) ./dir/a/b/c/d\n> +[f] (a/e) ./dir/a/e\n> +[f] (b) ./dir/b\n> +[f] (c) ./dir/c\n> +[f] (d/e/d/a) ./dir/d/e/d/a'\n> +\n> +test_expect_success 'dir-iterator should iterate through all files' '\n> +\tmkdir -p dir &&\n> +\tmkdir -p dir/a/b/c/ &&\n> +\tdate >dir/b &&\n\nIs there a special reason to write the date to the file as opposed to, say\n\n    touch dir/b\n\n? (Some people use `: >dir/b` for this purpose, though I've never found\nout why.) If you write the date to the file, the reader will be\ndistracted unnecessarily wondering whether the contents are important to\nthe test.\n\n> +\tdate >dir/c &&\n> +\tmkdir -p dir/d/e/d/ &&\n> +\tdate >dir/a/b/c/d &&\n> +\tdate >dir/a/e &&\n> +\tdate >dir/d/e/d/a &&\n> +\n> +\ttest-dir-iterator ./dir >it &&\n> +\ttest \"$(sort it)\" == \"$ITER_SORTED_OUTPUT\"\n\nWe generally prefer to write the expected and actual program outputs to\nfiles, then `test_cmp` the files. This has a couple of advantages:\n\n* It doesn't risk missing an error return from the program under test,\n  which could happen in your second test below.\n\n* If there is a disagreement, `test_cmp` shows how the files differed.\n\n* If you run the test using `-d`, you can inspect the contents of the\n  files by hand to help investigate what happened.\n\nTypically that would be done like\n\n\tcat >expected-sorted <<-\\EOF &&\n\t[d] (a) ./dir/a\n\t[d] (a/b) ./dir/a/b\n\t[d] (a/b/c) ./dir/a/b/c\n\t[d] (d) ./dir/d\n\t[d] (d/e) ./dir/d/e\n\t[d] (d/e/d) ./dir/d/e/d\n\t[f] (a/b/c/d) ./dir/a/b/c/d\n\t[f] (a/e) ./dir/a/e\n\t[f] (b) ./dir/b\n\t[f] (c) ./dir/c\n\t[f] (d/e/d/a) ./dir/d/e/d/a\n\tEOF\n\n\ttest-dir-iterator ./dir >actual &&\n\tsort actual >actual-sorted &&\n\ttest_cmp expected-sorted actual-sorted\n\nI also think it would be prudent to run this test twice; once for\npre-order and once for post-order traversal. You could use the same\ndirectory setup and the same `expected-sorted` file for both tests\n(e.g., set them up in a preliminary \"setup\" step).\n\n> +'\n> +\n> +ITER_POST_ORDER_OUTPUT='[f] (a/b/c/d) ./dir2/a/b/c/d\n> +[d] (a/b/c) ./dir2/a/b/c\n> +[d] (a/b) ./dir2/a/b\n> +[d] (a) ./dir2/a'\n> +\n> +test_expect_success 'dir-iterator should list files properly on post-order mode' '\n> +\tmkdir -p dir2/a/b/c/ &&\n> +\tdate >dir2/a/b/c/d &&\n> +\n> +\ttest \"$(test-dir-iterator ./dir2 --post-order)\" == \"$ITER_POST_ORDER_OUTPUT\"\n> +'\n\nHow about adding a test for pre-order traversal, too? It would look\nalmost identical to this test.\n\n> [...]\n\nMichael\n\n"},{"id":"315756","messageId":"55790129-737e-6bec-ff9c-147c969dd584@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-5-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T07:48:34Z","receivedAt":"2017-03-30T07:48:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> Create t/helper/test-dir-iterator.c, which prints relevant information\n> about a directory tree iterated over with dir_iterator.\n> \n> Create t/t0065-dir-iterator.sh, which tests that dir_iterator does\n> iterate through a whole directory tree and that post-order directory\n> iteration is correctly implemented.\n\nPlease also add the compiled test program `test-dir-iterator` to\n`t/helper/.gitignore`.\n\nMichael\n\n"},{"id":"315757","messageId":"a738d683-0f2d-61b9-5ecf-ef8bc8f98ff6@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-5-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T08:05:16Z","receivedAt":"2017-03-30T08:05:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> Create t/helper/test-dir-iterator.c, which prints relevant information\n> about a directory tree iterated over with dir_iterator.\n> \n> Create t/t0065-dir-iterator.sh, which tests that dir_iterator does\n> iterate through a whole directory tree and that post-order directory\n> iteration is correctly implemented.\n> \n> [...]\n> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\n> new file mode 100644\n> index 0000000..b4a148f\n> --- /dev/null\n> +++ b/t/helper/test-dir-iterator.c\n> @@ -0,0 +1,32 @@\n> [...]\n> +\n> +int cmd_main(int argc, const char **argv) {\n> +\tif (argc < 2) {\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstrbuf_add(&path, argv[1], strlen(argv[1]));\n> +\n> +\tunsigned flag = 0;\n> +\tif (argc == 3 && strcmp(argv[2], \"--post-order\") == 0)\n> +\t\tflag = DIR_ITERATOR_POST_ORDER_TRAVERSAL;\n> +\n> +\tstruct dir_iterator *diter = dir_iterator_begin((&path)->buf, flag);\n> +\n> +\twhile (dir_iterator_advance(diter) == ITER_OK) {\n> +\t\tif (S_ISDIR(diter->st.st_mode))\n> +\t\t\tprintf(\"[d] \");\n> +\t\telse\n> +\t\t\tprintf(\"[f] \");\n> +\n> +\t\tprintf(\"(%s) %s\\n\", diter->relative_path, diter->path.buf);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> [...]\n\nOh I forgot to mention, in the Git project we don't allow declarations\nto be mixed with code. Apparently there's some ancient compiler\nsomewhere that doesn't allow it. Declarations always have to be\ntogether, at the top of a block. (Compile with\n`-Werror=declaration-after-statement` to detect this.)\n\nMichael\n\n"},{"id":"315758","messageId":"8838e12a-41b9-fd31-da2c-12352fd374b8@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-3-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 2/6] dir_iterator: refactor state machine model","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T08:18:04Z","receivedAt":"2017-03-30T08:18:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> Remove the \"initialized\" member of dir_iterator_level. Replace its\n> functionality with a DIR_STATE_PUSH state in the\n> dir_iterator_level.dir_state enum.\n> \n> This serves to remove a redundant property in the dir_iterator_level\n> struct and ease comprehension of the state machine's behavior.\n\nNice.\n\nMichael\n\n"},{"id":"315764","messageId":"53fa1238-d649-0cb9-d40a-ad9c38677aa1@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-4-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 3/6] dir_iterator: iterate over dir after its contents","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T11:03:10Z","receivedAt":"2017-03-30T11:03:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> Create an option for the dir_iterator API to iterate over subdirectories\n> only after having iterated through their contents. This feature was\n> predicted, although not implemented by 0fe5043 (\"dir_iterator: new API\n> for iterating over a directory tree\", 2016-06-18).\n> \n> Add the \"flags\" parameter to dir_iterator_create, allowing for the\n> aforementioned \"depth-first\" iteration mode to be enabled. Currently,\n> the only acceptable flag is DIR_ITERATOR_DEPTH_FIRST.\n\nThe flag name has been changed.\n\n> This is useful for recursively removing a directory and calling rmdir()\n> on a directory only after all of its contents have been wiped.\n> \n> Amend a call to dir_iterator_begin() to pass the flags parameter\n> introduced.\n> \n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n> ---\n>  dir-iterator.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++----\n>  dir-iterator.h       | 17 ++++++++++++-----\n>  refs/files-backend.c |  2 +-\n>  3 files changed, 62 insertions(+), 10 deletions(-)\n> \n> diff --git a/dir-iterator.c b/dir-iterator.c\n> index 3ac984b..05d53d2 100644\n> --- a/dir-iterator.c\n> +++ b/dir-iterator.c\n> @@ -47,6 +47,9 @@ struct dir_iterator_int {\n>  \t * that will be included in this iteration.\n>  \t */\n>  \tstruct dir_iterator_level *levels;\n> +\n> +\t/* Holds the flags passed to dir_iterator_begin(). */\n> +\tunsigned flags;\n>  };\n>  \n>  static inline void push_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level)\n> @@ -113,12 +116,14 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \t\t\t\t\titer->base.path.buf, strerror(errno));\n>  \t\t\t\t/* Popping the level is handled below */\n>  \t\t\t}\n> -\t\t} else if (S_ISDIR(iter->base.st.st_mode)) {\n> +\t\t} else if (S_ISDIR(iter->base.st.st_mode) &&\n> +\t\t\t!(iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL)) {\n>  \t\t\tif (level->dir_state == DIR_STATE_ITER) {\n>  \t\t\t\t/*\n>  \t\t\t\t * The directory was just iterated\n>  \t\t\t\t * over; now prepare to iterate into\n> -\t\t\t\t * it.\n> +\t\t\t\t * it (unless an option is set for us\n> +\t\t\t\t * to do otherwise).\n>  \t\t\t\t */\n>  \t\t\t\tpush_dir_level(iter, level);\n>  \t\t\t\tcontinue;\n> @@ -152,7 +157,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \t\t\tde = readdir(level->dir);\n>  \n>  \t\t\tif (!de) {\n> -\t\t\t\t/* This level is exhausted; pop up a level. */\n> +\t\t\t\t/* This level is exhausted  */\n>  \t\t\t\tif (errno) {\n>  \t\t\t\t\twarning(\"error reading directory %s: %s\",\n>  \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n> @@ -160,6 +165,32 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \t\t\t\t\twarning(\"error closing directory %s: %s\",\n>  \t\t\t\t\t\titer->base.path.buf, strerror(errno));\n>  \n> +\t\t\t\tif (iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL) {\n> +\t\t\t\t\t/* If we are handling dirpaths after their contents,\n\nThe comment opening and closing delimiters should be on their own lines.\n\n> +\t\t\t\t\t * we have to iterate over the directory now that we'll\n> +\t\t\t\t\t * have finished iterating into it. */\n> +\t\t\t\t\tlevel->dir = NULL;\n\n`level->dir = NULL` can be done before the `if` statement rather than\nboth here and after the `if` statement.\n\n> +\n> +\t\t\t\t\tif (pop_dir_level(iter) == 0)\n> +\t\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> +\n> +\t\t\t\t\tlevel = &iter->levels[iter->levels_nr - 1];\n> +\t\t\t\t\t/* Since we are iterating through the dirpath\n\nComment opening should be on its own line.\n\n> +\t\t\t\t\t * after we have gone through it, we still need\n> +\t\t\t\t\t * to get rid of the trailing slash we appended.\n> +\t\t\t\t\t *\n> +\t\t\t\t\t * This may generate issues if we ever want to\n> +\t\t\t\t\t * iterate through the root directory AND have\n> +\t\t\t\t\t * post-order traversal enabled.\n> +\t\t\t\t\t */\n> +\t\t\t\t\tstrbuf_strip_suffix(&iter->base.path, \"/\");\n\nHmmm, I was wrong again. Remember that this code path is stripping the\n\"/\" from the directory that has just been iterated over, and has already\nbeen closed and popped off the stack. But if we are iterating over \"/\",\nthen when it is popped off the stack, `pop_dir_level(iter)` returns\nzero, and we never get to this line. So I think you can remove this warning.\n\n> +\n> +\t\t\t\t\tif (set_iterator_data(iter, level))\n> +\t\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\t\treturn ITER_OK;\n> +\t\t\t\t}\n> +\n>  \t\t\t\tlevel->dir = NULL;\n>  \t\t\t\tif (pop_dir_level(iter) == 0)\n>  \t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> @@ -174,6 +205,18 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \t\t\tif (set_iterator_data(iter, level))\n>  \t\t\t\tcontinue;\n>  \n> +\t\t\t/*\n> +\t\t\t * If we want to iterate dirs after files, we shall\n> +\t\t\t * begin looking into them *before* we return the dir\n> +\t\t\t * itself.\n> +\t\t\t */\n> +\t\t\tif (S_ISDIR(iter->base.st.st_mode) &&\n> +\t\t\t\t(iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL)) {\n> +\t\t\t\tpush_dir_level(iter, level);\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n>  \t\t\treturn ITER_OK;\n>  \t\t}\n>  \t}\n> @@ -200,7 +243,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, unsigned flags)\n>  {\n>  \tstruct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));\n>  \tstruct dir_iterator *dir_iterator = &iter->base;\n> @@ -208,6 +251,8 @@ struct dir_iterator *dir_iterator_begin(const char *path)\n>  \tif (!path || !*path)\n>  \t\tdie(\"BUG: empty path passed to dir_iterator_begin()\");\n>  \n> +\titer->flags = flags;\n> +\n>  \tstrbuf_init(&iter->base.path, PATH_MAX);\n>  \tstrbuf_addstr(&iter->base.path, path);\n>  \n> diff --git a/dir-iterator.h b/dir-iterator.h\n> index 27739e6..649ccf6 100644\n> --- a/dir-iterator.h\n> +++ b/dir-iterator.h\n> @@ -11,8 +11,7 @@\n>   * Every time dir_iterator_advance() is called, update the members of\n>   * the dir_iterator structure to reflect the next path in the\n>   * iteration. The order that paths are iterated over within a\n> - * directory is undefined, but directory paths are always iterated\n> - * over before the subdirectory contents.\n> + * directory is undefined.\n>   *\n>   * A typical iteration looks like this:\n>   *\n> @@ -38,6 +37,13 @@\n>   * dir_iterator_advance() again.\n>   */\n>  \n> +/* Possible flags for dir_iterator_begin().\n\nComment opening should be on its own line.\n\n> [...]\n\nSo, using your handy-dandy `test-dir-iterator` program, I did a bunch of\nmanual testing.\n\nIt's a little bit unfortunate (though certainly not a showstopper) that\n`lstat()` is called twice on directories in `POST_ORDER` mode.\n\nMore significant is that pre-order traversal includes unreadable\ndirectory entries (e.g., those whose permissions don't allow reading),\nwhereas post-order traversal omits them. The reason is that when\n`opendir()` fails, the `if (!level->dir)` code block pops it off the\nstack before the `if (iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL)`\nblock gets a chance to return it to the caller.\n\nThis is not great. It seems like callers would want to know about\nunreadable directories within the tree.\n\nI haven't checked whether that problem can be fixed up easily in the\ncurrent code. But I'm getting convinced that the code (i.e., even my\ncode, before your changes) is more convoluted than it needs to be, and\nthat makes it too hard to work with. Quite likely you have the same\nfeeling by now... :-/\n\nImagine something like this:\n\nChange the `dir_state` enumeration to the following:\n\n* DIR_STATE_PUSHED -- the top-of-stack directory has just been pushed\n  onto the stack but not iterated over or opened.\n* DIR_STATE_PRE_ITERATED -- the top-of-stack directory has just been\n  returned to the caller (pre-order), but not opened.\n* DIR_STATE_ITERATING -- the top-of-stack directory has been opened and\n  we are currently iterating over its entries.\n* DIR_STATE_ITERATED -- the top-of-stack directory's entries have all\n  been iterated over, and the directory has been closed.\n* DIR_STATE_POST_ITERATED -- the top-of-stack directory has been\n  returned to the caller (post-order) and is ready to be popped.\n\nThen the function could have a single loop with a more conventional\nstate machine structure *doing exactly one thing per iteration*,\nsomething like:\n\nwhile (1) {\n\tif (level->dir_state == DIR_STATE_PUSHED) {\n\t\tif (iter->flags & DIR_ITERATOR_DIRS_BEFORE) {\n\t\t\t...set iterator data...\n\t\t\tlevel->dir_state = DIR_STATE_PRE_ITERATED;\n\t\t\treturn ITER_OK;\n\t\t} else {\n\t\t\tlevel->dir_state = DIR_STATE_PRE_ITERATED;\n\t\t}\n\t} else if (level->dir_state == DIR_STATE_PRE_ITERATED) {\n\t\tif (iter->flags & DIR_ITERATOR_RECURSE) {\n\t\t\t...open directory...\n\t\t\tlevel->dir_state = DIR_STATE_ITERATING;\n\t\t} else {\n\t\t\tlevel->dir_state = DIR_STATE_ITERATED;\n\t\t}\n\t} else if (level->dir_state == DIR_STATE_PRE_ITERATING) {\n\t\tif (readdir(...)) {\n\t\t\tif (S_ISDIR) {\n\t\t\t\t...push dir...\n\t\t\t\tlevel->dir_state = DIR_STATE_PUSHED;\n\t\t\t} else {\n\t\t\t\t...set iterator data...\n\t\t\t\treturn ITER_OK;\n\t\t\t}\n\t\t} else {\n\t\t\tlevel->dir_state = DIR_STATE_ITERATED;\n\t\t}\n\t} else if (level->dir_state == DIR_STATE_ITERATED) {\n\t\tif (iter->flags & DIR_ITERATOR_DIRS_AFTER) {\n\t\t\t...set iterator data...\n\t\t\tlevel->dir_state = DIR_STATE_POST_ITERATED;\n\t\t\treturn ITER_OK;\n\t\t} else {\n\t\t\tlevel->dir_state = DIR_STATE_POST_ITERATED;\n\t\t}\n\t} else if (level->dir_state == DIR_STATE_POST_ITERATED) {\n\t\t...pop dir...\n\t\tif (...done...) {\n\t\t\tbreak;\n\t\t}\n\t}\n}\n\nIt's a biggish rewrite (and I'm not implying that you have to do it),\nbut I think the results would be more readable than the current code.\n\nMichael\n\n"},{"id":"315765","messageId":"520a6bbb-5915-b51e-7292-ec67e3274f9e@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-7-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 6/6] remove_subtree(): test removing nested directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T11:07:01Z","receivedAt":"2017-03-30T11:07:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> Test removing a nested directory when an attempt is made to restore the\n> index to a state where it does not exist. A similar test could be found\n> previously in t/t2000-checkout-cache-clash.sh, but it did not check for\n> nested directories, which could allow a faulty implementation of\n> remove_subtree() pass the tests.\n> \n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n> ---\n>  t/t2000-checkout-cache-clash.sh | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n> \n> diff --git a/t/t2000-checkout-cache-clash.sh b/t/t2000-checkout-cache-clash.sh\n> index de3edb5..ac10ba3 100755\n> --- a/t/t2000-checkout-cache-clash.sh\n> +++ b/t/t2000-checkout-cache-clash.sh\n> @@ -57,4 +57,15 @@ test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '\n>  \tgit checkout-index -a -f --prefix=there/\n>  '\n>  \n> +test_expect_success 'git checkout-index -f should remove nested subtrees' '\n> +\techo content >path &&\n> +\tgit update-index --add path &&\n> +\trm path &&\n> +\tmkdir -p path/with/nested/paths &&\n> +\techo content >path/file1 &&\n> +\techo content >path/with/nested/paths/file2 &&\n> +\tgit checkout-index -f -a &&\n> +\ttest ! -d path\n> +'\n> +\n>  test_done\n> \n\nIt would be better for this patch to precede \"remove_subtree():\nreimplement using iterators\", as a slightly better proof that the change\nto using iterators doesn't change the behavior.\n\nMichael\n\n"},{"id":"315767","messageId":"708a97ce-9384-f863-37db-32383dd5a984@alum.mit.edu","threadId":"45544","inReplyTo":"1490844730-47634-1-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v5 0/6] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-30T11:27:32Z","receivedAt":"2017-03-30T11:27:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2017 05:32 AM, Daniel Ferreira wrote:\n> This is the fifth version of a patch series that implements the GSoC\n> microproject of converting a recursive call to readdir() to use dir_iterator.\n> \n> v1: https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com/T/#t\n> v2: https://public-inbox.org/git/CACsJy8Dxh-QPBBLfaFWPAWUsbA9GVXA7x+mXLjEvYKhk1zOpig@mail.gmail.com/T/#t\n> v3: https://public-inbox.org/git/CAGZ79kYtpmURSQWPumobA=e3JBFjKhWCdv_LPhKCd71ZRwMovA@mail.gmail.com/T/#t\n> v4: https://public-inbox.org/git/1490747533-89143-1-git-send-email-bnmvco@gmail.com/T/#e437a63e0c22c00c69b5d92977c9b438ed2b9fd3a\n> \n> I would like to really thank Michael for the incredibly thorough review of\n> the last version of this series. I never expected anyone to give that\n> level of attention to this change, and it's really, really appreciated.\n> \n> All of the points he addressed are fixed in this version. As always, more\n> feedback is greatly appreciated.\n\nThanks for the reroll. I have reviewed v5 and it's much improved\nrelative to v4. I'm very encouraged by your quick and constructive\nresponses and especially that you are sticking with it even though the\nproject is getting much, much more involved than a typical GSoC\nmicroproject. Perseverance is a very important prerequisite to success\nin the Git project.\n\nPlease pay attention to formatting trivia like the formatting of\nmultiline constants. It's boring to give the same feedback multiple\ntimes [1].\n\nIt's common that nontrivial patch series have to go though many\niterations before being accepted, so don't get discouraged. Even\nexperienced developers sometimes get past v10! The important thing is\nthat every round gets better than the last, that we learn from the\nexperience together, and that you don't give up, and then it is almost\ncertain that it will converge eventually!\n\nI can't speak for the GSoC mentors who will make the decision, but I\nwould expect that your taking on an ambitious project and sticking with\nit will leave a good impression, even if you don't quite get your change\naccepted by whatever deadline. So don't panic that you are ruining your\nchances by picking an ambitious project; quite the opposite is more\nlikely to be the case.\n\nMichael\n\n[1] softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html\n\n"},{"id":"315770","messageId":"CACsJy8Akf2QU_qUF1i2c-RnT7=vQZReTSx0kJO_Rw3dDwxb1FA@mail.gmail.com","threadId":"45544","inReplyTo":"708a97ce-9384-f863-37db-32383dd5a984@alum.mit.edu","subject":"Re: [PATCH v5 0/6] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-30T12:10:56Z","receivedAt":"2017-03-30T12:11:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 30, 2017 at 6:27 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> I'm very encouraged by your quick and constructive\n> responses and especially that you are sticking with it even though the\n> project is getting much, much more involved than a typical GSoC\n> microproject.\n\nI'm impressed too. The new series made me want to read it, probably\nbecause it's so easy to get into. Unfortunately I don't know enough\nabout dir-iterator.c to contribute any good feedback. And other minor\nthings have been all caught by you :)\n\n> It's common that nontrivial patch series have to go though many\n> iterations before being accepted, so don't get discouraged. Even\n> experienced developers sometimes get past v10!\n\nPffft.. v10 is for amateurs. Experienced devs aim for v20+!\n-- \nDuy\n"},{"id":"315805","messageId":"CAEA2_RL18TarW6k5keP+ADFDv+VZE+gG3e6EkCmWby7GjG-T_w@mail.gmail.com","threadId":"45544","inReplyTo":"ab7a326c-0189-4542-76e8-649eb6e804c9@alum.mit.edu","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T18:25:43Z","receivedAt":"2017-03-30T18:26:19Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"On Thu, Mar 30, 2017 at 4:46 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Is there a special reason to write the date to the file as opposed to, say\n>\n>     touch dir/b\n>\n> ? (Some people use `: >dir/b` for this purpose, though I've never found\n> out why.) If you write the date to the file, the reader will be\n> distracted unnecessarily wondering whether the contents are important to\n> the test.\n>\n\nThere's no reason. They will be `touch`ed instead of written in a next version.\n\n`:` is a bash builtin that that returns an exit code of zero and\nproduces no output. On my Mac at least:\n\nbash-3.2$ type :\n: is a shell builtin\nbash-3.2$ type touch\ntouch is /usr/bin/touch\n\nI suppose there are reasons to try to keep the most of a shell\nscript's logic within the shell itself, without involving external\nbinaries.\n"},{"id":"315806","messageId":"CAEA2_R+w+Pty41RykqBt0XKeNi-4eowNSc=Yh9RL=Lq+Xo=mJg@mail.gmail.com","threadId":"45544","inReplyTo":"a738d683-0f2d-61b9-5ecf-ef8bc8f98ff6@alum.mit.edu","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Daniel Ferreira (theiostream)","fromEmail":"bnmvco@gmail.com","sentAt":"2017-03-30T18:26:33Z","receivedAt":"2017-03-30T18:27:09Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"On Thu, Mar 30, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Oh I forgot to mention, in the Git project we don't allow declarations\n> to be mixed with code. Apparently there's some ancient compiler\n> somewhere that doesn't allow it. Declarations always have to be\n> together, at the top of a block. (Compile with\n> `-Werror=declaration-after-statement` to detect this.)\n\nSorry about that. I'm compiling git on clang and it has a bug that\nignores this warning (it shows up on gcc). I'll watch out for it.\n"},{"id":"316003","messageId":"20170401090350.t2tlpul2hm55xt6b@sigill.intra.peff.net","threadId":"45544","inReplyTo":"CAEA2_RL18TarW6k5keP+ADFDv+VZE+gG3e6EkCmWby7GjG-T_w@mail.gmail.com","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T09:03:50Z","receivedAt":"2017-04-01T09:04:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2017 at 03:25:43PM -0300, Daniel Ferreira (theiostream) wrote:\n\n> On Thu, Mar 30, 2017 at 4:46 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> > Is there a special reason to write the date to the file as opposed to, say\n> >\n> >     touch dir/b\n> >\n> > ? (Some people use `: >dir/b` for this purpose, though I've never found\n> > out why.) If you write the date to the file, the reader will be\n> > distracted unnecessarily wondering whether the contents are important to\n> > the test.\n> >\n> \n> There's no reason. They will be `touch`ed instead of written in a next version.\n> \n> `:` is a bash builtin that that returns an exit code of zero and\n> produces no output. On my Mac at least:\n> \n> bash-3.2$ type :\n> : is a shell builtin\n> bash-3.2$ type touch\n> touch is /usr/bin/touch\n> \n> I suppose there are reasons to try to keep the most of a shell\n> script's logic within the shell itself, without involving external\n> binaries.\n\nI think we actually prefer just:\n\n  >dir/b\n\nin our tests. The advantages over touch are:\n\n  1. It is clear that the output will be empty afterwards (whereas with\n     touch, it might just update the timestamp on an existing file).\n\n  2. It's faster, since it doesn't require an extra process.\n\nIt's equivalent to \": >dir/b\". I think you'll find all three forms in\nour test suite, but \">dir/b\" is the most common.\n\n-Peff\n"},{"id":"316011","messageId":"xmqq8tnkvyj6.fsf@gitster.mtv.corp.google.com","threadId":"45544","inReplyTo":"20170401090350.t2tlpul2hm55xt6b@sigill.intra.peff.net","subject":"Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T17:16:29Z","receivedAt":"2017-04-01T17:16:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think we actually prefer just:\n>\n>   >dir/b\n>\n> in our tests. The advantages over touch are:\n>\n>   1. It is clear that the output will be empty afterwards (whereas with\n>      touch, it might just update the timestamp on an existing file).\n>\n>   2. It's faster, since it doesn't require an extra process.\n>\n> It's equivalent to \": >dir/b\". I think you'll find all three forms in\n> our test suite, but \">dir/b\" is the most common.\n\nAnother one.\n\n    3. Avoiding \"touch\" when we only want to ensure existence of a\n       file and sticking to \">dir/b\" (with or without \":\" no-op)\n       lets us reserve \"touch\" for the intended use of the command.\n       When seeing a \"touch\", the reader is signalled that the\n       script cares about the timestamp of the target file updated.\n\nAmong the three, this one is the most important reason from the\npoint of view of making the codebase maintainable in the longer\nterm.  By forcing the writer to think when choosing between\n\">target\" and \"touch target\", we reduce the mental load of the\nreaders.  As is true for most things in computer field, read happens\nmore often than write while maintaining the code, and that is why\nthis matters.\n\n"}]}