[PATCH v2 00/15][GSoC] iterate dirs before or after their contents

[PATCH v2 00/15][GSoC] iterate dirs before or after their contents

[Plato Kiorpelidis]

This is the second version of a patch series which implements pre-order and
post-order iteration over directories. In this version I use the new iteration
scheme to convert entry.c remove_subtree() function from using opendir/readdir/
closedir API to dir-iterator API.

v1: https://lore.kernel.org/git/20220410111852.2097418-1-kioplato@gmail.com/

Fork: https://github.com/kioplato/git/tree/dir-iterator-v2
CI: https://github.com/kioplato/git/actions/runs/2295239114
I've ran the full test suite for each commit.

In comparison to v1 I changed/fixed/improved:

* In this version I followed Phillip's suggestion[1] to explain why I'm making
the changes in each commit's description and create smaller, targeted commits.
In v1 I did a lot of renaming to directory paths, test descriptions and files.
In this version these renames have their own commits.

* Explained why I converted dir_iterator_advance() from iterative implementation
to recursive in the related commit. Ævar[2] and Phillip[3] asked about why I did
this conversion, so I thought it was appropriate to include an explanation in
the commit's description.

* I talked about a subtle unexpected behavior of dir_iterator_begin() [4]. I was
wrong about the two new states for the deepest directory. I introduced those
states in order to implement recursive dir_iterator_advance() and not to deal
with the unexpected behavior of dir_iterator_begin(). Therefore I splitted the
conversion of dir_iteartor_advance() and the changes to deal with the unexpected
behavior of dir_iteartor_begin() into two different commits. They need their own
commits since they are two unrelated changes.

* Split the commit that introduces pre-order and post-order iteration into two.
Like Phillip suggested here[5] the first changes dir-iterator default behavior
and the second implements post-order iterator over directories. This helps split
the tests introduced, which helps generate smaller diffs.

* Like Phillip suggested [6], I'll work with the existing code and change one
aspect at a time in stages. In this series I converted entry.c remove_subtree()
to use dir-iterator API instead of opendir/readdir/closedir API.

* The redundant tests [7] in this version were not removed. Instead I kept them
for more detailed testing which helps in case a test fails.

* Ævar's idea[8] to change the default swich case in test-dir-iterator made me
realize that test-dir-iterator does not print errno codes set by failed calls to
dir_iterator_advance(). Therefore I made test-dir-iterator print them which
makes all tests test_cmp for the errno code set by either dir_iteartor_begin()
or dir_iterator_advance(). This presumably has the same effect as changing the
default switch case to use BUG() macro. This change also led to two others, one
that makes test-dir-iterator explicitly print ELOOP errno and the second that
improves readability by consistently returning, C standard constants, either
EXIT_SUCCESS or EXIT_FAILURE, instead of mixed integers and exit() calls.

* fixed coding style and introduces an enum for returning constants instead of
more integers which worsens readability, as suggested by Ævar [9].

I also didn't CC Michael and Matheus in this version, since they weren't
interested in v1 where I did CC them.

[1]: https://lore.kernel.org/git/ed6656e0-a865-319e-0f56-23ab1da3eef3@gmail.com/
[2]: https://lore.kernel.org/git/220411.86o817j2dt.gmgdl@evledraar.gmail.com/
[3]: https://lore.kernel.org/git/35160d46-d337-2110-f968-238abb7e9f0e@gmail.com/
[4]: https://lore.kernel.org/git/20220427154526.uuhpkoee322l7kmz@compass/
[5]: https://lore.kernel.org/git/b75aaee8-c037-e8e0-6ee0-729922059352@gmail.com/
[6]: https://lore.kernel.org/git/df287d4f-e9da-4ce0-d7e9-1b1fe7671aab@gmail.com/
[7]: https://lore.kernel.org/git/220411.86sfqjj2o0.gmgdl@evledraar.gmail.com/
[8]: https://lore.kernel.org/git/220411.86wnfvj2q6.gmgdl@evledraar.gmail.com/
[9]: https://lore.kernel.org/git/220411.86o817j2dt.gmgdl@evledraar.gmail.com/

Plato Kiorpelidis (15):
  t0066: refactor dir-iterator tests
  t0066: remove dependency between unrelated tests
  t0066: shorter expected and actual output file names
  test-dir-iterator: consistently return EXIT_FAILURE or EXIT_SUCCESS
  test-dir-iterator: print EACCES and ELOOP errno set by dir_iterator
  test-dir-iterator: print errno name set by dir_iterator_advance
  t0066: better test coverage for dir-iterator
  t0066: reorder tests from simple to more complex
  t0066: rename test directories
  dir-iterator: refactor dir_iterator_advance()
  dir-iterator: open root dir in dir_iterator_begin()
  t0066: rename subtest descriptions
  dir-iterator: option to iterate dirs in pre-order
  dir-iterator: option to iterate dirs in post-order
  entry.c: use dir-iterator to avoid explicit dir traversal

 builtin/clone.c              |    4 +-
 dir-iterator.c               |  334 ++++++--
 dir-iterator.h               |   36 +-
 entry.c                      |   38 +-
 refs/files-backend.c         |    2 +-
 t/helper/test-dir-iterator.c |   24 +-
 t/t0066-dir-iterator.sh      | 1478 +++++++++++++++++++++++++++++++---
 7 files changed, 1704 insertions(+), 212 deletions(-)

-- 
2.36.1

[PATCH v2 01/15] t0066: refactor dir-iterator tests

[Plato Kiorpelidis]

Be consistent throughout the dir-iterator tests regarding the order in
which we:
  * create test directories
  * create expected outputs
  * test if actual and expected outputs differ

These changes improve the readability of dir-iterator tests, separating
setup tests as we want to test different iteration schemes over the same
hierarchy. They also simplify the introduction of new tests that cover
current and future iteration schemes by providing a standardized
structure for testing dir-iterator with various directory hierarchies.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 55 +++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 63a1a45cd3..807c43d447 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -5,7 +5,7 @@ test_description='Test the dir-iterator functionality'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'setup' '
+test_expect_success 'setup -- dir w/ complex structure' '
 	mkdir -p dir &&
 	mkdir -p dir/a/b/c/ &&
 	>dir/b &&
@@ -13,12 +13,8 @@ test_expect_success 'setup' '
 	mkdir -p dir/d/e/d/ &&
 	>dir/a/b/c/d &&
 	>dir/a/e &&
-	>dir/d/e/d/a &&
-
-	mkdir -p dir2/a/b/c/ &&
-	>dir2/a/b/c/d
+	>dir/d/e/d/a
 '
-
 test_expect_success 'dir-iterator should iterate through all files' '
 	cat >expected-iteration-sorted-output <<-EOF &&
 	[d] (a) [a] ./dir/a
@@ -40,6 +36,10 @@ test_expect_success 'dir-iterator should iterate through all files' '
 	test_cmp expected-iteration-sorted-output actual-iteration-sorted-output
 '
 
+test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
+	mkdir -p dir2/a/b/c &&
+	>dir2/a/b/c/d
+'
 test_expect_success 'dir-iterator should list files in the correct order' '
 	cat >expected-pre-order-output <<-EOF &&
 	[d] (a) [a] ./dir2/a
@@ -54,65 +54,59 @@ test_expect_success 'dir-iterator should list files in the correct order' '
 '
 
 test_expect_success 'begin should fail upon inexistent paths' '
+	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-output &&
+
 	test_must_fail test-tool dir-iterator ./inexistent-path \
 		>actual-inexistent-path-output &&
-	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-output &&
+
 	test_cmp expected-inexistent-path-output actual-inexistent-path-output
 '
 
 test_expect_success 'begin should fail upon non directory paths' '
-	test_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&
 	echo "dir_iterator_begin failure: ENOTDIR" >expected-non-dir-output &&
+
+	test_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&
+
 	test_cmp expected-non-dir-output actual-non-dir-output
 '
 
+test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
+	mkdir -p dir3/a &&
+	>dir3/a/b
+'
 test_expect_success POSIXPERM,SANITY 'advance should not fail on errors by default' '
 	cat >expected-no-permissions-output <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	EOF
 
-	mkdir -p dir3/a &&
-	>dir3/a/b &&
 	chmod 0 dir3/a &&
 
 	test-tool dir-iterator ./dir3 >actual-no-permissions-output &&
 	test_cmp expected-no-permissions-output actual-no-permissions-output &&
-	chmod 755 dir3/a &&
-	rm -rf dir3
+	chmod 755 dir3/a
 '
-
 test_expect_success POSIXPERM,SANITY 'advance should fail on errors, w/ pedantic flag' '
 	cat >expected-no-permissions-pedantic-output <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	dir_iterator_advance failure
 	EOF
 
-	mkdir -p dir3/a &&
-	>dir3/a/b &&
 	chmod 0 dir3/a &&
 
 	test_must_fail test-tool dir-iterator --pedantic ./dir3 \
 		>actual-no-permissions-pedantic-output &&
 	test_cmp expected-no-permissions-pedantic-output \
 		actual-no-permissions-pedantic-output &&
-	chmod 755 dir3/a &&
-	rm -rf dir3
+	chmod 755 dir3/a
 '
 
-test_expect_success SYMLINKS 'setup dirs with symlinks' '
+test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	mkdir -p dir4/a &&
 	mkdir -p dir4/b/c &&
 	>dir4/a/d &&
 	ln -s d dir4/a/e &&
-	ln -s ../b dir4/a/f &&
-
-	mkdir -p dir5/a/b &&
-	mkdir -p dir5/a/c &&
-	ln -s ../c dir5/a/b/d &&
-	ln -s ../ dir5/a/b/e &&
-	ln -s ../../ dir5/a/b/f
+	ln -s ../b dir4/a/f
 '
-
 test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
 	cat >expected-no-follow-sorted-output <<-EOF &&
 	[d] (a) [a] ./dir4/a
@@ -128,7 +122,6 @@ test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default
 
 	test_cmp expected-no-follow-sorted-output actual-no-follow-sorted-output
 '
-
 test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
 	cat >expected-follow-sorted-output <<-EOF &&
 	[d] (a) [a] ./dir4/a
@@ -146,4 +139,12 @@ test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag
 	test_cmp expected-follow-sorted-output actual-follow-sorted-output
 '
 
+test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
+	mkdir -p dir5/a/b &&
+	mkdir -p dir5/a/c &&
+	ln -s ../c dir5/a/b/d &&
+	ln -s ../ dir5/a/b/e &&
+	ln -s ../../ dir5/a/b/f
+'
+
 test_done
-- 
2.36.1

[PATCH v2 02/15] t0066: remove dependency between unrelated tests

[Plato Kiorpelidis]

Two unrelated tests were using the same path for testing. That's
incorrect because if the test that depends on the other test, which
creates the directory hierarcy, is ran using the --run flag then it
will fail, even though it should pass, because the path doesn't exist.
Unrelated tests should not have dependencies among them.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 807c43d447..801749eb7e 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -63,9 +63,11 @@ test_expect_success 'begin should fail upon inexistent paths' '
 '
 
 test_expect_success 'begin should fail upon non directory paths' '
+	>some-file &&
+
 	echo "dir_iterator_begin failure: ENOTDIR" >expected-non-dir-output &&
 
-	test_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&
+	test_must_fail test-tool dir-iterator ./some-file >actual-non-dir-output &&
 
 	test_cmp expected-non-dir-output actual-non-dir-output
 '
-- 
2.36.1

[PATCH v2 03/15] t0066: shorter expected and actual output file names

[Plato Kiorpelidis]

Change the names of expected-* and actual-* files to shorter ones to
improve readability, avoiding line breaks. There is no reason to have
descriptive expected-* and actual-* file names since subtests
descriptions fulfill this need.

If a test fails we can re-run it with --run flag and read its expected
and actual outputs, since each subtest shares the same file names for
expected and actual outputs.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 59 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 801749eb7e..b21c58ade3 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup -- dir w/ complex structure' '
 	>dir/d/e/d/a
 '
 test_expect_success 'dir-iterator should iterate through all files' '
-	cat >expected-iteration-sorted-output <<-EOF &&
+	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir/a
 	[d] (a/b) [b] ./dir/a/b
 	[d] (a/b/c) [c] ./dir/a/b/c
@@ -30,10 +30,10 @@ test_expect_success 'dir-iterator should iterate through all files' '
 	[f] (d/e/d/a) [a] ./dir/d/e/d/a
 	EOF
 
-	test-tool dir-iterator ./dir >out &&
-	sort out >./actual-iteration-sorted-output &&
+	test-tool dir-iterator ./dir >actual-out &&
+	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-iteration-sorted-output actual-iteration-sorted-output
+	test_cmp expected-sorted-out actual-sorted-out
 '
 
 test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
@@ -41,35 +41,34 @@ test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
 	>dir2/a/b/c/d
 '
 test_expect_success 'dir-iterator should list files in the correct order' '
-	cat >expected-pre-order-output <<-EOF &&
+	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir2/a
 	[d] (a/b) [b] ./dir2/a/b
 	[d] (a/b/c) [c] ./dir2/a/b/c
 	[f] (a/b/c/d) [d] ./dir2/a/b/c/d
 	EOF
 
-	test-tool dir-iterator ./dir2 >actual-pre-order-output &&
+	test-tool dir-iterator ./dir2 >actual-out &&
 
-	test_cmp expected-pre-order-output actual-pre-order-output
+	test_cmp expected-out actual-out
 '
 
 test_expect_success 'begin should fail upon inexistent paths' '
-	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-output &&
+	echo "dir_iterator_begin failure: ENOENT" >expected-out &&
 
-	test_must_fail test-tool dir-iterator ./inexistent-path \
-		>actual-inexistent-path-output &&
+	test_must_fail test-tool dir-iterator ./inexistent-path >actual-out &&
 
-	test_cmp expected-inexistent-path-output actual-inexistent-path-output
+	test_cmp expected-out actual-out
 '
 
 test_expect_success 'begin should fail upon non directory paths' '
 	>some-file &&
 
-	echo "dir_iterator_begin failure: ENOTDIR" >expected-non-dir-output &&
+	echo "dir_iterator_begin failure: ENOTDIR" >expected-out &&
 
-	test_must_fail test-tool dir-iterator ./some-file >actual-non-dir-output &&
+	test_must_fail test-tool dir-iterator ./some-file >actual-out &&
 
-	test_cmp expected-non-dir-output actual-non-dir-output
+	test_cmp expected-out actual-out
 '
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
@@ -77,28 +76,28 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
 	>dir3/a/b
 '
 test_expect_success POSIXPERM,SANITY 'advance should not fail on errors by default' '
-	cat >expected-no-permissions-output <<-EOF &&
+	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	EOF
 
 	chmod 0 dir3/a &&
 
-	test-tool dir-iterator ./dir3 >actual-no-permissions-output &&
-	test_cmp expected-no-permissions-output actual-no-permissions-output &&
+	test-tool dir-iterator ./dir3 >actual-out &&
+	test_cmp expected-out actual-out &&
+
 	chmod 755 dir3/a
 '
 test_expect_success POSIXPERM,SANITY 'advance should fail on errors, w/ pedantic flag' '
-	cat >expected-no-permissions-pedantic-output <<-EOF &&
+	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	dir_iterator_advance failure
 	EOF
 
 	chmod 0 dir3/a &&
 
-	test_must_fail test-tool dir-iterator --pedantic ./dir3 \
-		>actual-no-permissions-pedantic-output &&
-	test_cmp expected-no-permissions-pedantic-output \
-		actual-no-permissions-pedantic-output &&
+	test_must_fail test-tool dir-iterator --pedantic ./dir3 >actual-out &&
+	test_cmp expected-out actual-out &&
+
 	chmod 755 dir3/a
 '
 
@@ -110,7 +109,7 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	ln -s ../b dir4/a/f
 '
 test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
-	cat >expected-no-follow-sorted-output <<-EOF &&
+	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir4/a
 	[d] (b) [b] ./dir4/b
 	[d] (b/c) [c] ./dir4/b/c
@@ -119,13 +118,13 @@ test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default
 	[s] (a/f) [f] ./dir4/a/f
 	EOF
 
-	test-tool dir-iterator ./dir4 >out &&
-	sort out >actual-no-follow-sorted-output &&
+	test-tool dir-iterator ./dir4 >actual-out &&
+	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-no-follow-sorted-output actual-no-follow-sorted-output
+	test_cmp expected-sorted-out actual-sorted-out
 '
 test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
-	cat >expected-follow-sorted-output <<-EOF &&
+	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir4/a
 	[d] (a/f) [f] ./dir4/a/f
 	[d] (a/f/c) [c] ./dir4/a/f/c
@@ -135,10 +134,10 @@ test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag
 	[f] (a/e) [e] ./dir4/a/e
 	EOF
 
-	test-tool dir-iterator --follow-symlinks ./dir4 >out &&
-	sort out >actual-follow-sorted-output &&
+	test-tool dir-iterator --follow-symlinks ./dir4 >actual-out &&
+	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-follow-sorted-output actual-follow-sorted-output
+	test_cmp expected-sorted-out actual-sorted-out
 '
 
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
-- 
2.36.1

[PATCH v2 04/15] test-dir-iterator: consistently return EXIT_FAILURE or EXIT_SUCCESS

[Plato Kiorpelidis]

Throughout test-dir-iterator.c we were returning/exiting with either
integers or EXIT_FAILURE. Improve readability and reduce mental load
by being consistent with what test-dir-iterator returns through the
test-tool. Returning mixed constants and integers could indicate that
it matters for some reason e.g. architecture of test-tool and cmd__*
functions.

EXIT_SUCCESS and EXIT_FAILURE are specified by the C standard.
That makes the code more portable and standardized.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/helper/test-dir-iterator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index 659b6bfa81..81e931673e 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -39,7 +39,7 @@ int cmd__dir_iterator(int argc, const char **argv)
 
 	if (!diter) {
 		printf("dir_iterator_begin failure: %s\n", error_name(errno));
-		exit(EXIT_FAILURE);
+		return EXIT_FAILURE;
 	}
 
 	while ((iter_status = dir_iterator_advance(diter)) == ITER_OK) {
@@ -58,8 +58,8 @@ int cmd__dir_iterator(int argc, const char **argv)
 
 	if (iter_status != ITER_DONE) {
 		printf("dir_iterator_advance failure\n");
-		return 1;
+		return EXIT_FAILURE;
 	}
 
-	return 0;
+	return EXIT_SUCCESS;
 }
-- 
2.36.1

[PATCH v2 05/15] test-dir-iterator: print EACCES and ELOOP errno set by dir_iterator

[Plato Kiorpelidis]

Currently, only ENOENT and ENOTDIR errno names are explicitly printed
when an unsuccessful call to dir_iterator_begin occurs. In case any
other errno happens it's collapsed and ESOMETHINGELSE is printed.
ESOMETHINGELSE collapses a lot of errno error numbers which makes
explicitly testing for specific cases of directory hierarcies
impossible.

This commit allows for more thorought testing by seperating EACCES and
ELOOP errno from ESOMETHINGELSE, which makes them explicitly printed
instead of just collapsed into ESOMETHINGELSE.

Right now, there isn't any test that depends on ESOMETHINGELSE directly,
neither in EACCES or ELOOP as a result, since only errno error codes set
by unsuccessful dir_iterator_begin calls are printed and the ones set by
dir_iterator_advance are ignored. However, this commit provides the
required support that future tests need to improve test coverage.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/helper/test-dir-iterator.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index 81e931673e..cdb9269ad5 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -9,6 +9,8 @@ static const char *error_name(int error_number)
 	switch (error_number) {
 	case ENOENT: return "ENOENT";
 	case ENOTDIR: return "ENOTDIR";
+	case EACCES: return "EACCES";
+	case ELOOP: return "ELOOP";
 	default: return "ESOMETHINGELSE";
 	}
 }
-- 
2.36.1

[PATCH v2 06/15] test-dir-iterator: print errno name set by dir_iterator_advance

[Plato Kiorpelidis]

Currently, test-dir-iterator does not check for errno error codes set by
an unsuccessful dir_iterator_advance() call. We do check for them,
however, in dir_iterator_begin(). Make it such that test-dir-iterator
does checks for them in dir_iterator_advance() as well.

This improves test coverage because it provides more detailed outputs
regarding errno error codes set by dir_iterator_advance(), which in turn
allows for the addition of more thorough tests over dir-iterator.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/helper/test-dir-iterator.c | 2 +-
 t/t0066-dir-iterator.sh      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index cdb9269ad5..55d8a58836 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -59,7 +59,7 @@ int cmd__dir_iterator(int argc, const char **argv)
 	}
 
 	if (iter_status != ITER_DONE) {
-		printf("dir_iterator_advance failure\n");
+		printf("dir_iterator_advance failure: %s\n", error_name(errno));
 		return EXIT_FAILURE;
 	}
 
diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index b21c58ade3..61b02423d5 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -90,7 +90,7 @@ test_expect_success POSIXPERM,SANITY 'advance should not fail on errors by defau
 test_expect_success POSIXPERM,SANITY 'advance should fail on errors, w/ pedantic flag' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
-	dir_iterator_advance failure
+	dir_iterator_advance failure: EACCES
 	EOF
 
 	chmod 0 dir3/a &&
-- 
2.36.1

[PATCH v2 07/15] t0066: better test coverage for dir-iterator

[Plato Kiorpelidis]

dir-iterator is lacking test coverage for some cases:
  * root directory and/or subdirectories w/o perms
  * pedantic runs on directories with cyclical symlinks
  * more thorough tests without sorting expected and actual outputs

Improve test coverage by introducing tests that test ELOOP and EACCES
errno and various directory hierarchies that cover most cases.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 290 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 289 insertions(+), 1 deletion(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 61b02423d5..932b1f291c 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -5,6 +5,97 @@ test_description='Test the dir-iterator functionality'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+test_expect_success 'setup -- dir with a single file' '
+	mkdir dir1 &&
+	>dir1/a
+'
+test_expect_success 'iteration of dir with a file' '
+	cat >expected-out <<-EOF &&
+	[f] (a) [a] ./dir1/a
+	EOF
+
+	test-tool dir-iterator ./dir1 >actual-out &&
+	test_cmp expected-out actual-out
+'
+
+test_expect_success 'setup -- dir with a single dir' '
+	mkdir -p dir6/a
+'
+test_expect_success 'iteration of dir with a single dir' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir6/a
+	EOF
+
+	test-tool dir-iterator ./dir6 >actual-out &&
+	test_cmp expected-out actual-out
+'
+
+test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
+	mkdir -p dir13/a
+'
+test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test-tool dir-iterator ./dir13/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+test_expect_success POSIXPERM,SANITY 'pedantic iteration of dir w/ dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir13/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+
+test_expect_success 'setup -- dir w/ five files' '
+	mkdir dir14 &&
+	>dir14/a &&
+	>dir14/b &&
+	>dir14/c &&
+	>dir14/d &&
+	>dir14/e
+'
+test_expect_success 'iteration of dir w/ five files' '
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a) [a] ./dir14/a
+	[f] (b) [b] ./dir14/b
+	[f] (c) [c] ./dir14/c
+	[f] (d) [d] ./dir14/d
+	[f] (e) [e] ./dir14/e
+	EOF
+
+	test-tool dir-iterator ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+
+test_expect_success 'setup -- dir w/ dir w/ a file' '
+	mkdir -p dir15/a &&
+	>dir15/a/b
+'
+test_expect_success 'iteration of dir w/ dir w/ a file' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir15/a
+	[f] (a/b) [b] ./dir15/a/b
+	EOF
+
+	test-tool dir-iterator ./dir15 >actual-out &&
+	test_cmp expected-out actual-out
+'
+
 test_expect_success 'setup -- dir w/ complex structure' '
 	mkdir -p dir &&
 	mkdir -p dir/a/b/c/ &&
