Re: Lessons from commit fest

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Chris Browne <cbbrowne(at)acm(dot)org>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Lessons from commit fest
Date: 2008-04-16 21:30:18
Message-ID: 200804162130.m3GLUIi08200@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Tom Lane wrote:
> The main problem I see with "pgindent early and often" is that it only
> works well if everyone is using exactly the same pgindent code (and
> exactly the same typedef list). Otherwise you just get buried in
> useless whitespace diffs.
>
> It's bad enough that Bruce whacks around his copy from time to time :-(.
> I would say that the single greatest annoyance for maintaining our back
> branches is that patches tend to not back-patch cleanly, and well over
> half the time it's because of random reformatting done by pgindent
> to code that hadn't changed at all, but it had formatted differently
> the prior year.

I hate to say it but pgindent formatting changes are usually made in
cases where you or the community want pgindent to improve its indenting. :-)
So we improve pgindent but pay for it in backpatching difficulties. :-(

> I guess an advantage of maintaining our own fork is that there'd be Only
> One True pgindent, thereby alleviating that problem; but I'm still not
> eager to go there. If we were going to do it, I'd really wish that we
> could standardize on a version that didn't need a typedef list. The

I don't think that is possible. GNU indent 2.2.9 requires the same -T
option:

You must use the -T option to tell indent the name of all the
type names in your program that are defined by typedef. -T can be
specified more than once, and all names specified are used. For
example, if your program contains

typedef unsigned long CODE_ADDR;
typedef enum {red, blue, green} COLOR;

you would use the options -T CODE_ADDR -T COLOR

astyle doesn't seem to require it but perhaps it just mishandles them.
As I remember there isn't anything about the C grammar that can tell
identify a typedef when it needs to.

> But in any case, this is all focusing on trivialities. The stuff
> pgindent can fix is, by definition, stuff that committers don't really
> have to worry about during patch review. The stuff I'm worried about
> is at a higher level than that.

Agreed. Reformatting is small compared to understanding the patch,
adjusting in the comments, testing, documentation, etc.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2008-04-16 21:33:18 Re: Timely reporting of COPY errors
Previous Message Alvaro Herrera 2008-04-16 21:27:46 Re: printTable API (was: Show INHERIT in \du)