[RFC PATCH 0/6][GSoC] iterate dirs before or after their contents

[RFC PATCH 0/6][GSoC] iterate dirs before or after their contents

[Plato Kiorpelidis]

Hello. Some time ago I worked on converting opendir/readdir calls to use
dir-iterator [1], however this cleanup required to iterate directory paths after
their contents. Matheus pointed me out to a previous patch series[2] that
attempted to implement such functionality in dir-iterator. From it, I mainly
used Michael's feedback and feature requests and tried to include it in my work.

My fork: https://github.com/kioplato/git/tree/dir-iterator
CI: https://github.com/kioplato/git/actions/runs/2141288008
There are some memleaks, I'll track them down in v2.

I aim to implement more functionality in dir-iterator, my goal being to simplify
the codebase by introducing an abstraction layer for iterating directories.
I would like to eventually simplify read_directory_recursive(). I wanted to
check in with you to make sure I'm heading in the right direction with what I've
implemented.
  * Are my tests overly exhaustive?
  * As of now we can't thoroughly test dir-iterator on directories with complex
  structure since readdir produce dirents with undefined order in a directory.
  I thought about introducing a tool for generating permutations with stable
  parts in test-lib. Is there a need to something like this for other tests?
  Or maybe should I sort each level iterated by dir-iterator inside
  test-dir-iterator before printing to stdout? In these patches I did enumerate
  the path permutations for some tests by hand, but that's not viable really.
  * We also don't test for deleted entries between dir_iterator_advance() calls.
  * Are my comments too much? Throughout git, .c files don't have many comments,
  should I remove mine as well? I think they provide better context when reading
  through the source code.

I do understand that it probably is too early to worry about most of these.
However I wanted to communicate my thoughts and setup for the following
versions.

While I wait for review, I'll implement/fix:
  * DIR_ITERATOR_RECURSE proposed here[3], but with finer control. Instead of a
  flag I'll introduce a new integer parameter in dir_iterator_begin(), which
  will specify the maximum iteration depth.
    * Supplying 0 will have the same behavior as DIR_ITERATOR_RECURSE i.e. it
    will iterate over the entries of the root directory.
    * Supplying -1 will iterate to maximum depth. This is the default right now.
  * DIR_ITERATOR_DIRS_ONLY proposed here[4]. Enabling this, dir-iterator will
  show only directories. Failing to enable DIR_ITERATOR_DIRS_BEFORE and/or
  DIR_ITERATOR_DIRS_AFTER will result in dir_iterator_begin() returning NULL.
  Is this a good way to encode "show only directories" in the flags?

I'll include them along with feedback and suggestions from this version in the
next one.

I didn't refactor entry.c to use dir-iterator. It's a good first issue for
someone else to become introduced to the community. I applied my patch[1] and it
does not pass t/, as it used to, because of 15th test in t1092. Should I work on
entry.c in my next version or leave it alone for a newcomer?

This serves as my microproject for GSoC. Could my future work on dir-iterator
and cleanup of read_directory_recursive() and other customers of dir-iterator
become a seperate GSoC project I could undertake?

[1]: https://public-inbox.org/git/20191208180439.19018-1-otalpster@gmail.com/
[2]: https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/
[3]: https://public-inbox.org/git/CACsJy8DBa-oH3i+5P=iVr9NhJwsicZ43DO89WmvpYEQu90RrMw@mail.gmail.com/
[4]: https://public-inbox.org/git/xmqqmvc265hk.fsf@gitster.mtv.corp.google.com/

Plato Kiorpelidis (6):
  t0066: improve readablity of dir-iterator tests
  t0066: better test coverage for dir-iterator
  dir-iterator: refactor dir_iterator_advance()
  dir-iterator: iterate dirs before or after their contents
  t0066: remove redundant tests
  test-dir-iterator: handle EACCES errno by dir-iterator

 builtin/clone.c              |    4 +-
 dir-iterator.c               |  302 ++++++---
 dir-iterator.h               |   34 +-
 refs/files-backend.c         |    2 +-
 t/helper/test-dir-iterator.c |   23 +-
 t/t0066-dir-iterator.sh      | 1202 +++++++++++++++++++++++++++++++---
 6 files changed, 1371 insertions(+), 196 deletions(-)

-- 
2.35.1

[RFC PATCH 1/6] t0066: improve readablity of 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

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

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 63a1a45cd3..fb20219487 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -5,145 +5,154 @@ test_description='Test the dir-iterator functionality'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'setup' '
-	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 dir2/a/b/c/ &&
-	>dir2/a/b/c/d
+test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
+	mkdir -p dir6/a/b/c &&
+	>dir6/a/b/c/d &&
+
+
+	cat >expected-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
+	EOF
+'
+test_expect_success 'iteration of dir w/ three nested dirs w/ file' '
+	test-tool dir-iterator ./dir6 >actual-out &&
+	test_cmp expected-out actual-out
 '
 
-test_expect_success 'dir-iterator should iterate through all files' '
-	cat >expected-iteration-sorted-output <<-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
+test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
+	mkdir -p dir11/a/b/c/ &&
+	>dir11/b &&
+	>dir11/c &&
+	>dir11/a/e &&
+	>dir11/a/b/c/d &&
+	mkdir -p dir11/d/e/d/ &&
+	>dir11/d/e/d/a &&
+
+
+	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_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
+	test-tool dir-iterator ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
 
-	test-tool dir-iterator ./dir >out &&
-	sort out >./actual-iteration-sorted-output &&
+	test_cmp expected-sorted-out actual-sorted-out
+'
 
-	test_cmp expected-iteration-sorted-output actual-iteration-sorted-output
+test_expect_success 'dir_iterator_begin() should fail upon inexistent paths' '
+	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-out &&
+
+	test_must_fail test-tool dir-iterator ./inexistent-path >actual-out &&
+	test_cmp expected-inexistent-path-out actual-out
 '
 
-test_expect_success 'dir-iterator should list files in the correct order' '
-	cat >expected-pre-order-output <<-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_expect_success 'dir_iterator_begin() should fail upon non directory paths' '
+	>some-file &&
 
-	test-tool dir-iterator ./dir2 >actual-pre-order-output &&
 
-	test_cmp expected-pre-order-output actual-pre-order-output
-'
+	echo "dir_iterator_begin failure: ENOTDIR" >expected-non-dir-out &&
 
-test_expect_success 'begin should fail upon inexistent paths' '
-	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_must_fail test-tool dir-iterator ./some-file >actual-out &&
+	test_cmp expected-non-dir-out actual-out &&
 
-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_cmp expected-non-dir-output actual-non-dir-output
+	test_must_fail test-tool dir-iterator --pedantic ./some-file >actual-out &&
+	test_cmp expected-non-dir-out actual-out
 '
 
-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
+test_expect_success POSIXPERM,SANITY \
+'dir_iterator_advance() should not fail on errors by default' '
 
-	mkdir -p dir3/a &&
-	>dir3/a/b &&
-	chmod 0 dir3/a &&
+	mkdir -p dir13/a &&
+	>dir13/a/b &&
+	chmod 0 dir13/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
-'
 
-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
+	cat >expected-no-permissions-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
 	EOF
 
-	mkdir -p dir3/a &&
-	>dir3/a/b &&
-	chmod 0 dir3/a &&
+	test-tool dir-iterator ./dir13 >actual-out &&
+	test_cmp expected-no-permissions-out actual-out &&
 
-	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 dir13/a &&
+	rm -rf dir13
 '
 
-test_expect_success SYMLINKS 'setup dirs with symlinks' '
-	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
-'
+test_expect_success POSIXPERM,SANITY \
+'dir_iterator_advance() should fail on errors, w/ pedantic flag' '
 
-test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
-	cat >expected-no-follow-sorted-output <<-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
+	mkdir -p dir13/a &&
+	>dir13/a/b &&
+	chmod 0 dir13/a &&
+
+
+	cat >expected-no-permissions-pedantic-out <<-EOF &&
+	[d] (a) [a] ./dir13/a
+	dir_iterator_advance failure
 	EOF
 
-	test-tool dir-iterator ./dir4 >out &&
-	sort out >actual-no-follow-sorted-output &&
+	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
+	test_cmp expected-no-permissions-pedantic-out actual-out &&
 
-	test_cmp expected-no-follow-sorted-output actual-no-follow-sorted-output
+	chmod 755 dir13/a &&
+	rm -rf dir13
 '
 
-test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
-	cat >expected-follow-sorted-output <<-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
+test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
+	mkdir -p dir14/a &&
+	mkdir -p dir14/b/c &&
+	>dir14/a/d &&
+	ln -s d dir14/a/e &&
+	ln -s ../b dir14/a/f &&
+
+
+	cat >expected-dont-follow-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
+	cat >expected-follow-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_expect_success SYMLINKS \
+'dont-follow-symlinks of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks of dir w/ symlinks w/o cycle' '
 
-	test-tool dir-iterator --follow-symlinks ./dir4 >out &&
-	sort out >actual-follow-sorted-output &&
+	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-follow-sorted-output actual-follow-sorted-output
+	test_cmp expected-follow-sorted-out actual-sorted-out
 '
 
 test_done
-- 
2.35.1

Re: [RFC PATCH 1/6] t0066: improve readablity of dir-iterator tests

[Phillip Wood]

Hi Plato

On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> 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

When writing a commit message it is important to explain why you are 
making the changes in the patch, rather than just describe the changes 
themselves.

In general modernizing or standardizing existing tests before adding new 
ones is a good idea. However it is important to do it in a way that lets 
reviewers see there are no unintended changes. In this patch so much is 
changed it's really hard to tell if you have changed any of the tests or 
not. It would be much easier if you had not renamed all the directories 
and files that are created and renamed the tests as well.

I think that having separate setup tests makes sense as we're going to 
want to test different iteration schemes over the same hierarchy but it 
would be helpful to have the "expect" files written in the same test 
that calls test_cmp. That way it is much easier to debug test failures 
in the CI as it one can see the contents of "expect" as well as the diff 
from test_cmp in the CI output.

Best Wishes

Phillip

> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>   t/t0066-dir-iterator.sh | 227 +++++++++++++++++++++-------------------
>   1 file changed, 118 insertions(+), 109 deletions(-)
> 
> diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
> index 63a1a45cd3..fb20219487 100755
> --- a/t/t0066-dir-iterator.sh
> +++ b/t/t0066-dir-iterator.sh
> @@ -5,145 +5,154 @@ test_description='Test the dir-iterator functionality'
>   TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
> -test_expect_success 'setup' '
> -	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 dir2/a/b/c/ &&
> -	>dir2/a/b/c/d
> +test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
> +	mkdir -p dir6/a/b/c &&
> +	>dir6/a/b/c/d &&
> +
> +
> +	cat >expected-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
> +	EOF
> +'
> +test_expect_success 'iteration of dir w/ three nested dirs w/ file' '
> +	test-tool dir-iterator ./dir6 >actual-out &&
> +	test_cmp expected-out actual-out
>   '
>   
> -test_expect_success 'dir-iterator should iterate through all files' '
> -	cat >expected-iteration-sorted-output <<-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
> +test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
> +	mkdir -p dir11/a/b/c/ &&
> +	>dir11/b &&
> +	>dir11/c &&
> +	>dir11/a/e &&
> +	>dir11/a/b/c/d &&
> +	mkdir -p dir11/d/e/d/ &&
> +	>dir11/d/e/d/a &&
> +
> +
> +	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_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
> +	test-tool dir-iterator ./dir11 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
>   
> -	test-tool dir-iterator ./dir >out &&
> -	sort out >./actual-iteration-sorted-output &&
> +	test_cmp expected-sorted-out actual-sorted-out
> +'
>   
> -	test_cmp expected-iteration-sorted-output actual-iteration-sorted-output
> +test_expect_success 'dir_iterator_begin() should fail upon inexistent paths' '
> +	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-out &&
> +
> +	test_must_fail test-tool dir-iterator ./inexistent-path >actual-out &&
> +	test_cmp expected-inexistent-path-out actual-out
>   '
>   
> -test_expect_success 'dir-iterator should list files in the correct order' '
> -	cat >expected-pre-order-output <<-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_expect_success 'dir_iterator_begin() should fail upon non directory paths' '
> +	>some-file &&
>   
> -	test-tool dir-iterator ./dir2 >actual-pre-order-output &&
>   
> -	test_cmp expected-pre-order-output actual-pre-order-output
> -'
> +	echo "dir_iterator_begin failure: ENOTDIR" >expected-non-dir-out &&
>   
> -test_expect_success 'begin should fail upon inexistent paths' '
> -	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_must_fail test-tool dir-iterator ./some-file >actual-out &&
> +	test_cmp expected-non-dir-out actual-out &&
>   
> -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_cmp expected-non-dir-output actual-non-dir-output
> +	test_must_fail test-tool dir-iterator --pedantic ./some-file >actual-out &&
> +	test_cmp expected-non-dir-out actual-out
>   '
>   
> -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
> +test_expect_success POSIXPERM,SANITY \
> +'dir_iterator_advance() should not fail on errors by default' '
>   
> -	mkdir -p dir3/a &&
> -	>dir3/a/b &&
> -	chmod 0 dir3/a &&
> +	mkdir -p dir13/a &&
> +	>dir13/a/b &&
> +	chmod 0 dir13/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
> -'
>   
> -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
> +	cat >expected-no-permissions-out <<-EOF &&
> +	[d] (a) [a] ./dir13/a
>   	EOF
>   
> -	mkdir -p dir3/a &&
> -	>dir3/a/b &&
> -	chmod 0 dir3/a &&
> +	test-tool dir-iterator ./dir13 >actual-out &&
> +	test_cmp expected-no-permissions-out actual-out &&
>   
> -	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 dir13/a &&
> +	rm -rf dir13
>   '
>   
> -test_expect_success SYMLINKS 'setup dirs with symlinks' '
> -	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
> -'
> +test_expect_success POSIXPERM,SANITY \
> +'dir_iterator_advance() should fail on errors, w/ pedantic flag' '
>   
> -test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
> -	cat >expected-no-follow-sorted-output <<-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
> +	mkdir -p dir13/a &&
> +	>dir13/a/b &&
> +	chmod 0 dir13/a &&
> +
> +
> +	cat >expected-no-permissions-pedantic-out <<-EOF &&
> +	[d] (a) [a] ./dir13/a
> +	dir_iterator_advance failure
>   	EOF
>   
> -	test-tool dir-iterator ./dir4 >out &&
> -	sort out >actual-no-follow-sorted-output &&
> +	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
> +	test_cmp expected-no-permissions-pedantic-out actual-out &&
>   
> -	test_cmp expected-no-follow-sorted-output actual-no-follow-sorted-output
> +	chmod 755 dir13/a &&
> +	rm -rf dir13
>   '
>   
> -test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
> -	cat >expected-follow-sorted-output <<-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
> +test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
> +	mkdir -p dir14/a &&
> +	mkdir -p dir14/b/c &&
> +	>dir14/a/d &&
> +	ln -s d dir14/a/e &&
> +	ln -s ../b dir14/a/f &&
> +
> +
> +	cat >expected-dont-follow-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
> +	cat >expected-follow-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_expect_success SYMLINKS \
> +'dont-follow-symlinks of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'follow-symlinks of dir w/ symlinks w/o cycle' '
>   
> -	test-tool dir-iterator --follow-symlinks ./dir4 >out &&
> -	sort out >actual-follow-sorted-output &&
> +	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
>   
> -	test_cmp expected-follow-sorted-output actual-follow-sorted-output
> +	test_cmp expected-follow-sorted-out actual-sorted-out
>   '
>   
>   test_done

