Re: [RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 11, 2022, 11:04 UTC
- Message-ID
- <220411.86wnfvj2q6.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220410111852.2097418-7-kioplato@gmail.com>
On Sun, Apr 10 2022, Plato Kiorpelidis wrote:
Show 27 quoted lines
> 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?
Show 13 quoted lines
> 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?