Обсуждение: git diff --check whitespace checks, gitattributes
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. 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.
Вложения
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
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
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 http://www.postgresql.org/message-id/1285093687.5468.18.camel@vanquo.pezone.net
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.
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
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 > http://www.postgresql.org/message-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
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
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