Everyone using git diff in color mode will already or soon be aware that
psql, for what I can only think is an implementation oversight, produces
trailing whitespace in the table headers, like this:

two | f1 $
-----+------------$
asdfghjkl;$
d34aaasdf$
(2 rows)$

($ is the line end; cf. cat -A). Note that this only applies to
headers, not content cells.

Attached is a patch to fix that.

Search Discussions

  • Itagaki Takahiro at Sep 22, 2010 at 12:48 am

    On Wed, Sep 22, 2010 at 3:28 AM, Peter Eisentraut wrote:
    Everyone using git diff in color mode will already or soon be aware that
    psql, for what I can only think is an implementation oversight, produces
    trailing whitespace in the table headers,
    I think removing trailing whitespace in headers itself is reasonable,
    but the change breaks almost all of regression tests. Will we adjust
    expected results for the change?

    --
    Itagaki Takahiro
  • David Fetter at Sep 22, 2010 at 2:02 am

    On Wed, Sep 22, 2010 at 09:48:12AM +0900, Itagaki Takahiro wrote:
    On Wed, Sep 22, 2010 at 3:28 AM, Peter Eisentraut wrote:
    Everyone using git diff in color mode will already or soon be
    aware that psql, for what I can only think is an implementation
    oversight, produces trailing whitespace in the table headers,
    I think removing trailing whitespace in headers itself is
    reasonable, but the change breaks almost all of regression tests.
    Will we adjust expected results for the change?
    On its face, this doesn't seem like a hard change to make.

    Cheers,
    David.
    --
    David Fetter <david@fetter.org> http://fetter.org/
    Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
    Skype: davidfetter XMPP: david.fetter@gmail.com
    iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

    Remember to vote!
    Consider donating to Postgres: http://www.postgresql.org/about/donate
  • Roger Leigh at Sep 24, 2010 at 9:44 pm

    On Tue, Sep 21, 2010 at 09:28:07PM +0300, Peter Eisentraut wrote:
    Everyone using git diff in color mode will already or soon be aware that
    psql, for what I can only think is an implementation oversight, produces
    trailing whitespace in the table headers, like this:

    two | f1 $
    -----+------------$
    asdfghjkl;$
    d34aaasdf$
    (2 rows)$
    Does this break the output with "\pset border 2"?

    IIRC when I was doing the "\pset linestyle" work, I did look at
    doing this, but found that the padding was required in some
    cases. I couldn't tell from looking over the patch whether or
    not you were already taking this into account though?


    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.
  • Peter Eisentraut at Sep 27, 2010 at 12:05 pm

    On fre, 2010-09-24 at 22:38 +0100, Roger Leigh wrote:
    On Tue, Sep 21, 2010 at 09:28:07PM +0300, Peter Eisentraut wrote:
    Everyone using git diff in color mode will already or soon be aware that
    psql, for what I can only think is an implementation oversight, produces
    trailing whitespace in the table headers, like this:

    two | f1 $
    -----+------------$
    asdfghjkl;$
    d34aaasdf$
    (2 rows)$
    Does this break the output with "\pset border 2"?
    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output. We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change. Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
  • David E. Wheeler at Sep 27, 2010 at 4:25 pm

    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output. We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change. Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.

    David
  • Alvaro Herrera at Sep 27, 2010 at 5:34 pm

    Excerpts from David E. Wheeler's message of lun sep 27 12:25:31 -0400 2010:
    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output. We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change. Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.
    Perhaps using unaligned mode? The problem with that is that it becomes
    very difficult to review changes to expected output.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Robert Haas at Sep 27, 2010 at 5:53 pm

    On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera wrote:
    Excerpts from David E. Wheeler's message of lun sep 27 12:25:31 -0400 2010:
    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output.  We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change.  Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.
    Perhaps using unaligned mode?  The problem with that is that it becomes
    very difficult to review changes to expected output.
    Uh, yuck! If we don't care about changing the expected output, we can
    just trim the whitespace as Peter suggested originally.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • David Fetter at Sep 27, 2010 at 6:09 pm

    On Mon, Sep 27, 2010 at 01:53:45PM -0400, Robert Haas wrote:
    On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera
    wrote:
    Excerpts from David E. Wheeler's message of lun sep 27 12:25:31 -0400 2010:
    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output.  We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change.  Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.
    Perhaps using unaligned mode?  The problem with that is that it becomes
    very difficult to review changes to expected output.
    Uh, yuck! If we don't care about changing the expected output, we can
    just trim the whitespace as Peter suggested originally.
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests. Wouldn't trimming
    all trailing whitespace do the trick?

    Cheers,
    David.
    --
    David Fetter <david@fetter.org> http://fetter.org/
    Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
    Skype: davidfetter XMPP: david.fetter@gmail.com
    iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

    Remember to vote!
    Consider donating to Postgres: http://www.postgresql.org/about/donate
  • Robert Haas at Sep 27, 2010 at 7:11 pm

    On Mon, Sep 27, 2010 at 2:09 PM, David Fetter wrote:
    On Mon, Sep 27, 2010 at 01:53:45PM -0400, Robert Haas wrote:
    On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera
    wrote:
    Excerpts from David E. Wheeler's message of lun sep 27 12:25:31 -0400 2010:
    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output.  We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change.  Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.
    Perhaps using unaligned mode?  The problem with that is that it becomes
    very difficult to review changes to expected output.
    Uh, yuck!  If we don't care about changing the expected output, we can
    just trim the whitespace as Peter suggested originally.
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests.  Wouldn't trimming
    all trailing whitespace do the trick?
    Sure. But everyone using pg_regress will have to update their
    regression test expected outputs.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • David Fetter at Sep 27, 2010 at 8:12 pm

    On Mon, Sep 27, 2010 at 03:11:07PM -0400, Robert Haas wrote:
    On Mon, Sep 27, 2010 at 2:09 PM, David Fetter wrote:
    On Mon, Sep 27, 2010 at 01:53:45PM -0400, Robert Haas wrote:
    On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera
    wrote:
    Excerpts from David E. Wheeler's message of lun sep 27 12:25:31 -0400 2010:
    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output.  We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change.  Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.
    Perhaps using unaligned mode?  The problem with that is that it becomes
    very difficult to review changes to expected output.
    Uh, yuck!  If we don't care about changing the expected output, we can
    just trim the whitespace as Peter suggested originally.
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests.  Wouldn't trimming
    all trailing whitespace do the trick?
    Sure. But everyone using pg_regress will have to update their
    regression test expected outputs.
    Again, I must be missing something super important. What is it that
    prevents people from doing

    find . -type f |xargs perl -pi.bak -e 's/\s+$//g'

    or moral equivalent on their pg_regression tree?

    Cheers,
    David.
    --
    David Fetter <david@fetter.org> http://fetter.org/
    Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
    Skype: davidfetter XMPP: david.fetter@gmail.com
    iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

    Remember to vote!
    Consider donating to Postgres: http://www.postgresql.org/about/donate
  • Robert Haas at Sep 27, 2010 at 8:19 pm

    On Mon, Sep 27, 2010 at 4:12 PM, David Fetter wrote:
    On Mon, Sep 27, 2010 at 03:11:07PM -0400, Robert Haas wrote:
    On Mon, Sep 27, 2010 at 2:09 PM, David Fetter wrote:
    On Mon, Sep 27, 2010 at 01:53:45PM -0400, Robert Haas wrote:
    On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera
    wrote:
    Excerpts from David E. Wheeler's message of lun sep 27 12:25:31 -0400 2010:
    On Sep 27, 2010, at 5:05 AM, Peter Eisentraut wrote:

    Um, no.

    In the meantime, I have arrived at the conclusion that doing this isn't
    worth it because it will break all regression test output.  We can fix
    the stuff in our tree, but pg_regress is also used externally, and those
    guys would have a nightmare with this change.  Perhaps if there is
    another more significant revision of the table style in the future, we
    should keep this issue in mind.
    Or change the way pg_regress works.
    Perhaps using unaligned mode?  The problem with that is that it becomes
    very difficult to review changes to expected output.
    Uh, yuck!  If we don't care about changing the expected output, we can
    just trim the whitespace as Peter suggested originally.
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests.  Wouldn't trimming
    all trailing whitespace do the trick?
    Sure.  But everyone using pg_regress will have to update their
    regression test expected outputs.
    Again, I must be missing something super important.  What is it that
    prevents people from doing

    find . -type f |xargs perl -pi.bak -e 's/\s+$//g'

    or moral equivalent on their pg_regression tree?
    I assume it's not that simple, but I haven't tried it.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Tom Lane at Sep 28, 2010 at 5:06 pm

    David Fetter writes:
    On Mon, Sep 27, 2010 at 03:11:07PM -0400, Robert Haas wrote:
    Sure. But everyone using pg_regress will have to update their
    regression test expected outputs.
    Again, I must be missing something super important. What is it that
    prevents people from doing
    find . -type f |xargs perl -pi.bak -e 's/\s+$//g'
    I think the concern isn't about the change being hard to make, it's
    about the fallout from having to have different expected-result files
    for 9.1 versus previous releases. A lot of third-party modules try
    to maintain source code compatibility across all PG releases they
    support, and this would break that.

    That's not necessarily a sufficient reason for us not to do it ---
    but it's definitely not a negligible issue, either.

    It should be noted that this won't be zero-cost for the core project
    either. For example, any back-patched fix that adjusts regression test
    outputs will have to deal with the incompatibility. And if we do put in
    some git-level check on whitespace, it'll have to be made to only apply
    to master not back branches.

    regards, tom lane
  • Tom Lane at Sep 28, 2010 at 4:19 pm

    Robert Haas writes:
    On Mon, Sep 27, 2010 at 2:09 PM, David Fetter wrote:
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests.  Wouldn't trimming
    all trailing whitespace do the trick?
    Sure. But everyone using pg_regress will have to update their
    regression test expected outputs.
    I'm inclined to think that that's not a fatal objection; it's not like
    we haven't felt free to change psql's output format before. As long as
    we don't back-patch this change, it should be no worse than other things
    we've done to third-party code without a backwards glance.

    It would be good to get rid of this whitespace because (I believe) it is
    one of very few reasons for needing to have any trailing whitespace in
    git-controlled files. If we could get to a point where trailing
    whitespace in patches could be rejected automatically, it'd eliminate
    one small pet peeve.

    regards, tom lane
  • Robert Haas at Sep 28, 2010 at 4:23 pm

    On Tue, Sep 28, 2010 at 12:18 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Mon, Sep 27, 2010 at 2:09 PM, David Fetter wrote:
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests.  Wouldn't trimming
    all trailing whitespace do the trick?
    Sure.  But everyone using pg_regress will have to update their
    regression test expected outputs.
    I'm inclined to think that that's not a fatal objection; it's not like
    we haven't felt free to change psql's output format before.  As long as
    we don't back-patch this change, it should be no worse than other things
    we've done to third-party code without a backwards glance.

    It would be good to get rid of this whitespace because (I believe) it is
    one of very few reasons for needing to have any trailing whitespace in
    git-controlled files.  If we could get to a point where trailing
    whitespace in patches could be rejected automatically, it'd eliminate
    one small pet peeve.
    I have to agree that would be nice.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise Postgres Company
  • Peter Eisentraut at Sep 28, 2010 at 5:31 pm

    On tis, 2010-09-28 at 12:18 -0400, Tom Lane wrote:
    I'm inclined to think that that's not a fatal objection; it's not like
    we haven't felt free to change psql's output format before. As long as
    we don't back-patch this change, it should be no worse than other things
    we've done to third-party code without a backwards glance.
    In the past, pg_regress used diff -b or -w, so making whitespace changes
    in psql was not a problem.
    It would be good to get rid of this whitespace because (I believe) it is
    one of very few reasons for needing to have any trailing whitespace in
    git-controlled files. If we could get to a point where trailing
    whitespace in patches could be rejected automatically, it'd eliminate
    one small pet peeve.
    You won't be able to programmatically forbid all trailing whitespace (at
    least without additional arrangements) because of:

    psql -c 'select 1 as a, null as b' | cat -A
    a | b$
    ---+---$
    1 | $<===

    Plus, there might be tests that check trailing space behavior or some
    such, but I haven't looked for those.
  • Tom Lane at Sep 28, 2010 at 5:45 pm

    Peter Eisentraut writes:
    On tis, 2010-09-28 at 12:18 -0400, Tom Lane wrote:
    It would be good to get rid of this whitespace because (I believe) it is
    one of very few reasons for needing to have any trailing whitespace in
    git-controlled files. If we could get to a point where trailing
    whitespace in patches could be rejected automatically, it'd eliminate
    one small pet peeve.
    You won't be able to programmatically forbid all trailing whitespace (at
    least without additional arrangements) because of:
    Yeah, if we can't get all the way there easily, the above argument isn't
    worth much.

    regards, tom lane
  • Peter Eisentraut at Sep 28, 2010 at 5:26 pm

    On mån, 2010-09-27 at 11:09 -0700, David Fetter wrote:
    I must be missing something pretty crucial here as far as the
    complexity of changing all the regression tests. Wouldn't trimming
    all trailing whitespace do the trick?
    No, there is trailing whitespace that is significant.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 21, '10 at 6:28p
activeSep 28, '10 at 5:45p
posts18
users8
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase