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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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?