From: Selena Deckelmann on
Hi!

Below is from Brad Slinger. I'm just forwarding his message so that
the review will be in the thread for the archives. (Sorry, Brad that I
missed this earlier.. I thought you'd already replied to the list for
some reason.)

Brad says:

Please forgive my top posting. Below is my patch review.

The purpose of this patch is to improve psql output
readability. This is accomplished through table formatting
improvements and line art improvements.

The patch is a unified diff, rather than a context diff. The
patch applies from the project root with with -p1 set. Patch(1)
reports "with fuzz 1" when patching src/bin/psql/print.h. The patch
touches two files:

src/bin/psql/print.h
src/bin/psql/print.c

The patch does not include any documentation or tests.
Comments and style are similar to existing code.

The patched code compiles without any additional warnings.
Lint gripes about a trailing ',' in 'typedef enum printTextRule' in
print.h. Other additional lint seem to be false positives. The
regression tests pass against the new patch.

The patched code installs and runs. There were no crashes,
although I didn't try very hard. The ASCII line art works:

createdb test
psql test
\pset border 2
\pset format wrapped
SELECT 'short' AS short, lpad('', 1000, 'long') AS long;

I can trigger the new line art by setting LC_ALL=en_US.UTF-8 in
the environment.

As far as I can tell, the patch works as advertised. The patch
does not change the server/backend. I don't see any performance
problems.


Thanks,

--bts

--
http://chesnok.com/daily - me
http://endpoint.com - work

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Roger Leigh on
On Fri, Sep 18, 2009 at 11:30:05AM -0700, Selena Deckelmann wrote:
> Brad says:
>
> The patched code compiles without any additional warnings.
> Lint gripes about a trailing ',' in 'typedef enum printTextRule' in
> print.h. Other additional lint seem to be false positives. The
> regression tests pass against the new patch.

I've attached a new patch which tidies up those extra commas, plus
a patch showing the changes from the previous patch.


Regards,
Roger

--
.''`. Roger Leigh
: :' : Debian GNU/Linux http://people.debian.org/~rleigh/
`. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/
`- GPG Public Key: 0x25BFB848 Please GPG sign your mail.
From: Selena Deckelmann on
Hi!

On Wed, Sep 23, 2009 at 2:16 AM, Roger Leigh <rleigh(a)codelibre.net> wrote:
> On Fri, Sep 18, 2009 at 11:30:05AM -0700, Selena Deckelmann wrote:
>> Brad says:
>>
>>        The patched code compiles without any additional warnings.
>> Lint gripes about a trailing ',' in 'typedef enum printTextRule' in
>> print.h.  Other additional lint seem to be false positives.  The
>> regression tests pass against the new patch.
>
> I've attached a new patch which tidies up those extra commas, plus
> a patch showing the changes from the previous patch.

Great! Thank you.

Brad -- can you review this patch? Is it ready for a committer?

Thanks,
-selena


--
http://chesnok.com/daily - me
http://endpoint.com - work

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on
On Sun, Sep 27, 2009 at 9:24 PM, Selena Deckelmann
<selenamarie(a)gmail.com> wrote:
> Hi!
>
> On Wed, Sep 23, 2009 at 2:16 AM, Roger Leigh <rleigh(a)codelibre.net> wrote:
>> On Fri, Sep 18, 2009 at 11:30:05AM -0700, Selena Deckelmann wrote:
>>> Brad says:
>>>
>>>        The patched code compiles without any additional warnings.
>>> Lint gripes about a trailing ',' in 'typedef enum printTextRule' in
>>> print.h.  Other additional lint seem to be false positives.  The
>>> regression tests pass against the new patch.
>>
>> I've attached a new patch which tidies up those extra commas, plus
>> a patch showing the changes from the previous patch.
>
> Great!  Thank you.
>
> Brad -- can you review this patch? Is it ready for a committer?

Brad already marked it that way on the CommitFest application, so I
think that probably means yes. :-)

....Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: "Brad T. Sliger" on
On Sunday 27 September 2009 19:03:33 Robert Haas wrote:
> On Sun, Sep 27, 2009 at 9:24 PM, Selena Deckelmann
>
> <selenamarie(a)gmail.com> wrote:
> > Hi!
> >
> > On Wed, Sep 23, 2009 at 2:16 AM, Roger Leigh <rleigh(a)codelibre.net> wrote:
> >> On Fri, Sep 18, 2009 at 11:30:05AM -0700, Selena Deckelmann wrote:
> >>> Brad says:
> >>>
> >>>        The patched code compiles without any additional warnings.
> >>> Lint gripes about a trailing ',' in 'typedef enum printTextRule' in
> >>> print.h.  Other additional lint seem to be false positives.  The
> >>> regression tests pass against the new patch.
> >>
> >> I've attached a new patch which tidies up those extra commas, plus
> >> a patch showing the changes from the previous patch.
> >
> > Great!  Thank you.
> >
> > Brad -- can you review this patch? Is it ready for a committer?
>
> Brad already marked it that way on the CommitFest application, so I
> think that probably means yes. :-)
>
> ...Robert

I looked at the new (psql-utf8-table-4.patch) patch. I think the style issues are fixed.

During this review I found that `gmake check` will fail when LANG=en_US.UTF-8 in the environment. In this case
the patched psql produces UTF8 line art and the tests expect ASCII line art.

pg_regress clears LC_ALL by default, but does not clear LANG by default. Please find attached a patch that
causes pg_regress to also clear LANG by default.

Thanks,

--bts

 |  Next  |  Last
Pages: 1 2 3 4 5 6 7 8 9 10 11
Prev: Hot Standby 0.2.1
Next: Rejecting weak passwords