@@ -49,10 +140,178 @@ test_expect_success 'dir-iterator should list files in the correct order' '
 	EOF
 
 	test-tool dir-iterator ./dir2 >actual-out &&
-
 	test_cmp expected-out actual-out
 '
 
+test_expect_success POSIXPERM,SANITY \
+'setup -- dir w/ three nested dirs w/ file, second nested dir w/o perms' '
+
+	mkdir -p dir7/a/b/c &&
+	>dir7/a/b/c/d
+'
+test_expect_success POSIXPERM,SANITY \
+'iteration of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic iteration of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+
+test_expect_success 'setup -- dir w/ two dirs each w/ file' '
+	mkdir -p dir8/a &&
+	>dir8/a/b &&
+	mkdir dir8/c &&
+	>dir8/c/d
+'
+test_expect_success 'iteration of dir w/ two dirs each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir8/a
+	[f] (a/b) [b] ./dir8/a/b
+	[d] (c) [c] ./dir8/c
+	[f] (c/d) [d] ./dir8/c/d
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[d] (c) [c] ./dir8/c
+	[f] (c/d) [d] ./dir8/c/d
+	[d] (a) [a] ./dir8/a
+	[f] (a/b) [b] ./dir8/a/b
+	EOF
+
+	test-tool dir-iterator ./dir8 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
+
+test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files' '
+	mkdir -p dir9/a &&
+	>dir9/a/b &&
+	>dir9/a/c &&
+	mkdir dir9/d &&
+	>dir9/d/e
+'
+test_expect_success \
+'iteration of dir w/ two dirs, one w/ two and one w/ one files' '
+
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir9/a
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[d] (a) [a] ./dir9/a
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	EOF
+	cat >expected-out3 <<-EOF &&
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (a) [a] ./dir9/a
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	EOF
+	cat >expected-out4 <<-EOF &&
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (a) [a] ./dir9/a
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	EOF
+
+	test-tool dir-iterator ./dir9 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out ||
+		test_cmp expected-out3 actual-out ||
+		test_cmp expected-out4 actual-out
+	)
+'
+
+test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
+	mkdir -p dir10/a &&
+	>dir10/a/b &&
+	mkdir dir10/a/c &&
+	>dir10/a/c/d
+'
+test_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir10/a
+	[f] (a/b) [b] ./dir10/a/b
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[d] (a) [a] ./dir10/a
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[f] (a/b) [b] ./dir10/a/b
+	EOF
+
+	test-tool dir-iterator ./dir10/ >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
+
+test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
+	mkdir -p dir12/a &&
+	>dir12/a/b
+'
+test_expect_success POSIXPERM,SANITY 'iteration of root dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
+test_expect_success POSIXPERM,SANITY 'pedantic iteration of root dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
+
 test_expect_success 'begin should fail upon inexistent paths' '
 	echo "dir_iterator_begin failure: ENOENT" >expected-out &&
 
@@ -147,5 +406,34 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
 	ln -s ../ dir5/a/b/e &&
 	ln -s ../../ dir5/a/b/f
 '
+test_expect_success SYMLINKS 'iteration of dir w/ symlinks w/ cycle' '
+
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir5/a
+	[d] (a/b) [b] ./dir5/a/b
+	[d] (a/c) [c] ./dir5/a/c
+	[s] (a/b/d) [d] ./dir5/a/b/d
+	[s] (a/b/e) [e] ./dir5/a/b/e
+	[s] (a/b/f) [f] ./dir5/a/b/f
+	EOF
+
+	test-tool dir-iterator ./dir5 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks iteration of dir w/ symlinks w/ cycle' '
+
+	cat >expected-tailed-out <<-EOF &&
+	dir_iterator_advance failure: ELOOP
+	EOF
+
+	test_must_fail test-tool dir-iterator \
+		--pedantic --follow-symlinks ./dir5 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-tailed-out actual-tailed-out
+'
 
 test_done
-- 
2.36.1

[PATCH v2 08/15] t0066: reorder tests from simple to more complex

[Plato Kiorpelidis]

Gradually increase the complexity of the tests. This reordering helps
when testing dir-iterator, explicitly or by running the full test suite.
In case a test fails it provides additional aid searching for and finding
the part of dir-iterator that causes the test to fail, making it easier.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 62 ++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 932b1f291c..9b3ce99382 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -96,37 +96,6 @@ test_expect_success 'iteration of dir w/ dir w/ a file' '
 	test_cmp expected-out actual-out
 '
 
-test_expect_success 'setup -- dir w/ complex structure' '
-	mkdir -p dir &&
-	mkdir -p dir/a/b/c/ &&
-	>dir/b &&
-	>dir/c &&
-	mkdir -p dir/d/e/d/ &&
-	>dir/a/b/c/d &&
-	>dir/a/e &&
-	>dir/d/e/d/a
-'
-test_expect_success 'dir-iterator should iterate through all files' '
-	cat >expected-sorted-out <<-EOF &&
-	[d] (a) [a] ./dir/a
-	[d] (a/b) [b] ./dir/a/b
-	[d] (a/b/c) [c] ./dir/a/b/c
-	[d] (d) [d] ./dir/d
-	[d] (d/e) [e] ./dir/d/e
-	[d] (d/e/d) [d] ./dir/d/e/d
-	[f] (a/b/c/d) [d] ./dir/a/b/c/d
-	[f] (a/e) [e] ./dir/a/e
-	[f] (b) [b] ./dir/b
-	[f] (c) [c] ./dir/c
-	[f] (d/e/d/a) [a] ./dir/d/e/d/a
-	EOF
-
-	test-tool dir-iterator ./dir >actual-out &&
-	sort actual-out >actual-sorted-out &&
-
-	test_cmp expected-sorted-out actual-sorted-out
-'
-
 test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
 	mkdir -p dir2/a/b/c &&
 	>dir2/a/b/c/d
@@ -283,6 +252,37 @@ test_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
 	)
 '
 
+test_expect_success 'setup -- dir w/ complex structure' '
+	mkdir -p dir &&
+	mkdir -p dir/a/b/c/ &&
+	>dir/b &&
+	>dir/c &&
+	mkdir -p dir/d/e/d/ &&
+	>dir/a/b/c/d &&
+	>dir/a/e &&
+	>dir/d/e/d/a
+'
+test_expect_success 'dir-iterator should iterate through all files' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir/a
+	[d] (a/b) [b] ./dir/a/b
+	[d] (a/b/c) [c] ./dir/a/b/c
+	[d] (d) [d] ./dir/d
+	[d] (d/e) [e] ./dir/d/e
+	[d] (d/e/d) [d] ./dir/d/e/d
+	[f] (a/b/c/d) [d] ./dir/a/b/c/d
+	[f] (a/e) [e] ./dir/a/e
+	[f] (b) [b] ./dir/b
+	[f] (c) [c] ./dir/c
+	[f] (d/e/d/a) [a] ./dir/d/e/d/a
+	EOF
+
+	test-tool dir-iterator ./dir >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
 	mkdir -p dir12/a &&
 	>dir12/a/b
-- 
2.36.1

[PATCH v2 09/15] t0066: rename test directories

[Plato Kiorpelidis]

