Prev: Hot Standby 0.2.1
Next: Rejecting weak passwords
From: Selena Deckelmann on 18 Sep 2009 14:30 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 23 Sep 2009 05:16 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 27 Sep 2009 21:24 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 27 Sep 2009 22:03 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 28 Sep 2009 23:49
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 |