From: Plato Kiorpelidis <kioplato@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, matheus.bernardino@usp.br, mhagger@alum.mit.edu
Subject: Re: [RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator
Date: Wed, 27 Apr 2022 20:30:02 +0300 [thread overview]
Message-ID: <20220427173002.srn7cidveao7pk55@compass> (raw)
In-Reply-To: <220411.86wnfvj2q6.gmgdl@evledraar.gmail.com>
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
next prev parent reply other threads:[~2022-04-27 17:34 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-10 11:18 [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 1/6] t0066: improve readablity of dir-iterator tests Plato Kiorpelidis
2022-04-11 13:16 ` Phillip Wood
2022-04-24 19:25 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 2/6] t0066: better test coverage for dir-iterator Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance() Plato Kiorpelidis
2022-04-11 11:11 ` Ævar Arnfjörð Bjarmason
2022-04-11 13:40 ` Phillip Wood
2022-04-27 15:45 ` Plato Kiorpelidis
2022-04-11 13:26 ` Phillip Wood
2022-04-27 14:32 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 4/6] dir-iterator: iterate dirs before or after their contents Plato Kiorpelidis
2022-04-11 13:31 ` Phillip Wood
2022-04-27 14:57 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 5/6] t0066: remove redundant tests Plato Kiorpelidis
2022-04-11 11:10 ` Ævar Arnfjörð Bjarmason
2022-04-27 16:00 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator Plato Kiorpelidis
2022-04-11 11:04 ` Ævar Arnfjörð Bjarmason
2022-04-27 17:30 ` Plato Kiorpelidis [this message]
2022-04-11 13:37 ` [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents Phillip Wood
2022-04-19 13:06 ` Plato Kiorpelidis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220427173002.srn7cidveao7pk55@compass \
--to=kioplato@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=matheus.bernardino@usp.br \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).