Re: [RFC PATCH 1/6] t0066: improve readablity of dir-iterator tests

[Plato Kiorpelidis]

On 22/04/11 02:16PM, Phillip Wood wrote:
> Hi Plato
> 
> On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> > 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
> 
> When writing a commit message it is important to explain why you are making
> the changes in the patch, rather than just describe the changes themselves.
> 
> In general modernizing or standardizing existing tests before adding new
> ones is a good idea. However it is important to do it in a way that lets
> reviewers see there are no unintended changes. In this patch so much is
> changed it's really hard to tell if you have changed any of the tests or
> not. It would be much easier if you had not renamed all the directories and
> files that are created and renamed the tests as well.
> 
> I think that having separate setup tests makes sense as we're going to want
> to test different iteration schemes over the same hierarchy but it would be
> helpful to have the "expect" files written in the same test that calls
> test_cmp. That way it is much easier to debug test failures in the CI as it
> one can see the contents of "expect" as well as the diff from test_cmp in
> the CI output.

I agree. All three points have been adopted in the upcoming v2:
1. Explain _why_ I'm making these changes.
2. Create targeted commits with smaller diffs. (This commit has been splitted)
3. Write "expected-*" files in the same test that calls test_cmp.

Thanks,
Plato

> 
> Best Wishes
> 
> Phillip
> 
> > Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> > ---
> >   t/t0066-dir-iterator.sh | 227 +++++++++++++++++++++-------------------
> >   1 file changed, 118 insertions(+), 109 deletions(-)
> > 
> > diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
> > index 63a1a45cd3..fb20219487 100755
> > --- a/t/t0066-dir-iterator.sh
> > +++ b/t/t0066-dir-iterator.sh
> > @@ -5,145 +5,154 @@ test_description='Test the dir-iterator functionality'
> >   TEST_PASSES_SANITIZE_LEAK=true
> >   . ./test-lib.sh
> > -test_expect_success 'setup' '
> > -	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 dir2/a/b/c/ &&
> > -	>dir2/a/b/c/d
> > +test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
> > +	mkdir -p dir6/a/b/c &&
> > +	>dir6/a/b/c/d &&
> > +
> > +
> > +	cat >expected-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
> > +	EOF
> > +'
> > +test_expect_success 'iteration of dir w/ three nested dirs w/ file' '
> > +	test-tool dir-iterator ./dir6 >actual-out &&
> > +	test_cmp expected-out actual-out
> >   '
> > -test_expect_success 'dir-iterator should iterate through all files' '
> > -	cat >expected-iteration-sorted-output <<-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
> > +test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
> > +	mkdir -p dir11/a/b/c/ &&
> > +	>dir11/b &&
> > +	>dir11/c &&
> > +	>dir11/a/e &&
> > +	>dir11/a/b/c/d &&
> > +	mkdir -p dir11/d/e/d/ &&
> > +	>dir11/d/e/d/a &&
> > +
> > +
> > +	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_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
> > +	test-tool dir-iterator ./dir11 >actual-out &&
> > +	sort actual-out >actual-sorted-out &&
> > -	test-tool dir-iterator ./dir >out &&
> > -	sort out >./actual-iteration-sorted-output &&
> > +	test_cmp expected-sorted-out actual-sorted-out
> > +'
> > -	test_cmp expected-iteration-sorted-output actual-iteration-sorted-output
> > +test_expect_success 'dir_iterator_begin() should fail upon inexistent paths' '
> > +	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-out &&
> > +
> > +	test_must_fail test-tool dir-iterator ./inexistent-path >actual-out &&
> > +	test_cmp expected-inexistent-path-out actual-out
> >   '
> > -test_expect_success 'dir-iterator should list files in the correct order' '
> > -	cat >expected-pre-order-output <<-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_expect_success 'dir_iterator_begin() should fail upon non directory paths' '
> > +	>some-file &&
> > -	test-tool dir-iterator ./dir2 >actual-pre-order-output &&
> > -	test_cmp expected-pre-order-output actual-pre-order-output
> > -'
> > +	echo "dir_iterator_begin failure: ENOTDIR" >expected-non-dir-out &&
> > -test_expect_success 'begin should fail upon inexistent paths' '
> > -	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_must_fail test-tool dir-iterator ./some-file >actual-out &&
> > +	test_cmp expected-non-dir-out actual-out &&
> > -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_cmp expected-non-dir-output actual-non-dir-output
> > +	test_must_fail test-tool dir-iterator --pedantic ./some-file >actual-out &&
> > +	test_cmp expected-non-dir-out actual-out
> >   '
> > -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
> > +test_expect_success POSIXPERM,SANITY \
> > +'dir_iterator_advance() should not fail on errors by default' '
> > -	mkdir -p dir3/a &&
> > -	>dir3/a/b &&
> > -	chmod 0 dir3/a &&
> > +	mkdir -p dir13/a &&
> > +	>dir13/a/b &&
> > +	chmod 0 dir13/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
> > -'
> > -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
> > +	cat >expected-no-permissions-out <<-EOF &&
> > +	[d] (a) [a] ./dir13/a
> >   	EOF
> > -	mkdir -p dir3/a &&
> > -	>dir3/a/b &&
> > -	chmod 0 dir3/a &&
> > +	test-tool dir-iterator ./dir13 >actual-out &&
> > +	test_cmp expected-no-permissions-out actual-out &&
> > -	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 dir13/a &&
> > +	rm -rf dir13
> >   '
> > -test_expect_success SYMLINKS 'setup dirs with symlinks' '
> > -	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
> > -'
> > +test_expect_success POSIXPERM,SANITY \
> > +'dir_iterator_advance() should fail on errors, w/ pedantic flag' '
> > -test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
> > -	cat >expected-no-follow-sorted-output <<-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
> > +	mkdir -p dir13/a &&
> > +	>dir13/a/b &&
> > +	chmod 0 dir13/a &&
> > +
> > +
> > +	cat >expected-no-permissions-pedantic-out <<-EOF &&
> > +	[d] (a) [a] ./dir13/a
> > +	dir_iterator_advance failure
> >   	EOF
> > -	test-tool dir-iterator ./dir4 >out &&
> > -	sort out >actual-no-follow-sorted-output &&
> > +	test_must_fail test-tool dir-iterator --pedantic ./dir13 >actual-out &&
> > +	test_cmp expected-no-permissions-pedantic-out actual-out &&
> > -	test_cmp expected-no-follow-sorted-output actual-no-follow-sorted-output
> > +	chmod 755 dir13/a &&
> > +	rm -rf dir13
> >   '
> > -test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '
> > -	cat >expected-follow-sorted-output <<-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
> > +test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
> > +	mkdir -p dir14/a &&
> > +	mkdir -p dir14/b/c &&
> > +	>dir14/a/d &&
> > +	ln -s d dir14/a/e &&
> > +	ln -s ../b dir14/a/f &&
> > +
> > +
> > +	cat >expected-dont-follow-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
> > +	cat >expected-follow-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_expect_success SYMLINKS \
> > +'dont-follow-symlinks of dir w/ symlinks w/o cycle' '
> > +
> > +	test-tool dir-iterator ./dir14 >actual-out &&
> > +	sort actual-out >actual-sorted-out &&
> > +
> > +	test_cmp expected-dont-follow-sorted-out actual-sorted-out
> > +'
> > +test_expect_success SYMLINKS \
> > +'follow-symlinks of dir w/ symlinks w/o cycle' '
> > -	test-tool dir-iterator --follow-symlinks ./dir4 >out &&
> > -	sort out >actual-follow-sorted-output &&
> > +	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
> > +	sort actual-out >actual-sorted-out &&
> > -	test_cmp expected-follow-sorted-output actual-follow-sorted-output
> > +	test_cmp expected-follow-sorted-out actual-sorted-out
> >   '
> >   test_done
> 

[RFC PATCH 2/6] 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

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

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index fb20219487..0a98dd54ba 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -5,6 +5,94 @@ 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 &&
+
+
+	cat >expected-out <<-EOF
+	[f] (a) [a] ./dir1/a
+	EOF
+'
+test_expect_success 'iteration of dir with a file' '
+	test-tool dir-iterator ./dir1 >actual-out &&
+	test_cmp expected-out actual-out
+'
+
+test_expect_success 'setup -- dir with a single dir' '
+	mkdir -p dir2/a &&
+
+
+	cat >expected-out <<-EOF
+	[d] (a) [a] ./dir2/a
+	EOF
+'
+test_expect_success 'iteration of dir with a single dir' '
+	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 dir3/a &&
+	chmod 0 dir3/a &&
+
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir3/a
+	EOF
+	cat >expected-pedantic-out <<-EOF
+	[d] (a) [a] ./dir3/a
+	dir_iterator_advance failure
+	EOF
+'
+test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
+	test-tool dir-iterator ./dir3/ >actual-out &&
+	test_cmp expected-out actual-out
+'
+test_expect_success POSIXPERM,SANITY 'pedantic iteration of dir w/ dir w/o perms' '
+	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
+	test_cmp expected-pedantic-out actual-out
+'
+
+test_expect_success 'setup -- dir w/ five files' '
+	mkdir dir4 &&
+	>dir4/a &&
+	>dir4/b &&
+	>dir4/c &&
+	>dir4/d &&
+	>dir4/e &&
+
+
+	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_expect_success 'iteration of dir w/ five files' '
+	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 dir5/a &&
+	>dir5/a/b &&
+
+
+	cat >expected-out <<-EOF
+	[d] (a) [a] ./dir5/a
+	[f] (a/b) [b] ./dir5/a/b
+	EOF
+'
+test_expect_success 'iteration of dir w/ dir w/ a file' '
+	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 dir6/a/b/c &&
 	>dir6/a/b/c/d &&
@@ -22,6 +110,149 @@ test_expect_success 'iteration of dir w/ three nested dirs w/ file' '
 	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 &&
+
+	cat >expected-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	EOF
+	cat >expected-pedantic-out <<-EOF
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	dir_iterator_advance failure
+	EOF
+'
+test_expect_success POSIXPERM,SANITY \
+'iteration of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	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' '
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
+	test_cmp expected-pedantic-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 &&
+
+
+	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_expect_success 'iteration of dir w/ two dirs each w/ file' '
+	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 &&
+
+
+	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_expect_success \
+'iteration of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
+
+	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 &&
+
+
+	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_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
+	test-tool dir-iterator ./dir10/ >actual-out &&
+	(
+		test_cmp expected-out1 actual-out ||
+		test_cmp expected-out2 actual-out
+	)
+'
+
 test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
 	mkdir -p dir11/a/b/c/ &&
 	>dir11/b &&
@@ -53,6 +284,28 @@ test_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
 	test_cmp expected-sorted-out actual-sorted-out
 '
 
+test_expect_success POSIXPERM,SANITY \
+'dir_iterator_advance() should fail on root dir w/o perms' '
+
+	mkdir -p dir12/a &&
+	>dir12/a/b &&
+	chmod 0 dir12 &&
+
+
+	cat >expected-no-permissions-out <<-EOF &&
+	dir_iterator_advance failure
+	EOF
+
+	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&
+	test_cmp expected-no-permissions-out actual-out &&
+
+	test_must_fail test-tool dir-iterator --pedantic ./dir12 >actual-out &&
+	test_cmp expected-no-permissions-out actual-out &&
+
+	chmod 755 dir12 &&
+	rm -rf dir12
+'
+
 test_expect_success 'dir_iterator_begin() should fail upon inexistent paths' '
 	echo "dir_iterator_begin failure: ENOENT" >expected-inexistent-path-out &&
 
@@ -155,4 +408,42 @@ test_expect_success SYMLINKS \
 	test_cmp expected-follow-sorted-out actual-sorted-out
 '
 
+test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
+	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 &&
+
+	cat >expected-dont-follow-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
+
+	cat >expected-pedantic-follow-tailed-out <<-EOF
+	dir_iterator_advance failure
+	EOF
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks of dir w/ symlinks w/ cycle' '
+
+	test-tool dir-iterator ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks of dir w/ symlinks w/ cycle' '
+
+	test_must_fail test-tool dir-iterator \
+		--pedantic --follow-symlinks ./dir15 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-pedantic-follow-tailed-out actual-tailed-out
+'
+
 test_done
-- 
2.35.1

[RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

[Plato Kiorpelidis]

Simplify dir_iterator_advance by switch from iterative to recursive
implementation. In each recursive step one action is performed.

This makes dir-iterator easier to work with, understand and introduce
new functionality.

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

diff --git a/dir-iterator.c b/dir-iterator.c
index b17e9f970a..0f9ed95958 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;
 
@@ -45,34 +45,53 @@ struct dir_iterator_int {
 
 /*
  * 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.
+ *
+ * Return 1 on success.
+ * Return -1 on failure when errno == ENOENT, leaving the stack unchanged.
+ * Return -2 on failure when errno != ENOENT, leaving the stack unchanged.
+ */
+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 1;
+
+	if ((level->dir = opendir(iter->base.path.buf)) != NULL)
+		return 1;
+
+	saved_errno = errno;
+	if (errno != ENOENT) {
+		warning_errno("error opening directory '%s'", iter->base.path.buf);
 		errno = saved_errno;
-		return -1;
+		return -2;
 	}
-
-	return 0;
+	errno = saved_errno;
+	return -1;
 }
 
 /*
@@ -81,12 +100,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 +111,121 @@ 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:
+ * 1 on successful exposure of the provided entry.
+ * -1 on failed exposure because entry does not exist.
+ * -2 on failed exposure because of error 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 -2;  // Stat failed not with ENOENT.
+	} else if (stat_err && errno == ENOENT)
+		return -1;  // Stat failed with 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 1;
 }
 
 int dir_iterator_advance(struct dir_iterator *dir_iterator)
 {
-	struct dir_iterator_int *iter =
-		(struct dir_iterator_int *)dir_iterator;
-
-	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)
-			goto error_out;
+	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;
+
+	/*
+	 * 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 == -2 && PEDANTIC)
+		goto error_out;
+	else if (activate_err == -2 || activate_err == -1) {
+		/*
+		 * We activate the root level at `dir_iterator_begin()`.
+		 * Therefore, there isn't any case to run out of levels.
+		 */
+
+		--iter->levels_nr;
+
+		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);
 
-		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) {
+	if (dir_entry == NULL) {
+		if (errno) {
+			warning_errno("errno reading dir '%s'", iter->base.path.buf);
+			if (iter->flags & DIR_ITERATOR_PEDANTIC) goto error_out;
+			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 == -2 && PEDANTIC)
+		goto error_out;
+
+	if (expose_err == 1)
+		push_level(iter);
+
+	if (expose_err == -1)
+		return dir_iterator_advance(dir_iterator);
+
+	return ITER_OK;
 
 error_out:
 	dir_iterator_abort(dir_iterator);
@@ -207,6 +263,7 @@ 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);
+	strbuf_trim_trailing_dir_sep(&iter->base.path);
 
 	ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
 	iter->levels_nr = 0;
@@ -226,6 +283,16 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
 		goto error_out;
 	}
 
