On Mon, Dec 14, 2009 at 7:42 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> There are no changes in this patch that are purely whitespace changes.
> Those were removed prior to patch submission. This is all about code I
> am adding or changing. My additions may disrupt their patches, but not
> because of the whitespace.
>
> If we are going to run pgindent anyway, what is the point? Seems like we
> need a tool to fix patches, not a tool to fix the code.
>
> I've made some changes to the larger files.
I'll try to explain this again because it is a little bit subtle.
Whenever someone commits ANY patch, there is a danger of disrupting
other outstanding patches that touch the same code. That's basically
unavoidable, although certainly it's a reason to avoid superfluous
changes like whitespace adjustments or useless identifier renaming.
However, there's a second, completely independent issue, which is that
eventually we will run pgindent for 8.5, and when we do that, it's
going to reformat stuff, and that reformatting is going to break
things. The amount of reformatting that it does (and therefore the
amount of breakage that it causes) is directly related to the extent
to which people have committed patches throughout the release cycle
that don't conform to the layout that pgident is going to want. If
every patch perfectly matched the pgident style, then the pgindent run
would change nothing and we would all be VERY happy. The more
deviations there are, the more stuff breaks.
So I agree with you: we need a tool to fix patches. However, since we
haven't got one, we owe it to other contributors not to make the
problem any worse than necessary by adhering to the project's
formatting conventions as best we're able when committing things,
especially with regards to trivial things like trailing white-space
that git diff --check can easily find. Actually, git apply has an
option to fix these simple types of problems, so it's possible to fix
it diffing the patch set against the master branch, applying it to a
separate copy of the master branch using git apply --whitespace=fix,
then diffing that against the original batch and applying the changes.Although that's usually overkill unless it's
reallybad.
There's some interesting discussion on whitespace more generally from
Tom Lane here:
http://archives.postgresql.org/pgsql-hackers/2009-08/msg01001.php
...Robert
