git diff --check whitespace checks, gitattributes

Peter Eisentraut <peter_e@gmx.net> writes:

Attached is a patch that
- Adds a .gitattributes file to configure appropriate whitespace checks
for git diff --check.

- Cleans up all whitespace errors found in this way in existing code.
Most of that is in files not covered by pgindent, some in new code since
the last pgindent.

This makes the entire tree git diff --check clean. After this, future
patches can be inspected for whitespace errors with git diff --check,
something that has been discussed on occasion.

I always (well, almost always) do git diff --check, so making it stronger
sounds good to me. But it sounds like this still leaves it to the
committer to remember to run it. Can we do anything about that?

One open question is whether psql output pasted into documentation, in
particular .sgml files, should preserve the trailing whitespace that
psql produces. This is currently done inconsistently.

My preference is to trim the trailing whitespace, because otherwise it's
impossible to check for trailing whitespace errors in other parts of
those files.

Maybe we should think about fixing psql to not generate that whitespace.

regards, tom lane

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

Peter Eisentraut wrote:

This makes the entire tree git diff --check clean. After this, future
patches can be inspected for whitespace errors with git diff --check,
something that has been discussed on occasion.

+1 to this, and also +1 to Tom's suggestion of making it more strongly
enforced.

One open question is whether psql output pasted into documentation, in
particular .sgml files, should preserve the trailing whitespace that
psql produces. This is currently done inconsistently.

I think pasting psql output verbatim isn't such a great idea anyway.
Maybe it's okay for certain specific examples, but in most cases I think
it'd be better to produce native SGML tables instead of <programoutput>
stuff. After all, it's the result set that's interesting, not the way
psql presents it.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On 11/5/13, 10:31 PM, Tom Lane wrote:

I always (well, almost always) do git diff --check, so making it stronger
sounds good to me. But it sounds like this still leaves it to the
committer to remember to run it. Can we do anything about that?

Sure, you could install an update hook on the server. I'm generally not
in favor of such things, but that's a separate discussion. Build
servers could also do this checking.

Maybe we should think about fixing psql to not generate that whitespace.

Not easy, see
/messages/by-id/1285093687.5468.18.camel@vanquo.pezone.net

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

On 11/5/13, 11:17 PM, Alvaro Herrera wrote:

I think pasting psql output verbatim isn't such a great idea anyway.
Maybe it's okay for certain specific examples, but in most cases I think
it'd be better to produce native SGML tables instead of <programoutput>
stuff. After all, it's the result set that's interesting, not the way
psql presents it.

I'm not convinced that that would be better, but it's worth a try.

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

On 11/05/2013 10:31 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Attached is a patch that
- Adds a .gitattributes file to configure appropriate whitespace checks
for git diff --check.
- Cleans up all whitespace errors found in this way in existing code.
Most of that is in files not covered by pgindent, some in new code since
the last pgindent.
This makes the entire tree git diff --check clean. After this, future
patches can be inspected for whitespace errors with git diff --check,
something that has been discussed on occasion.

I always (well, almost always) do git diff --check, so making it stronger
sounds good to me. But it sounds like this still leaves it to the
committer to remember to run it. Can we do anything about that?

Personally I don't get the mania about trailing whitespace, but maybe
that's just me.

We could certainly implement a hook on the server that would reject
cases that fail this test, but we might find it hamstrings us a bit in
future. I'm not sure what else could be done to remedy committer
forgetfulness, though.

cheers

andrew

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

Peter Eisentraut <peter_e@gmx.net> writes:

On 11/5/13, 10:31 PM, Tom Lane wrote:

Maybe we should think about fixing psql to not generate that whitespace.

Not easy, see
/messages/by-id/1285093687.5468.18.camel@vanquo.pezone.net

Ah, thanks for the reminder.  One killer point in that discussion seemed
to be that even if we got rid of the bogus whitespace in result header
lines, there might be legitimate trailing whitespace *in the result data*
--- consider SELECT 'foo '::text as an example.  So we can't install a
blanket prohibition on trailing whitespace in *.out files.  (Not that we
need one anyway, since those aren't hand-edited source; but at the time,
it was not clear --- at least not to me --- that we could apply different
check rules to different files.)

But, returning to the question of psql output pasted into SGML, it's less
clear to me that we shouldn't complain about trailing whitespace in SGML
files. If we suppressed psql's header-line whitespace, we'd greatly ease
copying-and-pasting example output without triggering such warnings. So
that might be a use-case that's sufficient to justify changing what psql
does.

On the third hand, there were also claims in that discussion that
third-party extensions would be annoyed if we changed psql's header
formatting, because they couldn't use the same regression output
files across PG versions. Don't know how much weight to put on that
argument.

regards, tom lane

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

Andrew Dunstan <andrew@dunslane.net> writes:

Personally I don't get the mania about trailing whitespace, but maybe
that's just me.

For me, it's an easily-checked thing that will reduce pgindent noise
later. (Actually I tend to pgindent stuff before committing, these
days, but sometimes that's impractical because somebody's already
committed some not-well-indented stuff elsewhere in the same file.)

We could certainly implement a hook on the server that would reject
cases that fail this test, but we might find it hamstrings us a bit in
future. I'm not sure what else could be done to remedy committer
forgetfulness, though.

I agree that that would not be a good idea.

regards, tom lane

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

Tom Lane wrote:

(Actually I tend to pgindent stuff before committing, these
days, but sometimes that's impractical because somebody's already
committed some not-well-indented stuff elsewhere in the same file.)

What I normally do is commit the changes, then pgindent, then manually
review the pgindent changes (which are normally tiny because my editor
is configured to produce very similar results to pgindent, as I'm sure
yours is.) Then I manually revert the parts not in my commit (also
eased by editor support). Then I squash the commits.

This doesn't take very long.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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