+	if (push_level(iter) != 1) {
+		saved_errno = ENOTDIR;
+		goto error_out;
+	}
+
+	if (activate_level(iter) != 1) {
+		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 0a98dd54ba..2437ab21c4 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -285,7 +285,7 @@ test_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
 '
 
 test_expect_success POSIXPERM,SANITY \
-'dir_iterator_advance() should fail on root dir w/o perms' '
+'dir_iterator_begin() should fail on root dir w/o perms' '
 
 	mkdir -p dir12/a &&
 	>dir12/a/b &&
@@ -293,7 +293,7 @@ test_expect_success POSIXPERM,SANITY \
 
 
 	cat >expected-no-permissions-out <<-EOF &&
-	dir_iterator_advance failure
+	dir_iterator_begin failure: ESOMETHINGELSE
 	EOF
 
 	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&
-- 
2.35.1

Re: [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 10 2022, Plato Kiorpelidis wrote:

> Simplify dir_iterator_advance by switch from iterative to recursive
> implementation. In each recursive step one action is performed.
>
> This makes dir-iterator easier to work with, understand and introduce
> new functionality.
>
> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>  dir-iterator.c          | 225 ++++++++++++++++++++++++++--------------
>  t/t0066-dir-iterator.sh |   4 +-
>  2 files changed, 148 insertions(+), 81 deletions(-)
>
> diff --git a/dir-iterator.c b/dir-iterator.c
> index b17e9f970a..0f9ed95958 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;
>  
> @@ -45,34 +45,53 @@ struct dir_iterator_int {
>  
>  /*
>   * 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;

style: missing \n before "return".

Also, the one existing caller before this patch is:

    if (S_ISDIR(iter->base.st.st_mode) && push_level(iter))

Why not continue to do that?

> +/*
> + * Activate most recent pushed level.
> + *
> + * Return 1 on success.
> + * Return -1 on failure when errno == ENOENT, leaving the stack unchanged.
> + * Return -2 on failure when errno != ENOENT, leaving the stack unchanged.
> + */
> +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 1;
> +
> +	if ((level->dir = opendir(iter->base.path.buf)) != NULL)
> +		return 1;
> +
> +	saved_errno = errno;
> +	if (errno != ENOENT) {
> +		warning_errno("error opening directory '%s'", iter->base.path.buf);
>  		errno = saved_errno;
> -		return -1;
> +		return -2;

Perhaps we should just add an enum for these return values instead of
adding more negative/positive values here. That makes your later calls
of activate_level() more idiomaic. E.g. !activate_level() instead of
activate_level() == 1.

>  		warning_errno("failed to stat '%s'", iter->base.path.buf);
> +		return -2;  // Stat failed not with ENOENT.

Don't use // comments, use /* .. */
> +	} else if (stat_err && errno == ENOENT)
> +		return -1;  // Stat failed with ENOENT.

Missing {} here for the else if.

> +	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;

We usually don't add \n\n in the middle of variable decls.

> +
> +	/*
> +	 * 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 == -2 && PEDANTIC)
> +		goto error_out;
> +	else if (activate_err == -2 || activate_err == -1) {
> +		/*
> +		 * We activate the root level at `dir_iterator_begin()`.
> +		 * Therefore, there isn't any case to run out of levels.
> +		 */
> +
> +		--iter->levels_nr;
> +
> +		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);
>  
> -		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) {
> +	if (dir_entry == NULL) {

Don't compare against NULL, use !dir_entry.

Also: Manually resetting "errno" before isn't needed. It will be (re)set
if readdir() returns NULL().

> +		if (errno) {
> +			warning_errno("errno reading dir '%s'", iter->base.path.buf);
> +			if (iter->flags & DIR_ITERATOR_PEDANTIC) goto error_out;

more missing \n before goto/return.

Re: [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

[Phillip Wood]

On 11/04/2022 12:11, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Apr 10 2022, Plato Kiorpelidis wrote:
> 
>> +
>> +	errno = 0;
>> +	dir_entry = readdir(level->dir);
>>   
>> -		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) {
>> +	if (dir_entry == NULL) {
> 
> Don't compare against NULL, use !dir_entry.
> 
> Also: Manually resetting "errno" before isn't needed. It will be (re)set
> if readdir() returns NULL().

That's not what the man page says

     If the end of the directory stream is reached, NULL is returned 

     and errno is not changed.  If an error occurs, NULL is returned 

     and errno is set to indicate the error.  To distinguish end of 

     stream from an error, set errno to zero before calling readdir() 

     and then check the value of errno if NULL is returned. 


Best Wishes

Phillip

Re: [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

[Plato Kiorpelidis]

On 22/04/11 01:11PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Apr 10 2022, Plato Kiorpelidis wrote:
> 
> > Simplify dir_iterator_advance by switch from iterative to recursive
> > implementation. In each recursive step one action is performed.
> >
> > This makes dir-iterator easier to work with, understand and introduce
> > new functionality.
> >
> > Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> > ---
> >
> > [...]
> >
> > @@ -45,34 +45,53 @@ struct dir_iterator_int {
> >  
> >  /*
> >   * 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;
> 
> style: missing \n before "return".
> 
> Also, the one existing caller before this patch is:
> 
>     if (S_ISDIR(iter->base.st.st_mode) && push_level(iter))
> 
> Why not continue to do that?

In this patch I also fixed a problem that's subtle. Previously, when we called
dir_iterator_begin() and the specified path is invalid the call won't return
NULL. Instead the call succeeds and the first call to dir_iterator_advance()
fails. I believe that's unexpected. The expected behavior would be to return
NULL from dir_iterator_begin() if the specified path is invalid. Successful call
to dir_iterator_begin() suggests that the root path is valid.

To deal with that I introduced two states for the most recent directory. The
first is just pushed into the levels stack and the other pushed and activated.
This way we can "activate" the root directory in dir_iterator_begin(), but we
also need to reorder the calls to push_level() and activate_level() as a result.
We need to push directories after we read them and activate the most recent one
when dir_iterator_advance() is called.

I'll document that change in the related commit in v2.

Do you have any objection about this change?

> > +/*
> > + * Activate most recent pushed level.
> > + *
> > + * Return 1 on success.
> > + * Return -1 on failure when errno == ENOENT, leaving the stack unchanged.
> > + * Return -2 on failure when errno != ENOENT, leaving the stack unchanged.
> > + */
> > +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 1;
> > +
> > +	if ((level->dir = opendir(iter->base.path.buf)) != NULL)
> > +		return 1;
> > +
> > +	saved_errno = errno;
> > +	if (errno != ENOENT) {
> > +		warning_errno("error opening directory '%s'", iter->base.path.buf);
> >  		errno = saved_errno;
> > -		return -1;
> > +		return -2;
>
> Perhaps we should just add an enum for these return values instead of
> adding more negative/positive values here. That makes your later calls
> of activate_level() more idiomaic. E.g. !activate_level() instead of
> activate_level() == 1.

Yes, I agree. It was bothering me while I was writing these parts. I'm happy you
suggested a way to make this cleaner by using an enum.

> >  		warning_errno("failed to stat '%s'", iter->base.path.buf);
> > +		return -2;  // Stat failed not with ENOENT.
> 
> Don't use // comments, use /* .. */
> > +	} else if (stat_err && errno == ENOENT)
> > +		return -1;  // Stat failed with ENOENT.
> 
> Missing {} here for the else if.
> 
> > +	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;
> 
> We usually don't add \n\n in the middle of variable decls.
> 
> > [...]
> >
> > -		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) {
> > +	if (dir_entry == NULL) {
> 
> Don't compare against NULL, use !dir_entry.
> 
> [...]
> 
> > +		if (errno) {
> > +			warning_errno("errno reading dir '%s'", iter->base.path.buf);
> > +			if (iter->flags & DIR_ITERATOR_PEDANTIC) goto error_out;
> 
> more missing \n before goto/return.

I'll fix the coding style in these parts in v2. I'm sorry about that.

Re: [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

[Phillip Wood]

Hi Plato

On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> Simplify dir_iterator_advance by switch from iterative to recursive
> implementation. In each recursive step one action is performed.
> 
> This makes dir-iterator easier to work with, understand and introduce
> new functionality.

I think that this message could explain a bit more about why it is 
necessary to use a recursive approach. As we are implementing an 
iterator we cannot keep state in the stack between iterations so how 
does using recursive approach help us?

I reviewed this series as a way to learn the directory iterator code as 
I'd not looked at it before. Having read the existing code I think the 
it is fairly easy to follow. I'm not sure the changes here improve that. 
I also found these changes harder to follow because they renamed 
existing functions and variables in addition to moving the code around.

In order in implement returning the directory name after its contents I 
think all we need is to remember whether we've returned the name when we 
readdir() returns NULL so we don't pop it too soon. It's not clear that 
we need a lot of refactoring to do that, especially as the patches you 
linked to in the cover letter seem to avoid that.

Best Wishes

Phillip

> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>   dir-iterator.c          | 225 ++++++++++++++++++++++++++--------------
>   t/t0066-dir-iterator.sh |   4 +-
>   2 files changed, 148 insertions(+), 81 deletions(-)
> 
> diff --git a/dir-iterator.c b/dir-iterator.c
> index b17e9f970a..0f9ed95958 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;
>   
> @@ -45,34 +45,53 @@ struct dir_iterator_int {
>   
>   /*
>    * 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.
> + *
> + * Return 1 on success.
> + * Return -1 on failure when errno == ENOENT, leaving the stack unchanged.
> + * Return -2 on failure when errno != ENOENT, leaving the stack unchanged.
> + */
> +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 1;
> +
> +	if ((level->dir = opendir(iter->base.path.buf)) != NULL)
> +		return 1;
> +
> +	saved_errno = errno;
> +	if (errno != ENOENT) {
> +		warning_errno("error opening directory '%s'", iter->base.path.buf);
>   		errno = saved_errno;
> -		return -1;
> +		return -2;
>   	}
> -
> -	return 0;
> +	errno = saved_errno;
> +	return -1;
>   }
>   
>   /*
> @@ -81,12 +100,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 +111,121 @@ 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:
> + * 1 on successful exposure of the provided entry.
> + * -1 on failed exposure because entry does not exist.
> + * -2 on failed exposure because of error 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 -2;  // Stat failed not with ENOENT.
> +	} else if (stat_err && errno == ENOENT)
> +		return -1;  // Stat failed with 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 1;
>   }
>   
>   int dir_iterator_advance(struct dir_iterator *dir_iterator)
>   {
> -	struct dir_iterator_int *iter =
> -		(struct dir_iterator_int *)dir_iterator;
> -
> -	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)
> -			goto error_out;
> +	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;
> +
> +	/*
> +	 * 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 == -2 && PEDANTIC)
> +		goto error_out;
> +	else if (activate_err == -2 || activate_err == -1) {
> +		/*
> +		 * We activate the root level at `dir_iterator_begin()`.
> +		 * Therefore, there isn't any case to run out of levels.
> +		 */
> +
> +		--iter->levels_nr;
> +
> +		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);
>   
> -		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) {
> +	if (dir_entry == NULL) {
> +		if (errno) {
> +			warning_errno("errno reading dir '%s'", iter->base.path.buf);
> +			if (iter->flags & DIR_ITERATOR_PEDANTIC) goto error_out;
> +			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 == -2 && PEDANTIC)
> +		goto error_out;
> +
> +	if (expose_err == 1)
> +		push_level(iter);
> +
> +	if (expose_err == -1)
> +		return dir_iterator_advance(dir_iterator);
> +
> +	return ITER_OK;
>   
>   error_out:
>   	dir_iterator_abort(dir_iterator);
> @@ -207,6 +263,7 @@ 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);
> +	strbuf_trim_trailing_dir_sep(&iter->base.path);
>   
>   	ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
>   	iter->levels_nr = 0;
> @@ -226,6 +283,16 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
>   		goto error_out;
>   	}
>   
> +	if (push_level(iter) != 1) {
> +		saved_errno = ENOTDIR;
> +		goto error_out;
> +	}
> +
> +	if (activate_level(iter) != 1) {
> +		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 0a98dd54ba..2437ab21c4 100755
> --- a/t/t0066-dir-iterator.sh
> +++ b/t/t0066-dir-iterator.sh
> @@ -285,7 +285,7 @@ test_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
>   '
>   
>   test_expect_success POSIXPERM,SANITY \
> -'dir_iterator_advance() should fail on root dir w/o perms' '
> +'dir_iterator_begin() should fail on root dir w/o perms' '
>   
>   	mkdir -p dir12/a &&
>   	>dir12/a/b &&
> @@ -293,7 +293,7 @@ test_expect_success POSIXPERM,SANITY \
>   
>   
>   	cat >expected-no-permissions-out <<-EOF &&
> -	dir_iterator_advance failure
> +	dir_iterator_begin failure: ESOMETHINGELSE
>   	EOF
>   
>   	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&

Re: [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

[Plato Kiorpelidis]

On 22/04/11 02:26PM, Phillip Wood wrote:
> Hi Plato
> 
> On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> > Simplify dir_iterator_advance by switch from iterative to recursive
> > implementation. In each recursive step one action is performed.
> > 
> > This makes dir-iterator easier to work with, understand and introduce
> > new functionality.
> 
> I think that this message could explain a bit more about why it is necessary
> to use a recursive approach. As we are implementing an iterator we cannot
> keep state in the stack between iterations so how does using recursive
> approach help us?

It's not necessary from a functioning perspective. This iterator works on tree
like structures (exception cyclic symlinks), performing pre-order and post-order
traversal on some vertices, in our case directories. Traditionally, these
operations, i.e. pre-order, in-order, post-order, in textbooks, lectures and
online sources are implemented recursively and not iteratively. Therefore it
helps reduce mental load for the reader, since it's easier to follow as it
reminds of the same code paradigm, e.g. post-order:
  * Recurse left child
  * Recurse right child
  * Visit subroot

In general, graph algorithms are more readable when implemented recursively.
In each recursive step a set of instructions are executed and recursion lays out
the code in such a way that these instructions stand out symmetrically for each
recursive step.

As a result, recursive dir-iterator is more maintainable, as its code structure
is more similar to other graph algorithms and to what we're already used to,
than its iterative implementation.

A nice side effect of recursion is that it requires one less indentation depth.
This further simplifies the code.

> I reviewed this series as a way to learn the directory iterator code as I'd
> not looked at it before. Having read the existing code I think the it is
> fairly easy to follow. I'm not sure the changes here improve that. I also
> found these changes harder to follow because they renamed existing functions
> and variables in addition to moving the code around.

Michael Haggerty said that dir-iterator code is quite intricate[1], I agree and
tried to simplify it. My conversion from iterative to recursive implementation
tries to deal with this problem. The proposed advantages are specified above.

> In order in implement returning the directory name after its contents I
> think all we need is to remember whether we've returned the name when we
> readdir() returns NULL so we don't pop it too soon. It's not clear that we
> need a lot of refactoring to do that, especially as the patches you linked
> to in the cover letter seem to avoid that.

It's one more thing we need to keep track of, that's not required and can be
avoided. It further tangles the code, introducing more cases. The recursive
implementation that I propose, does not need to keep track of this information.
Probably there's an equal iterative implementation that also does not need to
keep track of this information.


[1]: https://public-inbox.org/git/7a665631-da6a-4b9f-b9e7-750f2504eccd@alum.mit.edu/

Thanks,
Plato

> Best Wishes
> 
> Phillip
> 
> > [...]

[RFC PATCH 4/6] dir-iterator: iterate dirs before or after their contents

[Plato Kiorpelidis]

Introduce a new feature to dir-iterator, using dir_iterator_begin()
flags parameter, allowing to control whether or not directories will
appear before or after their contents.

The new default behavior (i.e. flags set to 0) does not iterate over
directories. Flag DIR_ITERATOR_DIRS_BEFORE iterates over a directory
before doing so over its contents. Flag DIR_ITERATOR_DIRS_AFTER
iterates over a directory after doing so over its contents. These flags
do not conflict with each other and may be used simultaneously:
  * ignore directories
  (DIR_ITERATOR_DIRS_BEFORE and DIR_ITERATOR_DIRS_AFTER are both false)
  * expose them before opening and after exhausting them
  (DIR_ITERATOR_DIRS_BEFORE and DIR_ITERATOR_DIRS_AFTER are both true).

Amend a call to dir_iterator_begin() in refs/files-backend.c and
builtin/clone.c to enable DIR_ITERATOR_DIRS_BEFORE, since they need
iteration over a directory before doing so over its contents.

Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to test
the new flags DIR_ITERATOR_DIRS_BEFORE, DIR_ITERATOR_DIRS_AFTER.

Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
---
 builtin/clone.c              |   4 +-
 dir-iterator.c               | 101 ++++-
 dir-iterator.h               |  34 +-
 refs/files-backend.c         |   2 +-
 t/helper/test-dir-iterator.c |  15 +-
 t/t0066-dir-iterator.sh      | 844 ++++++++++++++++++++++++++++++++---
 6 files changed, 923 insertions(+), 77 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 5231656379..87ee764ba3 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 0f9ed95958..41a1481bb4 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -116,16 +116,26 @@ static int pop_level(struct dir_iterator_int *iter)
  *
  * Return values:
  * 1 on successful exposure of the provided entry.
+ * 0 on failed exposure because entry is directory and flags don't allow it.
  * -1 on failed exposure because entry does not exist.
  * -2 on failed exposure because of error 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;
+	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
@@ -137,6 +147,11 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name)
 	} else if (stat_err && errno == ENOENT)
 		return -1;  // Stat failed with ENOENT.
 
+	if (S_ISDIR(iter->base.st.st_mode)) {
+		if (!DIRS_BEFORE && !strcmp(dir_state, "before")) return 0;
+		if (!DIRS_AFTER && !strcmp(dir_state, "after")) return 0;
+	}
+
 	/*
 	 * We have to reset relative path and basename because the path strbuf
 	 * might have been realloc()'ed at the previous strbuf_addstr().
@@ -150,6 +165,21 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name)
 	return 1;
 }
 
+/*
+ * 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, '/') + 1;
+	char *basename = calloc(1, strlen(start) + 1);
+
+	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;
@@ -180,9 +210,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 == -2 && PEDANTIC)
+			goto error_out;
+		else if (expose_err == -1 || expose_err == 0)
+			return dir_iterator_advance(dir_iterator);
+		else
+			return ITER_OK;
 	}
 
 	strbuf_setlen(&iter->base.path, level->prefix_len);
@@ -198,12 +247,41 @@ 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.
+			 */
+
+			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)
+			if (pop_level(iter) == 0) {
 				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 == -2 && PEDANTIC)
+				goto error_out;
+			else if (expose_err == -1 || expose_err == 0)
+				return dir_iterator_advance(dir_iterator);
+			else
+				return ITER_OK;
 		}
 	}
 
@@ -212,17 +290,26 @@ 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 == -2 && PEDANTIC)
 		goto error_out;
 
-	if (expose_err == 1)
+	if (expose_err == 0 || expose_err == 1)
 		push_level(iter);
 
-	if (expose_err == -1)
+	if (expose_err == 0 || expose_err == -1)
 		return dir_iterator_advance(dir_iterator);
 
 	return ITER_OK;
diff --git a/dir-iterator.h b/dir-iterator.h
index 08229157c6..dea4fc8f3e 100644
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -8,19 +8,21 @@
  *
  * 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.
+ * 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 and after when
+ * DIR_ITERATOR_DIRS_AFTER is set. Failure to set any of them results
+ * in directories themselves not being exposed, only their contents will be.
  *
  * 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 +63,25 @@
  *   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.
+ *
+ * - 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
  * 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)
+#define DIR_ITERATOR_DIRS_AFTER (1 << 3)
 
 struct dir_iterator {
 	/* The current path: */
@@ -97,12 +112,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 non-directory directory contents.
  */
 struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
 
@@ -110,6 +126,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 the dir_iterator 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 +136,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 95acab78ee..951a673e71 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2247,7 +2247,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 659b6bfa81..c92616bd69 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -15,7 +15,16 @@ 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
+ *	--dirs-after
+ *
+ * example:
+ * test-tool dir-iterator --pedantic --dirs-before --dirs-after ./somedir
  */
 int cmd__dir_iterator(int argc, const char **argv)
 {
@@ -28,6 +37,10 @@ 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 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 2437ab21c4..24a4f1afef 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -14,44 +14,157 @@ test_expect_success 'setup -- dir with a single file' '
 	[f] (a) [a] ./dir1/a
 	EOF
 '
-test_expect_success 'iteration of dir with a file' '
+test_expect_success 'dirs-ignore 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' '
+	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' '
+	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' '
+	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 &&
 
 
-	cat >expected-out <<-EOF
+	cat >expected-ignore-out <<-EOF &&
+	EOF
+
+	cat >expected-before-out <<-EOF &&
+	[d] (a) [a] ./dir2/a
+	EOF
+
+	cat expected-before-out >expected-after-out &&
+
+	cat >expected-before-after-out <<-EOF
+	[d] (a) [a] ./dir2/a
 	[d] (a) [a] ./dir2/a
 	EOF
 '
-test_expect_success 'iteration of dir with a single dir' '
+test_expect_success 'dirs-ignore of dir with a single dir' '
 	test-tool dir-iterator ./dir2 >actual-out &&
-	test_cmp expected-out actual-out
+	test_cmp expected-ignore-out actual-out
+'
+test_expect_success 'dirs-before of dir with a single dir' '
+	test-tool dir-iterator --dirs-before ./dir2 >actual-out &&
+	test_cmp expected-before-out actual-out
+'
+test_expect_success 'dirs-after of dir with a single dir' '
+	test-tool dir-iterator --dirs-after ./dir2 >actual-out &&
+	test_cmp expected-after-out actual-out
+'
+test_expect_success 'dirs-before/dirs-after of dir with a single dir' '
+	test-tool dir-iterator --dirs-before --dirs-after ./dir2 >actual-out &&
+	test_cmp expected-before-after-out actual-out
 '
 
 test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
 	mkdir -p dir3/a &&
-	chmod 0 dir3/a &&
 
 
-	cat >expected-out <<-EOF &&
+	cat >expected-ignore-out <<-EOF &&
+	EOF
+	cat >expected-pedantic-ignore-out <<-EOF &&
+	dir_iterator_advance failure
+	EOF
+
+	cat >expected-before-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	EOF
-	cat >expected-pedantic-out <<-EOF
+	cat >expected-pedantic-before-out <<-EOF &&
 	[d] (a) [a] ./dir3/a
 	dir_iterator_advance failure
 	EOF
+
+	cat expected-before-out >expected-after-out &&
+	cat >expected-pedantic-after-out <<-EOF &&
+	dir_iterator_advance failure
+	EOF
+
+	cat >expected-before-after-out <<-EOF &&
+	[d] (a) [a] ./dir3/a
+	[d] (a) [a] ./dir3/a
+	EOF
+	cat expected-pedantic-before-out >expected-pedantic-before-after-out
 '
-test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
+test_expect_success POSIXPERM,SANITY 'dirs-ignore of dir w/ dir w/o perms' '
+	chmod 0 dir3/a &&
+
 	test-tool dir-iterator ./dir3/ >actual-out &&
-	test_cmp expected-out actual-out
+	test_cmp expected-ignore-out actual-out &&
+
+	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-ignore of dir w/ dir w/o perms' '
+	chmod 0 dir3/a &&
+
 	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
-	test_cmp expected-pedantic-out actual-out
+	test_cmp expected-pedantic-ignore-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
+	chmod 0 dir3/a &&
+
+	test-tool dir-iterator --dirs-before ./dir3/ >actual-out &&
+	test_cmp expected-before-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of dir w/ dir w/o perms' '
+	chmod 0 dir3/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic ./dir3/ >actual-out &&
+	test_cmp expected-pedantic-before-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY 'dirs-after of dir w/ dir w/o perms' '
+	chmod 0 dir3/a &&
+
+	test-tool dir-iterator --dirs-after ./dir3/ >actual-out &&
+	test_cmp expected-after-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY 'pedantic dirs-after of dir w/ dir w/o perms' '
+	chmod 0 dir3/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic ./dir3/ >actual-out &&
+	test_cmp expected-pedantic-after-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY \
+'dirs-before/dirs-after of dir w/ dir w/o perms' '
+
+	chmod 0 dir3/a &&
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir3/ >actual-out &&
+	test_cmp expected-before-after-out actual-out &&
+
+	chmod 755 dir3/a
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before/dirs-after of dir w/ dir w/o perms' '
+
+	chmod 0 dir3/a &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic ./dir3/ >actual-out &&
+	test_cmp expected-pedantic-before-after-out actual-out &&
+
+	chmod 755 dir3/a
 '
 
 test_expect_success 'setup -- dir w/ five files' '
@@ -71,26 +184,71 @@ test_expect_success 'setup -- dir w/ five files' '
 	[f] (e) [e] ./dir4/e
 	EOF
 '
-test_expect_success 'iteration of dir w/ five files' '
+test_expect_success 'dirs-ignore of dir w/ five files' '
 	test-tool dir-iterator ./dir4 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
 	test_cmp expected-sorted-out actual-sorted-out
 '
+test_expect_success 'dirs-before of dir w/ five files' '
+	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 'dirs-after of dir w/ five files' '
+	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' '
+	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 &&
 	>dir5/a/b &&
 
 
-	cat >expected-out <<-EOF
+	cat >expected-ignore-out <<-EOF &&
+	[f] (a/b) [b] ./dir5/a/b
+	EOF
+
+	cat >expected-before-out <<-EOF &&
+	[d] (a) [a] ./dir5/a
+	[f] (a/b) [b] ./dir5/a/b
+	EOF
+
+	cat >expected-after-out <<-EOF &&
+	[f] (a/b) [b] ./dir5/a/b
+	[d] (a) [a] ./dir5/a
+	EOF
+
+	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_expect_success 'iteration of dir w/ dir w/ a file' '
+test_expect_success 'dirs-ignore of dir w/ dir w/ a file' '
 	test-tool dir-iterator ./dir5 >actual-out &&
-	test_cmp expected-out actual-out
+	test_cmp expected-ignore-out actual-out
+'
+test_expect_success 'dirs-before of dir w/ dir w/ a file' '
+	test-tool dir-iterator --dirs-before ./dir5 >actual-out &&
+	test_cmp expected-before-out actual-out
+'
+test_expect_success 'dirs-after of dir w/ dir w/ a file' '
+	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' '
+	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' '
@@ -98,16 +256,49 @@ test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
 	>dir6/a/b/c/d &&
 
 
-	cat >expected-out <<-EOF
+	cat >expected-ignore-out <<-EOF &&
+	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
+	EOF
+
+	cat >expected-before-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
 	EOF
+
+	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
+
+	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_expect_success 'iteration of dir w/ three nested dirs w/ file' '
+test_expect_success 'dirs-ignore of dir w/ three nested dirs w/ file' '
 	test-tool dir-iterator ./dir6 >actual-out &&
-	test_cmp expected-out actual-out
+	test_cmp expected-ignore-out actual-out
+'
+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-before-out actual-out
+'
+test_expect_success 'dirs-after of dir w/ three nested dirs w/ file' '
+	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' '
+	test-tool dir-iterator --dirs-before --dirs-after ./dir6 >actual-out &&
+	test_cmp expected-before-after-out actual-out
 '
 
 test_expect_success POSIXPERM,SANITY \
@@ -116,33 +307,123 @@ test_expect_success POSIXPERM,SANITY \
 	mkdir -p dir7/a/b/c &&
 	>dir7/a/b/c/d &&
 
-	cat >expected-out <<-EOF &&
+
+	cat >expected-ignore-out <<-EOF &&
+	EOF
+	cat >expected-pedantic-ignore-out <<-EOF &&
+	dir_iterator_advance failure
+	EOF
+
+	cat >expected-before-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	EOF
+	cat >expected-pedantic-before-out <<-EOF &&
+	[d] (a) [a] ./dir7/a
+	[d] (a/b) [b] ./dir7/a/b
+	dir_iterator_advance failure
+	EOF
+
+	cat >expected-after-out <<-EOF &&
+	[d] (a/b) [b] ./dir7/a/b
+	[d] (a) [a] ./dir7/a
+	EOF
+	cat >expected-pedantic-after-out <<-EOF &&
+	dir_iterator_advance failure
+	EOF
+
+	cat >expected-before-after-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
-	cat >expected-pedantic-out <<-EOF
+	cat >expected-pedantic-before-after-out <<-EOF
 	[d] (a) [a] ./dir7/a
 	[d] (a/b) [b] ./dir7/a/b
 	dir_iterator_advance failure
 	EOF
 '
 test_expect_success POSIXPERM,SANITY \
-'iteration of dir w/ three nested dirs w/ file, second w/o perms' '
+'dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
 
 	chmod 0 dir7/a/b &&
 
 	test-tool dir-iterator ./dir7 >actual-out &&
-	test_cmp expected-out actual-out &&
+	test_cmp expected-ignore-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' '
+'pedantic dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
 
 	chmod 0 dir7/a/b &&
 
 	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
-	test_cmp expected-pedantic-out actual-out &&
+	test_cmp expected-pedantic-ignore-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' '
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator --dirs-before ./dir7 >actual-out &&
+	test_cmp expected-before-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'pedantic dirs-before of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic ./dir7 >actual-out &&
+	test_cmp expected-pedantic-before-out actual-out &&
+
+	chmod 755 dir7/a/b
+'
+test_expect_success POSIXPERM,SANITY \
+'dirs-after of dir w/ three nested dirs w/ file, second w/o perms' '
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator --dirs-after ./dir7 >actual-out &&
+	test_cmp expected-after-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' '
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --dirs-after \
+		--pedantic ./dir7 >actual-out &&
+	test_cmp expected-pedantic-after-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' '
+
+	chmod 0 dir7/a/b &&
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir7 >actual-out &&
+	test_cmp expected-before-after-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' '
+
+	chmod 0 dir7/a/b &&
+
+	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
+		--pedantic ./dir7 >actual-out &&
+	test_cmp expected-pedantic-before-after-out actual-out &&
 
 	chmod 755 dir7/a/b
 '
@@ -154,24 +435,84 @@ test_expect_success 'setup -- dir w/ two dirs each w/ file' '
 	>dir8/c/d &&
 
 
-	cat >expected-out1 <<-EOF &&
+	cat >expected-ignore-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir8/a/b
+	[f] (c/d) [d] ./dir8/c/d
+	EOF
+	cat >expected-ignore-out2 <<-EOF &&
+	[f] (c/d) [d] ./dir8/c/d
+	[f] (a/b) [b] ./dir8/a/b
+	EOF
+
+	cat >expected-before-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
+	cat >expected-before-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
+
+	cat >expected-after-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-after-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
+
+	cat >expected-before-after-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-before-after-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_expect_success 'iteration of dir w/ two dirs each w/ file' '
+test_expect_success 'dirs-ignore of dir w/ two dirs each w/ file' '
 	test-tool dir-iterator ./dir8 >actual-out &&
 	(
-		test_cmp expected-out1 actual-out ||
-		test_cmp expected-out2 actual-out
+		test_cmp expected-ignore-out1 actual-out ||
+		test_cmp expected-ignore-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
+	test-tool dir-iterator --dirs-before ./dir8 >actual-out &&
+	(
+		test_cmp expected-before-out1 actual-out ||
+		test_cmp expected-before-out2 actual-out
+	)
+'
+test_expect_success 'dirs-after of dir w/ two dirs each w/ file' '
+	test-tool dir-iterator --dirs-after ./dir8 >actual-out &&
+	(
+		test_cmp expected-after-out1 actual-out ||
+		test_cmp expected-after-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ two dirs each w/ file' '
+	test-tool dir-iterator --dirs-before --dirs-after ./dir8 >actual-out &&
+	(
+		test_cmp expected-before-after-out1 actual-out ||
+		test_cmp expected-before-after-out2 actual-out
 	)
 '
 
@@ -183,44 +524,164 @@ test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files'
 	>dir9/d/e &&
 
 
-	cat >expected-out1 <<-EOF &&
+	cat >expected-ignore-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-ignore-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-ignore-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-ignore-out4 <<-EOF &&
+	[f] (d/e) [e] ./dir9/d/e
+	[f] (a/c) [c] ./dir9/a/c
+	[f] (a/b) [b] ./dir9/a/b
+	EOF
+
+	cat >expected-before-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 &&
+	cat >expected-before-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 &&
+	cat >expected-before-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
+	cat >expected-before-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
+
+	cat >expected-after-out1 <<-EOF &&
+	[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-after-out2 <<-EOF &&
+	[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
+	cat >expected-after-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-after-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
+
+	cat >expected-before-after-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-before-after-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-before-after-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-before-after-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_expect_success \
-'iteration of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
+'dirs-ignore of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
 
 	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_cmp expected-ignore-out1 actual-out ||
+		test_cmp expected-ignore-out2 actual-out ||
+		test_cmp expected-ignore-out3 actual-out ||
+		test_cmp expected-ignore-out4 actual-out
+	)
+'
+test_expect_success \
+'dirs-before of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
+
+	test-tool dir-iterator --dirs-before ./dir9 >actual-out &&
+	(
+		test_cmp expected-before-out1 actual-out ||
+		test_cmp expected-before-out2 actual-out ||
+		test_cmp expected-before-out3 actual-out ||
+		test_cmp expected-before-out4 actual-out
+	)
+'
+test_expect_success \
+'dirs-after of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
+
+	test-tool dir-iterator --dirs-after ./dir9 >actual-out &&
+	(
+		test_cmp expected-after-out1 actual-out ||
+		test_cmp expected-after-out2 actual-out ||
+		test_cmp expected-after-out3 actual-out ||
+		test_cmp expected-after-out4 actual-out
+	)
+'
+test_expect_success \
+'dirs-before/dirs-after of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir9 >actual-out &&
+	(
+		test_cmp expected-before-after-out1 actual-out ||
+		test_cmp expected-before-after-out2 actual-out ||
+		test_cmp expected-before-after-out3 actual-out ||
+		test_cmp expected-before-after-out4 actual-out
 	)
 '
 
@@ -231,25 +692,84 @@ test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
 	>dir10/a/c/d &&
 
 
-	cat >expected-out1 <<-EOF &&
+	cat >expected-ignore-out1 <<-EOF &&
+	[f] (a/b) [b] ./dir10/a/b
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	EOF
+	cat >expected-ignore-out2 <<-EOF &&
+	[f] (a/c/d) [d] ./dir10/a/c/d
+	[f] (a/b) [b] ./dir10/a/b
+	EOF
+
+	cat >expected-before-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
+	cat >expected-before-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
 
+	cat >expected-after-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-after-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
+
+	cat >expected-before-after-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-before-after-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_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
+test_expect_success 'dirs-ignore of dir w/ two nested dirs, each w/ file' '
 	test-tool dir-iterator ./dir10/ >actual-out &&
 	(
-		test_cmp expected-out1 actual-out ||
-		test_cmp expected-out2 actual-out
+		test_cmp expected-ignore-out1 actual-out ||
+		test_cmp expected-ignore-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
+	test-tool dir-iterator --dirs-before ./dir10/ >actual-out &&
+	(
+		test_cmp expected-before-out1 actual-out ||
+		test_cmp expected-before-out2 actual-out
+	)
+'
+test_expect_success 'dirs-after of dir w/ two nested dirs, each w/ file' '
+	test-tool dir-iterator --dirs-after ./dir10/ >actual-out &&
+	(
+		test_cmp expected-after-out1 actual-out ||
+		test_cmp expected-after-out2 actual-out
+	)
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ two nested dirs, each w/ file' '
+	test-tool dir-iterator --dirs-before --dirs-after ./dir10 >actual-out &&
+	(
+		test_cmp expected-before-after-out1 actual-out ||
+		test_cmp expected-before-after-out2 actual-out
 	)
 '
 
@@ -263,7 +783,15 @@ test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
 	>dir11/d/e/d/a &&
 
 
-	cat >expected-sorted-out <<-EOF
+	cat >expected-ignore-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
+
+	cat >expected-before-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir11/a
 	[d] (a/b) [b] ./dir11/a/b
 	[d] (a/b/c) [c] ./dir11/a/b/c
@@ -276,12 +804,52 @@ test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
 	[f] (c) [c] ./dir11/c
 	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
 	EOF
+
+	cat expected-before-sorted-out >expected-after-sorted-out &&
+
+	cat >expected-before-after-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_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
+test_expect_success 'dirs-ignore of dir w/ complex structure w/o symlinks' '
 	test-tool dir-iterator ./dir11 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-sorted-out actual-sorted-out
+	test_cmp expected-ignore-sorted-out actual-sorted-out
+'
+test_expect_success 'dirs-before of dir w/ complex structure w/o symlinks' '
+	test-tool dir-iterator --dirs-before ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-before-sorted-out actual-sorted-out
+'
+test_expect_success 'dirs-after of dir w/ complex structure w/o symlinks' '
+	test-tool dir-iterator --dirs-after ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-after-sorted-out actual-sorted-out
+'
+test_expect_success 'dirs-before/dirs-after of dir w/ complex structure w/o symlinks' '
+	test-tool dir-iterator --dirs-before --dirs-after ./dir11 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-before-after-sorted-out actual-sorted-out
 '
 
 test_expect_success POSIXPERM,SANITY \
@@ -338,7 +906,7 @@ test_expect_success POSIXPERM,SANITY \
 	[d] (a) [a] ./dir13/a
 	EOF
 
-	test-tool dir-iterator ./dir13 >actual-out &&
+	test-tool dir-iterator --dirs-before ./dir13 >actual-out &&
 	test_cmp expected-no-permissions-out actual-out &&
 
 	chmod 755 dir13/a &&
@@ -358,7 +926,8 @@ test_expect_success POSIXPERM,SANITY \
 	dir_iterator_advance failure
 	EOF
 
-	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-no-permissions-pedantic-out actual-out &&
 
 	chmod 755 dir13/a &&
@@ -373,7 +942,17 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	ln -s ../b dir14/a/f &&
 
 
-	cat >expected-dont-follow-sorted-out <<-EOF &&
+	cat >expected-dont-follow-ignore-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
+	cat >expected-follow-ignore-sorted-out <<-EOF &&
+	[f] (a/d) [d] ./dir14/a/d
+	[f] (a/e) [e] ./dir14/a/e
+	EOF
+
+	cat >expected-dont-follow-before-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir14/a
 	[d] (b) [b] ./dir14/b
 	[d] (b/c) [c] ./dir14/b/c
@@ -381,7 +960,7 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	[s] (a/e) [e] ./dir14/a/e
 	[s] (a/f) [f] ./dir14/a/f
 	EOF
-	cat >expected-follow-sorted-out <<-EOF
+	cat >expected-follow-before-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir14/a
 	[d] (a/f) [f] ./dir14/a/f
 	[d] (a/f/c) [c] ./dir14/a/f/c
@@ -390,22 +969,99 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	[f] (a/d) [d] ./dir14/a/d
 	[f] (a/e) [e] ./dir14/a/e
 	EOF
+
+	cat expected-dont-follow-before-sorted-out >expected-dont-follow-after-sorted-out &&
+	cat expected-follow-before-sorted-out >expected-follow-after-sorted-out &&
+
+	cat >expected-dont-follow-before-after-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
+	cat >expected-follow-before-after-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_expect_success SYMLINKS \
-'dont-follow-symlinks of dir w/ symlinks w/o cycle' '
+'dont-follow-symlinks dirs-ignore of dir w/ symlinks w/o cycle' '
 
 	test-tool dir-iterator ./dir14 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-dont-follow-sorted-out actual-sorted-out
+	test_cmp expected-dont-follow-ignore-sorted-out actual-sorted-out
 '
 test_expect_success SYMLINKS \
-'follow-symlinks of dir w/ symlinks w/o cycle' '
+'follow-symlinks dirs-ignore of dir w/ symlinks w/o cycle' '
 
 	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-follow-sorted-out actual-sorted-out
+	test_cmp expected-follow-ignore-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks dirs-before of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator --dirs-before ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-before-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-before of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator --dirs-before --follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-follow-before-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks dirs-after of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator --dirs-after ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-after-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-after of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator --dirs-after --follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-follow-after-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-before-after-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/o cycle' '
+
+	test-tool dir-iterator --dirs-before --dirs-after --follow-symlinks ./dir14 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-follow-before-after-sorted-out actual-sorted-out
 '
 
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
@@ -415,7 +1071,14 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
 	ln -s ../ dir15/a/b/e &&
 	ln -s ../../ dir15/a/b/f &&
 
-	cat >expected-dont-follow-sorted-out <<-EOF &&
+
+	cat >expected-dont-follow-ignore-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
+
+	cat >expected-dont-follow-before-sorted-out <<-EOF &&
 	[d] (a) [a] ./dir15/a
 	[d] (a/b) [b] ./dir15/a/b
 	[d] (a/c) [c] ./dir15/a/c
@@ -424,22 +1087,87 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
 	[s] (a/b/f) [f] ./dir15/a/b/f
 	EOF
 
+	cat expected-dont-follow-before-sorted-out >expected-dont-follow-after-sorted-out &&
+
+	cat >expected-dont-follow-before-after-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
+
 	cat >expected-pedantic-follow-tailed-out <<-EOF
 	dir_iterator_advance failure
 	EOF
 '
 test_expect_success SYMLINKS \
-'dont-follow-symlinks of dir w/ symlinks w/ cycle' '
+'dont-follow-symlinks dirs-ignore of dir w/ symlinks w/ cycle' '
 
 	test-tool dir-iterator ./dir15 >actual-out &&
 	sort actual-out >actual-sorted-out &&
 
-	test_cmp expected-dont-follow-sorted-out actual-sorted-out
+	test_cmp expected-dont-follow-ignore-sorted-out actual-sorted-out
 '
 test_expect_success SYMLINKS \
-'pedantic follow-symlinks of dir w/ symlinks w/ cycle' '
+'pedantic follow-symlinks dirs-ignore of dir w/ symlinks w/ cycle' '
 
 	test_must_fail test-tool dir-iterator \
+		--follow-symlinks --pedantic ./dir15 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-pedantic-follow-tailed-out actual-tailed-out
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks dirs-before of dir w/ symlinks w/ cycle' '
+
+	test-tool dir-iterator --dirs-before ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-before-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks dirs-before of dir w/ symlinks w/ cycle' '
+
+	test_must_fail test-tool dir-iterator --dirs-before \
+		--pedantic --follow-symlinks ./dir15 >actual-out &&
+	tail -n 1 actual-out >actual-tailed-out &&
+
+	test_cmp expected-pedantic-follow-tailed-out actual-tailed-out
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks dirs-after of dir w/ symlinks w/ cycle' '
+
+	test-tool dir-iterator --dirs-after ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-after-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks dirs-after of dir w/ symlinks w/ cycle' '
+
+	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-pedantic-follow-tailed-out actual-tailed-out
+'
+test_expect_success SYMLINKS \
+'dont-follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/ cycle' '
+
+	test-tool dir-iterator --dirs-before --dirs-after ./dir15 >actual-out &&
+	sort actual-out >actual-sorted-out &&
+
+	test_cmp expected-dont-follow-before-after-sorted-out actual-sorted-out
+'
+test_expect_success SYMLINKS \
+'pedantic follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/ cycle' '
+
+	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 &&
 
-- 
2.35.1

Re: [RFC PATCH 4/6] dir-iterator: iterate dirs before or after their contents

[Phillip Wood]

Hi Plato

On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> Introduce a new feature to dir-iterator, using dir_iterator_begin()
> flags parameter, allowing to control whether or not directories will
> appear before or after their contents.
> 
> The new default behavior (i.e. flags set to 0) does not iterate over
> directories.

This is introducing two changes at once adding the "AFTER" flag and 
changing the default behavior. I think it wolud make sence to separate 
those into two separate commits. You could change the default adding the 
"BEFORE" flag and adjusting the existing callers first and then 
implement the "AFTER" flag in a second commit.

It would also be nice to see the "AFTER" flag being used by changing 
remove_dir_recursively() for example.

Best Wishes

Phillip

  Flag DIR_ITERATOR_DIRS_BEFORE iterates over a directory
> before doing so over its contents. Flag DIR_ITERATOR_DIRS_AFTER
> iterates over a directory after doing so over its contents. These flags
> do not conflict with each other and may be used simultaneously:
>    * ignore directories
>    (DIR_ITERATOR_DIRS_BEFORE and DIR_ITERATOR_DIRS_AFTER are both false)
>    * expose them before opening and after exhausting them
>    (DIR_ITERATOR_DIRS_BEFORE and DIR_ITERATOR_DIRS_AFTER are both true).
> 
> Amend a call to dir_iterator_begin() in refs/files-backend.c and
> builtin/clone.c to enable DIR_ITERATOR_DIRS_BEFORE, since they need
> iteration over a directory before doing so over its contents.
> 
> Update t/t0066-dir-iterator.sh and t/helper/test-dir-iterator.c to test
> the new flags DIR_ITERATOR_DIRS_BEFORE, DIR_ITERATOR_DIRS_AFTER.
> 
> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>   builtin/clone.c              |   4 +-
>   dir-iterator.c               | 101 ++++-
>   dir-iterator.h               |  34 +-
>   refs/files-backend.c         |   2 +-
>   t/helper/test-dir-iterator.c |  15 +-
>   t/t0066-dir-iterator.sh      | 844 ++++++++++++++++++++++++++++++++---
>   6 files changed, 923 insertions(+), 77 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5231656379..87ee764ba3 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 0f9ed95958..41a1481bb4 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -116,16 +116,26 @@ static int pop_level(struct dir_iterator_int *iter)
>    *
>    * Return values:
>    * 1 on successful exposure of the provided entry.
> + * 0 on failed exposure because entry is directory and flags don't allow it.
>    * -1 on failed exposure because entry does not exist.
>    * -2 on failed exposure because of error 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;
> +	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
> @@ -137,6 +147,11 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name)
>   	} else if (stat_err && errno == ENOENT)
>   		return -1;  // Stat failed with ENOENT.
>   
> +	if (S_ISDIR(iter->base.st.st_mode)) {
> +		if (!DIRS_BEFORE && !strcmp(dir_state, "before")) return 0;
> +		if (!DIRS_AFTER && !strcmp(dir_state, "after")) return 0;
> +	}
> +
>   	/*
>   	 * We have to reset relative path and basename because the path strbuf
>   	 * might have been realloc()'ed at the previous strbuf_addstr().
> @@ -150,6 +165,21 @@ static int expose_entry(struct dir_iterator_int *iter, char *d_name)
>   	return 1;
>   }
>   
> +/*
> + * 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, '/') + 1;
> +	char *basename = calloc(1, strlen(start) + 1);
> +
> +	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;
> @@ -180,9 +210,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 == -2 && PEDANTIC)
> +			goto error_out;
> +		else if (expose_err == -1 || expose_err == 0)
> +			return dir_iterator_advance(dir_iterator);
> +		else
> +			return ITER_OK;
>   	}
>   
>   	strbuf_setlen(&iter->base.path, level->prefix_len);
> @@ -198,12 +247,41 @@ 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.
> +			 */
> +
> +			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)
> +			if (pop_level(iter) == 0) {
>   				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 == -2 && PEDANTIC)
> +				goto error_out;
> +			else if (expose_err == -1 || expose_err == 0)
> +				return dir_iterator_advance(dir_iterator);
> +			else
> +				return ITER_OK;
>   		}
>   	}
>   
> @@ -212,17 +290,26 @@ 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 == -2 && PEDANTIC)
>   		goto error_out;
>   
> -	if (expose_err == 1)
> +	if (expose_err == 0 || expose_err == 1)
>   		push_level(iter);
>   
> -	if (expose_err == -1)
> +	if (expose_err == 0 || expose_err == -1)
>   		return dir_iterator_advance(dir_iterator);
>   
>   	return ITER_OK;
> diff --git a/dir-iterator.h b/dir-iterator.h
> index 08229157c6..dea4fc8f3e 100644
> --- a/dir-iterator.h
> +++ b/dir-iterator.h
> @@ -8,19 +8,21 @@
>    *
>    * 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.
> + * 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 and after when
> + * DIR_ITERATOR_DIRS_AFTER is set. Failure to set any of them results
> + * in directories themselves not being exposed, only their contents will be.
>    *
>    * 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 +63,25 @@
>    *   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.
> + *
> + * - 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
>    * 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)
> +#define DIR_ITERATOR_DIRS_AFTER (1 << 3)
>   
>   struct dir_iterator {
>   	/* The current path: */
> @@ -97,12 +112,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 non-directory directory contents.
>    */
>   struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
>   
> @@ -110,6 +126,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 the dir_iterator 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 +136,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 95acab78ee..951a673e71 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2247,7 +2247,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 659b6bfa81..c92616bd69 100644
> --- a/t/helper/test-dir-iterator.c
> +++ b/t/helper/test-dir-iterator.c
> @@ -15,7 +15,16 @@ 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
> + *	--dirs-after
> + *
> + * example:
> + * test-tool dir-iterator --pedantic --dirs-before --dirs-after ./somedir
>    */
>   int cmd__dir_iterator(int argc, const char **argv)
>   {
> @@ -28,6 +37,10 @@ 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 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 2437ab21c4..24a4f1afef 100755
> --- a/t/t0066-dir-iterator.sh
> +++ b/t/t0066-dir-iterator.sh
> @@ -14,44 +14,157 @@ test_expect_success 'setup -- dir with a single file' '
>   	[f] (a) [a] ./dir1/a
>   	EOF
>   '
> -test_expect_success 'iteration of dir with a file' '
> +test_expect_success 'dirs-ignore 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' '
> +	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' '
> +	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' '
> +	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 &&
>   
>   
> -	cat >expected-out <<-EOF
> +	cat >expected-ignore-out <<-EOF &&
> +	EOF
> +
> +	cat >expected-before-out <<-EOF &&
> +	[d] (a) [a] ./dir2/a
> +	EOF
> +
> +	cat expected-before-out >expected-after-out &&
> +
> +	cat >expected-before-after-out <<-EOF
> +	[d] (a) [a] ./dir2/a
>   	[d] (a) [a] ./dir2/a
>   	EOF
>   '
> -test_expect_success 'iteration of dir with a single dir' '
> +test_expect_success 'dirs-ignore of dir with a single dir' '
>   	test-tool dir-iterator ./dir2 >actual-out &&
> -	test_cmp expected-out actual-out
> +	test_cmp expected-ignore-out actual-out
> +'
> +test_expect_success 'dirs-before of dir with a single dir' '
> +	test-tool dir-iterator --dirs-before ./dir2 >actual-out &&
> +	test_cmp expected-before-out actual-out
> +'
> +test_expect_success 'dirs-after of dir with a single dir' '
> +	test-tool dir-iterator --dirs-after ./dir2 >actual-out &&
> +	test_cmp expected-after-out actual-out
> +'
> +test_expect_success 'dirs-before/dirs-after of dir with a single dir' '
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir2 >actual-out &&
> +	test_cmp expected-before-after-out actual-out
>   '
>   
>   test_expect_success POSIXPERM,SANITY 'setup -- dir w/ single dir w/o perms' '
>   	mkdir -p dir3/a &&
> -	chmod 0 dir3/a &&
>   
>   
> -	cat >expected-out <<-EOF &&
> +	cat >expected-ignore-out <<-EOF &&
> +	EOF
> +	cat >expected-pedantic-ignore-out <<-EOF &&
> +	dir_iterator_advance failure
> +	EOF
> +
> +	cat >expected-before-out <<-EOF &&
>   	[d] (a) [a] ./dir3/a
>   	EOF
> -	cat >expected-pedantic-out <<-EOF
> +	cat >expected-pedantic-before-out <<-EOF &&
>   	[d] (a) [a] ./dir3/a
>   	dir_iterator_advance failure
>   	EOF
> +
> +	cat expected-before-out >expected-after-out &&
> +	cat >expected-pedantic-after-out <<-EOF &&
> +	dir_iterator_advance failure
> +	EOF
> +
> +	cat >expected-before-after-out <<-EOF &&
> +	[d] (a) [a] ./dir3/a
> +	[d] (a) [a] ./dir3/a
> +	EOF
> +	cat expected-pedantic-before-out >expected-pedantic-before-after-out
>   '
> -test_expect_success POSIXPERM,SANITY 'iteration of dir w/ dir w/o perms' '
> +test_expect_success POSIXPERM,SANITY 'dirs-ignore of dir w/ dir w/o perms' '
> +	chmod 0 dir3/a &&
> +
>   	test-tool dir-iterator ./dir3/ >actual-out &&
> -	test_cmp expected-out actual-out
> +	test_cmp expected-ignore-out actual-out &&
> +
> +	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-ignore of dir w/ dir w/o perms' '
> +	chmod 0 dir3/a &&
> +
>   	test_must_fail test-tool dir-iterator --pedantic ./dir3/ >actual-out &&
> -	test_cmp expected-pedantic-out actual-out
> +	test_cmp expected-pedantic-ignore-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY 'dirs-before of dir w/ dir w/o perms' '
> +	chmod 0 dir3/a &&
> +
> +	test-tool dir-iterator --dirs-before ./dir3/ >actual-out &&
> +	test_cmp expected-before-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY 'pedantic dirs-before of dir w/ dir w/o perms' '
> +	chmod 0 dir3/a &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic ./dir3/ >actual-out &&
> +	test_cmp expected-pedantic-before-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY 'dirs-after of dir w/ dir w/o perms' '
> +	chmod 0 dir3/a &&
> +
> +	test-tool dir-iterator --dirs-after ./dir3/ >actual-out &&
> +	test_cmp expected-after-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY 'pedantic dirs-after of dir w/ dir w/o perms' '
> +	chmod 0 dir3/a &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-after \
> +		--pedantic ./dir3/ >actual-out &&
> +	test_cmp expected-pedantic-after-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY \
> +'dirs-before/dirs-after of dir w/ dir w/o perms' '
> +
> +	chmod 0 dir3/a &&
> +
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir3/ >actual-out &&
> +	test_cmp expected-before-after-out actual-out &&
> +
> +	chmod 755 dir3/a
> +'
> +test_expect_success POSIXPERM,SANITY \
> +'pedantic dirs-before/dirs-after of dir w/ dir w/o perms' '
> +
> +	chmod 0 dir3/a &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
> +		--pedantic ./dir3/ >actual-out &&
> +	test_cmp expected-pedantic-before-after-out actual-out &&
> +
> +	chmod 755 dir3/a
>   '
>   
>   test_expect_success 'setup -- dir w/ five files' '
> @@ -71,26 +184,71 @@ test_expect_success 'setup -- dir w/ five files' '
>   	[f] (e) [e] ./dir4/e
>   	EOF
>   '
> -test_expect_success 'iteration of dir w/ five files' '
> +test_expect_success 'dirs-ignore of dir w/ five files' '
>   	test-tool dir-iterator ./dir4 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
>   	test_cmp expected-sorted-out actual-sorted-out
>   '
> +test_expect_success 'dirs-before of dir w/ five files' '
> +	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 'dirs-after of dir w/ five files' '
> +	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' '
> +	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 &&
>   	>dir5/a/b &&
>   
>   
> -	cat >expected-out <<-EOF
> +	cat >expected-ignore-out <<-EOF &&
> +	[f] (a/b) [b] ./dir5/a/b
> +	EOF
> +
> +	cat >expected-before-out <<-EOF &&
> +	[d] (a) [a] ./dir5/a
> +	[f] (a/b) [b] ./dir5/a/b
> +	EOF
> +
> +	cat >expected-after-out <<-EOF &&
> +	[f] (a/b) [b] ./dir5/a/b
> +	[d] (a) [a] ./dir5/a
> +	EOF
> +
> +	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_expect_success 'iteration of dir w/ dir w/ a file' '
> +test_expect_success 'dirs-ignore of dir w/ dir w/ a file' '
>   	test-tool dir-iterator ./dir5 >actual-out &&
> -	test_cmp expected-out actual-out
> +	test_cmp expected-ignore-out actual-out
> +'
> +test_expect_success 'dirs-before of dir w/ dir w/ a file' '
> +	test-tool dir-iterator --dirs-before ./dir5 >actual-out &&
> +	test_cmp expected-before-out actual-out
> +'
> +test_expect_success 'dirs-after of dir w/ dir w/ a file' '
> +	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' '
> +	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' '
> @@ -98,16 +256,49 @@ test_expect_success 'setup -- dir w/ three nested dirs w/ file' '
>   	>dir6/a/b/c/d &&
>   
>   
> -	cat >expected-out <<-EOF
> +	cat >expected-ignore-out <<-EOF &&
> +	[f] (a/b/c/d) [d] ./dir6/a/b/c/d
> +	EOF
> +
> +	cat >expected-before-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
>   	EOF
> +
> +	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
> +
> +	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_expect_success 'iteration of dir w/ three nested dirs w/ file' '
> +test_expect_success 'dirs-ignore of dir w/ three nested dirs w/ file' '
>   	test-tool dir-iterator ./dir6 >actual-out &&
> -	test_cmp expected-out actual-out
> +	test_cmp expected-ignore-out actual-out
> +'
> +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-before-out actual-out
> +'
> +test_expect_success 'dirs-after of dir w/ three nested dirs w/ file' '
> +	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' '
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir6 >actual-out &&
> +	test_cmp expected-before-after-out actual-out
>   '
>   
>   test_expect_success POSIXPERM,SANITY \
> @@ -116,33 +307,123 @@ test_expect_success POSIXPERM,SANITY \
>   	mkdir -p dir7/a/b/c &&
>   	>dir7/a/b/c/d &&
>   
> -	cat >expected-out <<-EOF &&
> +
> +	cat >expected-ignore-out <<-EOF &&
> +	EOF
> +	cat >expected-pedantic-ignore-out <<-EOF &&
> +	dir_iterator_advance failure
> +	EOF
> +
> +	cat >expected-before-out <<-EOF &&
> +	[d] (a) [a] ./dir7/a
> +	[d] (a/b) [b] ./dir7/a/b
> +	EOF
> +	cat >expected-pedantic-before-out <<-EOF &&
> +	[d] (a) [a] ./dir7/a
> +	[d] (a/b) [b] ./dir7/a/b
> +	dir_iterator_advance failure
> +	EOF
> +
> +	cat >expected-after-out <<-EOF &&
> +	[d] (a/b) [b] ./dir7/a/b
> +	[d] (a) [a] ./dir7/a
> +	EOF
> +	cat >expected-pedantic-after-out <<-EOF &&
> +	dir_iterator_advance failure
> +	EOF
> +
> +	cat >expected-before-after-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
> -	cat >expected-pedantic-out <<-EOF
> +	cat >expected-pedantic-before-after-out <<-EOF
>   	[d] (a) [a] ./dir7/a
>   	[d] (a/b) [b] ./dir7/a/b
>   	dir_iterator_advance failure
>   	EOF
>   '
>   test_expect_success POSIXPERM,SANITY \
> -'iteration of dir w/ three nested dirs w/ file, second w/o perms' '
> +'dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
>   
>   	chmod 0 dir7/a/b &&
>   
>   	test-tool dir-iterator ./dir7 >actual-out &&
> -	test_cmp expected-out actual-out &&
> +	test_cmp expected-ignore-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' '
> +'pedantic dirs-ignore of dir w/ three nested dirs w/ file, second w/o perms' '
>   
>   	chmod 0 dir7/a/b &&
>   
>   	test_must_fail test-tool dir-iterator --pedantic ./dir7 >actual-out &&
> -	test_cmp expected-pedantic-out actual-out &&
> +	test_cmp expected-pedantic-ignore-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' '
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test-tool dir-iterator --dirs-before ./dir7 >actual-out &&
> +	test_cmp expected-before-out actual-out &&
> +
> +	chmod 755 dir7/a/b
> +'
> +test_expect_success POSIXPERM,SANITY \
> +'pedantic dirs-before of dir w/ three nested dirs w/ file, second w/o perms' '
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic ./dir7 >actual-out &&
> +	test_cmp expected-pedantic-before-out actual-out &&
> +
> +	chmod 755 dir7/a/b
> +'
> +test_expect_success POSIXPERM,SANITY \
> +'dirs-after of dir w/ three nested dirs w/ file, second w/o perms' '
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test-tool dir-iterator --dirs-after ./dir7 >actual-out &&
> +	test_cmp expected-after-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' '
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-after \
> +		--pedantic ./dir7 >actual-out &&
> +	test_cmp expected-pedantic-after-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' '
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir7 >actual-out &&
> +	test_cmp expected-before-after-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' '
> +
> +	chmod 0 dir7/a/b &&
> +
> +	test_must_fail test-tool dir-iterator --dirs-before --dirs-after \
> +		--pedantic ./dir7 >actual-out &&
> +	test_cmp expected-pedantic-before-after-out actual-out &&
>   
>   	chmod 755 dir7/a/b
>   '
> @@ -154,24 +435,84 @@ test_expect_success 'setup -- dir w/ two dirs each w/ file' '
>   	>dir8/c/d &&
>   
>   
> -	cat >expected-out1 <<-EOF &&
> +	cat >expected-ignore-out1 <<-EOF &&
> +	[f] (a/b) [b] ./dir8/a/b
> +	[f] (c/d) [d] ./dir8/c/d
> +	EOF
> +	cat >expected-ignore-out2 <<-EOF &&
> +	[f] (c/d) [d] ./dir8/c/d
> +	[f] (a/b) [b] ./dir8/a/b
> +	EOF
> +
> +	cat >expected-before-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
> +	cat >expected-before-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
> +
> +	cat >expected-after-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-after-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
> +
> +	cat >expected-before-after-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-before-after-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_expect_success 'iteration of dir w/ two dirs each w/ file' '
> +test_expect_success 'dirs-ignore of dir w/ two dirs each w/ file' '
>   	test-tool dir-iterator ./dir8 >actual-out &&
>   	(
> -		test_cmp expected-out1 actual-out ||
> -		test_cmp expected-out2 actual-out
> +		test_cmp expected-ignore-out1 actual-out ||
> +		test_cmp expected-ignore-out2 actual-out
> +	)
> +'
> +test_expect_success 'dirs-before of dir w/ two dirs each w/ file' '
> +	test-tool dir-iterator --dirs-before ./dir8 >actual-out &&
> +	(
> +		test_cmp expected-before-out1 actual-out ||
> +		test_cmp expected-before-out2 actual-out
> +	)
> +'
> +test_expect_success 'dirs-after of dir w/ two dirs each w/ file' '
> +	test-tool dir-iterator --dirs-after ./dir8 >actual-out &&
> +	(
> +		test_cmp expected-after-out1 actual-out ||
> +		test_cmp expected-after-out2 actual-out
> +	)
> +'
> +test_expect_success 'dirs-before/dirs-after of dir w/ two dirs each w/ file' '
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir8 >actual-out &&
> +	(
> +		test_cmp expected-before-after-out1 actual-out ||
> +		test_cmp expected-before-after-out2 actual-out
>   	)
>   '
>   
> @@ -183,44 +524,164 @@ test_expect_success 'setup -- dir w/ two dirs, one w/ two and one w/ one files'
>   	>dir9/d/e &&
>   
>   
> -	cat >expected-out1 <<-EOF &&
> +	cat >expected-ignore-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-ignore-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-ignore-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-ignore-out4 <<-EOF &&
> +	[f] (d/e) [e] ./dir9/d/e
> +	[f] (a/c) [c] ./dir9/a/c
> +	[f] (a/b) [b] ./dir9/a/b
> +	EOF
> +
> +	cat >expected-before-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 &&
> +	cat >expected-before-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 &&
> +	cat >expected-before-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
> +	cat >expected-before-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
> +
> +	cat >expected-after-out1 <<-EOF &&
> +	[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-after-out2 <<-EOF &&
> +	[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
> +	cat >expected-after-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-after-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
> +
> +	cat >expected-before-after-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-before-after-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-before-after-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-before-after-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_expect_success \
> -'iteration of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
> +'dirs-ignore of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
>   
>   	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_cmp expected-ignore-out1 actual-out ||
> +		test_cmp expected-ignore-out2 actual-out ||
> +		test_cmp expected-ignore-out3 actual-out ||
> +		test_cmp expected-ignore-out4 actual-out
> +	)
> +'
> +test_expect_success \
> +'dirs-before of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
> +
> +	test-tool dir-iterator --dirs-before ./dir9 >actual-out &&
> +	(
> +		test_cmp expected-before-out1 actual-out ||
> +		test_cmp expected-before-out2 actual-out ||
> +		test_cmp expected-before-out3 actual-out ||
> +		test_cmp expected-before-out4 actual-out
> +	)
> +'
> +test_expect_success \
> +'dirs-after of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
> +
> +	test-tool dir-iterator --dirs-after ./dir9 >actual-out &&
> +	(
> +		test_cmp expected-after-out1 actual-out ||
> +		test_cmp expected-after-out2 actual-out ||
> +		test_cmp expected-after-out3 actual-out ||
> +		test_cmp expected-after-out4 actual-out
> +	)
> +'
> +test_expect_success \
> +'dirs-before/dirs-after of dir w/ three dirs, one w/ two, one w/ one and one w/ none files' '
> +
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir9 >actual-out &&
> +	(
> +		test_cmp expected-before-after-out1 actual-out ||
> +		test_cmp expected-before-after-out2 actual-out ||
> +		test_cmp expected-before-after-out3 actual-out ||
> +		test_cmp expected-before-after-out4 actual-out
>   	)
>   '
>   
> @@ -231,25 +692,84 @@ test_expect_success 'setup -- dir w/ two nested dirs, each w/ file' '
>   	>dir10/a/c/d &&
>   
>   
> -	cat >expected-out1 <<-EOF &&
> +	cat >expected-ignore-out1 <<-EOF &&
> +	[f] (a/b) [b] ./dir10/a/b
> +	[f] (a/c/d) [d] ./dir10/a/c/d
> +	EOF
> +	cat >expected-ignore-out2 <<-EOF &&
> +	[f] (a/c/d) [d] ./dir10/a/c/d
> +	[f] (a/b) [b] ./dir10/a/b
> +	EOF
> +
> +	cat >expected-before-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
> +	cat >expected-before-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
>   
> +	cat >expected-after-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-after-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
> +
> +	cat >expected-before-after-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-before-after-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_expect_success 'iteration of dir w/ two nested dirs, each w/ file' '
> +test_expect_success 'dirs-ignore of dir w/ two nested dirs, each w/ file' '
>   	test-tool dir-iterator ./dir10/ >actual-out &&
>   	(
> -		test_cmp expected-out1 actual-out ||
> -		test_cmp expected-out2 actual-out
> +		test_cmp expected-ignore-out1 actual-out ||
> +		test_cmp expected-ignore-out2 actual-out
> +	)
> +'
> +test_expect_success 'dirs-before of dir w/ two nested dirs, each w/ file' '
> +	test-tool dir-iterator --dirs-before ./dir10/ >actual-out &&
> +	(
> +		test_cmp expected-before-out1 actual-out ||
> +		test_cmp expected-before-out2 actual-out
> +	)
> +'
> +test_expect_success 'dirs-after of dir w/ two nested dirs, each w/ file' '
> +	test-tool dir-iterator --dirs-after ./dir10/ >actual-out &&
> +	(
> +		test_cmp expected-after-out1 actual-out ||
> +		test_cmp expected-after-out2 actual-out
> +	)
> +'
> +test_expect_success 'dirs-before/dirs-after of dir w/ two nested dirs, each w/ file' '
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir10 >actual-out &&
> +	(
> +		test_cmp expected-before-after-out1 actual-out ||
> +		test_cmp expected-before-after-out2 actual-out
>   	)
>   '
>   
> @@ -263,7 +783,15 @@ test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
>   	>dir11/d/e/d/a &&
>   
>   
> -	cat >expected-sorted-out <<-EOF
> +	cat >expected-ignore-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
> +
> +	cat >expected-before-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir11/a
>   	[d] (a/b) [b] ./dir11/a/b
>   	[d] (a/b/c) [c] ./dir11/a/b/c
> @@ -276,12 +804,52 @@ test_expect_success 'setup -- dir w/ complex structure w/o symlinks' '
>   	[f] (c) [c] ./dir11/c
>   	[f] (d/e/d/a) [a] ./dir11/d/e/d/a
>   	EOF
> +
> +	cat expected-before-sorted-out >expected-after-sorted-out &&
> +
> +	cat >expected-before-after-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_expect_success 'iteration of dir w/ complex structure w/o symlinks' '
> +test_expect_success 'dirs-ignore of dir w/ complex structure w/o symlinks' '
>   	test-tool dir-iterator ./dir11 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
> -	test_cmp expected-sorted-out actual-sorted-out
> +	test_cmp expected-ignore-sorted-out actual-sorted-out
> +'
> +test_expect_success 'dirs-before of dir w/ complex structure w/o symlinks' '
> +	test-tool dir-iterator --dirs-before ./dir11 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-before-sorted-out actual-sorted-out
> +'
> +test_expect_success 'dirs-after of dir w/ complex structure w/o symlinks' '
> +	test-tool dir-iterator --dirs-after ./dir11 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-after-sorted-out actual-sorted-out
> +'
> +test_expect_success 'dirs-before/dirs-after of dir w/ complex structure w/o symlinks' '
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir11 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-before-after-sorted-out actual-sorted-out
>   '
>   
>   test_expect_success POSIXPERM,SANITY \
> @@ -338,7 +906,7 @@ test_expect_success POSIXPERM,SANITY \
>   	[d] (a) [a] ./dir13/a
>   	EOF
>   
> -	test-tool dir-iterator ./dir13 >actual-out &&
> +	test-tool dir-iterator --dirs-before ./dir13 >actual-out &&
>   	test_cmp expected-no-permissions-out actual-out &&
>   
>   	chmod 755 dir13/a &&
> @@ -358,7 +926,8 @@ test_expect_success POSIXPERM,SANITY \
>   	dir_iterator_advance failure
>   	EOF
>   
> -	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-no-permissions-pedantic-out actual-out &&
>   
>   	chmod 755 dir13/a &&
> @@ -373,7 +942,17 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
>   	ln -s ../b dir14/a/f &&
>   
>   
> -	cat >expected-dont-follow-sorted-out <<-EOF &&
> +	cat >expected-dont-follow-ignore-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
> +	cat >expected-follow-ignore-sorted-out <<-EOF &&
> +	[f] (a/d) [d] ./dir14/a/d
> +	[f] (a/e) [e] ./dir14/a/e
> +	EOF
> +
> +	cat >expected-dont-follow-before-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir14/a
>   	[d] (b) [b] ./dir14/b
>   	[d] (b/c) [c] ./dir14/b/c
> @@ -381,7 +960,7 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
>   	[s] (a/e) [e] ./dir14/a/e
>   	[s] (a/f) [f] ./dir14/a/f
>   	EOF
> -	cat >expected-follow-sorted-out <<-EOF
> +	cat >expected-follow-before-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir14/a
>   	[d] (a/f) [f] ./dir14/a/f
>   	[d] (a/f/c) [c] ./dir14/a/f/c
> @@ -390,22 +969,99 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
>   	[f] (a/d) [d] ./dir14/a/d
>   	[f] (a/e) [e] ./dir14/a/e
>   	EOF
> +
> +	cat expected-dont-follow-before-sorted-out >expected-dont-follow-after-sorted-out &&
> +	cat expected-follow-before-sorted-out >expected-follow-after-sorted-out &&
> +
> +	cat >expected-dont-follow-before-after-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
> +	cat >expected-follow-before-after-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_expect_success SYMLINKS \
> -'dont-follow-symlinks of dir w/ symlinks w/o cycle' '
> +'dont-follow-symlinks dirs-ignore of dir w/ symlinks w/o cycle' '
>   
>   	test-tool dir-iterator ./dir14 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
> -	test_cmp expected-dont-follow-sorted-out actual-sorted-out
> +	test_cmp expected-dont-follow-ignore-sorted-out actual-sorted-out
>   '
>   test_expect_success SYMLINKS \
> -'follow-symlinks of dir w/ symlinks w/o cycle' '
> +'follow-symlinks dirs-ignore of dir w/ symlinks w/o cycle' '
>   
>   	test-tool dir-iterator --follow-symlinks ./dir14 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
> -	test_cmp expected-follow-sorted-out actual-sorted-out
> +	test_cmp expected-follow-ignore-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'dont-follow-symlinks dirs-before of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator --dirs-before ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-before-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'follow-symlinks dirs-before of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator --dirs-before --follow-symlinks ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-follow-before-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'dont-follow-symlinks dirs-after of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator --dirs-after ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-after-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'follow-symlinks dirs-after of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator --dirs-after --follow-symlinks ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-follow-after-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'dont-follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-before-after-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/o cycle' '
> +
> +	test-tool dir-iterator --dirs-before --dirs-after --follow-symlinks ./dir14 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-follow-before-after-sorted-out actual-sorted-out
>   '
>   
>   test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
> @@ -415,7 +1071,14 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
>   	ln -s ../ dir15/a/b/e &&
>   	ln -s ../../ dir15/a/b/f &&
>   
> -	cat >expected-dont-follow-sorted-out <<-EOF &&
> +
> +	cat >expected-dont-follow-ignore-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
> +
> +	cat >expected-dont-follow-before-sorted-out <<-EOF &&
>   	[d] (a) [a] ./dir15/a
>   	[d] (a/b) [b] ./dir15/a/b
>   	[d] (a/c) [c] ./dir15/a/c
> @@ -424,22 +1087,87 @@ test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/ cycle' '
>   	[s] (a/b/f) [f] ./dir15/a/b/f
>   	EOF
>   
> +	cat expected-dont-follow-before-sorted-out >expected-dont-follow-after-sorted-out &&
> +
> +	cat >expected-dont-follow-before-after-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
> +
>   	cat >expected-pedantic-follow-tailed-out <<-EOF
>   	dir_iterator_advance failure
>   	EOF
>   '
>   test_expect_success SYMLINKS \
> -'dont-follow-symlinks of dir w/ symlinks w/ cycle' '
> +'dont-follow-symlinks dirs-ignore of dir w/ symlinks w/ cycle' '
>   
>   	test-tool dir-iterator ./dir15 >actual-out &&
>   	sort actual-out >actual-sorted-out &&
>   
> -	test_cmp expected-dont-follow-sorted-out actual-sorted-out
> +	test_cmp expected-dont-follow-ignore-sorted-out actual-sorted-out
>   '
>   test_expect_success SYMLINKS \
> -'pedantic follow-symlinks of dir w/ symlinks w/ cycle' '
> +'pedantic follow-symlinks dirs-ignore of dir w/ symlinks w/ cycle' '
>   
>   	test_must_fail test-tool dir-iterator \
> +		--follow-symlinks --pedantic ./dir15 >actual-out &&
> +	tail -n 1 actual-out >actual-tailed-out &&
> +
> +	test_cmp expected-pedantic-follow-tailed-out actual-tailed-out
> +'
> +test_expect_success SYMLINKS \
> +'dont-follow-symlinks dirs-before of dir w/ symlinks w/ cycle' '
> +
> +	test-tool dir-iterator --dirs-before ./dir15 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-before-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'pedantic follow-symlinks dirs-before of dir w/ symlinks w/ cycle' '
> +
> +	test_must_fail test-tool dir-iterator --dirs-before \
> +		--pedantic --follow-symlinks ./dir15 >actual-out &&
> +	tail -n 1 actual-out >actual-tailed-out &&
> +
> +	test_cmp expected-pedantic-follow-tailed-out actual-tailed-out
> +'
> +test_expect_success SYMLINKS \
> +'dont-follow-symlinks dirs-after of dir w/ symlinks w/ cycle' '
> +
> +	test-tool dir-iterator --dirs-after ./dir15 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-after-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'pedantic follow-symlinks dirs-after of dir w/ symlinks w/ cycle' '
> +
> +	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-pedantic-follow-tailed-out actual-tailed-out
> +'
> +test_expect_success SYMLINKS \
> +'dont-follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/ cycle' '
> +
> +	test-tool dir-iterator --dirs-before --dirs-after ./dir15 >actual-out &&
> +	sort actual-out >actual-sorted-out &&
> +
> +	test_cmp expected-dont-follow-before-after-sorted-out actual-sorted-out
> +'
> +test_expect_success SYMLINKS \
> +'pedantic follow-symlinks dirs-before/dirs-after of dir w/ symlinks w/ cycle' '
> +
> +	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 &&
>   

Re: [RFC PATCH 4/6] dir-iterator: iterate dirs before or after their contents

[Plato Kiorpelidis]

On 22/04/11 02:31PM, Phillip Wood wrote:
> Hi Plato
> 
> On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> > Introduce a new feature to dir-iterator, using dir_iterator_begin()
> > flags parameter, allowing to control whether or not directories will
> > appear before or after their contents.
> > 
> > The new default behavior (i.e. flags set to 0) does not iterate over
> > directories.
> 
> This is introducing two changes at once adding the "AFTER" flag and changing
> the default behavior. I think it would make sence to separate those into two
> separate commits. You could change the default adding the "BEFORE" flag and
> adjusting the existing callers first and then implement the "AFTER" flag in
> a second commit.

Nice, I'll split this commit into two, like you suggest.

> It would also be nice to see the "AFTER" flag being used by changing
> remove_dir_recursively() for example.

I'll make use of "AFTER" flag to convert remove_subtree() function in entry.c
and include the conversion in v2.

Tackling remove_dir_recursively() will come later.

Thanks Phillip,
Plato

> Best Wishes
> 
> Phillip
> 
> > [...]

[RFC PATCH 5/6] t0066: remove redundant tests

[Plato Kiorpelidis]

Remove dir-iterator redundant tests since the new tests introduced in
045738486f (t0066: better test coverage for dir-iterator, 2022-04-07)
supersede them.

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

diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 24a4f1afef..974bb13092 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -894,46 +894,6 @@ test_expect_success 'dir_iterator_begin() should fail upon non directory paths'
 	test_cmp expected-non-dir-out actual-out
 '
 
-test_expect_success POSIXPERM,SANITY \
-'dir_iterator_advance() should not fail on errors by default' '
-
-	mkdir -p dir13/a &&
-	>dir13/a/b &&
-	chmod 0 dir13/a &&
-
-
-	cat >expected-no-permissions-out <<-EOF &&
-	[d] (a) [a] ./dir13/a
-	EOF
-
-	test-tool dir-iterator --dirs-before ./dir13 >actual-out &&
-	test_cmp expected-no-permissions-out actual-out &&
-
-	chmod 755 dir13/a &&
-	rm -rf dir13
-'
-
-test_expect_success POSIXPERM,SANITY \
-'dir_iterator_advance() should fail on errors, w/ pedantic flag' '
-
-	mkdir -p dir13/a &&
-	>dir13/a/b &&
-	chmod 0 dir13/a &&
-
-
-	cat >expected-no-permissions-pedantic-out <<-EOF &&
-	[d] (a) [a] ./dir13/a
-	dir_iterator_advance failure
-	EOF
-
-	test_must_fail test-tool dir-iterator --dirs-before \
-		--pedantic ./dir13 >actual-out &&
-	test_cmp expected-no-permissions-pedantic-out actual-out &&
-
-	chmod 755 dir13/a &&
-	rm -rf dir13
-'
-
 test_expect_success SYMLINKS 'setup -- dir w/ symlinks w/o cycle' '
 	mkdir -p dir14/a &&
 	mkdir -p dir14/b/c &&
-- 
2.35.1

Re: [RFC PATCH 5/6] t0066: remove redundant tests

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 10 2022, Plato Kiorpelidis wrote:

> Remove dir-iterator redundant tests since the new tests introduced in
> 045738486f (t0066: better test coverage for dir-iterator, 2022-04-07)
> supersede them.

This is presumably a forward-reference to the OID for your local version
of 2/6. Let's instead squash this change into that commit.

Re: [RFC PATCH 5/6] t0066: remove redundant tests

[Plato Kiorpelidis]

On 22/04/11 01:10PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Apr 10 2022, Plato Kiorpelidis wrote:
> 
> > Remove dir-iterator redundant tests since the new tests introduced in
> > 045738486f (t0066: better test coverage for dir-iterator, 2022-04-07)
> > supersede them.
> 
> This is presumably a forward-reference to the OID for your local version
> of 2/6. Let's instead squash this change into that commit.

Will do.

[RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator

[Plato Kiorpelidis]

Handle EACCES errno returned by dir_iterator_begin() by printing the
"EACCES" string instead of printing "ESOMETHINGELSE".

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

diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index c92616bd69..fd07429f90 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -7,9 +7,11 @@
 static const char *error_name(int error_number)
 {
 	switch (error_number) {
-	case ENOENT: return "ENOENT";
-	case ENOTDIR: return "ENOTDIR";
-	default: return "ESOMETHINGELSE";
+		case ENOENT: return "ENOENT";
+		case ENOTDIR: return "ENOTDIR";
+		case EACCES: return "EACCES";
+
+		default: return "ESOMETHINGELSE";
 	}
 }
 
diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 974bb13092..4bf6456735 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -861,7 +861,7 @@ test_expect_success POSIXPERM,SANITY \
 
 
 	cat >expected-no-permissions-out <<-EOF &&
-	dir_iterator_begin failure: ESOMETHINGELSE
+	dir_iterator_begin failure: EACCES
 	EOF
 
 	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&
-- 
2.35.1

Re: [RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 10 2022, Plato Kiorpelidis wrote:

> Handle EACCES errno returned by dir_iterator_begin() by printing the
> "EACCES" string instead of printing "ESOMETHINGELSE".
>
> Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> ---
>  t/helper/test-dir-iterator.c | 8 +++++---
>  t/t0066-dir-iterator.sh      | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
> index c92616bd69..fd07429f90 100644
> --- a/t/helper/test-dir-iterator.c
> +++ b/t/helper/test-dir-iterator.c
> @@ -7,9 +7,11 @@
>  static const char *error_name(int error_number)
>  {
>  	switch (error_number) {
> -	case ENOENT: return "ENOENT";
> -	case ENOTDIR: return "ENOTDIR";
> -	default: return "ESOMETHINGELSE";
> +		case ENOENT: return "ENOENT";
> +		case ENOTDIR: return "ENOTDIR";
> +		case EACCES: return "EACCES";
> +
> +		default: return "ESOMETHINGELSE";
>  	}
>  }

Please stick with the git coding style, see
Documentation/CodingGuidelines.

It's forgivable to "fix style while at it" to some extent, but in this
case it's both moving away from our normal style (by indenting case
labels), and in doing so making the diff larger/worse.

This should be a one-line addition of your new EACCES case.

I haven't read through the rest yet, but please self-review those
patches with a keen eye to diff size & seeing if there's similar
issues. E.g. are they the same lines changed under "git show -w", if not
are you sure you're correct in making the style change?

> diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
> index 974bb13092..4bf6456735 100755
> --- a/t/t0066-dir-iterator.sh
> +++ b/t/t0066-dir-iterator.sh
> @@ -861,7 +861,7 @@ test_expect_success POSIXPERM,SANITY \
>  
>  
>  	cat >expected-no-permissions-out <<-EOF &&
> -	dir_iterator_begin failure: ESOMETHINGELSE
> +	dir_iterator_begin failure: EACCES
>  	EOF
>  
>  	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&

I see this is changing the ESOMETHINGELSE added in your 3/6, if you make
this patch come first then presumably we won't have that churn.

And if we don't have a test that's relying on ESOMETHINGELSE (maybe we
do, but don't test_cmp it, I haven't checked) shouldn't this "default"
case just be:

    BUG("unknown errno %d: %s", errno_number, strerror(errno_number))

I.e. if we have OK coverage here we'd presumably fail the test case
itself anyway, so shouldn't we fail here too?

Re: [RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator

[Plato Kiorpelidis]

On 22/04/11 01:04PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Apr 10 2022, Plato Kiorpelidis wrote:
> 
> > Handle EACCES errno returned by dir_iterator_begin() by printing the
> > "EACCES" string instead of printing "ESOMETHINGELSE".
> >
> > Signed-off-by: Plato Kiorpelidis <kioplato@gmail.com>
> > ---
> >  t/helper/test-dir-iterator.c | 8 +++++---
> >  t/t0066-dir-iterator.sh      | 2 +-
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
> > index c92616bd69..fd07429f90 100644
> > --- a/t/helper/test-dir-iterator.c
> > +++ b/t/helper/test-dir-iterator.c
> > @@ -7,9 +7,11 @@
> >  static const char *error_name(int error_number)
> >  {
> >  	switch (error_number) {
> > -	case ENOENT: return "ENOENT";
> > -	case ENOTDIR: return "ENOTDIR";
> > -	default: return "ESOMETHINGELSE";
> > +		case ENOENT: return "ENOENT";
> > +		case ENOTDIR: return "ENOTDIR";
> > +		case EACCES: return "EACCES";
> > +
> > +		default: return "ESOMETHINGELSE";
> >  	}
> >  }
> 
> Please stick with the git coding style, see
> Documentation/CodingGuidelines.
> 
> It's forgivable to "fix style while at it" to some extent, but in this
> case it's both moving away from our normal style (by indenting case
> labels), and in doing so making the diff larger/worse.
> 
> This should be a one-line addition of your new EACCES case.

That's true. I should've consulted Documentation/CodingGuidelines in more
detail. I mainly copied the local style and mindlessly indented these case
labels. I'll be more mindful of the coding style. Thanks!

> I haven't read through the rest yet, but please self-review those
> patches with a keen eye to diff size & seeing if there's similar
> issues. E.g. are they the same lines changed under "git show -w", if not
> are you sure you're correct in making the style change?

That's great, I'll make the use of "git show -w" habitual.

> > diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
> > index 974bb13092..4bf6456735 100755
> > --- a/t/t0066-dir-iterator.sh
> > +++ b/t/t0066-dir-iterator.sh
> > @@ -861,7 +861,7 @@ test_expect_success POSIXPERM,SANITY \
> >  
> >  
> >  	cat >expected-no-permissions-out <<-EOF &&
> > -	dir_iterator_begin failure: ESOMETHINGELSE
> > +	dir_iterator_begin failure: EACCES
> >  	EOF
> >  
> >  	test_must_fail test-tool dir-iterator ./dir12 >actual-out &&
> 
> I see this is changing the ESOMETHINGELSE added in your 3/6, if you make
> this patch come first then presumably we won't have that churn.

Yes. I'll make this patch come first to avoid that churn.

> And if we don't have a test that's relying on ESOMETHINGELSE (maybe we
> do, but don't test_cmp it, I haven't checked) shouldn't this "default"
> case just be:
> 
>     BUG("unknown errno %d: %s", errno_number, strerror(errno_number))
> 
> I.e. if we have OK coverage here we'd presumably fail the test case
> itself anyway, so shouldn't we fail here too?

We can probably do that. I don't think we've got a test whose output isn't
test_cmp'ed anyways. There are two reasons I'm hesitant about this:
  1. I'm not sure whether or not the test suite t/ behaves correctly in case
  this macro triggers. I suppose you are positive it does since you suggested
  it. I need to check and figure this out myself because I haven't used this
  macro before.
  2. How does this improve testing? If ESOMETHINGELSE is returned when some
  other errno is expected then test_cmp will fail and the test suite will output
  an error. How will the introduction of BUG() macro as the default case for the
  switch statement in test-dir-iterator improve that?

Thanks,
Plato

Re: [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents

[Phillip Wood]

Hi Plato

I think this would be a useful addition to git, thanks for working on 
it. I've left some comments on the individual patches. In general I 
think this series would benefit from trying to work with the existing 
code and change one aspect at a time in stages rather than rewriting 
large chunks all at once.

Best Wishes

Phillip

On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> Hello. Some time ago I worked on converting opendir/readdir calls to use
> dir-iterator [1], however this cleanup required to iterate directory paths after
> their contents. Matheus pointed me out to a previous patch series[2] that
> attempted to implement such functionality in dir-iterator. From it, I mainly
> used Michael's feedback and feature requests and tried to include it in my work.
> 
> My fork: https://github.com/kioplato/git/tree/dir-iterator
> CI: https://github.com/kioplato/git/actions/runs/2141288008
> There are some memleaks, I'll track them down in v2.
> 
> I aim to implement more functionality in dir-iterator, my goal being to simplify
> the codebase by introducing an abstraction layer for iterating directories.
> I would like to eventually simplify read_directory_recursive(). I wanted to
> check in with you to make sure I'm heading in the right direction with what I've
> implemented.
>    * Are my tests overly exhaustive?
>    * As of now we can't thoroughly test dir-iterator on directories with complex
>    structure since readdir produce dirents with undefined order in a directory.
>    I thought about introducing a tool for generating permutations with stable
>    parts in test-lib. Is there a need to something like this for other tests?
>    Or maybe should I sort each level iterated by dir-iterator inside
>    test-dir-iterator before printing to stdout? In these patches I did enumerate
>    the path permutations for some tests by hand, but that's not viable really.
>    * We also don't test for deleted entries between dir_iterator_advance() calls.
>    * Are my comments too much? Throughout git, .c files don't have many comments,
>    should I remove mine as well? I think they provide better context when reading
>    through the source code.
> 
> I do understand that it probably is too early to worry about most of these.
> However I wanted to communicate my thoughts and setup for the following
> versions.
> 
> While I wait for review, I'll implement/fix:
>    * DIR_ITERATOR_RECURSE proposed here[3], but with finer control. Instead of a
>    flag I'll introduce a new integer parameter in dir_iterator_begin(), which
>    will specify the maximum iteration depth.
>      * Supplying 0 will have the same behavior as DIR_ITERATOR_RECURSE i.e. it
>      will iterate over the entries of the root directory.
>      * Supplying -1 will iterate to maximum depth. This is the default right now.
>    * DIR_ITERATOR_DIRS_ONLY proposed here[4]. Enabling this, dir-iterator will
>    show only directories. Failing to enable DIR_ITERATOR_DIRS_BEFORE and/or
>    DIR_ITERATOR_DIRS_AFTER will result in dir_iterator_begin() returning NULL.
>    Is this a good way to encode "show only directories" in the flags?
> 
> I'll include them along with feedback and suggestions from this version in the
> next one.
> 
> I didn't refactor entry.c to use dir-iterator. It's a good first issue for
> someone else to become introduced to the community. I applied my patch[1] and it
> does not pass t/, as it used to, because of 15th test in t1092. Should I work on
> entry.c in my next version or leave it alone for a newcomer?
> 
> This serves as my microproject for GSoC. Could my future work on dir-iterator
> and cleanup of read_directory_recursive() and other customers of dir-iterator
> become a seperate GSoC project I could undertake?
> 
> [1]: https://public-inbox.org/git/20191208180439.19018-1-otalpster@gmail.com/
> [2]: https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/
> [3]: https://public-inbox.org/git/CACsJy8DBa-oH3i+5P=iVr9NhJwsicZ43DO89WmvpYEQu90RrMw@mail.gmail.com/
> [4]: https://public-inbox.org/git/xmqqmvc265hk.fsf@gitster.mtv.corp.google.com/
> 
> Plato Kiorpelidis (6):
>    t0066: improve readablity of dir-iterator tests
>    t0066: better test coverage for dir-iterator
>    dir-iterator: refactor dir_iterator_advance()
>    dir-iterator: iterate dirs before or after their contents
>    t0066: remove redundant tests
>    test-dir-iterator: handle EACCES errno by dir-iterator
> 
>   builtin/clone.c              |    4 +-
>   dir-iterator.c               |  302 ++++++---
>   dir-iterator.h               |   34 +-
>   refs/files-backend.c         |    2 +-
>   t/helper/test-dir-iterator.c |   23 +-
>   t/t0066-dir-iterator.sh      | 1202 +++++++++++++++++++++++++++++++---
>   6 files changed, 1371 insertions(+), 196 deletions(-)
> 

Re: [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents

[Plato Kiorpelidis]

On Mon, Apr 11, 2022 at 4:37 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Plato

Hey Phillip,

Thanks for reviewing my patches, your input is welcomed! :)
I'll reply your reviews and Ævar's in the following days. I need some
time to read dir-iterator again in combination with your comments.

>
> I think this would be a useful addition to git, thanks for working on
> it. I've left some comments on the individual patches. In general I
> think this series would benefit from trying to work with the existing
> code and change one aspect at a time in stages rather than rewriting
> large chunks all at once.

Agreed, I'll change one aspect at a time. First, I'll include
suggested changes from this version and also convert entry.c to use
dir-iterator instead of opendir, readdir, closedir API and submit v2.
Then I'll spend some time understanding read_directory_recursive() and
search for possible parts that can be cleaned up using dir-iterator.
I'll compile a set of possible customer candidates for dir-iterator as
reference. This will take some time since I'll prepare for the
upcoming GSoC as a possible contributor.

Thanks,
Plato

>
> Best Wishes
>
> Phillip
>
> On 10/04/2022 12:18, Plato Kiorpelidis wrote:
> [...]