Improve readability by renaming directories such that their name
correspond to the order they appear in the file. An out of order
naming could suggest that the directory naming plays some role
in how the tests are structured and performed, which is not true.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 198 ++++++++++++++++++++--------------------
 1 file changed, 99 insertions(+), 99 deletions(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 9b3ce99382..04e51928bc 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -19,96 +19,96 @@ test_expect_success 'iteration of dir with a file' '
 '
 
 test_expect_success 'setup -- dir with a single dir' '
-	mkdir -p dir6/a
+	mkdir -p dir2/a
 '
 test_expect_success 'iteration of dir with a single dir' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir6/a
+	[d] (a) [a] ./dir2/a
 	EOF
 
-	test-tool dir-iterator ./dir6 >actual-out &&
+	test-tool dir-iterator ./dir2 >actual-out &&
 	test_cmp expected-out actual-out
 '
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
-	mkdir -p dir13/a
+	mkdir -p dir3/a
 '
 test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir13/a
+	[d] (a) [a] ./dir3/a
 	EOF
 
-	chmod 0 dir13/a &&
+	chmod 0 dir3/a &&
 
-	test-tool dir-iterator ./dir13/ >actual-out &&
+	test-tool dir-iterator ./dir3/ >actual-out &&
 	test_cmp expected-out actual-out &&
 
-	chmod 755 dir13/a
+	chmod 755 dir3/a
 '
 test_expect_success POSIXPERM,SANITY 'pedantic iteration of dir w/ dir w/o perms' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir13/a
+	[d] (a) [a] ./dir3/a
 	dir_iterator_advance failure: EACCES
 	EOF
 
-	chmod 0 dir13/a &&
+	chmod 0 dir3/a &&
 
-	test_must_fail test-tool dir-iterator --pedantic ./dir13/ >actual-out &&
+	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
 	test_cmp expected-out actual-out &&
 
-	chmod 755 dir13/a
+	chmod 755 dir3/a
 '
 
 test_expect_success 'setup -- dir w/ five files' '
-	mkdir dir14 &&
-	>dir14/a &&
-	>dir14/b &&
-	>dir14/c &&
-	>dir14/d &&
-	>dir14/e
+	mkdir dir4 &&
+	>dir4/a &&
+	>dir4/b &&
+	>dir4/c &&
+	>dir4/d &&
+	>dir4/e
 '
 test_expect_success 'iteration of dir w/ five files' '
 	cat >expected-sorted-out <<-EOF &&
-	[f] (a) [a] ./dir14/a
-	[f] (b) [b] ./dir14/b
-	[f] (c) [c] ./dir14/c
-	[f] (d) [d] ./dir14/d
-	[f] (e) [e] ./dir14/e
+	[f] (a) [a] ./dir4/a
+	[f] (b) [b] ./dir4/b
+	[f] (c) [c] ./dir4/c
+	[f] (d) [d] ./dir4/d
+	[f] (e) [e] ./dir4/e
 	EOF
 
-	test-tool dir-iterator ./dir14 >actual-out &&
+	test-tool dir-iterator ./dir4 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
 
 test_expect_success 'setup -- dir w/ dir w/ a file' '
-	mkdir -p dir15/a &&
-	>dir15/a/b
+	mkdir -p dir5/a &&
+	>dir5/a/b
 '
 test_expect_success 'iteration of dir w/ dir w/ a file' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir15/a
-	[f] (a/b) [b] ./dir15/a/b
+	[d] (a) [a] ./dir5/a
+	[f] (a/b) [b] ./dir5/a/b
 	EOF
 
-	test-tool dir-iterator ./dir15 >actual-out &&
+	test-tool dir-iterator ./dir5 >actual-out &&
 	test_cmp expected-out actual-out
 '
 
 test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
-	mkdir -p dir2/a/b/c &&
-	>dir2/a/b/c/d
+	mkdir -p dir6/a/b/c &&
+	>dir6/a/b/c/d
 '
 test_expect_success 'dir-iterator should list files in the correct order' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir2/a
-	[d] (a/b) [b] ./dir2/a/b
-	[d] (a/b/c) [c] ./dir2/a/b/c
-	[f] (a/b/c/d) [d] ./dir2/a/b/c/d
+	[d] (a) [a] ./dir6/a
+	[d] (a/b) [b] ./dir6/a/b
+	[d] (a/b/c) [c] ./dir6/a/b/c
+	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
 	EOF
 
-	test-tool dir-iterator ./dir2 >actual-out &&
+	test-tool dir-iterator ./dir6 >actual-out &&
 	test_cmp expected-out actual-out
 '
 
@@ -253,31 +253,31 @@ test_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
 '
 
 test_expect_success 'setup -- dir w/ complex structure' '
-	mkdir -p dir &&
-	mkdir -p dir/a/b/c/ &&
-	>dir/b &&
-	>dir/c &&
-	mkdir -p dir/d/e/d/ &&
-	>dir/a/b/c/d &&
-	>dir/a/e &&
-	>dir/d/e/d/a
+	mkdir -p dir11 &&
+	mkdir -p dir11/a/b/c/ &&
+	>dir11/b &&
+	>dir11/c &&
+	mkdir -p dir11/d/e/d/ &&
+	>dir11/a/b/c/d &&
+	>dir11/a/e &&
+	>dir11/d/e/d/a
 '
 test_expect_success 'dir-iterator should iterate through all files' '
 	cat >expected-sorted-out <<-EOF &&
-	[d] (a) [a] ./dir/a
-	[d] (a/b) [b] ./dir/a/b
-	[d] (a/b/c) [c] ./dir/a/b/c
-	[d] (d) [d] ./dir/d
-	[d] (d/e) [e] ./dir/d/e
-	[d] (d/e/d) [d] ./dir/d/e/d
-	[f] (a/b/c/d) [d] ./dir/a/b/c/d
-	[f] (a/e) [e] ./dir/a/e
-	[f] (b) [b] ./dir/b
-	[f] (c) [c] ./dir/c
-	[f] (d/e/d/a) [a] ./dir/d/e/d/a
+	[d] (a) [a] ./dir11/a
+	[d] (a/b) [b] ./dir11/a/b
+	[d] (a/b/c) [c] ./dir11/a/b/c
+	[d] (d) [d] ./dir11/d
+	[d] (d/e) [e] ./dir11/d/e
+	[d] (d/e/d) [d] ./dir11/d/e/d
+	[f] (a/b/c/d) [d] ./dir11/a/b/c/d
+	[f] (a/e) [e] ./dir11/a/e
+	[f] (b) [b] ./dir11/b
+	[f] (c) [c] ./dir11/c
+	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
 	EOF
 
-	test-tool dir-iterator ./dir >actual-out &&
+	test-tool dir-iterator ./dir11 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
@@ -331,93 +331,93 @@ test_expect_success 'begin should fail upon non directory paths' '
 '
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
-	mkdir -p dir3/a &&
-	>dir3/a/b
+	mkdir -p dir13/a &&
+	>dir13/a/b
 '
 test_expect_success POSIXPERM,SANITY 'advance should not fail on errors by default' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir3/a
+	[d] (a) [a] ./dir13/a
 	EOF
 
-	chmod 0 dir3/a &&
+	chmod 0 dir13/a &&
 
-	test-tool dir-iterator ./dir3 >actual-out &&
+	test-tool dir-iterator ./dir13 >actual-out &&
 	test_cmp expected-out actual-out &&
 
-	chmod 755 dir3/a
+	chmod 755 dir13/a
 '
 test_expect_success POSIXPERM,SANITY 'advance should fail on errors, w/ pedantic flag' '
 	cat >expected-out <<-EOF &&
-	[d] (a) [a] ./dir3/a
+	[d] (a) [a] ./dir13/a
 	dir_iterator_advance failure: EACCES
 	EOF
 
-	chmod 0 dir3/a &&
+	chmod 0 dir13/a &&
 
-	test_must_fail test-tool dir-iterator --pedantic ./dir3 >actual-out &&
+	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
 	test_cmp expected-out actual-out &&
 
-	chmod 755 dir3/a
+	chmod 755 dir13/a
 '
 
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
-	mkdir -p dir4/a &&
-	mkdir -p dir4/b/c &&
-	>dir4/a/d &&
-	ln -s d dir4/a/e &&
-	ln -s ../b dir4/a/f
+	mkdir -p dir14/a &&
+	mkdir -p dir14/b/c &&
+	>dir14/a/d &&
+	ln -s d dir14/a/e &&
+	ln -s ../b dir14/a/f
 '
 test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
 	cat >expected-sorted-out <<-EOF &&
-	[d] (a) [a] ./dir4/a
-	[d] (b) [b] ./dir4/b
-	[d] (b/c) [c] ./dir4/b/c
-	[f] (a/d) [d] ./dir4/a/d
-	[s] (a/e) [e] ./dir4/a/e
-	[s] (a/f) [f] ./dir4/a/f
+	[d] (a) [a] ./dir14/a
+	[d] (b) [b] ./dir14/b
+	[d] (b/c) [c] ./dir14/b/c
+	[f] (a/d) [d] ./dir14/a/d
+	[s] (a/e) [e] ./dir14/a/e
+	[s] (a/f) [f] ./dir14/a/f
 	EOF
 
-	test-tool dir-iterator ./dir4 >actual-out &&
+	test-tool dir-iterator ./dir14 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
 test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
 	cat >expected-sorted-out <<-EOF &&
-	[d] (a) [a] ./dir4/a
-	[d] (a/f) [f] ./dir4/a/f
-	[d] (a/f/c) [c] ./dir4/a/f/c
-	[d] (b) [b] ./dir4/b
-	[d] (b/c) [c] ./dir4/b/c
-	[f] (a/d) [d] ./dir4/a/d
-	[f] (a/e) [e] ./dir4/a/e
+	[d] (a) [a] ./dir14/a
+	[d] (a/f) [f] ./dir14/a/f
+	[d] (a/f/c) [c] ./dir14/a/f/c
+	[d] (b) [b] ./dir14/b
+	[d] (b/c) [c] ./dir14/b/c
+	[f] (a/d) [d] ./dir14/a/d
+	[f] (a/e) [e] ./dir14/a/e
 	EOF
 
-	test-tool dir-iterator --follow-symlinks ./dir4 >actual-out &&
+	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
 
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
-	mkdir -p dir5/a/b &&
-	mkdir -p dir5/a/c &&
-	ln -s ../c dir5/a/b/d &&
-	ln -s ../ dir5/a/b/e &&
-	ln -s ../../ dir5/a/b/f
+	mkdir -p dir15/a/b &&
+	mkdir -p dir15/a/c &&
+	ln -s ../c dir15/a/b/d &&
+	ln -s ../ dir15/a/b/e &&
+	ln -s ../../ dir15/a/b/f
 '
 test_expect_success SYMLINKS 'iteration of dir w/ symlinks w/ cycle' '
 
 	cat >expected-sorted-out <<-EOF &&
-	[d] (a) [a] ./dir5/a
-	[d] (a/b) [b] ./dir5/a/b
-	[d] (a/c) [c] ./dir5/a/c
-	[s] (a/b/d) [d] ./dir5/a/b/d
-	[s] (a/b/e) [e] ./dir5/a/b/e
-	[s] (a/b/f) [f] ./dir5/a/b/f
+	[d] (a) [a] ./dir15/a
+	[d] (a/b) [b] ./dir15/a/b
+	[d] (a/c) [c] ./dir15/a/c
+	[s] (a/b/d) [d] ./dir15/a/b/d
+	[s] (a/b/e) [e] ./dir15/a/b/e
+	[s] (a/b/f) [f] ./dir15/a/b/f
 	EOF
 
-	test-tool dir-iterator ./dir5 >actual-out &&
+	test-tool dir-iterator ./dir15 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
@@ -430,7 +430,7 @@ test_expect_success SYMLINKS \
 	EOF
 
 	test_must_fail test-tool dir-iterator \
-		--pedantic --follow-symlinks ./dir5 >actual-out &&
+		--pedantic --follow-symlinks ./dir15 >actual-out &&
 	tail -n 1 actual-out >actual-tailed-out &&
 
 	test_cmp expected-tailed-out actual-tailed-out
-- 
2.36.1

[PATCH v2 10/15] dir-iterator: refactor dir_iterator_advance()

[Plato Kiorpelidis]

Simplify dir_iterator_advance() by converting from iterative to
recursive implementation. This conversion makes dir-iterator more
maintainable for the following reasons:
  * dir-iterator iterates the file-system, which is a tree structure.
    Traditionally, tree traversals, in textbooks, lectures and online
    sources are implemented recursively and not iteratively. Therefore
    it helps reduce mental load for readers, since it's easier to follow
    as it reminds of the same tree traversals we use on tree structures.
  * recursion requires one less indentation depth because we get rid of
    the while loop and instead recurse, using the program's stack.
  * in each recursive step a set of instructions are executed and
    recursion lays out the code structurally in a better way, such that
    these instructions stand out symmetrically for each recursive step.

This makes dir-iterator easier to work with, understand and introduce
new functionality, like post-order on some directory entries, because it
reminds us of the same operations we use to traverse tree structures.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 dir-iterator.c | 223 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 146 insertions(+), 77 deletions(-)

diff --git a/dir-iterator.c b/dir-iterator.c
index b17e9f970a..3adcfbc966 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -7,8 +7,7 @@ struct dir_iterator_level {
 	DIR *dir;
 
 	/*
-	 * The length of the directory part of path at this level
-	 * (including a trailing '/'):
+	 * The length of the directory part of path at this level.
 	 */
 	size_t prefix_len;
 };
@@ -34,8 +33,9 @@ struct dir_iterator_int {
 	size_t levels_alloc;
 
 	/*
-	 * A stack of levels. levels[0] is the uppermost directory
-	 * that will be included in this iteration.
+	 * A stack of levels. levels[0] is the root directory.
+	 * It won't be included in the iteration, but iteration will happen
+	 * inside it's subdirectories.
 	 */
 	struct dir_iterator_level *levels;
 
@@ -43,36 +43,63 @@ struct dir_iterator_int {
 	unsigned int flags;
 };
 
+enum {
+	OK,
+	FAIL_ENOENT,
+	FAIL_NOT_ENOENT,
+};
+
 /*
  * Push a level in the iter stack and initialize it with information from
- * the directory pointed by iter->base->path. It is assumed that this
- * strbuf points to a valid directory path. Return 0 on success and -1
- * otherwise, setting errno accordingly and leaving the stack unchanged.
+ * the directory pointed by iter->base->path. Don't open the directory.
+ *
+ * Return 1 on success.
+ * Return 0 when `iter->base->path` isn't a directory.
  */
 static int push_level(struct dir_iterator_int *iter)
 {
 	struct dir_iterator_level *level;
 
+	if (!S_ISDIR(iter->base.st.st_mode))
+		return 0;
+
 	ALLOC_GROW(iter->levels, iter->levels_nr + 1, iter->levels_alloc);
 	level = &iter->levels[iter->levels_nr++];
 
-	if (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1]))
-		strbuf_addch(&iter->base.path, '/');
+	level->dir = NULL;
+
 	level->prefix_len = iter->base.path.len;
 
-	level->dir = opendir(iter->base.path.buf);
-	if (!level->dir) {
-		int saved_errno = errno;
-		if (errno != ENOENT) {
-			warning_errno("error opening directory '%s'",
-				      iter->base.path.buf);
-		}
-		iter->levels_nr--;
+	return 1;
+}
+
+/*
+ * Activate most recent pushed level. Stack is unchanged.
+ *
+ * Return values:
+ * OK on success.
+ * FAIL_ENOENT on failed exposure because entry does not exist.
+ * FAIL_NOT_ENOENT on failed exposure because of errno other than ENOENT.
+ */
+static int activate_level(struct dir_iterator_int *iter)
+{
+	struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
+	int saved_errno;
+
+	if (level->dir)
+		return OK;
+
+	if ((level->dir = opendir(iter->base.path.buf)) != NULL)
+		return OK;
+
+	saved_errno = errno;
+	if (errno != ENOENT) {
+		warning_errno("error opening directory '%s'", iter->base.path.buf);
 		errno = saved_errno;
-		return -1;
+		return FAIL_NOT_ENOENT;
 	}
-
-	return 0;
+	errno = saved_errno;
+	return FAIL_ENOENT;
 }
 
 /*
@@ -81,12 +108,10 @@ static int push_level(struct dir_iterator_int *iter)
  */
 static int pop_level(struct dir_iterator_int *iter)
 {
-	struct dir_iterator_level *level =
-		&iter->levels[iter->levels_nr - 1];
+	struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
 
 	if (level->dir && closedir(level->dir))
-		warning_errno("error closing directory '%s'",
-			      iter->base.path.buf);
+		warning_errno("error closing directory '%s'", iter->base.path.buf);
 	level->dir = NULL;
 
 	return --iter->levels_nr;
@@ -94,82 +119,119 @@ static int pop_level(struct dir_iterator_int *iter)
 
 /*
  * Populate iter->base with the necessary information on the next iteration
- * entry, represented by the given dirent de. Return 0 on success and -1
- * otherwise, setting errno accordingly.
+ * entry, represented by the given relative path to the lowermost directory,
+ * d_name.
+ *
+ * Return values:
+ * OK on successful exposure of the provided entry.
+ * FAIL_ENOENT on failed exposure because entry does not exist.
+ * FAIL_NOT_ENOENT on failed exposure because of errno other than ENOENT.
  */
-static int prepare_next_entry_data(struct dir_iterator_int *iter,
-				   struct dirent *de)
+static int expose_entry(struct dir_iterator_int *iter, char *d_name)
 {
-	int err, saved_errno;
+	int stat_err;
 
-	strbuf_addstr(&iter->base.path, de->d_name);
-	/*
-	 * We have to reset these because the path strbuf might have
-	 * been realloc()ed at the previous strbuf_addstr().
-	 */
-	iter->base.relative_path = iter->base.path.buf +
-				   iter->levels[0].prefix_len;
-	iter->base.basename = iter->base.path.buf +
-			      iter->levels[iter->levels_nr - 1].prefix_len;
+	strbuf_addch(&iter->base.path, '/');
+	strbuf_addstr(&iter->base.path, d_name);
 
 	if (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)
-		err = stat(iter->base.path.buf, &iter->base.st);
+		stat_err = stat(iter->base.path.buf, &iter->base.st);
 	else
-		err = lstat(iter->base.path.buf, &iter->base.st);
+		stat_err = lstat(iter->base.path.buf, &iter->base.st);
 
-	saved_errno = errno;
-	if (err && errno != ENOENT)
+	if (stat_err && errno != ENOENT) {
 		warning_errno("failed to stat '%s'", iter->base.path.buf);
+		return FAIL_NOT_ENOENT;
+	} else if (stat_err && errno == ENOENT) {
+		return FAIL_ENOENT;
+	}
 
-	errno = saved_errno;
-	return err;
+	/*
+	 * We have to reset relative path and basename because the path strbuf
+	 * might have been realloc()'ed at the previous strbuf_addstr().
+	 */
+
+	iter->base.relative_path =
+		iter->base.path.buf + iter->levels[0].prefix_len + 1;
+	iter->base.basename =
+		iter->base.path.buf + iter->levels[iter->levels_nr - 1].prefix_len + 1;
+
+	return OK;
 }
 
 int dir_iterator_advance(struct dir_iterator *dir_iterator)
 {
-	struct dir_iterator_int *iter =
-		(struct dir_iterator_int *)dir_iterator;
+	struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
+	struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
+	struct dirent *dir_entry = NULL;
+	int expose_err, activate_err;
+	/* For shorter code width-wise, more readable */
+	unsigned int PEDANTIC = iter->flags & DIR_ITERATOR_PEDANTIC;
 
-	if (S_ISDIR(iter->base.st.st_mode) && push_level(iter)) {
-		if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
-			goto error_out;
-		if (iter->levels_nr == 0)
+	/*
+	 * Attempt to open the directory of the last level if not opened yet.
+	 *
+	 * Remember that we ignore ENOENT errors so that the user of this API
+	 * can remove entries between calls to `dir_iterator_advance()`.
+	 * We care for errors other than ENOENT only when PEDANTIC is enabled.
+	 */
+
+	activate_err = activate_level(iter);
+
+	if (activate_err == FAIL_NOT_ENOENT && PEDANTIC) {
+		goto error_out;
+	} else if (activate_err != OK) {
+		--iter->levels_nr;
+
+		if (iter->levels_nr == 0)  /* Failed to open root directory */
 			goto error_out;
+
+		return dir_iterator_advance(dir_iterator);
 	}
 
-	/* Loop until we find an entry that we can give back to the caller. */
-	while (1) {
-		struct dirent *de;
-		struct dir_iterator_level *level =
-			&iter->levels[iter->levels_nr - 1];
+	strbuf_setlen(&iter->base.path, level->prefix_len);
+
+	errno = 0;
+	dir_entry = readdir(level->dir);
+
+	if (!dir_entry) {
+		if (errno) {
+			warning_errno("errno reading dir '%s'", iter->base.path.buf);
+			if (PEDANTIC)
+				goto error_out;
 
-		strbuf_setlen(&iter->base.path, level->prefix_len);
-		errno = 0;
-		de = readdir(level->dir);
-
-		if (!de) {
-			if (errno) {
-				warning_errno("error reading directory '%s'",
-					      iter->base.path.buf);
-				if (iter->flags & DIR_ITERATOR_PEDANTIC)
-					goto error_out;
-			} else if (pop_level(iter) == 0) {
+			return dir_iterator_advance(dir_iterator);
+		} else {
+			/*
+			 * Current directory has been iterated through.
+			 */
+
+			if (pop_level(iter) == 0)
 				return dir_iterator_abort(dir_iterator);
-			}
-			continue;
+
+			return dir_iterator_advance(dir_iterator);
 		}
+	}
 
-		if (is_dot_or_dotdot(de->d_name))
-			continue;
+	if (is_dot_or_dotdot(dir_entry->d_name))
+		return dir_iterator_advance(dir_iterator);
 
-		if (prepare_next_entry_data(iter, de)) {
-			if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
-				goto error_out;
-			continue;
-		}
+	/*
+	 * Successfully read entry from current directory level.
+	 */
 
-		return ITER_OK;
-	}
+	expose_err = expose_entry(iter, dir_entry->d_name);
+
+	if (expose_err == FAIL_NOT_ENOENT && PEDANTIC)
+		goto error_out;
+
+	if (expose_err == OK)
+		push_level(iter);
+
+	if (expose_err != OK)
+		return dir_iterator_advance(dir_iterator);
+
+	return ITER_OK;
 
 error_out:
 	dir_iterator_abort(dir_iterator);
@@ -207,6 +269,8 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
 
 	strbuf_init(&iter->base.path, PATH_MAX);
 	strbuf_addstr(&iter->base.path, path);
+	/* expose_entry() appends dir seperator before exposing an entry */
+	strbuf_trim_trailing_dir_sep(&iter->base.path);
 
 	ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
 	iter->levels_nr = 0;
@@ -226,6 +290,11 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
 		goto error_out;
 	}
 
+	if (!push_level(iter)) {
+		saved_errno = ENOTDIR;
+		goto error_out;
+	}
+
 	return dir_iterator;
 
 error_out:
-- 
2.36.1

[PATCH v2 11/15] dir-iterator: open root dir in dir_iterator_begin()

[Plato Kiorpelidis]

When we call dir_iterator_begin() and the provided path is invalid, the
call won't fail and return NULL. Instead, the call succeeds, and the
first call dir_iterator_advance() fails. That's unexpected behavior from
the perspective of a user of this API. The expected behavior would be to
fail and return NULL from dir_iterator_begin() if the provided path is
invalid. Successful call to dir_iterator_begin() suggests that the root
path is valid.

To deal with this, call activate_level() in dir_iterator_begin() which
opens the provided root path and confirms it's a valid directory path.
By doing this we can return NULL from dir_iterator_begin() in case the
provided root path isn't valid, implementing the expected behavior.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 dir-iterator.c          | 13 ++++++++++---
 t/t0066-dir-iterator.sh |  4 ++--
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/dir-iterator.c b/dir-iterator.c
index 3adcfbc966..c36f549a78 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -181,10 +181,12 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 	if (activate_err == FAIL_NOT_ENOENT && PEDANTIC) {
 		goto error_out;
 	} else if (activate_err != OK) {
-		--iter->levels_nr;
+		/*
+		 * We activate the root level at `dir_iterator_begin()`.
+		 * Therefore, there isn't any case to run out of levels.
+		 */
 
-		if (iter->levels_nr == 0)  /* Failed to open root directory */
-			goto error_out;
+		--iter->levels_nr;
 
 		return dir_iterator_advance(dir_iterator);
 	}
@@ -295,6 +297,11 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
 		goto error_out;
 	}
 
+	if (activate_level(iter) != OK) {
+		saved_errno = errno;
+		goto error_out;
+	}
+
 	return dir_iterator;
 
 error_out:
diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 04e51928bc..52b0217bde 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -289,7 +289,7 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
 '
 test_expect_success POSIXPERM,SANITY 'iteration of root dir w/o perms' '
 	cat >expected-out <<-EOF &&
-	dir_iterator_advance failure: EACCES
+	dir_iterator_begin failure: EACCES
 	EOF
 
 	chmod 0 dir12 &&
@@ -301,7 +301,7 @@ test_expect_success POSIXPERM,SANITY 'iteration of root dir w/o perms' '
 '
 test_expect_success POSIXPERM,SANITY 'pedantic iteration of root dir w/o perms' '
 	cat >expected-out <<-EOF &&
-	dir_iterator_advance failure: EACCES
+	dir_iterator_begin failure: EACCES
 	EOF
 
 	chmod 0 dir12 &&
-- 
2.36.1

[PATCH v2 12/15] t0066: rename subtest descriptions

[Plato Kiorpelidis]

Improve the structure of dir-iterator tests by renaming subtest
descriptions. This makes the introduction of new tests for future
iteration schemes easier, as they will share similar naming, only
differentiating over the iteration scheme part of the description
that the subtest is testing for.

These structured subtest descriptions also help when running the tests.
It's easier to understand what's tested and in case a test fails find
out which iteration scheme failed.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 t/t0066-dir-iterator.sh | 49 +++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 52b0217bde..8ab8811fb5 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -9,7 +9,7 @@ test_expect_success 'setup -- dir with a single file' '
 	mkdir dir1 &&
 	>dir1/a
 '
-test_expect_success 'iteration of dir with a file' '
+test_expect_success 'dirs-before of dir with a file' '
 	cat >expected-out <<-EOF &&
 	[f] (a) [a] ./dir1/a
 	EOF
@@ -21,7 +21,7 @@ test_expect_success 'iteration of dir with a file' '
 test_expect_success 'setup -- dir with a single dir' '
 	mkdir -p dir2/a
 '
-test_expect_success 'iteration of dir with a single dir' '
+test_expect_success 'dirs-before of dir with a single dir' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir2/a
 	EOF
@@ -33,7 +33,7 @@ test_expect_success 'iteration of dir with a single dir' '
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
 	mkdir -p dir3/a
 '
-test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	EOF
@@ -45,7 +45,7 @@ test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
 
 	chmod 755 dir3/a
 '
-test_expect_success POSIXPERM,SANITY 'pedantic iteration of dir w/ dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of dir w/ dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	dir_iterator_advance failure: EACCES
@@ -67,7 +67,7 @@ test_expect_success 'setup -- dir w/ five files' '
 	>dir4/d &&
 	>dir4/e
 '
-test_expect_success 'iteration of dir w/ five files' '
+test_expect_success 'dirs-before of dir w/ five files' '
 	cat >expected-sorted-out <<-EOF &&
 	[f] (a) [a] ./dir4/a
 	[f] (b) [b] ./dir4/b
@@ -86,7 +86,7 @@ test_expect_success 'setup -- dir w/ dir w/ a file' '
 	mkdir -p dir5/a &&
 	>dir5/a/b
 '
-test_expect_success 'iteration of dir w/ dir w/ a file' '
+test_expect_success 'dirs-before of dir w/ dir w/ a file' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir5/a
 	[f] (a/b) [b] ./dir5/a/b
@@ -100,7 +100,7 @@ test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
 	mkdir -p dir6/a/b/c &&
 	>dir6/a/b/c/d
 '
-test_expect_success 'dir-iterator should list files in the correct order' '
+test_expect_success 'dirs-before of dir w/ three nested dirs w/ file' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir6/a
 	[d] (a/b) [b] ./dir6/a/b
@@ -119,7 +119,7 @@ test_expect_success POSIXPERM,SANITY \
 	>dir7/a/b/c/d
 '
 test_expect_success POSIXPERM,SANITY \
-'iteration of dir w/ three nested dirs w/ file, second w/o perms' '
+'dirs-before of dir w/ three nested dirs w/ file, second w/o perms' '
 
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir7/a
@@ -134,7 +134,7 @@ test_expect_success POSIXPERM,SANITY \
 	chmod 755 dir7/a/b
 '
 test_expect_success POSIXPERM,SANITY \
-'pedantic iteration of dir w/ three nested dirs w/ file, second w/o perms' '
+'pedantic dirs-before of dir w/ three nested dirs w/ file, second w/o perms' '
 
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir7/a
@@ -156,7 +156,7 @@ test_expect_success 'setup -- dir w/ two dirs each w/ file' '
 	mkdir dir8/c &&
 	>dir8/c/d
 '
-test_expect_success 'iteration of dir w/ two dirs each w/ file' '
+test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
 	cat >expected-out1 <<-EOF &&
 	[d] (a) [a] ./dir8/a
 	[f] (a/b) [b] ./dir8/a/b
@@ -185,7 +185,7 @@ test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files'
 	>dir9/d/e
 '
 test_expect_success \
-'iteration of dir w/ two dirs, one w/ two and one w/ one files' '
+'dirs-before of dir w/ two dirs, one w/ two and one w/ one files' '
 
 	cat >expected-out1 <<-EOF &&
 	[d] (a) [a] ./dir9/a
@@ -231,7 +231,7 @@ test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
 	mkdir dir10/a/c &&
 	>dir10/a/c/d
 '
-test_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
+test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
 	cat >expected-out1 <<-EOF &&
 	[d] (a) [a] ./dir10/a
 	[f] (a/b) [b] ./dir10/a/b
@@ -262,7 +262,7 @@ test_expect_success 'setup -- dir w/ complex structure' '
 	>dir11/a/e &&
 	>dir11/d/e/d/a
 '
-test_expect_success 'dir-iterator should iterate through all files' '
+test_expect_success 'dirs-before of dir w/ complex structure' '
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir11/a
 	[d] (a/b) [b] ./dir11/a/b
@@ -287,7 +287,7 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
 	mkdir -p dir12/a &&
 	>dir12/a/b
 '
-test_expect_success POSIXPERM,SANITY 'iteration of root dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	dir_iterator_begin failure: EACCES
 	EOF
@@ -299,7 +299,7 @@ test_expect_success POSIXPERM,SANITY 'iteration of root dir w/o perms' '
 
 	chmod 755 dir12
 '
-test_expect_success POSIXPERM,SANITY 'pedantic iteration of root dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	dir_iterator_begin failure: EACCES
 	EOF
@@ -334,7 +334,9 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
 	mkdir -p dir13/a &&
 	>dir13/a/b
 '
-test_expect_success POSIXPERM,SANITY 'advance should not fail on errors by default' '
+test_expect_success POSIXPERM,SANITY \
+'dirs-before of dir w/ dir w/o perms w/ file' '
+
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir13/a
 	EOF
@@ -346,7 +348,9 @@ test_expect_success POSIXPERM,SANITY 'advance should not fail on errors by defau
 
 	chmod 755 dir13/a
 '
-test_expect_success POSIXPERM,SANITY 'advance should fail on errors, w/ pedantic flag' '
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before of dir w/ dir w/o perms w/ file' '
+
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir13/a
 	dir_iterator_advance failure: EACCES
@@ -367,7 +371,7 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	ln -s d dir14/a/e &&
 	ln -s ../b dir14/a/f
 '
-test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
+test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/o cycle' '
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir14/a
 	[d] (b) [b] ./dir14/b
@@ -382,7 +386,9 @@ test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
-test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-before of dir w/ symlinks w/o cycle' '
+
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir14/a
 	[d] (a/f) [f] ./dir14/a/f
@@ -406,8 +412,7 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
 	ln -s ../ dir15/a/b/e &&
 	ln -s ../../ dir15/a/b/f
 '
-test_expect_success SYMLINKS 'iteration of dir w/ symlinks w/ cycle' '
-
+test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/ cycle' '
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir15/a
 	[d] (a/b) [b] ./dir15/a/b
@@ -423,7 +428,7 @@ test_expect_success SYMLINKS 'iteration of dir w/ symlinks w/ cycle' '
 	test_cmp expected-sorted-out actual-sorted-out
 '
 test_expect_success SYMLINKS \
-'pedantic follow-symlinks iteration of dir w/ symlinks w/ cycle' '
+'pedantic follow-symlinks dirs-before of dir w/ symlinks w/ cycle' '
 
 	cat >expected-tailed-out <<-EOF &&
 	dir_iterator_advance failure: ELOOP
-- 
2.36.1

[PATCH v2 13/15] dir-iterator: option to iterate dirs in pre-order

[Plato Kiorpelidis]

Introduce a new option to dir-iterator, using dir_iterator_begin()
flags parameter, allowing to control whether or not directories will
be exposed before their contents. In essence, pre-order traversal over
file system entries that are directories.

This changes the default behavior of the dir-iterator API. Instead
of iterating directories before doing so over their contents, the new
default behavior does not expose directories at all. Iteration is still
performed, however, within directories, iterating over any other entry.
Only directory paths are ignored.

To iterate over directories in pre-order, reproducing the previous
default behavior, enable the new flag DIR_ITERATOR_DIRS_BEFORE in the
flags parameter of dir_iterator_begin():
  * ignore directories by not setting DIR_ITERATOR_DIRS_BEFORE
  * iterate directories pre-order by enabling DIR_ITERATOR_DIRS_BEFORE

Adjust existing callers, in refs/files-backend.c and builtin/clone.c
to enable DIR_ITERATOR_DIRS_BEFORE since these callers require iteration
over directories before doing so over their contents.

Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to test
the new iteration scheme, which is the new default behavior, and the new
flag DIR_ITERATOR_DIRS_BEFORE which reproduces the old default behavior.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 builtin/clone.c              |   4 +-
 dir-iterator.c               |  29 +++-
 dir-iterator.h               |  29 +++-
 refs/files-backend.c         |   2 +-
 t/helper/test-dir-iterator.c |  12 +-
 t/t0066-dir-iterator.sh      | 321 ++++++++++++++++++++++++++++++++---
 6 files changed, 361 insertions(+), 36 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 194d50f75f..68787623e7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -321,7 +321,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 
 	mkdir_if_missing(dest->buf, 0777);
 
-	flags = DIR_ITERATOR_PEDANTIC | DIR_ITERATOR_FOLLOW_SYMLINKS;
+	flags = DIR_ITERATOR_DIRS_BEFORE |
+		DIR_ITERATOR_PEDANTIC |
+		DIR_ITERATOR_FOLLOW_SYMLINKS;
 	iter = dir_iterator_begin(src->buf, flags);
 
 	if (!iter)
diff --git a/dir-iterator.c b/dir-iterator.c
index c36f549a78..c1475add27 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -47,6 +47,7 @@ enum {
 	OK,
 	FAIL_ENOENT,
 	FAIL_NOT_ENOENT,
+	FAIL_IGN_DIRS,
 };
 
 /*
@@ -124,12 +125,14 @@ static int pop_level(struct dir_iterator_int *iter)
  *
  * Return values:
  * OK on successful exposure of the provided entry.
+ * FAIL_IGN_DIR on failed exposure because entry is dir and flags don't allow it.
  * FAIL_ENOENT on failed exposure because entry does not exist.
  * FAIL_NOT_ENOENT on failed exposure because of errno other than ENOENT.
  */
-static int expose_entry(struct dir_iterator_int *iter, char *d_name)
+static int expose_entry(struct dir_iterator_int *iter, char *d_name, char *dir_state)
 {
 	int stat_err;
+	unsigned int DIRS_BEFORE = iter->flags & DIR_ITERATOR_DIRS_BEFORE;
 
 	strbuf_addch(&iter->base.path, '/');
 	strbuf_addstr(&iter->base.path, d_name);
@@ -146,6 +149,17 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name)
 		return FAIL_ENOENT;
 	}
 
+	/*
+	 * We've got to check whether or not this is a directory. We need to
+	 * perform this check since the user could've requested to ignore
+	 * directory entries.
+	 */
+
+	if (S_ISDIR(iter->base.st.st_mode)) {
+		if (!DIRS_BEFORE && !strcmp(dir_state, "before"))
+			return FAIL_IGN_DIRS;
+	}
+
 	/*
 	 * We have to reset relative path and basename because the path strbuf
 	 * might have been realloc()'ed at the previous strbuf_addstr().
@@ -220,14 +234,23 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 
 	/*
 	 * Successfully read entry from current directory level.
+	 * In case it's a directory, we need to check, before exposing it, if
+	 * it's allowed because of DIRS_BEFORE. In any case - allowed or not -
+	 * we must push the directory to the levels stack, so the next call will
+	 * read from it.
+	 */
+
+	/*
+	 * 'expose_entry()' function needs to know whether
+	 * the exposure call is about DIRS_BEFORE or DIRS_AFTER.
 	 */
 
-	expose_err = expose_entry(iter, dir_entry->d_name);
+	expose_err = expose_entry(iter, dir_entry->d_name, "before");
 
 	if (expose_err == FAIL_NOT_ENOENT && PEDANTIC)
 		goto error_out;
 
-	if (expose_err == OK)
+	if (expose_err == OK || expose_err == FAIL_IGN_DIRS)
 		push_level(iter);
 
 	if (expose_err != OK)
diff --git a/dir-iterator.h b/dir-iterator.h
index 08229157c6..c1d16a8c6d 100644
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -8,19 +8,22 @@
  *
  * Iterate over a directory tree, recursively, including paths of all
  * types and hidden paths. Skip "." and ".." entries and don't follow
- * symlinks except for the original path. Note that the original path
- * is not included in the iteration.
+ * symlinks except when DIR_ITERATOR_FOLLOW_SYMLINKS is set. If root
+ * path is a symlink it's followed regardless of flags. Note that the
+ * original path is not included in the iteration.
  *
  * Every time dir_iterator_advance() is called, update the members of
  * the dir_iterator structure to reflect the next path in the
  * iteration. The order that paths are iterated over within a
- * directory is undefined, directory paths are always given before
- * their contents.
+ * directory is undefined. Directory paths are given before their
+ * contents when DIR_ITERATOR_DIRS_BEFORE is set. Failure to set this
+ * flag results in directory paths not being exposed. Instead, iteration
+ * will happen within directories. Their contents will be exposed.
  *
  * A typical iteration looks like this:
  *
  *     int ok;
- *     unsigned int flags = DIR_ITERATOR_PEDANTIC;
+ *     unsigned int flags = DIR_ITERATOR_PEDANTIC | DIR_ITERATOR_DIRS_BEFORE;
  *     struct dir_iterator *iter = dir_iterator_begin(path, flags);
  *
  *     if (!iter)
@@ -61,12 +64,19 @@
  *   not the symlinks themselves, which is the default behavior. Broken
  *   symlinks are ignored.
  *
+ * - DIR_ITERATOR_DIRS_BEFORE: make dir-iterator expose a directory path
+ *   before iterating through that directory's contents.
+ *
+ * Note: Activating none of the flags will iterate through directories'
+ * contents but won't expose the directory paths.
+ *
  * Warning: circular symlinks are also followed when
  * DIR_ITERATOR_FOLLOW_SYMLINKS is set. The iteration may end up with
  * an ELOOP if they happen and DIR_ITERATOR_PEDANTIC is set.
  */
 #define DIR_ITERATOR_PEDANTIC (1 << 0)
 #define DIR_ITERATOR_FOLLOW_SYMLINKS (1 << 1)
+#define DIR_ITERATOR_DIRS_BEFORE (1 << 2)
 
 struct dir_iterator {
 	/* The current path: */
@@ -97,12 +107,13 @@ struct dir_iterator {
  * failure, return NULL and set errno accordingly.
  *
  * The iteration includes all paths under path, not including path
- * itself and not including "." or ".." entries.
+ * itself, "." or ".." entries and directories according to specified flags.
  *
  * Parameters are:
  *  - path is the starting directory. An internal copy will be made.
  *  - flags is a combination of the possible flags to initialize a
- *    dir-iterator or 0 for default behavior.
+ *    dir-iterator or 0 for default behavior which will ignore directory
+ *    paths, but will include the rest directory contents.
  */
 struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
 
@@ -110,6 +121,7 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
  * Advance the iterator to the first or next item and return ITER_OK.
  * If the iteration is exhausted, free the dir_iterator and any
  * resources associated with it and return ITER_DONE.
+ * On error, free dir_iterator memory and return ITER_ERROR.
  *
  * It is a bug to use iterator or call this function again after it
  * has returned ITER_DONE or ITER_ERROR (which may be returned iff
@@ -119,8 +131,7 @@ int dir_iterator_advance(struct dir_iterator *iterator);
 
 /*
  * End the iteration before it has been exhausted. Free the
- * dir_iterator and any associated resources and return ITER_DONE. On
- * error, free the dir_iterator and return ITER_ERROR.
+ * dir_iterator and any associated resources and return ITER_DONE.
  */
 int dir_iterator_abort(struct dir_iterator *iterator);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8db7882aac..812f00c0f4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2237,7 +2237,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
 
 	strbuf_addf(&sb, "%s/logs", gitdir);
 
-	diter = dir_iterator_begin(sb.buf, 0);
+	diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_DIRS_BEFORE);
 	if (!diter) {
 		strbuf_release(&sb);
 		return empty_ref_iterator_begin();
diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index 55d8a58836..f05d5fde9d 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -17,7 +17,15 @@ static const char *error_name(int error_number)
 
 /*
  * usage:
- * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path
+ * test-tool dir-iterator [OPTIONS] directory_path
+ *
+ * OPTIONS
+ *	--follow-symlinks
+ *	--pedantic
+ *	--dirs-before
+ *
+ * example:
+ * test-tool dir-iterator --pedantic --dirs-before ./somedir
  */
 int cmd__dir_iterator(int argc, const char **argv)
 {
@@ -30,6 +38,8 @@ int cmd__dir_iterator(int argc, const char **argv)
 			flags |= DIR_ITERATOR_FOLLOW_SYMLINKS;
 		else if (strcmp(*argv, "--pedantic") == 0)
 			flags |= DIR_ITERATOR_PEDANTIC;
+		else if (strcmp(*argv, "--dirs-before") == 0)
+			flags |= DIR_ITERATOR_DIRS_BEFORE;
 		else
 			die("invalid option '%s'", *argv);
 	}
diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 8ab8811fb5..badd82d8a4 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -9,7 +9,7 @@ test_expect_success 'setup -- dir with a single file' '
 	mkdir dir1 &&
 	>dir1/a
 '
-test_expect_success 'dirs-before of dir with a file' '
+test_expect_success 'dirs-ignore of dir with a file' '
 	cat >expected-out <<-EOF &&
 	[f] (a) [a] ./dir1/a
 	EOF
@@ -17,22 +17,60 @@ test_expect_success 'dirs-before of dir with a file' '
 	test-tool dir-iterator ./dir1 >actual-out &&
 	test_cmp expected-out actual-out
 '
+test_expect_success 'dirs-before of dir with a file' '
+	cat >expected-out <<-EOF &&
+	[f] (a) [a] ./dir1/a
+	EOF
+
+	test-tool dir-iterator --dirs-before ./dir1 >actual-out &&
+	test_cmp expected-out actual-out
+'
 
 test_expect_success 'setup -- dir with a single dir' '
 	mkdir -p dir2/a
 '
+test_expect_success 'dirs-ignore of dir with a single dir' '
+	cat >expected-out <<-EOF &&
+	EOF
+
+	test-tool dir-iterator ./dir2 >actual-out &&
+	test_cmp expected-out actual-out
+'
 test_expect_success 'dirs-before of dir with a single dir' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir2/a
 	EOF
 
-	test-tool dir-iterator ./dir2 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir2 >actual-out &&
 	test_cmp expected-out actual-out
 '
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
 	mkdir -p dir3/a
 '
+test_expect_success POSIXPERM,SANITY 'dirs-ignore of dir w/ dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	EOF
+
+	chmod 0 dir3/a &&
+
+	test-tool dir-iterator ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-ignore of dir w/ dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir3/a &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir3/a
+'
 test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
@@ -40,7 +78,7 @@ test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
 
 	chmod 0 dir3/a &&
 
-	test-tool dir-iterator ./dir3/ >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir3/ >actual-out &&
 	test_cmp expected-out actual-out &&
 
 	chmod 755 dir3/a
@@ -53,7 +91,8 @@ test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of dir w/ dir w/o per
 
 	chmod 0 dir3/a &&
 
-	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic ./dir3/ >actual-out &&
 	test_cmp expected-out actual-out &&
 
 	chmod 755 dir3/a
@@ -67,7 +106,7 @@ test_expect_success 'setup -- dir w/ five files' '
 	>dir4/d &&
 	>dir4/e
 '
-test_expect_success 'dirs-before of dir w/ five files' '
+test_expect_success 'dirs-ignore of dir w/ five files' '
 	cat >expected-sorted-out <<-EOF &&
 	[f] (a) [a] ./dir4/a
 	[f] (b) [b] ./dir4/b
@@ -81,18 +120,40 @@ test_expect_success 'dirs-before of dir w/ five files' '
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
+test_expect_success 'dirs-before of dir w/ five files' '
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a) [a] ./dir4/a
+	[f] (b) [b] ./dir4/b
+	[f] (c) [c] ./dir4/c
+	[f] (d) [d] ./dir4/d
+	[f] (e) [e] ./dir4/e
+	EOF
+
+	test-tool dir-iterator --dirs-before ./dir4 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
 
 test_expect_success 'setup -- dir w/ dir w/ a file' '
 	mkdir -p dir5/a &&
 	>dir5/a/b
 '
+test_expect_success 'dirs-ignore of dir w/ dir w/ a file' '
+	cat >expected-out <<-EOF &&
+	[f] (a/b) [b] ./dir5/a/b
+	EOF
+
+	test-tool dir-iterator ./dir5 >actual-out &&
+	test_cmp expected-out actual-out
+'
 test_expect_success 'dirs-before of dir w/ dir w/ a file' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir5/a
 	[f] (a/b) [b] ./dir5/a/b
 	EOF
 
-	test-tool dir-iterator ./dir5 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir5 >actual-out &&
 	test_cmp expected-out actual-out
 '
 
@@ -100,6 +161,14 @@ test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
 	mkdir -p dir6/a/b/c &&
 	>dir6/a/b/c/d
 '
+test_expect_success 'dirs-ignore of dir w/ three nested dirs w/ file' '
+	cat >expected-out <<-EOF &&
+	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
+	EOF
+
+	test-tool dir-iterator ./dir6 >actual-out &&
+	test_cmp expected-out actual-out
+'
 test_expect_success 'dirs-before of dir w/ three nested dirs w/ file' '
 	cat >expected-out <<-EOF &&
 	[d] (a) [a] ./dir6/a
@@ -108,7 +177,7 @@ test_expect_success 'dirs-before of dir w/ three nested dirs w/ file' '
 	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
 	EOF
 
-	test-tool dir-iterator ./dir6 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir6 >actual-out &&
 	test_cmp expected-out actual-out
 '
 
@@ -119,6 +188,33 @@ test_expect_success POSIXPERM,SANITY \
 	>dir7/a/b/c/d
 '
 test_expect_success POSIXPERM,SANITY \
+'dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
 'dirs-before of dir w/ three nested dirs w/ file, second w/o perms' '
 
 	cat >expected-out <<-EOF &&
@@ -128,7 +224,7 @@ test_expect_success POSIXPERM,SANITY \
 
 	chmod 0 dir7/a/b &&
 
-	test-tool dir-iterator ./dir7 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir7 >actual-out &&
 	test_cmp expected-out actual-out &&
 
 	chmod 755 dir7/a/b
@@ -144,7 +240,8 @@ test_expect_success POSIXPERM,SANITY \
 
 	chmod 0 dir7/a/b &&
 
-	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic ./dir7 >actual-out &&
 	test_cmp expected-out actual-out &&
 
 	chmod 755 dir7/a/b
@@ -156,6 +253,22 @@ test_expect_success 'setup -- dir w/ two dirs each w/ file' '
 	mkdir dir8/c &&
 	>dir8/c/d
 '
+test_expect_success 'dirs-ignore of dir w/ two dirs each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir8/a/b
+	[f] (c/d) [d] ./dir8/c/d
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[f] (c/d) [d] ./dir8/c/d
+	[f] (a/b) [b] ./dir8/a/b
+	EOF
+
+	test-tool dir-iterator ./dir8 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
 test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
 	cat >expected-out1 <<-EOF &&
 	[d] (a) [a] ./dir8/a
@@ -170,7 +283,7 @@ test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
 	[f] (a/b) [b] ./dir8/a/b
 	EOF
 
-	test-tool dir-iterator ./dir8 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir8 >actual-out &&
 	(
 		test_cmp expected-out1 actual-out ||
 		test_cmp expected-out2 actual-out
@@ -185,6 +298,38 @@ test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files'
 	>dir9/d/e
 '
 test_expect_success \
+'dirs-ignore of dir w/ two dirs, one w/ two and one w/ one files' '
+
+	cat >expected-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (d/e) [e] ./dir9/d/e
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (d/e) [e] ./dir9/d/e
+	EOF
+	cat >expected-out3 <<-EOF &&
+	[f] (d/e) [e] ./dir9/d/e
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	EOF
+	cat >expected-out4 <<-EOF &&
+	[f] (d/e) [e] ./dir9/d/e
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	EOF
+
+	test-tool dir-iterator ./dir9 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out ||
+		test_cmp expected-out3 actual-out ||
+		test_cmp expected-out4 actual-out
+	)
+'
+test_expect_success \
 'dirs-before of dir w/ two dirs, one w/ two and one w/ one files' '
 
 	cat >expected-out1 <<-EOF &&
@@ -216,7 +361,7 @@ test_expect_success \
 	[f] (a/b) [b] ./dir9/a/b
 	EOF
 
-	test-tool dir-iterator ./dir9 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir9 >actual-out &&
 	(
 		test_cmp expected-out1 actual-out ||
 		test_cmp expected-out2 actual-out ||
@@ -231,6 +376,22 @@ test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
 	mkdir dir10/a/c &&
 	>dir10/a/c/d
 '
+test_expect_success 'dirs-ignore of dir w/ two nested dirs, each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir10/a/b
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[f] (a/b) [b] ./dir10/a/b
+	EOF
+
+	test-tool dir-iterator ./dir10/ >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
 test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
 	cat >expected-out1 <<-EOF &&
 	[d] (a) [a] ./dir10/a
@@ -245,7 +406,7 @@ test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
 	[f] (a/b) [b] ./dir10/a/b
 	EOF
 
-	test-tool dir-iterator ./dir10/ >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir10/ >actual-out &&
 	(
 		test_cmp expected-out1 actual-out ||
 		test_cmp expected-out2 actual-out
@@ -262,6 +423,20 @@ test_expect_success 'setup -- dir w/ complex structure' '
 	>dir11/a/e &&
 	>dir11/d/e/d/a
 '
+test_expect_success 'dirs-ignore of dir w/ complex structure' '
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a/b/c/d) [d] ./dir11/a/b/c/d
+	[f] (a/e) [e] ./dir11/a/e
+	[f] (b) [b] ./dir11/b
+	[f] (c) [c] ./dir11/c
+	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
+	EOF
+
+	test-tool dir-iterator ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
 test_expect_success 'dirs-before of dir w/ complex structure' '
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir11/a
@@ -277,7 +452,7 @@ test_expect_success 'dirs-before of dir w/ complex structure' '
 	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
 	EOF
 
-	test-tool dir-iterator ./dir11 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir11 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
@@ -287,7 +462,7 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
 	mkdir -p dir12/a &&
 	>dir12/a/b
 '
-test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'dirs-ignore of root dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	dir_iterator_begin failure: EACCES
 	EOF
@@ -299,7 +474,7 @@ test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
 
 	chmod 755 dir12
 '
-test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-ignore of root dir w/o perms' '
 	cat >expected-out <<-EOF &&
 	dir_iterator_begin failure: EACCES
 	EOF
@@ -311,6 +486,31 @@ test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms
 
 	chmod 755 dir12
 '
+test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_begin failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --dirs-before ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_begin failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
 
 test_expect_success 'begin should fail upon inexistent paths' '
 	echo "dir_iterator_begin failure: ENOENT" >expected-out &&
@@ -335,6 +535,33 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
 	>dir13/a/b
 '
 test_expect_success POSIXPERM,SANITY \
+'dirs-ignore of dir w/ dir w/o perms w/ file' '
+
+	cat >expected-out <<-EOF &&
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test-tool dir-iterator ./dir13 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-ignore of dir w/ dir w/o perms w/ file' '
+
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+test_expect_success POSIXPERM,SANITY \
 'dirs-before of dir w/ dir w/o perms w/ file' '
 
 	cat >expected-out <<-EOF &&
@@ -343,7 +570,7 @@ test_expect_success POSIXPERM,SANITY \
 
 	chmod 0 dir13/a &&
 
-	test-tool dir-iterator ./dir13 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir13 >actual-out &&
 	test_cmp expected-out actual-out &&
 
 	chmod 755 dir13/a
@@ -358,7 +585,8 @@ test_expect_success POSIXPERM,SANITY \
 
 	chmod 0 dir13/a &&
 
-	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic ./dir13 >actual-out &&
 	test_cmp expected-out actual-out &&
 
 	chmod 755 dir13/a
@@ -371,6 +599,31 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	ln -s d dir14/a/e &&
 	ln -s ../b dir14/a/f
 '
+test_expect_success SYMLINKS 'dirs-ignore of dir w/ symlinks w/o cycle' '
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a/d) [d] ./dir14/a/d
+	[s] (a/e) [e] ./dir14/a/e
+	[s] (a/f) [f] ./dir14/a/f
+	EOF
+
+	test-tool dir-iterator ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-ignore of dir w/ symlinks, w/o cycle' '
+
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a/d) [d] ./dir14/a/d
+	[f] (a/e) [e] ./dir14/a/e
+	EOF
+
+	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
 test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/o cycle' '
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir14/a
@@ -381,7 +634,7 @@ test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/o cycle' '
 	[s] (a/f) [f] ./dir14/a/f
 	EOF
 
-	test-tool dir-iterator ./dir14 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir14 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
@@ -399,7 +652,7 @@ test_expect_success SYMLINKS \
 	[f] (a/e) [e] ./dir14/a/e
 	EOF
 
-	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
+	test-tool dir-iterator --dirs-before --follow-symlinks ./dir14 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
@@ -412,7 +665,33 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
 	ln -s ../ dir15/a/b/e &&
 	ln -s ../../ dir15/a/b/f
 '
+test_expect_success SYMLINKS 'dirs-ignore of dir w/ symlinks w/ cycle' '
+	cat >expected-sorted-out <<-EOF &&
+	[s] (a/b/d) [d] ./dir15/a/b/d
+	[s] (a/b/e) [e] ./dir15/a/b/e
+	[s] (a/b/f) [f] ./dir15/a/b/f
+	EOF
+
+	test-tool dir-iterator ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks dirs-ignore of dir w/ symlinks w/ cycle' '
+
+	cat >expected-tailed-out <<-EOF &&
+	dir_iterator_advance failure: ELOOP
+	EOF
+
+	test_must_fail test-tool dir-iterator \
+		--pedantic --follow-symlinks ./dir15 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-tailed-out actual-tailed-out
+'
 test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/ cycle' '
+
 	cat >expected-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir15/a
 	[d] (a/b) [b] ./dir15/a/b
@@ -422,7 +701,7 @@ test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/ cycle' '
 	[s] (a/b/f) [f] ./dir15/a/b/f
 	EOF
 
-	test-tool dir-iterator ./dir15 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir15 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
@@ -434,7 +713,7 @@ test_expect_success SYMLINKS \
 	dir_iterator_advance failure: ELOOP
 	EOF
 
-	test_must_fail test-tool dir-iterator \
+	test_must_fail test-tool dir-iterator --dirs-before \
 		--pedantic --follow-symlinks ./dir15 >actual-out &&
 	tail -n 1 actual-out >actual-tailed-out &&
 
-- 
2.36.1

[PATCH v2 14/15] dir-iterator: option to iterate dirs in post-order

[Plato Kiorpelidis]

Introduce a new option to dir-iterator, using dir_iterator_begin()
flags parameter, allowing to control whether or not directories will
be exposed after their contents. In essence, post-order traversal over
file system entries that are directories.

This new iteration scheme can be enabled with DIR_ITERATOR_DIRS_AFTER
flag which iterates over a directory after doing so over its contents.
The existing flag DIR_ITERATOR_DIRS_BEFORE can be combined with the new
flag DIR_ITERATOR_DIRS_AFTER in any way. These flags do not conflict
with each other and can be combined to:
  * ignore directories by not setting either of them
  * iterate directories pre-order and post-order by enabling both
  * iterate directories pre-order by enabling DIR_ITERATOR_DIRS_BEFORE
  * iterate directories post-order by enabling DIR_ITERATOR_DIRS_AFTER

Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to test
the new iteration scheme the new flag DIR_ITERATOR_DIRS_AFTER enables.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 dir-iterator.c               |  95 ++++-
 dir-iterator.h               |  17 +-
 t/helper/test-dir-iterator.c |   6 +-
 t/t0066-dir-iterator.sh      | 744 +++++++++++++++++++++++++++++++++--
 4 files changed, 828 insertions(+), 34 deletions(-)

diff --git a/dir-iterator.c b/dir-iterator.c
index c1475add27..dec15317f0 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -133,10 +133,17 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name, char *dir_s
 {
 	int stat_err;
 	unsigned int DIRS_BEFORE = iter->flags & DIR_ITERATOR_DIRS_BEFORE;
+	unsigned int DIRS_AFTER = iter->flags & DIR_ITERATOR_DIRS_AFTER;
 
 	strbuf_addch(&iter->base.path, '/');
 	strbuf_addstr(&iter->base.path, d_name);
 
+	/*
+	 * We've got to check whether or not this is a directory.
+	 * We need to perform this check since the user could've requested
+	 * to ignore directory entries.
+	 */
+
 	if (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)
 		stat_err = stat(iter->base.path.buf, &iter->base.st);
 	else
@@ -158,6 +165,9 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name, char *dir_s
 	if (S_ISDIR(iter->base.st.st_mode)) {
 		if (!DIRS_BEFORE && !strcmp(dir_state, "before"))
 			return FAIL_IGN_DIRS;
+
+		if (!DIRS_AFTER && !strcmp(dir_state, "after"))
+			return FAIL_IGN_DIRS;
 	}
 
 	/*
@@ -173,6 +183,36 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name, char *dir_s
 	return OK;
 }
 
+/*
+ * Get the basename of the current directory.
+ *
+ * Using iter->base.path.buf, find the current dir basename.
+ */
+static char *current_dir_basename(struct dir_iterator_int *iter)
+{
+	char *start = strrchr(iter->base.path.buf, '/');
+	char *basename = NULL;
+
+	if (!start) {
+		/*
+		 * dir-iterator's implementation searches for '/' characters to
+		 * figure out the "active" directory part. Therefore, in this
+		 * case, the current path is the current directory part.
+		 */
+
+		start = iter->base.path.buf;
+	} else {
+		start += 1;  /* Skip '/' character */
+	}
+
+	if (!(basename = calloc(1, strlen(start) + 1)))
+		die_errno("calloc");
+
+	memcpy(basename, start, strlen(start));
+
+	return basename;
+}
+
 int dir_iterator_advance(struct dir_iterator *dir_iterator)
 {
 	struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
@@ -200,9 +240,28 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 		 * Therefore, there isn't any case to run out of levels.
 		 */
 
+		/*
+		 * We need to make sure, in case DIRS_AFTER is enabled, to
+		 * expose the entry in order to be consistent with what
+		 * DIRS_BEFORE exposes in case of failed `opendir()` call.
+		 */
+
+		char *d_name = current_dir_basename(iter);
+
 		--iter->levels_nr;
 
-		return dir_iterator_advance(dir_iterator);
+		level = &iter->levels[iter->levels_nr - 1];
+		strbuf_setlen(&iter->base.path, level->prefix_len);
+
+		expose_err = expose_entry(iter, d_name, "after");
+		free(d_name);
+
+		if (expose_err == FAIL_NOT_ENOENT && PEDANTIC)
+			goto error_out;
+		else if (expose_err != OK)
+			return dir_iterator_advance(dir_iterator);
+		else
+			return ITER_OK;
 	}
 
 	strbuf_setlen(&iter->base.path, level->prefix_len);
@@ -220,12 +279,42 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 		} else {
 			/*
 			 * Current directory has been iterated through.
+			 * We need to check if we need to expose current dir
+			 * because of DIRS_AFTER flag.
 			 */
 
-			if (pop_level(iter) == 0)
+			char* d_name = current_dir_basename(iter);
+
+			/*
+			 * We don't care to expose the root directory.
+			 * Users of this API know when iteration starts on root
+			 * directory - they call `dir_iterator_begin()` - and
+			 * when ITER_DONE is returned they know when it's over.
+			 */
+
+			/*
+			 * Call to `pop_level()` needs to preceed call to
+			 * `expose_entry()` because `expose_entry()` appends to
+			 * current `iter->base` and we need to set it up.
+			 */
+
+			if (pop_level(iter) == 0) {
+				free(d_name);
 				return dir_iterator_abort(dir_iterator);
+			}
 
-			return dir_iterator_advance(dir_iterator);
+			level = &iter->levels[iter->levels_nr - 1];
+			strbuf_setlen(&iter->base.path, level->prefix_len);
+
+			expose_err = expose_entry(iter, d_name, "after");
+			free(d_name);
+
+			if (expose_err == FAIL_NOT_ENOENT && PEDANTIC)
+				goto error_out;
+			else if (expose_err != OK)
+				return dir_iterator_advance(dir_iterator);
+			else
+				return ITER_OK;
 		}
 	}
 
diff --git a/dir-iterator.h b/dir-iterator.h
index c1d16a8c6d..825db6b9b8 100644
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -16,9 +16,10 @@
  * the dir_iterator structure to reflect the next path in the
  * iteration. The order that paths are iterated over within a
  * directory is undefined. Directory paths are given before their
- * contents when DIR_ITERATOR_DIRS_BEFORE is set. Failure to set this
- * flag results in directory paths not being exposed. Instead, iteration
- * will happen within directories. Their contents will be exposed.
+ * contents when DIR_ITERATOR_DIRS_BEFORE is set and after when
+ * DIR_ITERATOR_DIRS_AFTER is set. Failure to set any of them results
+ * in directory paths not being exposed. Instead, iteration will happen
+ * within directories. Their contents will be exposed.
  *
  * A typical iteration looks like this:
  *
@@ -67,8 +68,13 @@
  * - DIR_ITERATOR_DIRS_BEFORE: make dir-iterator expose a directory path
  *   before iterating through that directory's contents.
  *
- * Note: Activating none of the flags will iterate through directories'
- * contents but won't expose the directory paths.
+ * - DIR_ITERATOR_DIRS_AFTER: make dir-iterator expose a directory path after
+ *   iterating through that directory's contents.
+ *
+ * Note: any combination of DIR_ITERATOR_BEFORE and DIR_ITERATOR_AFTER works.
+ * Activating both of them will expose directories when descending into one and
+ * when it's been exhausted. Activating none will iterate through directories'
+ * contents but won't expose the directories themselves.
  *
  * Warning: circular symlinks are also followed when
  * DIR_ITERATOR_FOLLOW_SYMLINKS is set. The iteration may end up with
@@ -77,6 +83,7 @@
 #define DIR_ITERATOR_PEDANTIC (1 << 0)
 #define DIR_ITERATOR_FOLLOW_SYMLINKS (1 << 1)
 #define DIR_ITERATOR_DIRS_BEFORE (1 << 2)
+#define DIR_ITERATOR_DIRS_AFTER (1 << 3)
 
 struct dir_iterator {
 	/* The current path: */
diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index f05d5fde9d..ac5f368335 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -23,9 +23,9 @@ static const char *error_name(int error_number)
  *	--follow-symlinks
  *	--pedantic
  *	--dirs-before
+ *	--dirs-after
  *
- * example:
- * test-tool dir-iterator --pedantic --dirs-before ./somedir
+ * test-tool dir-iterator --pedantic --dirs-before --dirs-after ./somedir
  */
 int cmd__dir_iterator(int argc, const char **argv)
 {
@@ -40,6 +40,8 @@ int cmd__dir_iterator(int argc, const char **argv)
 			flags |= DIR_ITERATOR_PEDANTIC;
 		else if (strcmp(*argv, "--dirs-before") == 0)
 			flags |= DIR_ITERATOR_DIRS_BEFORE;
+		else if (strcmp(*argv, "--dirs-after") == 0)
+			flags |= DIR_ITERATOR_DIRS_AFTER;
 		else
 			die("invalid option '%s'", *argv);
 	}
diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index badd82d8a4..63d31b8f7d 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -25,6 +25,22 @@ test_expect_success 'dirs-before of dir with a file' '
 	test-tool dir-iterator --dirs-before ./dir1 >actual-out &&
 	test_cmp expected-out actual-out
 '
+test_expect_success 'dirs-after of dir with a file' '
+	cat >expected-out <<-EOF &&
+	[f] (a) [a] ./dir1/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir1 >actual-out &&
+	test_cmp expected-out actual-out
+'
+test_expect_success 'dirs-before/dirs-after of dir with a file' '
+	cat >expected-out <<-EOF &&
+	[f] (a) [a] ./dir1/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir1 >actual-out &&
+	test_cmp expected-out actual-out
+'
 
 test_expect_success 'setup -- dir with a single dir' '
 	mkdir -p dir2/a
@@ -44,6 +60,23 @@ test_expect_success 'dirs-before of dir with a single dir' '
 	test-tool dir-iterator --dirs-before ./dir2 >actual-out &&
 	test_cmp expected-out actual-out
 '
+test_expect_success 'dirs-after of dir with a single dir' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir2/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir2 >actual-out &&
+	test_cmp expected-out actual-out
+'
+test_expect_success 'dirs-before/dirs-after of dir with a single dir' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir2/a
+	[d] (a) [a] ./dir2/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir2 >actual-out &&
+	test_cmp expected-out actual-out
+'
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
 	mkdir -p dir3/a
@@ -97,6 +130,62 @@ test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of dir w/ dir w/o per
 
 	chmod 755 dir3/a
 '
+test_expect_success POSIXPERM,SANITY 'dirs-after of dir w/ dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir3/a
+	EOF
+
+	chmod 0 dir3/a &&
+
+	test-tool dir-iterator --dirs-after ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-after of dir w/ dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir3/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY \
+'dirs-before/dirs-after of dir w/ dir w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir3/a
+	[d] (a) [a] ./dir3/a
+	EOF
+
+	chmod 0 dir3/a &&
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before/dirs-after of dir w/ dir w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir3/a
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir3/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir3/a
+'
 
 test_expect_success 'setup -- dir w/ five files' '
 	mkdir dir4 &&
@@ -134,6 +223,34 @@ test_expect_success 'dirs-before of dir w/ five files' '
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
+test_expect_success 'dirs-after of dir w/ five files' '
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a) [a] ./dir4/a
+	[f] (b) [b] ./dir4/b
+	[f] (c) [c] ./dir4/c
+	[f] (d) [d] ./dir4/d
+	[f] (e) [e] ./dir4/e
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir4 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ five files' '
+	cat >expected-sorted-out <<-EOF &&
+	[f] (a) [a] ./dir4/a
+	[f] (b) [b] ./dir4/b
+	[f] (c) [c] ./dir4/c
+	[f] (d) [d] ./dir4/d
+	[f] (e) [e] ./dir4/e
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir4 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
 
 test_expect_success 'setup -- dir w/ dir w/ a file' '
 	mkdir -p dir5/a &&
@@ -156,6 +273,25 @@ test_expect_success 'dirs-before of dir w/ dir w/ a file' '
 	test-tool dir-iterator --dirs-before ./dir5 >actual-out &&
 	test_cmp expected-out actual-out
 '
+test_expect_success 'dirs-after of dir w/ dir w/ a file' '
+	cat >expected-after-out <<-EOF &&
+	[f] (a/b) [b] ./dir5/a/b
+	[d] (a) [a] ./dir5/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir5 >actual-out &&
+	test_cmp expected-after-out actual-out
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ dir w/ a file' '
+	cat >expected-before-after-out <<-EOF &&
+	[d] (a) [a] ./dir5/a
+	[f] (a/b) [b] ./dir5/a/b
+	[d] (a) [a] ./dir5/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir5 >actual-out &&
+	test_cmp expected-before-after-out actual-out
+'
 
 test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
 	mkdir -p dir6/a/b/c &&
@@ -180,6 +316,31 @@ test_expect_success 'dirs-before of dir w/ three nested dirs w/ file' '
 	test-tool dir-iterator --dirs-before ./dir6 >actual-out &&
 	test_cmp expected-out actual-out
 '
+test_expect_success 'dirs-after of dir w/ three nested dirs w/ file' '
+	cat >expected-after-out <<-EOF &&
+	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
+	[d] (a/b/c) [c] ./dir6/a/b/c
+	[d] (a/b) [b] ./dir6/a/b
+	[d] (a) [a] ./dir6/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir6 >actual-out &&
+	test_cmp expected-after-out actual-out
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ three nested dirs w/ file' '
+	cat >expected-before-after-out <<-EOF &&
+	[d] (a) [a] ./dir6/a
+	[d] (a/b) [b] ./dir6/a/b
+	[d] (a/b/c) [c] ./dir6/a/b/c
+	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
+	[d] (a/b/c) [c] ./dir6/a/b/c
+	[d] (a/b) [b] ./dir6/a/b
+	[d] (a) [a] ./dir6/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir6 >actual-out &&
+	test_cmp expected-before-after-out actual-out
+'
 
 test_expect_success POSIXPERM,SANITY \
 'setup -- dir w/ three nested dirs w/ file, second nested dir w/o perms' '
@@ -246,6 +407,70 @@ test_expect_success POSIXPERM,SANITY \
 
 	chmod 755 dir7/a/b
 '
+test_expect_success POSIXPERM,SANITY \
+'dirs-after of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a/b) [b] ./dir7/a/b
+	[d] (a) [a] ./dir7/a
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator --dirs-after ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-after of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'dirs-before/dirs-after of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	[d] (a/b) [b] ./dir7/a/b
+	[d] (a) [a] ./dir7/a
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before/dirs-after of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic ./dir7 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
 
 test_expect_success 'setup -- dir w/ two dirs each w/ file' '
 	mkdir -p dir8/a &&
@@ -289,6 +514,50 @@ test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
 		test_cmp expected-out2 actual-out
 	)
 '
+test_expect_success 'dirs-after of dir w/ two dirs each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir8/a/b
+	[d] (a) [a] ./dir8/a
+	[f] (c/d) [d] ./dir8/c/d
+	[d] (c) [c] ./dir8/c
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[f] (c/d) [d] ./dir8/c/d
+	[d] (c) [c] ./dir8/c
+	[f] (a/b) [b] ./dir8/a/b
+	[d] (a) [a] ./dir8/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir8 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ two dirs each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir8/a
+	[f] (a/b) [b] ./dir8/a/b
+	[d] (a) [a] ./dir8/a
+	[d] (c) [c] ./dir8/c
+	[f] (c/d) [d] ./dir8/c/d
+	[d] (c) [c] ./dir8/c
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[d] (c) [c] ./dir8/c
+	[f] (c/d) [d] ./dir8/c/d
+	[d] (c) [c] ./dir8/c
+	[d] (a) [a] ./dir8/a
+	[f] (a/b) [b] ./dir8/a/b
+	[d] (a) [a] ./dir8/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir8 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
 
 test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files' '
 	mkdir -p dir9/a &&
@@ -369,35 +638,123 @@ test_expect_success \
 		test_cmp expected-out4 actual-out
 	)
 '
