Simon Riggs wrote:
> On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>> <heikki.linnakangas@enterprisedb.com> wrote:
>>> * Please remove any spurious whitespace. "git diff --color" makes them
>>> stand out like a sore thumb, in red. (pgindent will fix them but always
>>> better to fix them before committing, IMO).
>> +1 in general, not particularly for this patch (haven't checked that
>> in this patch).
>>
>> Actually, how about we add that to the page at
>> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>
> If we can define "spurious whitespace" it would help decide whether
> there is any action to take, and when.
There's two definitions that are useful:
- Anything that "git diff --color" shows up as glaring red. Important to
remove simply because the red whitespace is distracting when I review a
patch.
- Anything that pgindent would/will eventually fix. Because you might as
well fix them before committing and save the extra churn and potential
(although trivial) merge conflicts in people's outstanding patches when
pgindent is run. I don't run pgindent myself, so I wouldn't notice most
stuff, but I tend to fix things that I do notice.
> Why is (1) important, and if it is important, why is it being mentioned
> only now? Are we saying that all previous reviewers of my work (and
> others') removed these without ever mentioning they had done so?
I did it in one pass, Oct 15th according to git log.
I tend to silently fix whitespace issues like that in all patches I
commit. It's generally trivial enough to fix that it's easier to just
fix it myself than complain, explain, and look like a real nitpick.
I note that if it was easy to run pgindent yourself on a patch before
committing/submitting, we wouldn't need to have this discussion. I don't
know hard it is to get it working right, but I recall I tried once and
gave up. Any volunteers?
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com