{"thread":{"id":"45747","subject":"[PATCH v10 0/5] [GSoC] remove_subtree(): reimplement using iterators","startedAt":"2017-04-19T13:14:55Z","lastAt":"2017-04-20T09:36:49Z","messageCount":9,"participants":["Daniel Ferreira","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":10,"patchTotal":5},"messages":[{"id":"317230","messageId":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","threadId":"45747","inReplyTo":null,"subject":"[PATCH v10 0/5] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-04-19T13:14:07Z","receivedAt":"2017-04-19T13:14:55Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"This is the tenth version of a patch series that implements the GSoC\nmicroproject of converting a recursive call to readdir() to use\ndir_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\nv5: https://public-inbox.org/git/1490844730-47634-1-git-send-email-bnmvco@gmail.com/T/#m2323f15e45de699f2e09364f40a62e17047cf453\nv6: https://public-inbox.org/git/1491107726-21504-1-git-send-email-bnmvco@gmail.com/T/#t\nv7: https://public-inbox.org/git/1491163388-41255-1-git-send-email-bnmvco@gmail.com/T/#t\nv8: https://public-inbox.org/git/a60b2ed6-2b99-b134-05af-7c8492a6949c@alum.mit.edu/T/#t\nv9: https://public-inbox.org/git/CAGZ79kaBRS0SFAvrV4mN7-mVk+8QmPKPJMD55zPQ+A14ZzYFYA@mail.gmail.com/T/#me8988b7dd4adbc4ea24946ccb24fc1cf7baf44e3\n\nTravis CI build: https://travis-ci.org/theiostream/git/builds/223542902\n\nI do not know if \"queuing\" meant I did not have to change this series\nany further (specially after Stefan's \"ok\"), but anyway, this series\napplies Junio's corrections back from v9, that were mostly regarding\ncommit messages or style. I hope I got them right.\n\nThe only point I was in doubt was about Michael's signoff. Actually, he\ngave it not regarding the snippet for the new dir_iterator_advance()\nlogic, but to a small piece of actual code he wrote on the new dir\niterator test helper[1]. Thus I don't know whether this would require his\nsignoff to come before or after mine. Regardless, proper credit has\nbeen given in the commit message, as suggested. In the end, I kept\nhis before mine, but I suppose that can be adjusted by Junio if\nnecessary.\n\nI also didn't get whether I myself should have renamed t0065 to t0066\ngiven the other queued patch. I kept it as t0065 since I figured it\nwould be weird for this patch as a unit to \"skip\" a number.\n\nOnce again, thanks for all the time invested in the reviews for this\npatch.\n\n[1]: https://public-inbox.org/git/1491163388-41255-1-git-send-email-bnmvco@gmail.com/T/#m187b9e681e3369862ccc6083bbf6596cd2e19cd4\n\nDaniel Ferreira (5):\n  dir_iterator: add tests for dir_iterator API\n  remove_subtree(): test removing nested directories\n  dir_iterator: refactor dir_iterator_advance\n  dir_iterator: rewrite state machine model\n  remove_subtree(): reimplement using iterators\n\n Makefile                        |   1 +\n dir-iterator.c                  | 252 +++++++++++++++++++++++++++++-----------\n dir-iterator.h                  |  35 ++++--\n entry.c                         |  42 +++----\n refs/files-backend.c            |  51 +++++---\n t/helper/.gitignore             |   1 +\n t/helper/test-dir-iterator.c    |  53 +++++++++\n t/t0065-dir-iterator.sh         | 111 ++++++++++++++++++\n t/t2000-checkout-cache-clash.sh |  11 ++\n 9 files changed, 433 insertions(+), 124 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":"317231","messageId":"1492607652-36341-2-git-send-email-bnmvco@gmail.com","threadId":"45747","inReplyTo":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v10 1/5] dir_iterator: add tests for dir_iterator API","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-04-19T13:14:08Z","receivedAt":"2017-04-19T13:15:08Z","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.\n\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n Makefile                     |  1 +\n t/helper/.gitignore          |  1 +\n t/helper/test-dir-iterator.c | 30 ++++++++++++++++++++++++\n t/t0065-dir-iterator.sh      | 55 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 87 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 d595ea3..a5c1ac0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -614,6 +614,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/.gitignore b/t/helper/.gitignore\nindex 758ed2e..a7d74d3 100644\n--- a/t/helper/.gitignore\n+++ b/t/helper/.gitignore\n@@ -3,6 +3,7 @@\n /test-config\n /test-date\n /test-delta\n+/test-dir-iterator\n /test-dump-cache-tree\n /test-dump-split-index\n /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..a7d1470\n--- /dev/null\n+++ b/t/helper/test-dir-iterator.c\n@@ -0,0 +1,30 @@\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct dir_iterator *diter;\n+\n+\tif (argc < 2)\n+\t\tdie(\"BUG: test-dir-iterator needs one argument\");\n+\n+\tstrbuf_add(&path, argv[1], strlen(argv[1]));\n+\n+\tditer = dir_iterator_begin(path.buf);\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 if (S_ISREG(diter->st.st_mode))\n+\t\t\tprintf(\"[f] \");\n+\t\telse\n+\t\t\tprintf(\"[?] \");\n+\n+\t\tprintf(\"(%s) [%s] %s\\n\", diter->relative_path, diter->basename, 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..46e5ce5\n--- /dev/null\n+++ b/t/t0065-dir-iterator.sh\n@@ -0,0 +1,55 @@\n+#!/bin/sh\n+\n+test_description='Test directory iteration.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir -p dir &&\n+\tmkdir -p dir/a/b/c/ &&\n+\t>dir/b &&\n+\t>dir/c &&\n+\tmkdir -p dir/d/e/d/ &&\n+\t>dir/a/b/c/d &&\n+\t>dir/a/e &&\n+\t>dir/d/e/d/a &&\n+\n+\tmkdir -p dir2/a/b/c/ &&\n+\t>dir2/a/b/c/d\n+'\n+\n+test_expect_success 'dir-iterator should iterate through all files' '\n+\tcat >expect-sorted-output <<-\\EOF &&\n+\t[d] (a) [a] ./dir/a\n+\t[d] (a/b) [b] ./dir/a/b\n+\t[d] (a/b/c) [c] ./dir/a/b/c\n+\t[d] (d) [d] ./dir/d\n+\t[d] (d/e) [e] ./dir/d/e\n+\t[d] (d/e/d) [d] ./dir/d/e/d\n+\t[f] (a/b/c/d) [d] ./dir/a/b/c/d\n+\t[f] (a/e) [e] ./dir/a/e\n+\t[f] (b) [b] ./dir/b\n+\t[f] (c) [c] ./dir/c\n+\t[f] (d/e/d/a) [a] ./dir/d/e/d/a\n+\tEOF\n+\n+\ttest-dir-iterator ./dir >out &&\n+\tsort <out >./actual-pre-order-sorted-output &&\n+\n+\ttest_cmp expect-sorted-output actual-pre-order-sorted-output\n+'\n+\n+test_expect_success 'dir-iterator should list files in the correct order' '\n+\tcat >expect-pre-order-output <<-\\EOF &&\n+\t[d] (a) [a] ./dir2/a\n+\t[d] (a/b) [b] ./dir2/a/b\n+\t[d] (a/b/c) [c] ./dir2/a/b/c\n+\t[f] (a/b/c/d) [d] ./dir2/a/b/c/d\n+\tEOF\n+\n+\ttest-dir-iterator ./dir2 >actual-pre-order-output &&\n+\n+\ttest_cmp expect-pre-order-output actual-pre-order-output\n+'\n+\n+test_done\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"317232","messageId":"1492607652-36341-3-git-send-email-bnmvco@gmail.com","threadId":"45747","inReplyTo":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v10 2/5] remove_subtree(): test removing nested directories","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-04-19T13:14:09Z","receivedAt":"2017-04-19T13:15:21Z","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":"317233","messageId":"1492607652-36341-4-git-send-email-bnmvco@gmail.com","threadId":"45747","inReplyTo":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v10 3/5] dir_iterator: refactor dir_iterator_advance","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-04-19T13:14:10Z","receivedAt":"2017-04-19T13:15:35Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Factor out reusable helpers out of 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 | 66 ++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 43 insertions(+), 23 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 34182a9..d168cb2 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -50,6 +50,44 @@ struct dir_iterator_int {\n \tstruct dir_iterator_level *levels;\n };\n \n+static 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 int pop_dir_level(struct dir_iterator_int *iter)\n+{\n+\treturn --iter->levels_nr;\n+}\n+\n+static int adjust_iterator_data(struct dir_iterator_int *iter,\n+\t\tstruct 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 +122,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 +138,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 +163,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 +172,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 (adjust_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":"317234","messageId":"1492607652-36341-5-git-send-email-bnmvco@gmail.com","threadId":"45747","inReplyTo":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v10 4/5] dir_iterator: rewrite state machine model","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-04-19T13:14:11Z","receivedAt":"2017-04-19T13:15:52Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Perform a rewrite of dir_iterator_advance(). dir_iterator has\nceased to rely on a combination of level.initialized and level.dir_state\nstate variables and now only tracks the state with level.dir_state,\nwhich simplifies the iterator mechanism, makes the code easier to follow\nand eases additions of new features to the iterator.\n\nMake dir_iterator_begin() attempt to lstat() the path it receives, and\nreturn NULL and an appropriate errno if it fails or if the passed path\nwas not a directory.\n\nCreate 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). This is useful for\nrecursively removing a directory and calling rmdir() on a directory only\nafter all of its contents have been wiped.\n\nAdd an option for dir_iterator to also iterate over the initial\ndirectory (the one passed to dir_iterator_begin()).\n\nAdd the \"flags\" parameter to dir_iterator_create, allowing for the\naforementioned new features to be enabled. The new default behavior\n(i.e. flags set to 0) does not iterate over directories. Flag\nDIR_ITERATOR_PRE_ORDER_TRAVERSAL iterates over a directory before doing\nso over its contents. DIR_ITERATOR_POST_ORDER_TRAVERSAL iterates over a\ndirectory after doing so over its contents. DIR_ITERATOR_LIST_ROOT_DIR\niterates over the initial directory. These flags do not conflict with\neach other and may be used simultaneously.\n\nAmend a call to dir_iterator_begin() in refs/files-backend.c to pass\nthe flags parameter introduced, as well as handle the case in which it\nfails to open the directory.\n\nImprove t/t0065-dir-iterator.sh and t/helper/test-dir-iterator.c to\ntest \"post-order\" and \"iterate-over-root\" modes.\n\nMichael Haggerty contributed with the design of the new\ndir_iterator_advance() implementation, the code for\nt/helper/test-dir-iterator's option parser and numerous reviews that\ngradually shaped this code to its current form.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Daniel Ferreira <bnmvco@gmail.com>\n---\n dir-iterator.c               | 220 ++++++++++++++++++++++++++++++-------------\n dir-iterator.h               |  35 +++++--\n refs/files-backend.c         |  51 ++++++----\n t/helper/test-dir-iterator.c |  31 +++++-\n t/t0065-dir-iterator.sh      |  94 ++++++++++++++----\n 5 files changed, 316 insertions(+), 115 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex d168cb2..3c20e0e 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,9 +18,15 @@ struct dir_iterator_level {\n \t * iteration and also iterated into):\n \t */\n \tenum {\n-\t\tDIR_STATE_ITER,\n-\t\tDIR_STATE_RECURSE\n+\t\tDIR_STATE_PUSHED,\n+\t\tDIR_STATE_PRE_ITERATION,\n+\t\tDIR_STATE_ITERATING,\n+\t\tDIR_STATE_POST_ITERATION,\n+\t\tDIR_STATE_EXHAUSTED\n \t} dir_state;\n+\n+\t/* The stat structure for the directory this level represents. */\n+\tstruct stat st;\n };\n \n /*\n@@ -48,15 +52,23 @@ 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 void push_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level)\n+static void push_dir_level(struct dir_iterator_int *iter, struct stat *st)\n {\n-\tlevel->dir_state = DIR_STATE_RECURSE;\n+\tstruct dir_iterator_level *level;\n+\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 = NULL;\n+\tlevel->dir_state = DIR_STATE_PUSHED;\n+\tlevel->st = *st;\n }\n \n static int pop_dir_level(struct dir_iterator_int *iter)\n@@ -67,7 +79,11 @@ static int pop_dir_level(struct dir_iterator_int *iter)\n static int adjust_iterator_data(struct dir_iterator_int *iter,\n \t\tstruct dir_iterator_level *level)\n {\n-\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n+\tchar *last_path_component;\n+\n+\tif (level->dir_state != DIR_STATE_ITERATING) {\n+\t\titer->base.st = level->st;\n+\t} else if (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@@ -76,18 +92,48 @@ static int adjust_iterator_data(struct dir_iterator_int *iter,\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 * Check if we are dealing with the root directory as an\n+\t * item that's being iterated through.\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+\tif (level->dir_state != DIR_STATE_ITERATING &&\n+\t\titer->levels_nr == 1) {\n+\t\titer->base.relative_path = \".\";\n+\n+\t\t/*\n+\t\t * If we have a path like './dir', we'll get everything\n+\t\t * after the last slash a basename. If we don't find a\n+\t\t * slash (e.g. 'dir'), we return the whole path.\n+\t\t */\n+\t\tlast_path_component = strrchr(iter->base.path.buf, '/');\n+\t\titer->base.basename = last_path_component ?\n+\t\t\tlast_path_component + 1 : iter->base.path.buf;\n+\t} else {\n+\t\titer->base.relative_path =\n+\t\t\titer->base.path.buf + iter->levels[0].prefix_len;\n+\n+\t\tif (S_ISDIR(iter->base.st.st_mode))\n+\t\t\titer->base.basename =\n+\t\t\t\titer->base.path.buf + iter->levels[iter->levels_nr - 2].prefix_len;\n+\t\telse\n+\t\t\titer->base.basename =\n+\t\t\t\titer->base.path.buf + level->prefix_len;\n+\t}\n \n \treturn 0;\n }\n \n+/*\n+ * This function uses a state machine with the following states:\n+ * * DIR_STATE_PUSHED: the directory has been pushed to the\n+ * iterator traversal tree.\n+ * * DIR_STATE_PRE_ITERATION: the directory is not opened with opendir(). The\n+ * dirpath has already been returned if pre-order traversal is set.\n+ * * DIR_STATE_ITERATING: the directory is initialized. We are traversing\n+ * through it.\n+ * * DIR_STATE_POST_ITERATION: the directory has been iterated through.\n+ * We are ready to close it.\n+ * * DIR_STATE_EXHAUSTED: the directory is closed and ready to be popped.\n+ */\n int dir_iterator_advance(struct dir_iterator *dir_iterator)\n {\n \tstruct dir_iterator_int *iter =\n@@ -96,9 +142,26 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \twhile (1) {\n \t\tstruct dir_iterator_level *level =\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_PUSHED) {\n+\t\t\tlevel->dir_state = DIR_STATE_PRE_ITERATION;\n+\n+\t\t\t/* We may not want the root directory to be iterated over */\n+\t\t\tif ((iter->flags & DIR_ITERATOR_PRE_ORDER_TRAVERSAL) && (\n+\t\t\t\titer->levels_nr != 1 ||\n+\t\t\t\t(iter->flags & DIR_ITERATOR_LIST_ROOT_DIR))) {\n+\t\t\t\t/*\n+\t\t\t\t * This will only error if we fail to lstat() the\n+\t\t\t\t * root directory. In this case, we bail.\n+\t\t\t\t */\n+\t\t\t\tif (adjust_iterator_data(iter, level)) {\n+\t\t\t\t\tlevel->dir_state = DIR_STATE_EXHAUSTED;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\n+\t\t\t\treturn ITER_OK;\n+\t\t\t}\n+\t\t} else if (level->dir_state == DIR_STATE_PRE_ITERATION) {\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@@ -108,64 +171,37 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\tlevel->prefix_len = iter->base.path.len;\n \n \t\t\tlevel->dir = opendir(iter->base.path.buf);\n-\t\t\tif (!level->dir && errno != ENOENT) {\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-\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\tif (!level->dir) {\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 * This level wasn't opened sucessfully; pretend we\n+\t\t\t\t * iterated through it already.\n \t\t\t\t */\n-\t\t\t\tpush_dir_level(iter, level);\n+\t\t\t\tif (errno != ENOENT) {\n+\t\t\t\t\twarning(\"error opening directory %s: %s\",\n+\t\t\t\t\t\titer->base.path.buf, strerror(errno));\n+\t\t\t\t}\n+\n+\t\t\t\tlevel->dir_state = DIR_STATE_POST_ITERATION;\n \t\t\t\tcontinue;\n-\t\t\t} else {\n-\t\t\t\t/*\n-\t\t\t\t * The directory has already been\n-\t\t\t\t * iterated over and iterated into;\n-\t\t\t\t * we're done with it.\n-\t\t\t\t */\n \t\t\t}\n-\t\t}\n \n-\t\tif (!level->dir) {\n-\t\t\t/*\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 (pop_dir_level(iter) == 0)\n-\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\tlevel->dir_state = DIR_STATE_ITERATING;\n+\t\t} else if (level->dir_state == DIR_STATE_ITERATING) {\n+\t\t\tstruct dirent *de;\n \n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/*\n-\t\t * Loop until we find an entry that we can give back\n-\t\t * to the caller:\n-\t\t */\n-\t\twhile (1) {\n \t\t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n \t\t\terrno = 0;\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/* In case of readdir() error */\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-\t\t\t\t} else if (closedir(level->dir))\n-\t\t\t\t\twarning(\"error closing directory %s: %s\",\n-\t\t\t\t\t\titer->base.path.buf, strerror(errno));\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-\t\t\t\tbreak;\n+\t\t\t\tlevel->dir_state = DIR_STATE_POST_ITERATION;\n+\t\t\t\tcontinue;\n \t\t\t}\n \n \t\t\tif (is_dot_or_dotdot(de->d_name))\n@@ -176,7 +212,41 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\tif (adjust_iterator_data(iter, level))\n \t\t\t\tcontinue;\n \n+\t\t\tif (S_ISDIR(iter->base.st.st_mode)) {\n+\t\t\t\tpush_dir_level(iter, &iter->base.st);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n \t\t\treturn ITER_OK;\n+\t\t} else if (level->dir_state == DIR_STATE_POST_ITERATION) {\n+\t\t\tif (level->dir != NULL && closedir(level->dir)) {\n+\t\t\t\twarning(\"error closing directory %s: %s\",\n+\t\t\t\t\titer->base.path.buf, strerror(errno));\n+\t\t\t}\n+\t\t\tlevel->dir_state = DIR_STATE_EXHAUSTED;\n+\n+\t\t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n+\t\t\t/*\n+\t\t\t * Since we are iterating through the dirpath\n+\t\t\t * after we have gone through it, we still need\n+\t\t\t * to get rid of the trailing slash we appended.\n+\t\t\t */\n+\t\t\tstrbuf_strip_suffix(&iter->base.path, \"/\");\n+\n+\t\t\t/* We may not want the root directory to be iterated over */\n+\t\t\tif ((iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL) && (\n+\t\t\t\titer->levels_nr != 1 ||\n+\t\t\t\t(iter->flags & DIR_ITERATOR_LIST_ROOT_DIR))) {\n+\t\t\t\t/*\n+\t\t\t\t * In this state, adjust_iterator_data() should never return\n+\t\t\t\t * an error.\n+\t\t\t\t */\n+\t\t\t\tadjust_iterator_data(iter, level);\n+\t\t\t\treturn ITER_OK;\n+\t\t\t}\n+\t\t} else if (level->dir_state == DIR_STATE_EXHAUSTED) {\n+\t\t\tif (pop_dir_level(iter) == 0)\n+\t\t\t\treturn dir_iterator_abort(dir_iterator);\n \t\t}\n \t}\n }\n@@ -202,21 +272,41 @@ 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+\tstruct dir_iterator_int *iter;\n+\tstruct dir_iterator *dir_iterator;\n+\tstruct stat st;\n \n \tif (!path || !*path)\n \t\tdie(\"BUG: empty path passed to dir_iterator_begin()\");\n \n+\tif (lstat(path, &st) < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarning(\"error reading path '%s': %s\",\n+\t\t\t\t\tpath, strerror(errno));\n+\n+\t\treturn NULL;\n+\t}\n+\n+\tif (!S_ISDIR(st.st_mode)) {\n+\t\terrno = ENOTDIR;\n+\t\treturn NULL;\n+\t}\n+\n+\titer = xcalloc(1, sizeof(*iter));\n+\tdir_iterator = &iter->base;\n+\n+\titer->flags = flags;\n+\tdir_iterator->st = st;\n+\n \tstrbuf_init(&iter->base.path, PATH_MAX);\n \tstrbuf_addstr(&iter->base.path, path);\n \n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n+\titer->levels_nr = 0;\n \n-\titer->levels_nr = 1;\n-\titer->levels[0].initialized = 0;\n+\tpush_dir_level(iter, &dir_iterator->st);\n \n \treturn dir_iterator;\n }\ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 27739e6..a21e845 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -5,19 +5,17 @@\n  * Iterate over a directory tree.\n  *\n  * Iterate over a directory tree, recursively, including paths of all\n- * types and hidden paths. Skip \".\" and \"..\" entries and don't follow\n- * symlinks except for the original path.\n+ * types and hidden paths. Skip \".\" and \"..\" entries.\n  *\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  *     int ok;\n- *     struct iterator *iter = dir_iterator_begin(path);\n+ *     struct iterator *iter = dir_iterator_begin(path, flags);\n  *\n  *     while ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n  *             if (want_to_stop_iteration()) {\n@@ -38,6 +36,26 @@\n  * dir_iterator_advance() again.\n  */\n \n+/*\n+ * Possible flags for dir_iterator_begin().\n+ *\n+ * * DIR_ITERATOR_PRE_ORDER_TRAVERSAL: the iterator shall return\n+ * a dirpath it has found before iterating through that directory's\n+ * contents.\n+ * * DIR_ITERATOR_POST_ORDER_TRAVERSAL: the iterator shall return\n+ * a dirpath it has found after iterating through that directory's\n+ * contents.\n+ * * DIR_ITERATOR_LIST_ROOT_DIR: the iterator shall return the dirpath\n+ * of the root directory it is iterating through if either\n+ * DIR_ITERATOR_PRE_ORDER_TRAVERSAL or DIR_ITERATOR_POST_ORDER_TRAVERSAL\n+ * is set.\n+ *\n+ * All flags can be used in any combination.\n+ */\n+#define DIR_ITERATOR_PRE_ORDER_TRAVERSAL (1 << 0)\n+#define DIR_ITERATOR_POST_ORDER_TRAVERSAL (1 << 1)\n+#define DIR_ITERATOR_LIST_ROOT_DIR (1 << 2)\n+\n struct dir_iterator {\n \t/* The current path: */\n \tstruct strbuf path;\n@@ -57,15 +75,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..d376e1f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3284,29 +3284,42 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \tstruct dir_iterator *diter = iter->dir_iterator;\n \tint ok;\n \n-\twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n-\t\tint flags;\n-\n-\t\tif (!S_ISREG(diter->st.st_mode))\n-\t\t\tcontinue;\n-\t\tif (diter->basename[0] == '.')\n-\t\t\tcontinue;\n-\t\tif (ends_with(diter->basename, \".lock\"))\n-\t\t\tcontinue;\n+\tif (!iter->dir_iterator) {\n+\t\t/*\n+\t\t * If our dir_iterator is NULL at this stage, it means we failed\n+\t\t * to open the given path at dir_iterator_begin(), most likely due\n+\t\t * to it not existing.\n+\t\t *\n+\t\t * In this case, we pretend it was an empty directory and just\n+\t\t * iterate through nothing.\n+\t\t */\n+\t\tok = ITER_DONE;\n+\t} else {\n+\t\twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n+\t\t\tint flags;\n+\n+\t\t\tif (!S_ISREG(diter->st.st_mode))\n+\t\t\t\tcontinue;\n+\t\t\tif (diter->basename[0] == '.')\n+\t\t\t\tcontinue;\n+\t\t\tif (ends_with(diter->basename, \".lock\"))\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (read_ref_full(diter->relative_path, 0,\n+\t\t\t\t\t  iter->oid.hash, &flags)) {\n+\t\t\t\terror(\"bad ref for %s\", diter->path.buf);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \n-\t\tif (read_ref_full(diter->relative_path, 0,\n-\t\t\t\t  iter->oid.hash, &flags)) {\n-\t\t\terror(\"bad ref for %s\", diter->path.buf);\n-\t\t\tcontinue;\n+\t\t\titer->base.refname = diter->relative_path;\n+\t\t\titer->base.oid = &iter->oid;\n+\t\t\titer->base.flags = flags;\n+\t\t\treturn ITER_OK;\n \t\t}\n \n-\t\titer->base.refname = diter->relative_path;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n-\t\treturn ITER_OK;\n+\t\titer->dir_iterator = NULL;\n \t}\n \n-\titer->dir_iterator = NULL;\n \tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n \t\tok = ITER_ERROR;\n \treturn ok;\n@@ -3346,7 +3359,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 \ndiff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\nindex a7d1470..3b4948b 100644\n--- a/t/helper/test-dir-iterator.c\n+++ b/t/helper/test-dir-iterator.c\n@@ -5,15 +5,38 @@\n \n int cmd_main(int argc, const char **argv)\n {\n+\tconst char **myargv = argv;\n+\tint myargc = argc;\n+\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct dir_iterator *diter;\n \n-\tif (argc < 2)\n-\t\tdie(\"BUG: test-dir-iterator needs one argument\");\n+\tunsigned flag = 0;\n+\n+\twhile (--myargc && starts_with(*++myargv, \"--\")) {\n+\t\tif (!strcmp(*myargv, \"--pre-order\"))\n+\t\t\tflag |= DIR_ITERATOR_PRE_ORDER_TRAVERSAL;\n+\t\telse if (!strcmp(*myargv, \"--post-order\"))\n+\t\t\tflag |= DIR_ITERATOR_POST_ORDER_TRAVERSAL;\n+\t\telse if (!strcmp(*myargv, \"--list-root-dir\"))\n+\t\t\tflag |= DIR_ITERATOR_LIST_ROOT_DIR;\n+\t\telse if (!strcmp(*myargv, \"--\")) {\n+\t\t\tmyargc--;\n+\t\t\tmyargv++;\n+\t\t\tbreak;\n+\t\t} else\n+\t\t\tdie(\"Unrecognized option: %s\", *myargv);\n+\t}\n \n-\tstrbuf_add(&path, argv[1], strlen(argv[1]));\n+\tif (myargc != 1)\n+\t\tdie(\"expected exactly one non-option argument\");\n+\tstrbuf_addstr(&path, *myargv);\n \n-\tditer = dir_iterator_begin(path.buf);\n+\tditer = dir_iterator_begin(path.buf, flag);\n+\tif (diter == NULL) {\n+\t\tprintf(\"begin failed: %d\\n\", errno);\n+\t\treturn 0;\n+\t}\n \n \twhile (dir_iterator_advance(diter) == ITER_OK) {\n \t\tif (S_ISDIR(diter->st.st_mode))\ndiff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh\nindex 46e5ce5..4c6632f 100755\n--- a/t/t0065-dir-iterator.sh\n+++ b/t/t0065-dir-iterator.sh\n@@ -15,31 +15,41 @@ test_expect_success 'setup' '\n \t>dir/d/e/d/a &&\n \n \tmkdir -p dir2/a/b/c/ &&\n-\t>dir2/a/b/c/d\n+\t>dir2/a/b/c/d &&\n+\n+\t>file\n '\n \n-test_expect_success 'dir-iterator should iterate through all files' '\n-\tcat >expect-sorted-output <<-\\EOF &&\n-\t[d] (a) [a] ./dir/a\n-\t[d] (a/b) [b] ./dir/a/b\n-\t[d] (a/b/c) [c] ./dir/a/b/c\n-\t[d] (d) [d] ./dir/d\n-\t[d] (d/e) [e] ./dir/d/e\n-\t[d] (d/e/d) [d] ./dir/d/e/d\n-\t[f] (a/b/c/d) [d] ./dir/a/b/c/d\n-\t[f] (a/e) [e] ./dir/a/e\n-\t[f] (b) [b] ./dir/b\n-\t[f] (c) [c] ./dir/c\n-\t[f] (d/e/d/a) [a] ./dir/d/e/d/a\n-\tEOF\n+cat >expect-sorted-output <<-\\EOF &&\n+[d] (a) [a] ./dir/a\n+[d] (a/b) [b] ./dir/a/b\n+[d] (a/b/c) [c] ./dir/a/b/c\n+[d] (d) [d] ./dir/d\n+[d] (d/e) [e] ./dir/d/e\n+[d] (d/e/d) [d] ./dir/d/e/d\n+[f] (a/b/c/d) [d] ./dir/a/b/c/d\n+[f] (a/e) [e] ./dir/a/e\n+[f] (b) [b] ./dir/b\n+[f] (c) [c] ./dir/c\n+[f] (d/e/d/a) [a] ./dir/d/e/d/a\n+EOF\n \n-\ttest-dir-iterator ./dir >out &&\n+test_expect_success 'dir-iterator should iterate through all files' '\n+\ttest-dir-iterator --pre-order ./dir >out &&\n \tsort <out >./actual-pre-order-sorted-output &&\n \n \ttest_cmp expect-sorted-output actual-pre-order-sorted-output\n '\n \n-test_expect_success 'dir-iterator should list files in the correct order' '\n+test_expect_success 'dir-iterator should iterate through all files on post-order mode' '\n+\ttest-dir-iterator --post-order ./dir >out &&\n+\tsort <out >actual-post-order-sorted-output &&\n+\n+\ttest_cmp expect-sorted-output actual-post-order-sorted-output\n+'\n+\n+\n+test_expect_success 'dir-iterator should list files properly on pre-order mode' '\n \tcat >expect-pre-order-output <<-\\EOF &&\n \t[d] (a) [a] ./dir2/a\n \t[d] (a/b) [b] ./dir2/a/b\n@@ -47,9 +57,55 @@ test_expect_success 'dir-iterator should list files in the correct order' '\n \t[f] (a/b/c/d) [d] ./dir2/a/b/c/d\n \tEOF\n \n-\ttest-dir-iterator ./dir2 >actual-pre-order-output &&\n-\n+\ttest-dir-iterator --pre-order ./dir2 >actual-pre-order-output &&\n \ttest_cmp expect-pre-order-output actual-pre-order-output\n '\n \n+test_expect_success 'dir-iterator should list files properly on post-order mode' '\n+\tcat >expect-post-order-output <<-\\EOF &&\n+\t[f] (a/b/c/d) [d] ./dir2/a/b/c/d\n+\t[d] (a/b/c) [c] ./dir2/a/b/c\n+\t[d] (a/b) [b] ./dir2/a/b\n+\t[d] (a) [a] ./dir2/a\n+\tEOF\n+\n+\ttest-dir-iterator --post-order ./dir2 >actual-post-order-output &&\n+\ttest_cmp expect-post-order-output actual-post-order-output\n+'\n+\n+test_expect_success 'dir-iterator should list files properly on pre-order + post-order + root-dir mode' '\n+\tcat >expect-pre-order-post-order-root-dir-output <<-\\EOF &&\n+\t[d] (.) [dir2] ./dir2\n+\t[d] (a) [a] ./dir2/a\n+\t[d] (a/b) [b] ./dir2/a/b\n+\t[d] (a/b/c) [c] ./dir2/a/b/c\n+\t[f] (a/b/c/d) [d] ./dir2/a/b/c/d\n+\t[d] (a/b/c) [c] ./dir2/a/b/c\n+\t[d] (a/b) [b] ./dir2/a/b\n+\t[d] (a) [a] ./dir2/a\n+\t[d] (.) [dir2] ./dir2\n+\tEOF\n+\n+\ttest-dir-iterator --pre-order --post-order --list-root-dir ./dir2 >actual-pre-order-post-order-root-dir-output &&\n+\ttest_cmp expect-pre-order-post-order-root-dir-output actual-pre-order-post-order-root-dir-output\n+'\n+\n+test_expect_success 'dir-iterator should return ENOENT upon opening non-existing directory' '\n+\tcat >expect-non-existing-dir-output <<-\\EOF &&\n+\tbegin failed: 2\n+\tEOF\n+\n+\ttest-dir-iterator ./dir3 >actual-non-existing-dir-output &&\n+\ttest_cmp expect-non-existing-dir-output actual-non-existing-dir-output\n+'\n+\n+test_expect_success 'dir-iterator should return ENOTDIR upon opening non-directory path' '\n+\tcat >expect-not-a-directory-output <<-\\EOF &&\n+\tbegin failed: 20\n+\tEOF\n+\n+\ttest-dir-iterator ./file >actual-not-a-directory-output &&\n+\ttest_cmp expect-not-a-directory-output actual-not-a-directory-output\n+'\n+\n test_done\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"317235","messageId":"1492607652-36341-6-git-send-email-bnmvco@gmail.com","threadId":"45747","inReplyTo":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","subject":"[PATCH v10 5/5] remove_subtree(): reimplement using iterators","fromName":"Daniel Ferreira","fromEmail":"bnmvco@gmail.com","sentAt":"2017-04-19T13:14:12Z","receivedAt":"2017-04-19T13:16:02Z","isPatch":true,"sender":{"key":"bnmvco@gmail.com","avatar":"https://avatars.githubusercontent.com/u/735397?v=4"},"body":"Use 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 | 42 ++++++++++++++++--------------------------\n 1 file changed, 16 insertions(+), 26 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex d2b512d..a939432 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -3,6 +3,8 @@\n #include \"dir.h\"\n #include \"streaming.h\"\n #include \"submodule.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@@ -45,33 +47,21 @@ 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,\n+\t\tDIR_ITERATOR_POST_ORDER_TRAVERSAL | DIR_ITERATOR_LIST_ROOT_DIR);\n+\tif (!diter) {\n+\t\tdie_errno(\"cannot remove path '%s'\", path);\n+\t}\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 \n static int create_file(const char *path, unsigned int mode)\n@@ -312,7 +302,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":"317307","messageId":"xmqqk26fahjn.fsf@gitster.mtv.corp.google.com","threadId":"45747","inReplyTo":"1492607652-36341-1-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v10 0/5] [GSoC] remove_subtree(): reimplement using iterators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-20T03:23:24Z","receivedAt":"2017-04-20T03:23:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Ferreira <bnmvco@gmail.com> writes:\n\n> I do not know if \"queuing\" meant I did not have to change this series\n> any further (specially after Stefan's \"ok\"), but anyway, this series\n> applies Junio's corrections back from v9, that were mostly regarding\n> commit messages or style. I hope I got them right.\n\nQueuing merely means that I queued the series on its own topic\nbranch and merged it to 'pu', which is a bit more (but not much\nmore) permanent place than the list archive.  It does not mean\nanything more---specifically, it does not mean that it is now cast\nin stone.  It does not mean it will appear in the next release,\neither.\n\nIf you and others are happy with the state of the commits recorded,\nwe may then merge the topic to 'next' and then to 'master'.  But in\nthe meantime, if there are things that you are not exactly happy in\nthe series (e.g. you found a better way to approach the issue you\ntackled, you noticed that you didn't fully address the review\ncomments, etc.)  you are welcome to further polish your topic by\nsending out replacements.\n\n> The only point I was in doubt was about Michael's signoff. Actually, he\n> gave it not regarding the snippet for the new dir_iterator_advance()\n> logic, but to a small piece of actual code he wrote on the new dir\n> iterator test helper[1].\n\nIf part of your patch contains his code more or less verbatim, then\nit is the right thing to do to have his sign-off.  For that part you\nare relaying his patch, so he signs it off first, signaling that he\nwrote it and he has the authority to release it to the project, and\nthen you sign it off, signaling that you have the authority to relay\nit to the project (see DCO in Documentation/SubmittingPatches).\n\n> I also didn't get whether I myself should have renamed t0065 to t0066\n> given the other queued patch.\n\nI would have expected you to move it over to t0066, as I'd have to\nrename it myself otherwise.  There is no \"skipping\" of a number in\nthe result, as this series comes later than the one that adds t0065.\n\nHaving said that, there is no need to resend the series if the only\nchange necessary on top of v10 were renaming t0065 to t0066.\n\nThanks.\n"},{"id":"317308","messageId":"xmqqfuh3ah71.fsf@gitster.mtv.corp.google.com","threadId":"45747","inReplyTo":"1492607652-36341-5-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v10 4/5] dir_iterator: rewrite state machine model","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-20T03:30:58Z","receivedAt":"2017-04-20T03:31:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Ferreira <bnmvco@gmail.com> writes:\n\n> diff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh\n> index 46e5ce5..4c6632f 100755\n> --- a/t/t0065-dir-iterator.sh\n> +++ b/t/t0065-dir-iterator.sh\n> @@ -15,31 +15,41 @@ test_expect_success 'setup' '\n>  \t>dir/d/e/d/a &&\n>  \n>  \tmkdir -p dir2/a/b/c/ &&\n> -\t>dir2/a/b/c/d\n> +\t>dir2/a/b/c/d &&\n> +\n> +\t>file\n>  '\n>  \n> -test_expect_success 'dir-iterator should iterate through all files' '\n> -\tcat >expect-sorted-output <<-\\EOF &&\n> -\t[d] (a) [a] ./dir/a\n> -\t[d] (a/b) [b] ./dir/a/b\n> -\t[d] (a/b/c) [c] ./dir/a/b/c\n> -\t[d] (d) [d] ./dir/d\n> -\t[d] (d/e) [e] ./dir/d/e\n> -\t[d] (d/e/d) [d] ./dir/d/e/d\n> -\t[f] (a/b/c/d) [d] ./dir/a/b/c/d\n> -\t[f] (a/e) [e] ./dir/a/e\n> -\t[f] (b) [b] ./dir/b\n> -\t[f] (c) [c] ./dir/c\n> -\t[f] (d/e/d/a) [a] ./dir/d/e/d/a\n> -\tEOF\n> +cat >expect-sorted-output <<-\\EOF &&\n> +[d] (a) [a] ./dir/a\n> +[d] (a/b) [b] ./dir/a/b\n> +[d] (a/b/c) [c] ./dir/a/b/c\n> +[d] (d) [d] ./dir/d\n> +[d] (d/e) [e] ./dir/d/e\n> +[d] (d/e/d) [d] ./dir/d/e/d\n> +[f] (a/b/c/d) [d] ./dir/a/b/c/d\n> +[f] (a/e) [e] ./dir/a/e\n> +[f] (b) [b] ./dir/b\n> +[f] (c) [c] ./dir/c\n> +[f] (d/e/d/a) [a] ./dir/d/e/d/a\n> +EOF\n>  \n> -\ttest-dir-iterator ./dir >out &&\n\nThere is something fishy going on around here in this patch, pushing\nthe code to prepare test vector out of test_expect_success block.  A\nmistake in rebasing or something?\n\nIf you need to reroll the series to update this part, please rename\nthe test to t0066 and do remember to update the logmessage of a few\ncommits that refer to t0065.\n\nThanks.\n"},{"id":"317315","messageId":"e1ccebfd-d85b-25a6-1f36-13d1c7f566cd@alum.mit.edu","threadId":"45747","inReplyTo":"1492607652-36341-5-git-send-email-bnmvco@gmail.com","subject":"Re: [PATCH v10 4/5] dir_iterator: rewrite state machine model","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-20T09:36:32Z","receivedAt":"2017-04-20T09:36:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 03:14 PM, Daniel Ferreira wrote:\n> Perform a rewrite of dir_iterator_advance(). dir_iterator has\n> ceased to rely on a combination of level.initialized and level.dir_state\n> state variables and now only tracks the state with level.dir_state,\n> which simplifies the iterator mechanism, makes the code easier to follow\n> and eases additions of new features to the iterator.\n> \n> Make dir_iterator_begin() attempt to lstat() the path it receives, and\n> return NULL and an appropriate errno if it fails or if the passed path\n> was not a directory.\n> \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). This is useful for\n> recursively removing a directory and calling rmdir() on a directory only\n> after all of its contents have been wiped.\n> \n> Add an option for dir_iterator to also iterate over the initial\n> directory (the one passed to dir_iterator_begin()).\n> \n> Add the \"flags\" parameter to dir_iterator_create, allowing for the\n> aforementioned new features to be enabled. The new default behavior\n> (i.e. flags set to 0) does not iterate over directories. Flag\n> DIR_ITERATOR_PRE_ORDER_TRAVERSAL iterates over a directory before doing\n> so over its contents. DIR_ITERATOR_POST_ORDER_TRAVERSAL iterates over a\n> directory after doing so over its contents. DIR_ITERATOR_LIST_ROOT_DIR\n> iterates over the initial directory. These flags do not conflict with\n> each other and may be used simultaneously.\n> \n> Amend a call to dir_iterator_begin() in refs/files-backend.c to pass\n> the flags parameter introduced, as well as handle the case in which it\n> fails to open the directory.\n> \n> Improve t/t0065-dir-iterator.sh and t/helper/test-dir-iterator.c to\n> test \"post-order\" and \"iterate-over-root\" modes.\n> \n> Michael Haggerty contributed with the design of the new\n> dir_iterator_advance() implementation, the code for\n> t/helper/test-dir-iterator's option parser and numerous reviews that\n> gradually shaped this code to its current form.\n> \n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>\n> ---\n>  dir-iterator.c               | 220 ++++++++++++++++++++++++++++++-------------\n>  dir-iterator.h               |  35 +++++--\n>  refs/files-backend.c         |  51 ++++++----\n>  t/helper/test-dir-iterator.c |  31 +++++-\n>  t/t0065-dir-iterator.sh      |  94 ++++++++++++++----\n>  5 files changed, 316 insertions(+), 115 deletions(-)\n> \n> diff --git a/dir-iterator.c b/dir-iterator.c\n> index d168cb2..3c20e0e 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,9 +18,15 @@ struct dir_iterator_level {\n>  \t * iteration and also iterated into):\n>  \t */\n>  \tenum {\n> -\t\tDIR_STATE_ITER,\n> -\t\tDIR_STATE_RECURSE\n> +\t\tDIR_STATE_PUSHED,\n> +\t\tDIR_STATE_PRE_ITERATION,\n> +\t\tDIR_STATE_ITERATING,\n> +\t\tDIR_STATE_POST_ITERATION,\n> +\t\tDIR_STATE_EXHAUSTED\n>  \t} dir_state;\n> +\n> +\t/* The stat structure for the directory this level represents. */\n> +\tstruct stat st;\n>  };\n>  \n>  /*\n> @@ -48,15 +52,23 @@ 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 void push_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level)\n> +static void push_dir_level(struct dir_iterator_int *iter, struct stat *st)\n>  {\n> -\tlevel->dir_state = DIR_STATE_RECURSE;\n> +\tstruct dir_iterator_level *level;\n> +\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 = NULL;\n> +\tlevel->dir_state = DIR_STATE_PUSHED;\n> +\tlevel->st = *st;\n>  }\n>  \n>  static int pop_dir_level(struct dir_iterator_int *iter)\n> @@ -67,7 +79,11 @@ static int pop_dir_level(struct dir_iterator_int *iter)\n>  static int adjust_iterator_data(struct dir_iterator_int *iter,\n>  \t\tstruct dir_iterator_level *level)\n>  {\n> -\tif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n> +\tchar *last_path_component;\n> +\n> +\tif (level->dir_state != DIR_STATE_ITERATING) {\n> +\t\titer->base.st = level->st;\n> +\t} else if (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> @@ -76,18 +92,48 @@ static int adjust_iterator_data(struct dir_iterator_int *iter,\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 * Check if we are dealing with the root directory as an\n> +\t * item that's being iterated through.\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> +\tif (level->dir_state != DIR_STATE_ITERATING &&\n> +\t\titer->levels_nr == 1) {\n> +\t\titer->base.relative_path = \".\";\n> +\n> +\t\t/*\n> +\t\t * If we have a path like './dir', we'll get everything\n> +\t\t * after the last slash a basename. If we don't find a\n> +\t\t * slash (e.g. 'dir'), we return the whole path.\n> +\t\t */\n> +\t\tlast_path_component = strrchr(iter->base.path.buf, '/');\n> +\t\titer->base.basename = last_path_component ?\n> +\t\t\tlast_path_component + 1 : iter->base.path.buf;\n\nThere's kindof a lot of awkward code here that is only needed to support\n`DIR_ITERATOR_LIST_ROOT_DIR`. And the basename for the top-level\ndirectory is often not useful. For example, if the starting path is \"..\"\nor \"foo/..\" or \"bar/.\", then its basename is set to \"..\" or \".\". It\nwould be really awkward to find the real basename of such arguments.\n\nThis might be the code trying to tell us something. Maybe\n`DIR_ITERATOR_LIST_ROOT_DIR` isn't worth it as a feature. When I tried\nripping it out, I saved 40 LOC in `dir_iterator`, at a cost of only two\nextra lines in `entry.c`.\n\nAlternatively (given that the basename isn't really useful for the\ntop-level path), I think it would be acceptable to just set basename to\nNULL for the case of the top-level directory (and document that\npeculiarity).\n\n> +\t} else {\n> +\t\titer->base.relative_path =\n> +\t\t\titer->base.path.buf + iter->levels[0].prefix_len;\n> +\n> +\t\tif (S_ISDIR(iter->base.st.st_mode))\n> +\t\t\titer->base.basename =\n> +\t\t\t\titer->base.path.buf + iter->levels[iter->levels_nr - 2].prefix_len;\n> +\t\telse\n> +\t\t\titer->base.basename =\n> +\t\t\t\titer->base.path.buf + level->prefix_len;\n> +\t}\n>  \n>  \treturn 0;\n>  }\n>  \n> +/*\n> + * This function uses a state machine with the following states:\n> + * * DIR_STATE_PUSHED: the directory has been pushed to the\n> + * iterator traversal tree.\n> + * * DIR_STATE_PRE_ITERATION: the directory is not opened with opendir(). The\n> + * dirpath has already been returned if pre-order traversal is set.\n> + * * DIR_STATE_ITERATING: the directory is initialized. We are traversing\n> + * through it.\n> + * * DIR_STATE_POST_ITERATION: the directory has been iterated through.\n> + * We are ready to close it.\n> + * * DIR_STATE_EXHAUSTED: the directory is closed and ready to be popped.\n> + */\n\nA little indentation would make the bulleted list above a lot easier to\nread. And it is more common to use \"-\" for bullets, I guess because they\ndon't clash so much with the \"*\" that are part of the comment prefix:\n\n +/*\n + * This function uses a state machine with the following states:\n + * - DIR_STATE_PUSHED: the directory has been pushed to the\n + *   iterator traversal tree.\n + * - DIR_STATE_PRE_ITERATION: the directory is not opened with\nopendir(). The\n + *   dirpath has already been returned if pre-order traversal is set.\n + * - DIR_STATE_ITERATING: the directory is initialized. We are traversing\n + *   through it.\n + * - DIR_STATE_POST_ITERATION: the directory has been iterated through.\n + *   We are ready to close it.\n + * - DIR_STATE_EXHAUSTED: the directory is closed and ready to be popped.\n + */\n\n\n>  int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  {\n>  \tstruct dir_iterator_int *iter =\n> @@ -96,9 +142,26 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \twhile (1) {\n>  \t\tstruct dir_iterator_level *level =\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_PUSHED) {\n> +\t\t\tlevel->dir_state = DIR_STATE_PRE_ITERATION;\n> +\n> +\t\t\t/* We may not want the root directory to be iterated over */\n> +\t\t\tif ((iter->flags & DIR_ITERATOR_PRE_ORDER_TRAVERSAL) && (\n> +\t\t\t\titer->levels_nr != 1 ||\n> +\t\t\t\t(iter->flags & DIR_ITERATOR_LIST_ROOT_DIR))) {\n> +\t\t\t\t/*\n> +\t\t\t\t * This will only error if we fail to lstat() the\n> +\t\t\t\t * root directory. In this case, we bail.\n> +\t\t\t\t */\n> +\t\t\t\tif (adjust_iterator_data(iter, level)) {\n> +\t\t\t\t\tlevel->dir_state = DIR_STATE_EXHAUSTED;\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\n> +\t\t\t\treturn ITER_OK;\n> +\t\t\t}\n> +\t\t} else if (level->dir_state == DIR_STATE_PRE_ITERATION) {\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> @@ -108,64 +171,37 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \t\t\tlevel->prefix_len = iter->base.path.len;\n>  \n>  \t\t\tlevel->dir = opendir(iter->base.path.buf);\n> -\t\t\tif (!level->dir && errno != ENOENT) {\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> -\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\tif (!level->dir) {\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 * This level wasn't opened sucessfully; pretend we\n> +\t\t\t\t * iterated through it already.\n>  \t\t\t\t */\n> -\t\t\t\tpush_dir_level(iter, level);\n> +\t\t\t\tif (errno != ENOENT) {\n\nWe usually don't use braces for blocks consisting of a single statement.\n\n> +\t\t\t\t\twarning(\"error opening directory %s: %s\",\n> +\t\t\t\t\t\titer->base.path.buf, strerror(errno));\n> +\t\t\t\t}\n> +\n> +\t\t\t\tlevel->dir_state = DIR_STATE_POST_ITERATION;\n>  \t\t\t\tcontinue;\n> -\t\t\t} else {\n> -\t\t\t\t/*\n> -\t\t\t\t * The directory has already been\n> -\t\t\t\t * iterated over and iterated into;\n> -\t\t\t\t * we're done with it.\n> -\t\t\t\t */\n>  \t\t\t}\n> -\t\t}\n>  \n> -\t\tif (!level->dir) {\n> -\t\t\t/*\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 (pop_dir_level(iter) == 0)\n> -\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> +\t\t\tlevel->dir_state = DIR_STATE_ITERATING;\n> +\t\t} else if (level->dir_state == DIR_STATE_ITERATING) {\n> +\t\t\tstruct dirent *de;\n>  \n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\t/*\n> -\t\t * Loop until we find an entry that we can give back\n> -\t\t * to the caller:\n> -\t\t */\n> -\t\twhile (1) {\n>  \t\t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n>  \t\t\terrno = 0;\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/* In case of readdir() error */\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> -\t\t\t\t} else if (closedir(level->dir))\n> -\t\t\t\t\twarning(\"error closing directory %s: %s\",\n> -\t\t\t\t\t\titer->base.path.buf, strerror(errno));\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> -\t\t\t\tbreak;\n> +\t\t\t\tlevel->dir_state = DIR_STATE_POST_ITERATION;\n> +\t\t\t\tcontinue;\n>  \t\t\t}\n>  \n>  \t\t\tif (is_dot_or_dotdot(de->d_name))\n> @@ -176,7 +212,41 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \t\t\tif (adjust_iterator_data(iter, level))\n>  \t\t\t\tcontinue;\n>  \n> +\t\t\tif (S_ISDIR(iter->base.st.st_mode)) {\n> +\t\t\t\tpush_dir_level(iter, &iter->base.st);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n>  \t\t\treturn ITER_OK;\n> +\t\t} else if (level->dir_state == DIR_STATE_POST_ITERATION) {\n> +\t\t\tif (level->dir != NULL && closedir(level->dir)) {\n> +\t\t\t\twarning(\"error closing directory %s: %s\",\n> +\t\t\t\t\titer->base.path.buf, strerror(errno));\n> +\t\t\t}\n\nThe `closedir()` will only be attempted if we arrived at\n`DIR_STATE_POST_ITERATION` state from the `DIR_STATE_ITERATING` branch,\nright? So why not call `closedir()` up there? Then you wouldn't need the\n`level->dir != NULL` check (which, by the way, we spell `if\n(level->dir)`; the `!= NULL` is redundant).\n\n> +\t\t\tlevel->dir_state = DIR_STATE_EXHAUSTED;\n> +\n> +\t\t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n> +\t\t\t/*\n> +\t\t\t * Since we are iterating through the dirpath\n> +\t\t\t * after we have gone through it, we still need\n> +\t\t\t * to get rid of the trailing slash we appended.\n> +\t\t\t */\n> +\t\t\tstrbuf_strip_suffix(&iter->base.path, \"/\");\n> +\n> +\t\t\t/* We may not want the root directory to be iterated over */\n> +\t\t\tif ((iter->flags & DIR_ITERATOR_POST_ORDER_TRAVERSAL) && (\n> +\t\t\t\titer->levels_nr != 1 ||\n> +\t\t\t\t(iter->flags & DIR_ITERATOR_LIST_ROOT_DIR))) {\n> +\t\t\t\t/*\n> +\t\t\t\t * In this state, adjust_iterator_data() should never return\n> +\t\t\t\t * an error.\n> +\t\t\t\t */\n> +\t\t\t\tadjust_iterator_data(iter, level);\n> +\t\t\t\treturn ITER_OK;\n> +\t\t\t}\n> +\t\t} else if (level->dir_state == DIR_STATE_EXHAUSTED) {\n> +\t\t\tif (pop_dir_level(iter) == 0)\n> +\t\t\t\treturn dir_iterator_abort(dir_iterator);\n>  \t\t}\n>  \t}\n>  }\n> @@ -202,21 +272,41 @@ 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> +\tstruct dir_iterator_int *iter;\n> +\tstruct dir_iterator *dir_iterator;\n> +\tstruct stat st;\n>  \n>  \tif (!path || !*path)\n>  \t\tdie(\"BUG: empty path passed to dir_iterator_begin()\");\n>  \n> +\tif (lstat(path, &st) < 0) {\n> +\t\tif (errno != ENOENT)\n> +\t\t\twarning(\"error reading path '%s': %s\",\n> +\t\t\t\t\tpath, strerror(errno));\n\nIn a case like this, where we are providing an `errno` to the caller, it\nis probably better to leave it to the caller to emit a warning/error if\nit wants. The caller will have more context and so will be able to\nexplain the error in a way that the user can better understand. And some\ncallers might not want to emit an error at all, or base their decision\non `errno`.\n\n(The story is different *during* the iteration, because we currently\ndon't have a way to return an error to the caller short of aborting the\nwhole iteration.)\n\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\tif (!S_ISDIR(st.st_mode)) {\n> +\t\terrno = ENOTDIR;\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\titer = xcalloc(1, sizeof(*iter));\n> +\tdir_iterator = &iter->base;\n> +\n> +\titer->flags = flags;\n> +\tdir_iterator->st = st;\n> +\n>  \tstrbuf_init(&iter->base.path, PATH_MAX);\n>  \tstrbuf_addstr(&iter->base.path, path);\n>  \n>  \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n> +\titer->levels_nr = 0;\n>  \n> -\titer->levels_nr = 1;\n> -\titer->levels[0].initialized = 0;\n> +\tpush_dir_level(iter, &dir_iterator->st);\n>  \n>  \treturn dir_iterator;\n>  }\n> diff --git a/dir-iterator.h b/dir-iterator.h\n> index 27739e6..a21e845 100644\n> --- a/dir-iterator.h\n> +++ b/dir-iterator.h\n> @@ -5,19 +5,17 @@\n>   * Iterate over a directory tree.\n>   *\n>   * Iterate over a directory tree, recursively, including paths of all\n> - * types and hidden paths. Skip \".\" and \"..\" entries and don't follow\n> - * symlinks except for the original path.\n> + * types and hidden paths. Skip \".\" and \"..\" entries.\n>   *\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>   *     int ok;\n> - *     struct iterator *iter = dir_iterator_begin(path);\n> + *     struct iterator *iter = dir_iterator_begin(path, flags);\n>   *\n>   *     while ((ok = dir_iterator_advance(iter)) == ITER_OK) {\n>   *             if (want_to_stop_iteration()) {\n> @@ -38,6 +36,26 @@\n>   * dir_iterator_advance() again.\n>   */\n>  \n> +/*\n> + * Possible flags for dir_iterator_begin().\n> + *\n> + * * DIR_ITERATOR_PRE_ORDER_TRAVERSAL: the iterator shall return\n> + * a dirpath it has found before iterating through that directory's\n> + * contents.\n> + * * DIR_ITERATOR_POST_ORDER_TRAVERSAL: the iterator shall return\n> + * a dirpath it has found after iterating through that directory's\n> + * contents.\n> + * * DIR_ITERATOR_LIST_ROOT_DIR: the iterator shall return the dirpath\n> + * of the root directory it is iterating through if either\n> + * DIR_ITERATOR_PRE_ORDER_TRAVERSAL or DIR_ITERATOR_POST_ORDER_TRAVERSAL\n> + * is set.\n> + *\n> + * All flags can be used in any combination.\n> + */\n\nPlease indent this bulleted list, too.\n\n> +#define DIR_ITERATOR_PRE_ORDER_TRAVERSAL (1 << 0)\n> +#define DIR_ITERATOR_POST_ORDER_TRAVERSAL (1 << 1)\n> +#define DIR_ITERATOR_LIST_ROOT_DIR (1 << 2)\n> +\n>  struct dir_iterator {\n>  \t/* The current path: */\n>  \tstruct strbuf path;\n> @@ -57,15 +75,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.\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 50188e9..d376e1f 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3284,29 +3284,42 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n>  \tstruct dir_iterator *diter = iter->dir_iterator;\n>  \tint ok;\n>  \n> -\twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n> -\t\tint flags;\n> -\n> -\t\tif (!S_ISREG(diter->st.st_mode))\n> -\t\t\tcontinue;\n> -\t\tif (diter->basename[0] == '.')\n> -\t\t\tcontinue;\n> -\t\tif (ends_with(diter->basename, \".lock\"))\n> -\t\t\tcontinue;\n> +\tif (!iter->dir_iterator) {\n> +\t\t/*\n> +\t\t * If our dir_iterator is NULL at this stage, it means we failed\n> +\t\t * to open the given path at dir_iterator_begin(), most likely due\n> +\t\t * to it not existing.\n> +\t\t *\n> +\t\t * In this case, we pretend it was an empty directory and just\n> +\t\t * iterate through nothing.\n> +\t\t */\n> +\t\tok = ITER_DONE;\n> +\t} else {\n> +\t\twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n> +\t\t\tint flags;\n> +\n> +\t\t\tif (!S_ISREG(diter->st.st_mode))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (diter->basename[0] == '.')\n> +\t\t\t\tcontinue;\n> +\t\t\tif (ends_with(diter->basename, \".lock\"))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tif (read_ref_full(diter->relative_path, 0,\n> +\t\t\t\t\t  iter->oid.hash, &flags)) {\n> +\t\t\t\terror(\"bad ref for %s\", diter->path.buf);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n>  \n> -\t\tif (read_ref_full(diter->relative_path, 0,\n> -\t\t\t\t  iter->oid.hash, &flags)) {\n> -\t\t\terror(\"bad ref for %s\", diter->path.buf);\n> -\t\t\tcontinue;\n> +\t\t\titer->base.refname = diter->relative_path;\n> +\t\t\titer->base.oid = &iter->oid;\n> +\t\t\titer->base.flags = flags;\n> +\t\t\treturn ITER_OK;\n>  \t\t}\n>  \n> -\t\titer->base.refname = diter->relative_path;\n> -\t\titer->base.oid = &iter->oid;\n> -\t\titer->base.flags = flags;\n> -\t\treturn ITER_OK;\n> +\t\titer->dir_iterator = NULL;\n>  \t}\n>  \n> -\titer->dir_iterator = NULL;\n>  \tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n>  \t\tok = ITER_ERROR;\n>  \treturn ok;\n> @@ -3346,7 +3359,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\nAn easier way to deal with an error here would be to free `iter` (or not\nallocate it in the first place) and return `empty_ref_iterator_begin()`\ninstead.\n\n>  \treturn ref_iterator;\n>  }\n>  \n> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\n> index a7d1470..3b4948b 100644\n> --- a/t/helper/test-dir-iterator.c\n> +++ b/t/helper/test-dir-iterator.c\n> @@ -5,15 +5,38 @@\n>  \n>  int cmd_main(int argc, const char **argv)\n>  {\n> +\tconst char **myargv = argv;\n> +\tint myargc = argc;\n> +\n>  \tstruct strbuf path = STRBUF_INIT;\n>  \tstruct dir_iterator *diter;\n>  \n> -\tif (argc < 2)\n> -\t\tdie(\"BUG: test-dir-iterator needs one argument\");\n> +\tunsigned flag = 0;\n> +\n> +\twhile (--myargc && starts_with(*++myargv, \"--\")) {\n> +\t\tif (!strcmp(*myargv, \"--pre-order\"))\n> +\t\t\tflag |= DIR_ITERATOR_PRE_ORDER_TRAVERSAL;\n> +\t\telse if (!strcmp(*myargv, \"--post-order\"))\n> +\t\t\tflag |= DIR_ITERATOR_POST_ORDER_TRAVERSAL;\n> +\t\telse if (!strcmp(*myargv, \"--list-root-dir\"))\n> +\t\t\tflag |= DIR_ITERATOR_LIST_ROOT_DIR;\n> +\t\telse if (!strcmp(*myargv, \"--\")) {\n> +\t\t\tmyargc--;\n> +\t\t\tmyargv++;\n> +\t\t\tbreak;\n> +\t\t} else\n> +\t\t\tdie(\"Unrecognized option: %s\", *myargv);\n> +\t}\n>  \n> -\tstrbuf_add(&path, argv[1], strlen(argv[1]));\n> +\tif (myargc != 1)\n> +\t\tdie(\"expected exactly one non-option argument\");\n> +\tstrbuf_addstr(&path, *myargv);\n>  \n> -\tditer = dir_iterator_begin(path.buf);\n> +\tditer = dir_iterator_begin(path.buf, flag);\n> +\tif (diter == NULL) {\n> +\t\tprintf(\"begin failed: %d\\n\", errno);\n\nIt would be more helpful to include `strerror(errno)` rather than\n`errno` itself in the error output.\n\n> +\t\treturn 0;\n> +\t}\n>  \n>  \twhile (dir_iterator_advance(diter) == ITER_OK) {\n>  \t\tif (S_ISDIR(diter->st.st_mode))\n> diff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh\n> [...]\n\n\n"}]}