+test_expect_success \
+'dirs-after of dir w/ two dirs, one w/ two and one w/ one files' '
 
-test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
-	mkdir -p dir10/a &&
-	>dir10/a/b &&
-	mkdir dir10/a/c &&
-	>dir10/a/c/d
-'
-test_expect_success 'dirs-ignore of dir w/ two nested dirs, each w/ file' '
 	cat >expected-out1 <<-EOF &&
-	[f] (a/b) [b] ./dir10/a/b
-	[f] (a/c/d) [d] ./dir10/a/c/d
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[d] (a) [a] ./dir9/a
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
 	EOF
 	cat >expected-out2 <<-EOF &&
-	[f] (a/c/d) [d] ./dir10/a/c/d
-	[f] (a/b) [b] ./dir10/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	[d] (a) [a] ./dir9/a
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
 	EOF
-
-	test-tool dir-iterator ./dir10/ >actual-out &&
-	(
-		test_cmp expected-out1 actual-out ||
-		test_cmp expected-out2 actual-out
-	)
-'
-test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
-	cat >expected-out1 <<-EOF &&
-	[d] (a) [a] ./dir10/a
-	[f] (a/b) [b] ./dir10/a/b
-	[d] (a/c) [c] ./dir10/a/c
-	[f] (a/c/d) [d] ./dir10/a/c/d
+	cat >expected-out3 <<-EOF &&
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[d] (a) [a] ./dir9/a
+	EOF
+	cat >expected-out4 <<-EOF &&
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	[d] (a) [a] ./dir9/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir9 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out ||
+		test_cmp expected-out3 actual-out ||
+		test_cmp expected-out4 actual-out
+	)
+'
+test_expect_success \
+'dirs-before/dirs-after of dir w/ two dirs, one w/ two and one w/ one files' '
+
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir9/a
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[d] (a) [a] ./dir9/a
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[d] (a) [a] ./dir9/a
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	[d] (a) [a] ./dir9/a
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
+	EOF
+	cat >expected-out3 <<-EOF &&
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
+	[d] (a) [a] ./dir9/a
+	[f] (a/b) [b] ./dir9/a/b
+	[f] (a/c) [c] ./dir9/a/c
+	[d] (a) [a] ./dir9/a
+	EOF
+	cat >expected-out4 <<-EOF &&
+	[d] (d) [d] ./dir9/d
+	[f] (d/e) [e] ./dir9/d/e
+	[d] (d) [d] ./dir9/d
+	[d] (a) [a] ./dir9/a
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	[d] (a) [a] ./dir9/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir9 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out ||
+		test_cmp expected-out3 actual-out ||
+		test_cmp expected-out4 actual-out
+	)
+'
+
+test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
+	mkdir -p dir10/a &&
+	>dir10/a/b &&
+	mkdir dir10/a/c &&
+	>dir10/a/c/d
+'
+test_expect_success 'dirs-ignore of dir w/ two nested dirs, each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir10/a/b
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[f] (a/b) [b] ./dir10/a/b
+	EOF
+
+	test-tool dir-iterator ./dir10/ >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir10/a
+	[f] (a/b) [b] ./dir10/a/b
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/c/d) [d] ./dir10/a/c/d
 	EOF
 	cat >expected-out2 <<-EOF &&
 	[d] (a) [a] ./dir10/a
