From: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Tighten up a few overly lax regexes in pg_dump's tap tests |
Date: | 2019-02-06 14:41:20 |
Message-ID: | C4CC38F4-3D8D-4CE0-BA31-5B2DD1504C57@yesql.se |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On 6 Feb 2019, at 12:37, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Feb 06, 2019 at 10:50:27AM +0100, Daniel Gustafsson wrote:
>> I still think we should enforce one-or-more matching on the OWNER part as well,
>> since matching zero would be a syntax error. There are more .* matches but
>> I’ve only touched the ones which match SQL, since there is a defined grammar to
>> rely on there. The attached patch does that on top of your commit.
>
> - regexp => qr/^COMMENT ON DATABASE postgres IS .*;/m,
> + regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,
> So... With what's on HEAD the current regex means that all these are
> correct commands:
> COMMENT ON DATABASE postgres is ;
> COMMENT ON DATABASE postgres is foo;
> COMMENT ON DATABASE postgres is foo;
> And for the first one that's obviously wrong.
>
> So what you are suggesting is to actually make the first pattern
> something to complain about, right? And ".+" this makes sure that at
> least one character is present, while for ".*" it is fine to have zero
> characters. What you are suggesting looks right if I understood that
> right.
Correct. One could argue that the regex is still suboptimal since “COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the tradeoff
for readability wins.
cheers ./daniel
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-02-06 15:09:22 | Re: bug tracking system |
Previous Message | Andres Freund | 2019-02-06 14:29:15 | Re: Synchronize with imath upstream |