Horton

Thread

  1. trailing whitespace in psql table output

    Peter Eisentraut <peter_e@gmx.net> — 2010-09-21T18:28:07Z

    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.

  2. Re: trailing whitespace in psql table output

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2010-09-22T00:48:12Z

    On Wed, Sep 22, 2010 at 3:28 AM, Peter Eisentraut <peter_e@gmx.net> 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

  3. Re: trailing whitespace in psql table output

    David Fetter <david@fetter.org> — 2010-09-22T02:02:45Z

    On Wed, Sep 22, 2010 at 09:48:12AM +0900, Itagaki Takahiro wrote:
> On Wed, Sep 22, 2010 at 3:28 AM, Peter Eisentraut <peter_e@gmx.net> 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

  4. Re: trailing whitespace in psql table output

    Roger Leigh <rleigh@codelibre.net> — 2010-09-24T21:38:24Z

    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.

  5. Re: trailing whitespace in psql table output

    Peter Eisentraut <peter_e@gmx.net> — 2010-09-27T12:05:11Z

    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.

  6. Re: trailing whitespace in psql table output

    David Wheeler <david@kineticode.com> — 2010-09-27T16:25:31Z

    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

  7. Re: trailing whitespace in psql table output

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-09-27T17:34:00Z

    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

  8. Re: trailing whitespace in psql table output

    Robert Haas <robertmhaas@gmail.com> — 2010-09-27T17:53:45Z

    On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera
<alvherre@commandprompt.com> 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

  9. Re: trailing whitespace in psql table output

    David Fetter <david@fetter.org> — 2010-09-27T18:09:04Z

    On Mon, Sep 27, 2010 at 01:53:45PM -0400, Robert Haas wrote:
> On Mon, Sep 27, 2010 at 1:34 PM, Alvaro Herrera
> <alvherre@commandprompt.com> 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

  10. Re: trailing whitespace in psql table output

    Robert Haas <robertmhaas@gmail.com> — 2010-09-27T19:11:07Z

    On Mon, Sep 27, 2010 at 2:09 PM, David Fetter <david@fetter.org> 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
>> <alvherre@commandprompt.com> 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

  11. Re: trailing whitespace in psql table output

    David Fetter <david@fetter.org> — 2010-09-27T20:12:08Z

    On Mon, Sep 27, 2010 at 03:11:07PM -0400, Robert Haas wrote:
> On Mon, Sep 27, 2010 at 2:09 PM, David Fetter <david@fetter.org> 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
> >> <alvherre@commandprompt.com> 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

  12. Re: trailing whitespace in psql table output

    Robert Haas <robertmhaas@gmail.com> — 2010-09-27T20:19:09Z

    On Mon, Sep 27, 2010 at 4:12 PM, David Fetter <david@fetter.org> 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 <david@fetter.org> 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
>> >> <alvherre@commandprompt.com> 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

  13. Re: trailing whitespace in psql table output

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-09-28T16:18:59Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Sep 27, 2010 at 2:09 PM, David Fetter <david@fetter.org> 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

  14. Re: trailing whitespace in psql table output

    Robert Haas <robertmhaas@gmail.com> — 2010-09-28T16:23:37Z

    On Tue, Sep 28, 2010 at 12:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Sep 27, 2010 at 2:09 PM, David Fetter <david@fetter.org> 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

  15. Re: trailing whitespace in psql table output

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-09-28T17:06:28Z

    David Fetter <david@fetter.org> 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

  16. Re: trailing whitespace in psql table output

    Peter Eisentraut <peter_e@gmx.net> — 2010-09-28T17:26:14Z

    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.

  17. Re: trailing whitespace in psql table output

    Peter Eisentraut <peter_e@gmx.net> — 2010-09-28T17:31:17Z

    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.

  18. Re: trailing whitespace in psql table output

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-09-28T17:44:47Z

    Peter Eisentraut <peter_e@gmx.net> 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