Обсуждение: trailing whitespace in psql table output
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.
Вложения
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
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
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.
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.
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
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
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
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
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
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
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
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
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
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
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.
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 -Aa | b$ ---+---$1 | $<=== Plus, there might be tests that check trailing space behavior or some such, but I haven't looked for those.
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