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@xxxxxxxxx>
> > ---
> > 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