@@ -412,6 +769,50 @@ test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
 		test_cmp expected-out2 actual-out
 	)
 '
+test_expect_success 'dirs-after of dir w/ two nested dirs, each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir10/a/b
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[d] (a/c) [c] ./dir10/a/c
+	[d] (a) [a] ./dir10/a
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/b) [b] ./dir10/a/b
+	[d] (a) [a] ./dir10/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir10/ >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ two nested dirs, each w/ file' '
+	cat >expected-out1 <<-EOF &&
+	[d] (a) [a] ./dir10/a
+	[f] (a/b) [b] ./dir10/a/b
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[d] (a/c) [c] ./dir10/a/c
+	[d] (a) [a] ./dir10/a
+	EOF
+	cat >expected-out2 <<-EOF &&
+	[d] (a) [a] ./dir10/a
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[d] (a/c) [c] ./dir10/a/c
+	[f] (a/b) [b] ./dir10/a/b
+	[d] (a) [a] ./dir10/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir10 >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
 
 test_expect_success 'setup -- dir w/ complex structure' '
 	mkdir -p dir11 &&
@@ -457,6 +858,52 @@ test_expect_success 'dirs-before of dir w/ complex structure' '
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
+test_expect_success 'dirs-after of dir w/ complex structure' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir11/a
+	[d] (a/b) [b] ./dir11/a/b
+	[d] (a/b/c) [c] ./dir11/a/b/c
+	[d] (d) [d] ./dir11/d
+	[d] (d/e) [e] ./dir11/d/e
+	[d] (d/e/d) [d] ./dir11/d/e/d
+	[f] (a/b/c/d) [d] ./dir11/a/b/c/d
+	[f] (a/e) [e] ./dir11/a/e
+	[f] (b) [b] ./dir11/b
+	[f] (c) [c] ./dir11/c
+	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ complex structure' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir11/a
+	[d] (a) [a] ./dir11/a
+	[d] (a/b) [b] ./dir11/a/b
+	[d] (a/b) [b] ./dir11/a/b
+	[d] (a/b/c) [c] ./dir11/a/b/c
+	[d] (a/b/c) [c] ./dir11/a/b/c
+	[d] (d) [d] ./dir11/d
+	[d] (d) [d] ./dir11/d
+	[d] (d/e) [e] ./dir11/d/e
+	[d] (d/e) [e] ./dir11/d/e
+	[d] (d/e/d) [d] ./dir11/d/e/d
+	[d] (d/e/d) [d] ./dir11/d/e/d
+	[f] (a/b/c/d) [d] ./dir11/a/b/c/d
+	[f] (a/e) [e] ./dir11/a/e
+	[f] (b) [b] ./dir11/b
+	[f] (c) [c] ./dir11/c
+	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
 	mkdir -p dir12/a &&
@@ -511,6 +958,61 @@ test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms
 
 	chmod 755 dir12
 '
+test_expect_success POSIXPERM,SANITY 'dirs-after of root dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_begin failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --dirs-after ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-after of root dir w/o perms' '
+	cat >expected-out <<-EOF &&
+	dir_iterator_begin failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
+test_expect_success POSIXPERM,SANITY \
+'dirs-before/dirs-after of root dir w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	dir_iterator_begin failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before/dirs-after of root dir w/o perms' '
+
+	cat >expected-out <<-EOF &&
+	dir_iterator_begin failure: EACCES
+	EOF
+
+	chmod 0 dir12 &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic ./dir12 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir12
+'
 
 test_expect_success 'begin should fail upon inexistent paths' '
 	echo "dir_iterator_begin failure: ENOENT" >expected-out &&
@@ -591,6 +1093,66 @@ test_expect_success POSIXPERM,SANITY \
 
 	chmod 755 dir13/a
 '
+test_expect_success POSIXPERM,SANITY \
+'dirs-after of dir w/ dir w/o perms w/ file' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test-tool dir-iterator --dirs-after ./dir13 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-after of dir w/ dir w/o perms w/ file' '
+
+	cat >expected-out <<-EOF &&
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic ./dir13 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+test_expect_success POSIXPERM,SANITY \
+'dirs-before/dirs-after of dir w/ dir w/o perms w/ file' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
+	[d] (a) [a] ./dir13/a
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir13 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before/dirs-after of dir w/ dir w/o perms w/ file' '
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
+	dir_iterator_advance failure: EACCES
+	EOF
+
+	chmod 0 dir13/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic ./dir13 >actual-out &&
+	test_cmp expected-out actual-out &&
+
+	chmod 755 dir13/a
+'
 
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	mkdir -p dir14/a &&
@@ -657,6 +1219,81 @@ test_expect_success SYMLINKS \
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
+test_expect_success SYMLINKS 'dirs-after of dir w/ symlinks w/o cycle' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir14/a
+	[d] (b) [b] ./dir14/b
+	[d] (b/c) [c] ./dir14/b/c
+	[f] (a/d) [d] ./dir14/a/d
+	[s] (a/e) [e] ./dir14/a/e
+	[s] (a/f) [f] ./dir14/a/f
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-after of dir w/ symlinks w/o cycle' '
+
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir14/a
+	[d] (a/f) [f] ./dir14/a/f
+	[d] (a/f/c) [c] ./dir14/a/f/c
+	[d] (b) [b] ./dir14/b
+	[d] (b/c) [c] ./dir14/b/c
+	[f] (a/d) [d] ./dir14/a/d
+	[f] (a/e) [e] ./dir14/a/e
+	EOF
+
+	test-tool dir-iterator --dirs-after --follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS 'dirs-before/dirs-after of dir w/ symlinks w/o cycle' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir14/a
+	[d] (a) [a] ./dir14/a
+	[d] (b) [b] ./dir14/b
+	[d] (b) [b] ./dir14/b
+	[d] (b/c) [c] ./dir14/b/c
+	[d] (b/c) [c] ./dir14/b/c
+	[f] (a/d) [d] ./dir14/a/d
+	[s] (a/e) [e] ./dir14/a/e
+	[s] (a/f) [f] ./dir14/a/f
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/o cycle' '
+
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir14/a
+	[d] (a) [a] ./dir14/a
+	[d] (a/f) [f] ./dir14/a/f
+	[d] (a/f) [f] ./dir14/a/f
+	[d] (a/f/c) [c] ./dir14/a/f/c
+	[d] (a/f/c) [c] ./dir14/a/f/c
+	[d] (b) [b] ./dir14/b
+	[d] (b) [b] ./dir14/b
+	[d] (b/c) [c] ./dir14/b/c
+	[d] (b/c) [c] ./dir14/b/c
+	[f] (a/d) [d] ./dir14/a/d
+	[f] (a/e) [e] ./dir14/a/e
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after \
+		--follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
 
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
 	mkdir -p dir15/a/b &&
@@ -719,5 +1356,64 @@ test_expect_success SYMLINKS \
 
 	test_cmp expected-tailed-out actual-tailed-out
 '
+test_expect_success SYMLINKS 'dirs-after of dir w/ symlinks w/ cycle' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir15/a
+	[d] (a/b) [b] ./dir15/a/b
+	[d] (a/c) [c] ./dir15/a/c
+	[s] (a/b/d) [d] ./dir15/a/b/d
+	[s] (a/b/e) [e] ./dir15/a/b/e
+	[s] (a/b/f) [f] ./dir15/a/b/f
+	EOF
+
+	test-tool dir-iterator --dirs-after ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks dirs-after of dir w/ symlinks w/ cycle' '
+
+	cat >expected-tailed-out <<-EOF &&
+	dir_iterator_advance failure: ELOOP
+	EOF
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic --follow-symlinks ./dir15 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-tailed-out actual-tailed-out
+'
+test_expect_success SYMLINKS 'dirs-before/dirs-after of dir w/ symlinks w/ cycle' '
+	cat >expected-sorted-out <<-EOF &&
+	[d] (a) [a] ./dir15/a
+	[d] (a) [a] ./dir15/a
+	[d] (a/b) [b] ./dir15/a/b
+	[d] (a/b) [b] ./dir15/a/b
+	[d] (a/c) [c] ./dir15/a/c
+	[d] (a/c) [c] ./dir15/a/c
+	[s] (a/b/d) [d] ./dir15/a/b/d
+	[s] (a/b/e) [e] ./dir15/a/b/e
+	[s] (a/b/f) [f] ./dir15/a/b/f
+	EOF
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/ cycle' '
+
+	cat >expected-tailed-out <<-EOF &&
+	dir_iterator_advance failure: ELOOP
+	EOF
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic --follow-symlinks ./dir15 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-tailed-out actual-tailed-out
+'
 
 test_done
-- 
2.36.1

[PATCH v2 15/15] entry.c: use dir-iterator to avoid explicit dir traversal

[Plato Kiorpelidis]

Replace usage of opendir/readdir/closedir API to traverse directories
recursively, at remove_subtree() function, by the dir-iterator API. This
simplifies the code and avoids recursive calls to remove_subtree().

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 entry.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/entry.c b/entry.c
index 1c9df62b30..46ef145f97 100644
--- a/entry.c
+++ b/entry.c
@@ -8,6 +8,8 @@
 #include "fsmonitor.h"
 #include "entry.h"
 #include "parallel-checkout.h"
+#include "iterator.h"
+#include "dir-iterator.h"
 
 static void create_directories(const char *path, int path_len,
 			       const struct checkout *state)
