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

From: Plato Kiorpelidis <kioplato@gmail.com>
Date: Apr 27, 2022, 17:30 UTC
Message-ID: <20220427173002.srn7cidveao7pk55@compass>
In-Reply-To: <220411.86wnfvj2q6.gmgdl@evledraar.gmail.com>

On 22/04/11 01:04PM, Ævar Arnfjörð Bjarmason wrote:

Show 39 quoted lines

> 
> 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.

Show 16 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.

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

Show 8 quoted lines

> 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