@@ -52,26 +54,24 @@ static void create_directories(const char *path, int path_len,
 
 static void remove_subtree(struct strbuf *path)
 {
-	DIR *dir = opendir(path->buf);
-	struct dirent *de;
-	int origlen = path->len;
-
-	if (!dir)
-		die_errno("cannot opendir '%s'", path->buf);
-	while ((de = readdir_skip_dot_and_dotdot(dir)) != NULL) {
-		struct stat st;
-
-		strbuf_addch(path, '/');
-		strbuf_addstr(path, de->d_name);
-		if (lstat(path->buf, &st))
-			die_errno("cannot lstat '%s'", path->buf);
-		if (S_ISDIR(st.st_mode))
-			remove_subtree(path);
-		else if (unlink(path->buf))
-			die_errno("cannot unlink '%s'", path->buf);
-		strbuf_setlen(path, origlen);
+	unsigned int flags = DIR_ITERATOR_DIRS_AFTER;
+	struct dir_iterator *iter = NULL;
+	int ok;
+
+	if (!(iter = dir_iterator_begin(path->buf, flags)))
+		die_errno("cannot open directory '%s'", path->buf);
+
+	while ((ok = dir_iterator_advance(iter)) == ITER_OK) {
+		if (S_ISDIR(iter->st.st_mode) && rmdir(iter->path.buf))
+			die_errno("cannot rmdir '%s'", iter->path.buf);
+
+		if (!S_ISDIR(iter->st.st_mode) && unlink(iter->path.buf))
+			die_errno("cannot unlink '%s'", iter->path.buf);
 	}
-	closedir(dir);
+
+	if (ok != ITER_DONE)
+		die_errno("failed to iterate over directory '%s'", path->buf);
+
 	if (rmdir(path->buf))
 		die_errno("cannot rmdir '%s'", path->buf);
 }
-- 
2.36.1

Re: [PATCH v2 04/15] test-dir-iterator: consistently return EXIT_FAILURE or EXIT_SUCCESS

[Junio C Hamano]

Plato Kiorpelidis <kioplato@gmail.com> writes:

> Throughout test-dir-iterator.c we were returning/exiting with either
> integers or EXIT_FAILURE. Improve readability and reduce mental load
> by being consistent with what test-dir-iterator returns through the
> test-tool. Returning mixed constants and integers could indicate that
> it matters for some reason e.g. architecture of test-tool and cmd__*
> functions.
>
> EXIT_SUCCESS and EXIT_FAILURE are specified by the C standard.
> That makes the code more portable and standardized.

And less portable for the invoking process of "git".  The invoking
process used to be able to depend ou getting WEXITSTATUS() from our
exit status to obtain "1" when we exited with 1; if we start exiting
with EXIT_FAILURE, there is no guarantee what non-zero value is used.

So, I am not sure if this is a good direction to go in.

> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>  t/helper/test-dir-iterator.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Especially given that this is a helper for testing, we may want to
return/exit with different non-zero value at different places to
make it easier for the test script to tell where in the program we
decided to exit a failure.  IOW, if we return not EXIT_FAILURE but 2
(or whatever value that is not used elsewhere) in the first hunk,
and let the second hunk continue to return 1, then the calling test
script can tell which one decided to fail.

As to EXIT_SUCCESS, I do not have a strong opinion against it, but
because EXIT_FAILURE is a bad idea, we probably should avoid it for
consistency.

Thanks.

> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
> index 659b6bfa81..81e931673e 100644
> --- a/t/helper/test-dir-iterator.c
> +++ b/t/helper/test-dir-iterator.c
> @@ -39,7 +39,7 @@ int cmd__dir_iterator(int argc, const char **argv)
>  
>  	if (!diter) {
>  		printf("dir_iterator_begin failure: %s\n", error_name(errno));
> -		exit(EXIT_FAILURE);
> +		return EXIT_FAILURE;
>  	}
>  
>  	while ((iter_status = dir_iterator_advance(diter)) == ITER_OK) {
> @@ -58,8 +58,8 @@ int cmd__dir_iterator(int argc, const char **argv)
>  
>  	if (iter_status != ITER_DONE) {
>  		printf("dir_iterator_advance failure\n");
> -		return 1;
> +		return EXIT_FAILURE;
>  	}
>  
> -	return 0;
> +	return EXIT_SUCCESS;
>  }

Re: [PATCH v2 10/15] dir-iterator: refactor dir_iterator_advance()

[Junio C Hamano]

Plato Kiorpelidis <kioplato@gmail.com> writes:

> Simplify dir_iterator_advance() by converting from iterative to
> recursive implementation. This conversion makes dir-iterator more
> maintainable for the following reasons:
>   * dir-iterator iterates the file-system, which is a tree structure.
>     Traditionally, tree traversals, in textbooks, lectures and online
>     sources are implemented recursively and not iteratively. Therefore
>     it helps reduce mental load for readers, since it's easier to follow
>     as it reminds of the same tree traversals we use on tree structures.

Careful.

Many algorithms that are taught in the recursive form in textbooks
are turned into iterative in production systems for a reason.  To
avoid too deep a recursion wasting too much stack space.  A loop
with management of work items using in-program data structures like
stack or queue often makes a large traversal bearable.

The most obvious example is our history traversal code.  History
stored in Git is a tree structure, but no sane reimplementation of
Git (well, at least those that want to be able to deal with a
history larger than a toy project) would implement "git log" using a
recursive algorithm.

Re: [PATCH v2 10/15] dir-iterator: refactor dir_iterator_advance()

[Phillip Wood]

Hi Plato

On 09/05/2022 18:51, Plato Kiorpelidis wrote:
> Simplify dir_iterator_advance() by converting from iterative to
> recursive implementation. This conversion makes dir-iterator more
> maintainable for the following reasons:
>    * dir-iterator iterates the file-system, which is a tree structure.
>      Traditionally, tree traversals, in textbooks, lectures and online
>      sources are implemented recursively and not iteratively. Therefore
>      it helps reduce mental load for readers, since it's easier to follow
>      as it reminds of the same tree traversals we use on tree structures.

In a traditional recursive tree walk the function that walks the tree 
applies a user supplied function to each node, the function that walks 
the tree does not return until every node has been visited. The 
recursive implementation relies on the initial call not returning until 
all the nodes have been visited and as Junio pointed out will exhaust 
the stack if the tree is too deep. This is not a traditional recursive 
tree walk though because we're implementing a iterator so our function 
returns each node that it visits rather than applying a user supplied 
function to the node thereby loosing any benefit offered by a recursive 
implementation.


>    * recursion requires one less indentation depth because we get rid of
>      the while loop and instead recurse, using the program's stack.
>    * in each recursive step a set of instructions are executed and
>      recursion lays out the code structurally in a better way, such that
>      these instructions stand out symmetrically for each recursive step.
> 
> This makes dir-iterator easier to work with, understand and introduce
> new functionality, like post-order on some directory entries, because it
> reminds us of the same operations we use to traverse tree structures.

I'm afraid I'm still not convinced the end result is easier to 
understand or use. It is unusual for a patch that adds twice as many 
lines as it removes to make the code simpler.

Best Wishes

Phillip

> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>   dir-iterator.c | 223 ++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 146 insertions(+), 77 deletions(-)
> 
> diff --git a/dir-iterator.c b/dir-iterator.c
> index b17e9f970a..3adcfbc966 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -7,8 +7,7 @@ struct dir_iterator_level {
>   	DIR *dir;
>   
>   	/*
> -	 * The length of the directory part of path at this level
> -	 * (including a trailing '/'):
> +	 * The length of the directory part of path at this level.
>   	 */
>   	size_t prefix_len;
>   };
> @@ -34,8 +33,9 @@ struct dir_iterator_int {
>   	size_t levels_alloc;
>   
>   	/*
> -	 * A stack of levels. levels[0] is the uppermost directory
> -	 * that will be included in this iteration.
> +	 * A stack of levels. levels[0] is the root directory.
> +	 * It won't be included in the iteration, but iteration will happen
> +	 * inside it's subdirectories.
>   	 */
>   	struct dir_iterator_level *levels;
>   
> @@ -43,36 +43,63 @@ struct dir_iterator_int {
>   	unsigned int flags;
>   };
>   
> +enum {
> +	OK,
> +	FAIL_ENOENT,
> +	FAIL_NOT_ENOENT,
> +};
> +
>   /*
>    * Push a level in the iter stack and initialize it with information from
> - * the directory pointed by iter->base->path. It is assumed that this
> - * strbuf points to a valid directory path. Return 0 on success and -1
> - * otherwise, setting errno accordingly and leaving the stack unchanged.
> + * the directory pointed by iter->base->path. Don't open the directory.
> + *
> + * Return 1 on success.
> + * Return 0 when `iter->base->path` isn't a directory.
>    */
>   static int push_level(struct dir_iterator_int *iter)
>   {
>   	struct dir_iterator_level *level;
>   
> +	if (!S_ISDIR(iter->base.st.st_mode))
> +		return 0;
> +
>   	ALLOC_GROW(iter->levels, iter->levels_nr + 1, iter->levels_alloc);
>   	level = &iter->levels[iter->levels_nr++];
>   
> -	if (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1]))
> -		strbuf_addch(&iter->base.path, '/');
> +	level->dir = NULL;
> +
>   	level->prefix_len = iter->base.path.len;
>   
> -	level->dir = opendir(iter->base.path.buf);
> -	if (!level->dir) {
> -		int saved_errno = errno;
> -		if (errno != ENOENT) {
> -			warning_errno("error opening directory '%s'",
> -				      iter->base.path.buf);
> -		}
> -		iter->levels_nr--;
> +	return 1;
> +}
> +
> +/*
> + * Activate most recent pushed level. Stack is unchanged.
> + *
> + * Return values:
> + * OK on success.
> + * FAIL_ENOENT on failed exposure because entry does not exist.
> + * FAIL_NOT_ENOENT on failed exposure because of errno other than ENOENT.
> + */
> +static int activate_level(struct dir_iterator_int *iter)
> +{
> +	struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
> +	int saved_errno;
> +
> +	if (level->dir)
> +		return OK;
> +
> +	if ((level->dir = opendir(iter->base.path.buf)) != NULL)
> +		return OK;
> +
> +	saved_errno = errno;
> +	if (errno != ENOENT) {
> +		warning_errno("error opening directory '%s'", iter->base.path.buf);
>   		errno = saved_errno;
> -		return -1;
> +		return FAIL_NOT_ENOENT;
>   	}
> -
> -	return 0;
> +	errno = saved_errno;
> +	return FAIL_ENOENT;
>   }
>   
>   /*
> @@ -81,12 +108,10 @@ static int push_level(struct dir_iterator_int *iter)
>    */
>   static int pop_level(struct dir_iterator_int *iter)
>   {
> -	struct dir_iterator_level *level =
> -		&iter->levels[iter->levels_nr - 1];
> +	struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
>   
>   	if (level->dir && closedir(level->dir))
> -		warning_errno("error closing directory '%s'",
> -			      iter->base.path.buf);
> +		warning_errno("error closing directory '%s'", iter->base.path.buf);
>   	level->dir = NULL;
>   
>   	return --iter->levels_nr;
> @@ -94,82 +119,119 @@ static int pop_level(struct dir_iterator_int *iter)
>   
>   /*
>    * Populate iter->base with the necessary information on the next iteration
> - * entry, represented by the given dirent de. Return 0 on success and -1
> - * otherwise, setting errno accordingly.
> + * entry, represented by the given relative path to the lowermost directory,
> + * d_name.
> + *
> + * Return values:
> + * OK on successful exposure of the provided entry.
> + * FAIL_ENOENT on failed exposure because entry does not exist.
> + * FAIL_NOT_ENOENT on failed exposure because of errno other than ENOENT.
>    */
> -static int prepare_next_entry_data(struct dir_iterator_int *iter,
> -				   struct dirent *de)
> +static int expose_entry(struct dir_iterator_int *iter, char *d_name)
>   {
> -	int err, saved_errno;
> +	int stat_err;
>   
> -	strbuf_addstr(&iter->base.path, de->d_name);
> -	/*
> -	 * We have to reset these because the path strbuf might have
> -	 * been realloc()ed at the previous strbuf_addstr().
> -	 */
> -	iter->base.relative_path = iter->base.path.buf +
> -				   iter->levels[0].prefix_len;
> -	iter->base.basename = iter->base.path.buf +
> -			      iter->levels[iter->levels_nr - 1].prefix_len;
> +	strbuf_addch(&iter->base.path, '/');
> +	strbuf_addstr(&iter->base.path, d_name);
>   
>   	if (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)
> -		err = stat(iter->base.path.buf, &iter->base.st);
> +		stat_err = stat(iter->base.path.buf, &iter->base.st);
>   	else
> -		err = lstat(iter->base.path.buf, &iter->base.st);
> +		stat_err = lstat(iter->base.path.buf, &iter->base.st);
>   
> -	saved_errno = errno;
> -	if (err && errno != ENOENT)
> +	if (stat_err && errno != ENOENT) {
>   		warning_errno("failed to stat '%s'", iter->base.path.buf);
> +		return FAIL_NOT_ENOENT;
> +	} else if (stat_err && errno == ENOENT) {
> +		return FAIL_ENOENT;
> +	}
>   
> -	errno = saved_errno;
> -	return err;
> +	/*
> +	 * We have to reset relative path and basename because the path strbuf
> +	 * might have been realloc()'ed at the previous strbuf_addstr().
> +	 */
> +
> +	iter->base.relative_path =
> +		iter->base.path.buf + iter->levels[0].prefix_len + 1;
> +	iter->base.basename =
> +		iter->base.path.buf + iter->levels[iter->levels_nr - 1].prefix_len + 1;
> +
> +	return OK;
>   }
>   
>   int dir_iterator_advance(struct dir_iterator *dir_iterator)
>   {
> -	struct dir_iterator_int *iter =
> -		(struct dir_iterator_int *)dir_iterator;
> +	struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
> +	struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
> +	struct dirent *dir_entry = NULL;
> +	int expose_err, activate_err;
> +	/* For shorter code width-wise, more readable */
> +	unsigned int PEDANTIC = iter->flags & DIR_ITERATOR_PEDANTIC;
>   
> -	if (S_ISDIR(iter->base.st.st_mode) && push_level(iter)) {
> -		if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
> -			goto error_out;
> -		if (iter->levels_nr == 0)
> +	/*
> +	 * Attempt to open the directory of the last level if not opened yet.
> +	 *
> +	 * Remember that we ignore ENOENT errors so that the user of this API
> +	 * can remove entries between calls to `dir_iterator_advance()`.
> +	 * We care for errors other than ENOENT only when PEDANTIC is enabled.
> +	 */
> +
> +	activate_err = activate_level(iter);
> +
> +	if (activate_err == FAIL_NOT_ENOENT && PEDANTIC) {
> +		goto error_out;
> +	} else if (activate_err != OK) {
> +		--iter->levels_nr;
> +
> +		if (iter->levels_nr == 0)  /* Failed to open root directory */
>   			goto error_out;
> +
> +		return dir_iterator_advance(dir_iterator);
>   	}
>   
> -	/* Loop until we find an entry that we can give back to the caller. */
> -	while (1) {
> -		struct dirent *de;
> -		struct dir_iterator_level *level =
> -			&iter->levels[iter->levels_nr - 1];
> +	strbuf_setlen(&iter->base.path, level->prefix_len);
> +
> +	errno = 0;
> +	dir_entry = readdir(level->dir);
> +
> +	if (!dir_entry) {
> +		if (errno) {
> +			warning_errno("errno reading dir '%s'", iter->base.path.buf);
> +			if (PEDANTIC)
> +				goto error_out;
>   
> -		strbuf_setlen(&iter->base.path, level->prefix_len);
> -		errno = 0;
> -		de = readdir(level->dir);
> -
> -		if (!de) {
> -			if (errno) {
> -				warning_errno("error reading directory '%s'",
> -					      iter->base.path.buf);
> -				if (iter->flags & DIR_ITERATOR_PEDANTIC)
> -					goto error_out;
> -			} else if (pop_level(iter) == 0) {
> +			return dir_iterator_advance(dir_iterator);
> +		} else {
> +			/*
> +			 * Current directory has been iterated through.
> +			 */
> +
> +			if (pop_level(iter) == 0)
>   				return dir_iterator_abort(dir_iterator);
> -			}
> -			continue;
> +
> +			return dir_iterator_advance(dir_iterator);
>   		}
> +	}
>   
> -		if (is_dot_or_dotdot(de->d_name))
> -			continue;
> +	if (is_dot_or_dotdot(dir_entry->d_name))
> +		return dir_iterator_advance(dir_iterator);
>   
> -		if (prepare_next_entry_data(iter, de)) {
> -			if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
> -				goto error_out;
> -			continue;
> -		}
> +	/*
> +	 * Successfully read entry from current directory level.
> +	 */
>   
> -		return ITER_OK;
> -	}
> +	expose_err = expose_entry(iter, dir_entry->d_name);
> +
> +	if (expose_err == FAIL_NOT_ENOENT && PEDANTIC)
> +		goto error_out;
> +
> +	if (expose_err == OK)
> +		push_level(iter);
> +
> +	if (expose_err != OK)
> +		return dir_iterator_advance(dir_iterator);
> +
> +	return ITER_OK;
>   
>   error_out:
>   	dir_iterator_abort(dir_iterator);
> @@ -207,6 +269,8 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
>   
>   	strbuf_init(&iter->base.path, PATH_MAX);
>   	strbuf_addstr(&iter->base.path, path);
> +	/* expose_entry() appends dir seperator before exposing an entry */
> +	strbuf_trim_trailing_dir_sep(&iter->base.path);
>   
>   	ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
>   	iter->levels_nr = 0;
> @@ -226,6 +290,11 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
>   		goto error_out;
>   	}
>   
> +	if (!push_level(iter)) {
> +		saved_errno = ENOTDIR;
> +		goto error_out;
> +	}
> +
>   	return dir_iterator;
>   
>   error_out:

Re: [PATCH v2 13/15] dir-iterator: option to iterate dirs in pre-order

[Phillip Wood]

Hi Plato

On 09/05/2022 18:51, Plato Kiorpelidis wrote:
> Introduce a new option to dir-iterator, using dir_iterator_begin()
> flags parameter, allowing to control whether or not directories will
> be exposed before their contents. In essence, pre-order traversal over
> file system entries that are directories.
> 
> This changes the default behavior of the dir-iterator API. Instead
> of iterating directories before doing so over their contents, the new
> default behavior does not expose directories at all. Iteration is still
> performed, however, within directories, iterating over any other entry.
> Only directory paths are ignored.
> 
> To iterate over directories in pre-order, reproducing the previous
> default behavior, enable the new flag DIR_ITERATOR_DIRS_BEFORE in the
> flags parameter of dir_iterator_begin():
>    * ignore directories by not setting DIR_ITERATOR_DIRS_BEFORE
>    * iterate directories pre-order by enabling DIR_ITERATOR_DIRS_BEFORE
> 
> Adjust existing callers, in refs/files-backend.c and builtin/clone.c
> to enable DIR_ITERATOR_DIRS_BEFORE since these callers require iteration
> over directories before doing so over their contents.
> 
> Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to test
> the new iteration scheme, which is the new default behavior, and the new
> flag DIR_ITERATOR_DIRS_BEFORE which reproduces the old default behavior.

It's great that you've split this change out from the next patch. I had 
not realized when I looked at the last round that all the existing 
callers require pre-order traversal. Given that is the case I'm finding 
it hard to see how changing the default behavior to one that no caller 
is using is an improvement.

Best Wishes

Phillip

> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>   builtin/clone.c              |   4 +-
>   dir-iterator.c               |  29 +++-
>   dir-iterator.h               |  29 +++-
>   refs/files-backend.c         |   2 +-
>   t/helper/test-dir-iterator.c |  12 +-
>   t/t0066-dir-iterator.sh      | 321 ++++++++++++++++++++++++++++++++---
>   6 files changed, 361 insertions(+), 36 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 194d50f75f..68787623e7 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -321,7 +321,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>   
>   	mkdir_if_missing(dest->buf, 0777);
>   
> -	flags = DIR_ITERATOR_PEDANTIC | DIR_ITERATOR_FOLLOW_SYMLINKS;
> +	flags = DIR_ITERATOR_DIRS_BEFORE |
> +		DIR_ITERATOR_PEDANTIC |
> +		DIR_ITERATOR_FOLLOW_SYMLINKS;
>   	iter = dir_iterator_begin(src->buf, flags);
>   
>   	if (!iter)
> diff --git a/dir-iterator.c b/dir-iterator.c
> index c36f549a78..c1475add27 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -47,6 +47,7 @@ enum {
>   	OK,
>   	FAIL_ENOENT,
>   	FAIL_NOT_ENOENT,
> +	FAIL_IGN_DIRS,
>   };
>   
>   /*
> @@ -124,12 +125,14 @@ static int pop_level(struct dir_iterator_int *iter)
>    *
>    * Return values:
>    * OK on successful exposure of the provided entry.
> + * FAIL_IGN_DIR on failed exposure because entry is dir and flags don't allow it.
>    * FAIL_ENOENT on failed exposure because entry does not exist.
>    * FAIL_NOT_ENOENT on failed exposure because of errno other than ENOENT.
>    */
> -static int expose_entry(struct dir_iterator_int *iter, char *d_name)
> +static int expose_entry(struct dir_iterator_int *iter, char *d_name, char *dir_state)
>   {
>   	int stat_err;
> +	unsigned int DIRS_BEFORE = iter->flags & DIR_ITERATOR_DIRS_BEFORE;
>   
>   	strbuf_addch(&iter->base.path, '/');
>   	strbuf_addstr(&iter->base.path, d_name);
> @@ -146,6 +149,17 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name)
>   		return FAIL_ENOENT;
>   	}
>   
> +	/*
> +	 * We've got to check whether or not this is a directory. We need to
> +	 * perform this check since the user could've requested to ignore
> +	 * directory entries.
> +	 */
> +
> +	if (S_ISDIR(iter->base.st.st_mode)) {
> +		if (!DIRS_BEFORE && !strcmp(dir_state, "before"))
> +			return FAIL_IGN_DIRS;
> +	}
> +
>   	/*
>   	 * We have to reset relative path and basename because the path strbuf
>   	 * might have been realloc()'ed at the previous strbuf_addstr().
> @@ -220,14 +234,23 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>   
>   	/*
>   	 * Successfully read entry from current directory level.
> +	 * In case it's a directory, we need to check, before exposing it, if
> +	 * it's allowed because of DIRS_BEFORE. In any case - allowed or not -
> +	 * we must push the directory to the levels stack, so the next call will
> +	 * read from it.
> +	 */
> +
> +	/*
> +	 * 'expose_entry()' function needs to know whether
> +	 * the exposure call is about DIRS_BEFORE or DIRS_AFTER.
>   	 */
>   
> -	expose_err = expose_entry(iter, dir_entry->d_name);
> +	expose_err = expose_entry(iter, dir_entry->d_name, "before");
>   
>   	if (expose_err == FAIL_NOT_ENOENT && PEDANTIC)
>   		goto error_out;
>   
> -	if (expose_err == OK)
> +	if (expose_err == OK || expose_err == FAIL_IGN_DIRS)
>   		push_level(iter);
>   
>   	if (expose_err != OK)
> diff --git a/dir-iterator.h b/dir-iterator.h
> index 08229157c6..c1d16a8c6d 100644
> --- a/dir-iterator.h
> +++ b/dir-iterator.h
> @@ -8,19 +8,22 @@
>    *
>    * Iterate over a directory tree, recursively, including paths of all
>    * types and hidden paths. Skip "." and ".." entries and don't follow
> - * symlinks except for the original path. Note that the original path
> - * is not included in the iteration.
> + * symlinks except when DIR_ITERATOR_FOLLOW_SYMLINKS is set. If root
> + * path is a symlink it's followed regardless of flags. Note that the
> + * original path is not included in the iteration.
>    *
>    * Every time dir_iterator_advance() is called, update the members of
>    * the dir_iterator structure to reflect the next path in the
>    * iteration. The order that paths are iterated over within a
> - * directory is undefined, directory paths are always given before
> - * their contents.
> + * directory is undefined. Directory paths are given before their
> + * contents when DIR_ITERATOR_DIRS_BEFORE is set. Failure to set this
> + * flag results in directory paths not being exposed. Instead, iteration
> + * will happen within directories. Their contents will be exposed.
>    *
>    * A typical iteration looks like this:
>    *
>    *     int ok;
> - *     unsigned int flags = DIR_ITERATOR_PEDANTIC;
> + *     unsigned int flags = DIR_ITERATOR_PEDANTIC | DIR_ITERATOR_DIRS_BEFORE;
>    *     struct dir_iterator *iter = dir_iterator_begin(path, flags);
>    *
>    *     if (!iter)
> @@ -61,12 +64,19 @@
>    *   not the symlinks themselves, which is the default behavior. Broken
>    *   symlinks are ignored.
>    *
> + * - DIR_ITERATOR_DIRS_BEFORE: make dir-iterator expose a directory path
> + *   before iterating through that directory's contents.
> + *
> + * Note: Activating none of the flags will iterate through directories'
> + * contents but won't expose the directory paths.
> + *
>    * Warning: circular symlinks are also followed when
>    * DIR_ITERATOR_FOLLOW_SYMLINKS is set. The iteration may end up with
>    * an ELOOP if they happen and DIR_ITERATOR_PEDANTIC is set.
>    */
>   #define DIR_ITERATOR_PEDANTIC (1 << 0)
>   #define DIR_ITERATOR_FOLLOW_SYMLINKS (1 << 1)
> +#define DIR_ITERATOR_DIRS_BEFORE (1 << 2)
>   
>   struct dir_iterator {
>   	/* The current path: */
> @@ -97,12 +107,13 @@ struct dir_iterator {
>    * failure, return NULL and set errno accordingly.
>    *
>    * The iteration includes all paths under path, not including path
> - * itself and not including "." or ".." entries.
> + * itself, "." or ".." entries and directories according to specified flags.
>    *
>    * Parameters are:
>    *  - path is the starting directory. An internal copy will be made.
>    *  - flags is a combination of the possible flags to initialize a
> - *    dir-iterator or 0 for default behavior.
> + *    dir-iterator or 0 for default behavior which will ignore directory
> + *    paths, but will include the rest directory contents.
>    */
>   struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
>   
> @@ -110,6 +121,7 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
>    * Advance the iterator to the first or next item and return ITER_OK.
>    * If the iteration is exhausted, free the dir_iterator and any
>    * resources associated with it and return ITER_DONE.
> + * On error, free dir_iterator memory and return ITER_ERROR.
>    *
>    * It is a bug to use iterator or call this function again after it
>    * has returned ITER_DONE or ITER_ERROR (which may be returned iff
> @@ -119,8 +131,7 @@ int dir_iterator_advance(struct dir_iterator *iterator);
>   
>   /*
>    * End the iteration before it has been exhausted. Free the
> - * dir_iterator and any associated resources and return ITER_DONE. On
> - * error, free the dir_iterator and return ITER_ERROR.
> + * dir_iterator and any associated resources and return ITER_DONE.
>    */
>   int dir_iterator_abort(struct dir_iterator *iterator);
>   
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8db7882aac..812f00c0f4 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2237,7 +2237,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
>   
>   	strbuf_addf(&sb, "%s/logs", gitdir);
>   
> -	diter = dir_iterator_begin(sb.buf, 0);
> +	diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_DIRS_BEFORE);
>   	if (!diter) {
>   		strbuf_release(&sb);
>   		return empty_ref_iterator_begin();
> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
> index 55d8a58836..f05d5fde9d 100644
> --- a/t/helper/test-dir-iterator.c
> +++ b/t/helper/test-dir-iterator.c
> @@ -17,7 +17,15 @@ static const char *error_name(int error_number)
>   
>   /*
>    * usage:
> - * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path
> + * test-tool dir-iterator [OPTIONS] directory_path
> + *
> + * OPTIONS
> + *	--follow-symlinks
> + *	--pedantic
> + *	--dirs-before
> + *
> + * example:
> + * test-tool dir-iterator --pedantic --dirs-before ./somedir
>    */
>   int cmd__dir_iterator(int argc, const char **argv)
>   {
> @@ -30,6 +38,8 @@ int cmd__dir_iterator(int argc, const char **argv)
>   			flags |= DIR_ITERATOR_FOLLOW_SYMLINKS;
>   		else if (strcmp(*argv, "--pedantic") == 0)
>   			flags |= DIR_ITERATOR_PEDANTIC;
> +		else if (strcmp(*argv, "--dirs-before") == 0)
> +			flags |= DIR_ITERATOR_DIRS_BEFORE;
>   		else
>   			die("invalid option '%s'", *argv);
>   	}
> diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
> index 8ab8811fb5..badd82d8a4 100755
> --- a/t/t0066-dir-iterator.sh
> +++ b/t/t0066-dir-iterator.sh
> @@ -9,7 +9,7 @@ test_expect_success 'setup -- dir with a single file' '
>   	mkdir dir1 &&
>   	>dir1/a
>   '
> -test_expect_success 'dirs-before of dir with a file' '
> +test_expect_success 'dirs-ignore of dir with a file' '
>   	cat >expected-out <<-EOF &&
>   	[f] (a) [a] ./dir1/a
>   	EOF
> @@ -17,22 +17,60 @@ test_expect_success 'dirs-before of dir with a file' '
>   	test-tool dir-iterator ./dir1 >actual-out &&
>   	test_cmp expected-out actual-out
>   '
> +test_expect_success 'dirs-before of dir with a file' '
> +	cat >expected-out <<-EOF &&
> +	[f] (a) [a] ./dir1/a
> +	EOF
> +
> +	test-tool dir-iterator --dirs-before ./dir1 >actual-out &&
> +	test_cmp expected-out actual-out
> +'
>   
>   test_expect_success 'setup -- dir with a single dir' '
>   	mkdir -p dir2/a
>   '
> +test_expect_success 'dirs-ignore of dir with a single dir' '
> +	cat >expected-out <<-EOF &&
> +	EOF
> +
> +	test-tool dir-iterator ./dir2 >actual-out &&
> +	test_cmp expected-out actual-out
> +'
>   test_expect_success 'dirs-before of dir with a single dir' '
>   	cat >expected-out <<-EOF &&
>   	[d] (a) [a] ./dir2/a
>   	EOF
>   
> -	test-tool dir-iterator ./dir2 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir2 >actual-out &&
>   	test_cmp expected-out actual-out
>   '
>   
>   test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
>   	mkdir -p dir3/a
>   '
> +test_expect_success POSIXPERM,SANITY 'dirs-ignore of dir w/ dir w/o perms' '
> +	cat >expected-out <<-EOF &&
> +	EOF
> +
> +	chmod 0 dir3/a &&
> +
> +	test-tool dir-iterator ./dir3/ >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY 'pedantic dirs-ignore of dir w/ dir w/o perms' '
> +	cat >expected-out <<-EOF &&
> +	dir_iterator_advance failure: EACCES
> +	EOF
> +
> +	chmod 0 dir3/a &&
> +
> +	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
>   test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
>   	cat >expected-out <<-EOF &&
>   	[d] (a) [a] ./dir3/a
> @@ -40,7 +78,7 @@ test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
>   
>   	chmod 0 dir3/a &&
>   
> -	test-tool dir-iterator ./dir3/ >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir3/ >actual-out &&
>   	test_cmp expected-out actual-out &&
>   
>   	chmod 755 dir3/a
> @@ -53,7 +91,8 @@ test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of dir w/ dir w/o per
>   
>   	chmod 0 dir3/a &&
>   
> -	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic ./dir3/ >actual-out &&
>   	test_cmp expected-out actual-out &&
>   
>   	chmod 755 dir3/a
> @@ -67,7 +106,7 @@ test_expect_success 'setup -- dir w/ five files' '
>   	>dir4/d &&
>   	>dir4/e
>   '
> -test_expect_success 'dirs-before of dir w/ five files' '
> +test_expect_success 'dirs-ignore of dir w/ five files' '
>   	cat >expected-sorted-out <<-EOF &&
>   	[f] (a) [a] ./dir4/a
>   	[f] (b) [b] ./dir4/b
> @@ -81,18 +120,40 @@ test_expect_success 'dirs-before of dir w/ five files' '
>   
>   	test_cmp expected-sorted-out actual-sorted-out
>   '
> +test_expect_success 'dirs-before of dir w/ five files' '
> +	cat >expected-sorted-out <<-EOF &&
> +	[f] (a) [a] ./dir4/a
> +	[f] (b) [b] ./dir4/b
> +	[f] (c) [c] ./dir4/c
> +	[f] (d) [d] ./dir4/d
> +	[f] (e) [e] ./dir4/e
> +	EOF
> +
> +	test-tool dir-iterator --dirs-before ./dir4 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-sorted-out actual-sorted-out
> +'
>   
>   test_expect_success 'setup -- dir w/ dir w/ a file' '
>   	mkdir -p dir5/a &&
>   	>dir5/a/b
>   '
> +test_expect_success 'dirs-ignore of dir w/ dir w/ a file' '
> +	cat >expected-out <<-EOF &&
> +	[f] (a/b) [b] ./dir5/a/b
> +	EOF
> +
> +	test-tool dir-iterator ./dir5 >actual-out &&
> +	test_cmp expected-out actual-out
> +'
>   test_expect_success 'dirs-before of dir w/ dir w/ a file' '
>   	cat >expected-out <<-EOF &&
>   	[d] (a) [a] ./dir5/a
>   	[f] (a/b) [b] ./dir5/a/b
>   	EOF
>   
> -	test-tool dir-iterator ./dir5 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir5 >actual-out &&
>   	test_cmp expected-out actual-out
>   '
>   
> @@ -100,6 +161,14 @@ test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
>   	mkdir -p dir6/a/b/c &&
>   	>dir6/a/b/c/d
>   '
> +test_expect_success 'dirs-ignore of dir w/ three nested dirs w/ file' '
> +	cat >expected-out <<-EOF &&
> +	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
> +	EOF
> +
> +	test-tool dir-iterator ./dir6 >actual-out &&
> +	test_cmp expected-out actual-out
> +'
>   test_expect_success 'dirs-before of dir w/ three nested dirs w/ file' '
>   	cat >expected-out <<-EOF &&
>   	[d] (a) [a] ./dir6/a
> @@ -108,7 +177,7 @@ test_expect_success 'dirs-before of dir w/ three nested dirs w/ file' '
>   	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
>   	EOF
>   
> -	test-tool dir-iterator ./dir6 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir6 >actual-out &&
>   	test_cmp expected-out actual-out
>   '
>   
> @@ -119,6 +188,33 @@ test_expect_success POSIXPERM,SANITY \
>   	>dir7/a/b/c/d
>   '
>   test_expect_success POSIXPERM,SANITY \
> +'dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
> +
> +	cat >expected-out <<-EOF &&
> +	EOF
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test-tool dir-iterator ./dir7 >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir7/a/b
> +'
> +test_expect_success POSIXPERM,SANITY \
> +'pedantic dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
> +
> +	cat >expected-out <<-EOF &&
> +	dir_iterator_advance failure: EACCES
> +	EOF
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir7/a/b
> +'
> +test_expect_success POSIXPERM,SANITY \
>   'dirs-before of dir w/ three nested dirs w/ file, second w/o perms' '
>   
>   	cat >expected-out <<-EOF &&
> @@ -128,7 +224,7 @@ test_expect_success POSIXPERM,SANITY \
>   
>   	chmod 0 dir7/a/b &&
>   
> -	test-tool dir-iterator ./dir7 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir7 >actual-out &&
>   	test_cmp expected-out actual-out &&
>   
>   	chmod 755 dir7/a/b
> @@ -144,7 +240,8 @@ test_expect_success POSIXPERM,SANITY \
>   
>   	chmod 0 dir7/a/b &&
>   
> -	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic ./dir7 >actual-out &&
>   	test_cmp expected-out actual-out &&
>   
>   	chmod 755 dir7/a/b
> @@ -156,6 +253,22 @@ test_expect_success 'setup -- dir w/ two dirs each w/ file' '
>   	mkdir dir8/c &&
>   	>dir8/c/d
>   '
> +test_expect_success 'dirs-ignore of dir w/ two dirs each w/ file' '
> +	cat >expected-out1 <<-EOF &&
> +	[f] (a/b) [b] ./dir8/a/b
> +	[f] (c/d) [d] ./dir8/c/d
> +	EOF
> +	cat >expected-out2 <<-EOF &&
> +	[f] (c/d) [d] ./dir8/c/d
> +	[f] (a/b) [b] ./dir8/a/b
> +	EOF
> +
> +	test-tool dir-iterator ./dir8 >actual-out &&
> +	(
> +		test_cmp expected-out1 actual-out ||
> +		test_cmp expected-out2 actual-out
> +	)
> +'
>   test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
>   	cat >expected-out1 <<-EOF &&
>   	[d] (a) [a] ./dir8/a
> @@ -170,7 +283,7 @@ test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
>   	[f] (a/b) [b] ./dir8/a/b
>   	EOF
>   
> -	test-tool dir-iterator ./dir8 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir8 >actual-out &&
>   	(
>   		test_cmp expected-out1 actual-out ||
>   		test_cmp expected-out2 actual-out
> @@ -185,6 +298,38 @@ test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files'
>   	>dir9/d/e
>   '
>   test_expect_success \
> +'dirs-ignore of dir w/ two dirs, one w/ two and one w/ one files' '
> +
> +	cat >expected-out1 <<-EOF &&
> +	[f] (a/b) [b] ./dir9/a/b
> +	[f] (a/c) [c] ./dir9/a/c
> +	[f] (d/e) [e] ./dir9/d/e
> +	EOF
> +	cat >expected-out2 <<-EOF &&
> +	[f] (a/c) [c] ./dir9/a/c
> +	[f] (a/b) [b] ./dir9/a/b
> +	[f] (d/e) [e] ./dir9/d/e
> +	EOF
> +	cat >expected-out3 <<-EOF &&
> +	[f] (d/e) [e] ./dir9/d/e
> +	[f] (a/b) [b] ./dir9/a/b
> +	[f] (a/c) [c] ./dir9/a/c
> +	EOF
> +	cat >expected-out4 <<-EOF &&
> +	[f] (d/e) [e] ./dir9/d/e
> +	[f] (a/c) [c] ./dir9/a/c
> +	[f] (a/b) [b] ./dir9/a/b
> +	EOF
> +
> +	test-tool dir-iterator ./dir9 >actual-out &&
> +	(
> +		test_cmp expected-out1 actual-out ||
> +		test_cmp expected-out2 actual-out ||
> +		test_cmp expected-out3 actual-out ||
> +		test_cmp expected-out4 actual-out
> +	)
> +'
> +test_expect_success \
>   'dirs-before of dir w/ two dirs, one w/ two and one w/ one files' '
>   
>   	cat >expected-out1 <<-EOF &&
> @@ -216,7 +361,7 @@ test_expect_success \
>   	[f] (a/b) [b] ./dir9/a/b
>   	EOF
>   
> -	test-tool dir-iterator ./dir9 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir9 >actual-out &&
>   	(
>   		test_cmp expected-out1 actual-out ||
>   		test_cmp expected-out2 actual-out ||
> @@ -231,6 +376,22 @@ test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
>   	mkdir dir10/a/c &&
>   	>dir10/a/c/d
>   '
> +test_expect_success 'dirs-ignore of dir w/ two nested dirs, each w/ file' '
> +	cat >expected-out1 <<-EOF &&
> +	[f] (a/b) [b] ./dir10/a/b
> +	[f] (a/c/d) [d] ./dir10/a/c/d
> +	EOF
> +	cat >expected-out2 <<-EOF &&
> +	[f] (a/c/d) [d] ./dir10/a/c/d
> +	[f] (a/b) [b] ./dir10/a/b
> +	EOF
> +
> +	test-tool dir-iterator ./dir10/ >actual-out &&
> +	(
> +		test_cmp expected-out1 actual-out ||
> +		test_cmp expected-out2 actual-out
> +	)
> +'
>   test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
>   	cat >expected-out1 <<-EOF &&
>   	[d] (a) [a] ./dir10/a
> @@ -245,7 +406,7 @@ test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
>   	[f] (a/b) [b] ./dir10/a/b
>   	EOF
>   
> -	test-tool dir-iterator ./dir10/ >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir10/ >actual-out &&
>   	(
>   		test_cmp expected-out1 actual-out ||
>   		test_cmp expected-out2 actual-out
> @@ -262,6 +423,20 @@ test_expect_success 'setup -- dir w/ complex structure' '
>   	>dir11/a/e &&
>   	>dir11/d/e/d/a
>   '
> +test_expect_success 'dirs-ignore of dir w/ complex structure' '
> +	cat >expected-sorted-out <<-EOF &&
> +	[f] (a/b/c/d) [d] ./dir11/a/b/c/d
> +	[f] (a/e) [e] ./dir11/a/e
> +	[f] (b) [b] ./dir11/b
> +	[f] (c) [c] ./dir11/c
> +	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
> +	EOF
> +
> +	test-tool dir-iterator ./dir11 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-sorted-out actual-sorted-out
> +'
>   test_expect_success 'dirs-before of dir w/ complex structure' '
>   	cat >expected-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir11/a
> @@ -277,7 +452,7 @@ test_expect_success 'dirs-before of dir w/ complex structure' '
>   	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
>   	EOF
>   
> -	test-tool dir-iterator ./dir11 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir11 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
>   	test_cmp expected-sorted-out actual-sorted-out
> @@ -287,7 +462,7 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/o perms' '
>   	mkdir -p dir12/a &&
>   	>dir12/a/b
>   '
> -test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
> +test_expect_success POSIXPERM,SANITY 'dirs-ignore of root dir w/o perms' '
>   	cat >expected-out <<-EOF &&
>   	dir_iterator_begin failure: EACCES
>   	EOF
> @@ -299,7 +474,7 @@ test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
>   
>   	chmod 755 dir12
>   '
> -test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms' '
> +test_expect_success POSIXPERM,SANITY 'pedantic dirs-ignore of root dir w/o perms' '
>   	cat >expected-out <<-EOF &&
>   	dir_iterator_begin failure: EACCES
>   	EOF
> @@ -311,6 +486,31 @@ test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms
>   
>   	chmod 755 dir12
>   '
> +test_expect_success POSIXPERM,SANITY 'dirs-before of root dir w/o perms' '
> +	cat >expected-out <<-EOF &&
> +	dir_iterator_begin failure: EACCES
> +	EOF
> +
> +	chmod 0 dir12 &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-before ./dir12 >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir12
> +'
> +test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of root dir w/o perms' '
> +	cat >expected-out <<-EOF &&
> +	dir_iterator_begin failure: EACCES
> +	EOF
> +
> +	chmod 0 dir12 &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic ./dir12 >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir12
> +'
>   
>   test_expect_success 'begin should fail upon inexistent paths' '
>   	echo "dir_iterator_begin failure: ENOENT" >expected-out &&
> @@ -335,6 +535,33 @@ test_expect_success POSIXPERM,SANITY 'setup -- dir w/ dir w/o perms w/ file' '
>   	>dir13/a/b
>   '
>   test_expect_success POSIXPERM,SANITY \
> +'dirs-ignore of dir w/ dir w/o perms w/ file' '
> +
> +	cat >expected-out <<-EOF &&
> +	EOF
> +
> +	chmod 0 dir13/a &&
> +
> +	test-tool dir-iterator ./dir13 >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir13/a
> +'
> +test_expect_success POSIXPERM,SANITY \
> +'pedantic dirs-ignore of dir w/ dir w/o perms w/ file' '
> +
> +	cat >expected-out <<-EOF &&
> +	dir_iterator_advance failure: EACCES
> +	EOF
> +
> +	chmod 0 dir13/a &&
> +
> +	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
> +	test_cmp expected-out actual-out &&
> +
> +	chmod 755 dir13/a
> +'
> +test_expect_success POSIXPERM,SANITY \
>   'dirs-before of dir w/ dir w/o perms w/ file' '
>   
>   	cat >expected-out <<-EOF &&
> @@ -343,7 +570,7 @@ test_expect_success POSIXPERM,SANITY \
>   
>   	chmod 0 dir13/a &&
>   
> -	test-tool dir-iterator ./dir13 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir13 >actual-out &&
>   	test_cmp expected-out actual-out &&
>   
>   	chmod 755 dir13/a
> @@ -358,7 +585,8 @@ test_expect_success POSIXPERM,SANITY \
>   
>   	chmod 0 dir13/a &&
>   
> -	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic ./dir13 >actual-out &&
>   	test_cmp expected-out actual-out &&
>   
>   	chmod 755 dir13/a
> @@ -371,6 +599,31 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
>   	ln -s d dir14/a/e &&
>   	ln -s ../b dir14/a/f
>   '
> +test_expect_success SYMLINKS 'dirs-ignore of dir w/ symlinks w/o cycle' '
> +	cat >expected-sorted-out <<-EOF &&
> +	[f] (a/d) [d] ./dir14/a/d
> +	[s] (a/e) [e] ./dir14/a/e
> +	[s] (a/f) [f] ./dir14/a/f
> +	EOF
> +
> +	test-tool dir-iterator ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'follow-symlinks dirs-ignore of dir w/ symlinks, w/o cycle' '
> +
> +	cat >expected-sorted-out <<-EOF &&
> +	[f] (a/d) [d] ./dir14/a/d
> +	[f] (a/e) [e] ./dir14/a/e
> +	EOF
> +
> +	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-sorted-out actual-sorted-out
> +'
>   test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/o cycle' '
>   	cat >expected-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir14/a
> @@ -381,7 +634,7 @@ test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/o cycle' '
>   	[s] (a/f) [f] ./dir14/a/f
>   	EOF
>   
> -	test-tool dir-iterator ./dir14 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir14 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
>   	test_cmp expected-sorted-out actual-sorted-out
> @@ -399,7 +652,7 @@ test_expect_success SYMLINKS \
>   	[f] (a/e) [e] ./dir14/a/e
>   	EOF
>   
> -	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
> +	test-tool dir-iterator --dirs-before --follow-symlinks ./dir14 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
>   	test_cmp expected-sorted-out actual-sorted-out
> @@ -412,7 +665,33 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
>   	ln -s ../ dir15/a/b/e &&
>   	ln -s ../../ dir15/a/b/f
>   '
> +test_expect_success SYMLINKS 'dirs-ignore of dir w/ symlinks w/ cycle' '
> +	cat >expected-sorted-out <<-EOF &&
> +	[s] (a/b/d) [d] ./dir15/a/b/d
> +	[s] (a/b/e) [e] ./dir15/a/b/e
> +	[s] (a/b/f) [f] ./dir15/a/b/f
> +	EOF
> +
> +	test-tool dir-iterator ./dir15 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'pedantic follow-symlinks dirs-ignore of dir w/ symlinks w/ cycle' '
> +
> +	cat >expected-tailed-out <<-EOF &&
> +	dir_iterator_advance failure: ELOOP
> +	EOF
> +
> +	test_must_fail test-tool dir-iterator \
> +		--pedantic --follow-symlinks ./dir15 >actual-out &&
> +	tail -n 1 actual-out >actual-tailed-out &&
> +
> +	test_cmp expected-tailed-out actual-tailed-out
> +'
>   test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/ cycle' '
> +
>   	cat >expected-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir15/a
>   	[d] (a/b) [b] ./dir15/a/b
> @@ -422,7 +701,7 @@ test_expect_success SYMLINKS 'dirs-before of dir w/ symlinks w/ cycle' '
>   	[s] (a/b/f) [f] ./dir15/a/b/f
>   	EOF
>   
> -	test-tool dir-iterator ./dir15 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir15 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
>   	test_cmp expected-sorted-out actual-sorted-out
> @@ -434,7 +713,7 @@ test_expect_success SYMLINKS \
>   	dir_iterator_advance failure: ELOOP
>   	EOF
>   
> -	test_must_fail test-tool dir-iterator \
> +	test_must_fail test-tool dir-iterator --dirs-before \
>   		--pedantic --follow-symlinks ./dir15 >actual-out &&
>   	tail -n 1 actual-out >actual-tailed-out &&
>   

Re: [PATCH v2 15/15] entry.c: use dir-iterator to avoid explicit dir traversal

[Phillip Wood]

Hi Plato

On 09/05/2022 18:51, Plato Kiorpelidis wrote:
> Replace usage of opendir/readdir/closedir API to traverse directories
> recursively, at remove_subtree() function, by the dir-iterator API. This
> simplifies the code and avoids recursive calls to remove_subtree().

Thanks for adding this conversion, it is nice to see the post-order 
traversal being used and the end result is an improvement on the original.

Best Wishes

Phillip

> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>   entry.c | 38 +++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/entry.c b/entry.c
> index 1c9df62b30..46ef145f97 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -8,6 +8,8 @@
>   #include "fsmonitor.h"
>   #include "entry.h"
>   #include "parallel-checkout.h"
> +#include "iterator.h"
> +#include "dir-iterator.h"
>   
>   static void create_directories(const char *path, int path_len,
>   			       const struct checkout *state)
> @@ -52,26 +54,24 @@ static void create_directories(const char *path, int path_len,
>   
>   static void remove_subtree(struct strbuf *path)
>   {
> -	DIR *dir = opendir(path->buf);
> -	struct dirent *de;
> -	int origlen = path->len;
> -
> -	if (!dir)
> -		die_errno("cannot opendir '%s'", path->buf);
> -	while ((de = readdir_skip_dot_and_dotdot(dir)) != NULL) {
> -		struct stat st;
> -
> -		strbuf_addch(path, '/');
> -		strbuf_addstr(path, de->d_name);
> -		if (lstat(path->buf, &st))
> -			die_errno("cannot lstat '%s'", path->buf);
> -		if (S_ISDIR(st.st_mode))
> -			remove_subtree(path);
> -		else if (unlink(path->buf))
> -			die_errno("cannot unlink '%s'", path->buf);
> -		strbuf_setlen(path, origlen);
> +	unsigned int flags = DIR_ITERATOR_DIRS_AFTER;
> +	struct dir_iterator *iter = NULL;
> +	int ok;
> +
> +	if (!(iter = dir_iterator_begin(path->buf, flags)))
> +		die_errno("cannot open directory '%s'", path->buf);
> +
> +	while ((ok = dir_iterator_advance(iter)) == ITER_OK) {
> +		if (S_ISDIR(iter->st.st_mode) && rmdir(iter->path.buf))
> +			die_errno("cannot rmdir '%s'", iter->path.buf);
> +
> +		if (!S_ISDIR(iter->st.st_mode) && unlink(iter->path.buf))
> +			die_errno("cannot unlink '%s'", iter->path.buf);
>   	}
> -	closedir(dir);
> +
> +	if (ok != ITER_DONE)
> +		die_errno("failed to iterate over directory '%s'", path->buf);
> +
>   	if (rmdir(path->buf))
>   		die_errno("cannot rmdir '%s'", path->buf);
>   }

Re: [PATCH v2 00/15][GSoC] iterate dirs before or after their contents

[Phillip Wood]

On 09/05/2022 18:51, Plato Kiorpelidis wrote:
> This is the second version of a patch series which implements pre-order and
> post-order iteration over directories. In this version I use the new iteration
> scheme to convert entry.c remove_subtree() function from using opendir/readdir/
> closedir API to dir-iterator API.
> 
> v1: https://lore.kernel.org/git/20220410111852.2097418-1-kioplato@gmail.com/
> 
> Fork: https://github.com/kioplato/git/tree/dir-iterator-v2
> CI: https://github.com/kioplato/git/actions/runs/2295239114
> I've ran the full test suite for each commit.
> 
> In comparison to v1 I changed/fixed/improved:
> 
> * In this version I followed Phillip's suggestion[1] to explain why I'm making
> the changes in each commit's description and create smaller, targeted commits.
> In v1 I did a lot of renaming to directory paths, test descriptions and files.
> In this version these renames have their own commits.
> 
> * Explained why I converted dir_iterator_advance() from iterative implementation
> to recursive in the related commit. Ævar[2] and Phillip[3] asked about why I did
> this conversion, so I thought it was appropriate to include an explanation in
> the commit's description.
> 
> * I talked about a subtle unexpected behavior of dir_iterator_begin() [4]. I was
> wrong about the two new states for the deepest directory. I introduced those
> states in order to implement recursive dir_iterator_advance() and not to deal
> with the unexpected behavior of dir_iterator_begin(). Therefore I splitted the
> conversion of dir_iteartor_advance() and the changes to deal with the unexpected
> behavior of dir_iteartor_begin() into two different commits. They need their own
> commits since they are two unrelated changes.
> 
> * Split the commit that introduces pre-order and post-order iteration into two.
> Like Phillip suggested here[5] the first changes dir-iterator default behavior
> and the second implements post-order iterator over directories. This helps split
> the tests introduced, which helps generate smaller diffs.
> 
> * Like Phillip suggested [6], I'll work with the existing code and change one
> aspect at a time in stages. In this series I converted entry.c remove_subtree()
> to use dir-iterator API instead of opendir/readdir/closedir API.
> 
> * The redundant tests [7] in this version were not removed. Instead I kept them
> for more detailed testing which helps in case a test fails.
> 
> * Ævar's idea[8] to change the default swich case in test-dir-iterator made me
> realize that test-dir-iterator does not print errno codes set by failed calls to
> dir_iterator_advance(). Therefore I made test-dir-iterator print them which
> makes all tests test_cmp for the errno code set by either dir_iteartor_begin()
> or dir_iterator_advance(). This presumably has the same effect as changing the
> default switch case to use BUG() macro. This change also led to two others, one
> that makes test-dir-iterator explicitly print ELOOP errno and the second that
> improves readability by consistently returning, C standard constants, either
> EXIT_SUCCESS or EXIT_FAILURE, instead of mixed integers and exit() calls.
> 
> * fixed coding style and introduces an enum for returning constants instead of
> more integers which worsens readability, as suggested by Ævar [9].
> 
> I also didn't CC Michael and Matheus in this version, since they weren't
> interested in v1 where I did CC them.

Thanks for the detailed change description. I've commented on the 
implementation but I've not had time to look at the test changes. I'm 
afraid I still don't see the initial refactoring of dir-iterator.c as am 
improvement. It is nice to see the post order traversal being used in 
remove_subtree(). Just to let you know, I'm going to off the list for 
the next couple of weeks, I'll try and catch up with this series later.

Best Wishes

Phillip

> [1]: https://lore.kernel.org/git/ed6656e0-a865-319e-0f56-23ab1da3eef3@gmail.com/
> [2]: https://lore.kernel.org/git/220411.86o817j2dt.gmgdl@evledraar.gmail.com/
> [3]: https://lore.kernel.org/git/35160d46-d337-2110-f968-238abb7e9f0e@gmail.com/
> [4]: https://lore.kernel.org/git/20220427154526.uuhpkoee322l7kmz@compass/
> [5]: https://lore.kernel.org/git/b75aaee8-c037-e8e0-6ee0-729922059352@gmail.com/
> [6]: https://lore.kernel.org/git/df287d4f-e9da-4ce0-d7e9-1b1fe7671aab@gmail.com/
> [7]: https://lore.kernel.org/git/220411.86sfqjj2o0.gmgdl@evledraar.gmail.com/
> [8]: https://lore.kernel.org/git/220411.86wnfvj2q6.gmgdl@evledraar.gmail.com/
> [9]: https://lore.kernel.org/git/220411.86o817j2dt.gmgdl@evledraar.gmail.com/
> 
> Plato Kiorpelidis (15):
>    t0066: refactor dir-iterator tests
>    t0066: remove dependency between unrelated tests
>    t0066: shorter expected and actual output file names
>    test-dir-iterator: consistently return EXIT_FAILURE or EXIT_SUCCESS
>    test-dir-iterator: print EACCES and ELOOP errno set by dir_iterator
>    test-dir-iterator: print errno name set by dir_iterator_advance
>    t0066: better test coverage for dir-iterator
>    t0066: reorder tests from simple to more complex
>    t0066: rename test directories
>    dir-iterator: refactor dir_iterator_advance()
>    dir-iterator: open root dir in dir_iterator_begin()
>    t0066: rename subtest descriptions
>    dir-iterator: option to iterate dirs in pre-order
>    dir-iterator: option to iterate dirs in post-order
>    entry.c: use dir-iterator to avoid explicit dir traversal
> 
>   builtin/clone.c              |    4 +-
>   dir-iterator.c               |  334 ++++++--
>   dir-iterator.h               |   36 +-
>   entry.c                      |   38 +-
>   refs/files-backend.c         |    2 +-
>   t/helper/test-dir-iterator.c |   24 +-
>   t/t0066-dir-iterator.sh      | 1478 +++++++++++++++++++++++++++++++---
>   7 files changed, 1704 insertions(+), 212 deletions(-)
> 

Re: [PATCH v2 00/15][GSoC] iterate dirs before or after their contents

[Junio C Hamano]

Plato Kiorpelidis <kioplato@gmail.com> writes:

> This is the second version of a patch series which implements pre-order and
> post-order iteration over directories. In this version I use the new iteration
> scheme to convert entry.c remove_subtree() function from using opendir/readdir/
> closedir API to dir-iterator API.

This cover letter seems to be written specifically for those who
have read v1.  It is not very advisable way to write your cover
letter, unless you are aiming to shrink your reviewer base in each
iteration.

Do not assume that everybody who didn't give reviews to the previous
round is not interested in the topic.

Instead, at least repeat the justification and the motivation enough
in each iteration to make it easier for those who were not involved
in earlier rounds to join the discussion.

"implements pre-order and post-order" is "what" and not "why".  Even
in this version, "convert remove_subtree()" is only a "what" and not
"why".  The reviewers need to learn why helping these patches is a
worthwhile thing to do before they decide to devote their time, so
helping them would in return help your cause.

You highlighted the difference since the previous round very well
(omitted here).  It would be very appreciated by those who were in
the first round.  Keep up the good work ;-)

> I also didn't CC Michael and Matheus in this version, since they weren't
> interested in v1 where I did CC them.

This is OK as long as the message goes to the list, but again, "no
responses" should not be taken as more than "no information".
Seeing no review comments from somebody on CC: is not a vote of "no"
by the recipient (nor it is not a vote of "yes").

Thanks for working on the topic.

Re: [PATCH v2 04/15] test-dir-iterator: consistently return EXIT_FAILURE or EXIT_SUCCESS

[Plato Kiorpelidis]

On 22/05/09 02:03PM, Junio C Hamano wrote:
> Plato Kiorpelidis <kioplato@gmail.com> writes:
> 
> > Throughout test-dir-iterator.c we were returning/exiting with either
> > integers or EXIT_FAILURE. Improve readability and reduce mental load
> > by being consistent with what test-dir-iterator returns through the
> > test-tool. Returning mixed constants and integers could indicate that
> > it matters for some reason e.g. architecture of test-tool and cmd__*
> > functions.
> >
> > EXIT_SUCCESS and EXIT_FAILURE are specified by the C standard.
> > That makes the code more portable and standardized.
> 
> And less portable for the invoking process of "git".  The invoking
> process used to be able to depend ou getting WEXITSTATUS() from our
> exit status to obtain "1" when we exited with 1; if we start exiting
> with EXIT_FAILURE, there is no guarantee what non-zero value is used.
> 
> So, I am not sure if this is a good direction to go in.

From what I understand, this is a point about why EXIT_FAILURE and EXIT_SUCCESS
are a bad idea in Git's case in general; not specifically in test-tool's case.
The test-tool doesn't use child processes, therefore it doesn't use the macro
WEXITSTATUS. As a result, supposedly, we could use EXIT_FAILURE and EXIT_SUCCESS
constants in this case. However, we don't want to use them in order to stay
consistent throughtout Git's implementation. Is my understanding correct?

> > Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> > ---
> >  t/helper/test-dir-iterator.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Especially given that this is a helper for testing, we may want to
> return/exit with different non-zero value at different places to
> make it easier for the test script to tell where in the program we
> decided to exit a failure.  IOW, if we return not EXIT_FAILURE but 2
> (or whatever value that is not used elsewhere) in the first hunk,
> and let the second hunk continue to return 1, then the calling test
> script can tell which one decided to fail.
> 
> As to EXIT_SUCCESS, I do not have a strong opinion against it, but
> because EXIT_FAILURE is a bad idea, we probably should avoid it for
> consistency.

This improves upon my attempt to be consistent in what we return, by also giving
the option to tell where in the program we exited a failure. I'll adopt this in
the next iteration of these series, v3.

> Thanks.

Thanks,
Plato

Re: [PATCH v2 10/15] dir-iterator: refactor dir_iterator_advance()

[Plato Kiorpelidis]

On 22/05/09 02:16PM, Junio C Hamano wrote:
> Plato Kiorpelidis <kioplato@gmail.com> writes:
> 
> > Simplify dir_iterator_advance() by converting from iterative to
> > recursive implementation. This conversion makes dir-iterator more
> > maintainable for the following reasons:
> >   * dir-iterator iterates the file-system, which is a tree structure.
> >     Traditionally, tree traversals, in textbooks, lectures and online
> >     sources are implemented recursively and not iteratively. Therefore
> >     it helps reduce mental load for readers, since it's easier to follow
> >     as it reminds of the same tree traversals we use on tree structures.
> 
> Careful.
> 
> Many algorithms that are taught in the recursive form in textbooks
> are turned into iterative in production systems for a reason.  To
> avoid too deep a recursion wasting too much stack space.  A loop
> with management of work items using in-program data structures like
> stack or queue often makes a large traversal bearable.
> 
> The most obvious example is our history traversal code.  History
> stored in Git is a tree structure, but no sane reimplementation of
> Git (well, at least those that want to be able to deal with a
> history larger than a toy project) would implement "git log" using a
> recursive algorithm.

That's a good point, I didn't think about that. It's also something that's
objective, so it's easier to reach a conclusion. This whole refactoring of
dir_iterator_advance() is a matter of opinion on what's more readable or not.

In this case, however, the dir_iterator_advance() is performing tail recursion
which doesn't require stack space. It reuses the stack frame from the current
call for the next call. Does this still pose a problem and why? If it does, I've
got no problem to work with the existing iterative implementation.

Thanks,
Plato

Re: [PATCH v2 13/15] dir-iterator: option to iterate dirs in pre-order

[Plato Kiorpelidis]

On 22/05/10 02:07PM, Phillip Wood wrote:
> Hi Plato
> 
> On 09/05/2022 18:51, Plato Kiorpelidis wrote:
> > Introduce a new option to dir-iterator, using dir_iterator_begin()
> > flags parameter, allowing to control whether or not directories will
> > be exposed before their contents. In essence, pre-order traversal over
> > file system entries that are directories.
> > 
> > This changes the default behavior of the dir-iterator API. Instead
> > of iterating directories before doing so over their contents, the new
> > default behavior does not expose directories at all. Iteration is still
> > performed, however, within directories, iterating over any other entry.
> > Only directory paths are ignored.
> > 
> > To iterate over directories in pre-order, reproducing the previous
> > default behavior, enable the new flag DIR_ITERATOR_DIRS_BEFORE in the
> > flags parameter of dir_iterator_begin():
> >    * ignore directories by not setting DIR_ITERATOR_DIRS_BEFORE
> >    * iterate directories pre-order by enabling DIR_ITERATOR_DIRS_BEFORE
> > 
> > Adjust existing callers, in refs/files-backend.c and builtin/clone.c
> > to enable DIR_ITERATOR_DIRS_BEFORE since these callers require iteration
> > over directories before doing so over their contents.
> > 
> > Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to test
> > the new iteration scheme, which is the new default behavior, and the new
> > flag DIR_ITERATOR_DIRS_BEFORE which reproduces the old default behavior.
> 
> It's great that you've split this change out from the next patch. I had not
> realized when I looked at the last round that all the existing callers
> require pre-order traversal. Given that is the case I'm finding it hard to
> see how changing the default behavior to one that no caller is using is an
> improvement.

Changing the default behavior is required to simplify entry.c remove_subtree().
I would have kept dir-iterator's default iteration scheme as is, but how are we
going to deal with remove_subtree()? remove_subtree() requires iterating dirs
after their contents. We need to find a flag encoding that is a good design
choice and serves both existing and remove_subtree(), without limiting future
dir-iterator customers.

This encoding of flags was heavily discussed in the patch series that my work
is based on[1], most notably here[2].

[1]: https://lore.kernel.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/
[2]: https://public-inbox.org/git/1751d788-d1f1-1c97-b33b-f53dab78ef86@alum.mit.edu/

Thanks,
Plato

RE: [PATCH v2 13/15] dir-iterator: option to iterate dirs in pre-order

[rsbecker]

On May 18, 2022 1:41 PM, Plato Kiorpelidis wrote:
>On 22/05/10 02:07PM, Phillip Wood wrote:
>> Hi Plato
>>
>> On 09/05/2022 18:51, Plato Kiorpelidis wrote:
>> > Introduce a new option to dir-iterator, using dir_iterator_begin()
>> > flags parameter, allowing to control whether or not directories will
>> > be exposed before their contents. In essence, pre-order traversal
>> > over file system entries that are directories.
>> >
>> > This changes the default behavior of the dir-iterator API. Instead
>> > of iterating directories before doing so over their contents, the
>> > new default behavior does not expose directories at all. Iteration
>> > is still performed, however, within directories, iterating over any
other entry.
>> > Only directory paths are ignored.
>> >
>> > To iterate over directories in pre-order, reproducing the previous
>> > default behavior, enable the new flag DIR_ITERATOR_DIRS_BEFORE in
>> > the flags parameter of dir_iterator_begin():
>> >    * ignore directories by not setting DIR_ITERATOR_DIRS_BEFORE
>> >    * iterate directories pre-order by enabling
>> > DIR_ITERATOR_DIRS_BEFORE
>> >
>> > Adjust existing callers, in refs/files-backend.c and builtin/clone.c
>> > to enable DIR_ITERATOR_DIRS_BEFORE since these callers require
>> > iteration over directories before doing so over their contents.
>> >
>> > Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to
>> > test the new iteration scheme, which is the new default behavior,
>> > and the new flag DIR_ITERATOR_DIRS_BEFORE which reproduces the old
>default behavior.
>>
>> It's great that you've split this change out from the next patch. I
>> had not realized when I looked at the last round that all the existing
>> callers require pre-order traversal. Given that is the case I'm
>> finding it hard to see how changing the default behavior to one that
>> no caller is using is an improvement.
>
>Changing the default behavior is required to simplify entry.c
remove_subtree().
>I would have kept dir-iterator's default iteration scheme as is, but how
are we
>going to deal with remove_subtree()? remove_subtree() requires iterating
dirs
>after their contents. We need to find a flag encoding that is a good design
choice
>and serves both existing and remove_subtree(), without limiting future dir-
>iterator customers.
>
>This encoding of flags was heavily discussed in the patch series that my
work is
>based on[1], most notably here[2].
>
>[1]: https://lore.kernel.org/git/1493226219-33423-1-git-send-email-
>bnmvco@gmail.com/
>[2]: https://public-inbox.org/git/1751d788-d1f1-1c97-b33b-
>f53dab78ef86@alum.mit.edu/

Could this be a fallback position where nftw() is not available? I am not
how broadly nftw() is supported but it appears to do what you are looking
for.
--Randall

Re: [PATCH v2 04/15] test-dir-iterator: consistently return EXIT_FAILURE or EXIT_SUCCESS

[Junio C Hamano]

Plato Kiorpelidis <kioplato@gmail.com> writes:

>> And less portable for the invoking process of "git".  The invoking
>> process used to be able to depend ou getting WEXITSTATUS() from our
>> exit status to obtain "1" when we exited with 1; if we start exiting
>> with EXIT_FAILURE, there is no guarantee what non-zero value is used.
>> 
>> So, I am not sure if this is a good direction to go in.
>
> From what I understand, this is a point about why EXIT_FAILURE and EXIT_SUCCESS
> are a bad idea in Git's case in general; not specifically in test-tool's case.
> The test-tool doesn't use child processes, therefore it doesn't use the macro
> WEXITSTATUS. As a result, supposedly, we could use EXIT_FAILURE and EXIT_SUCCESS
> constants in this case. However, we don't want to use them in order to stay
> consistent throughtout Git's implementation. Is my understanding correct?

Yes, it is a bad idea for any tool (not limited to Git) whose
documentation does not say "on failure, it exits with non-zero exit
status", but signals what kind of failure with different non-zero
exit status values.  Perhaps the calling test scripts of test-tool
may only care about exit status being (or not being) zero, so we
could use EXIT_FAILURE vs EXIT_SUCCESS, as long as EXIT_FAILURE does
not exit with a value that confuses test_must_fail, but by design,
the code that uses EXIT_FAILURE cannot guarantee that---the whole
point of EXIT_FAILURE macro is to hide which exact value is used.
You are correct to assume that we'd want to avoid using EXIT_FAILURE
in test-tool because we do not want to tempt people to
copy-and-paste.

Thanks.

Re: [PATCH v2 13/15] dir-iterator: option to iterate dirs in pre-order

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> Could this be a fallback position where nftw() is not available? I am not
> how broadly nftw() is supported but it appears to do what you are looking
> for.

Yeah, nftw() sounds like a better approach than what we have been
doing for the past decade by writing our own iterator.

RE: [PATCH v2 13/15] dir-iterator: option to iterate dirs in pre-order

[rsbecker]

On May 18, 2022 2:09 PM Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> Could this be a fallback position where nftw() is not available? I am
>> not how broadly nftw() is supported but it appears to do what you are
>> looking for.
>
>Yeah, nftw() sounds like a better approach than what we have been doing for the
>past decade by writing our own iterator.

Not to mention being blindingly faster and lower resource usage on some "exotic" platforms 😊

Re: [PATCH v2 00/15][GSoC] iterate dirs before or after their contents

[Plato Kiorpelidis]

On 22/05/10 09:31AM, Junio C Hamano wrote:
> Plato Kiorpelidis <kioplato@gmail.com> writes:
> 
> > This is the second version of a patch series which implements pre-order and
> > post-order iteration over directories. In this version I use the new iteration
> > scheme to convert entry.c remove_subtree() function from using opendir/readdir/
> > closedir API to dir-iterator API.
> 
> This cover letter seems to be written specifically for those who
> have read v1.  It is not very advisable way to write your cover
> letter, unless you are aiming to shrink your reviewer base in each
> iteration.
> 
> Do not assume that everybody who didn't give reviews to the previous
> round is not interested in the topic.
> 
> Instead, at least repeat the justification and the motivation enough
> in each iteration to make it easier for those who were not involved
> in earlier rounds to join the discussion.
> 
> "implements pre-order and post-order" is "what" and not "why".  Even
> in this version, "convert remove_subtree()" is only a "what" and not
> "why".  The reviewers need to learn why helping these patches is a
> worthwhile thing to do before they decide to devote their time, so
> helping them would in return help your cause.
> 
> You highlighted the difference since the previous round very well
> (omitted here).  It would be very appreciated by those who were in
> the first round.  Keep up the good work ;-)
> 
> > I also didn't CC Michael and Matheus in this version, since they weren't
> > interested in v1 where I did CC them.
> 
> This is OK as long as the message goes to the list, but again, "no
> responses" should not be taken as more than "no information".
> Seeing no review comments from somebody on CC: is not a vote of "no"
> by the recipient (nor it is not a vote of "yes").
> 
> Thanks for working on the topic.

That's great advice, thank you. Indeed, I did assume the above. I'll
follow this up with a patch on how to write better cover letters in
the related section and file of Documentation/. I've seen these patch
series[1] and I've read through them in detail. I'll figure out how
to incorporate such information in them.

[1]: https://lore.kernel.org/git/pull.1226.v3.git.1652399017.gitgitgadget@gmail.com/

Thanks,
Plato