Re: Run pgindent now?

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Run pgindent now?
Date: 2015-05-26 18:55:15
Message-ID: CA+TgmobV3GodA1C_WW2t26EHQTv_jqwKc7+ujziLgKRw3C2AiQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, May 25, 2015 at 5:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote:
>>> Something is wrong. See aclchk.c changes.
>
>> Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0
>> and 9.1, and the use of that as a typedef in 9.4 is certainly odd:
>
>> - aclitem.ai_grantor = grantorId;
>> + aclitem. ai_grantor = grantorId;
>
> Yeah. I think we might've gotten rid of that typedef partially in order
> to fix this.
>
> A different strategy we could consider is "use HEAD's typedef list
> even in the back branches". This would in some situations lead to
> inferior-looking results in the back branches, but that's probably better
> than inferior results in HEAD. (In any case, we want the same typedef
> list across all branches. Then anyplace where the results diverge, there
> must have been non-pgindent code changes, so that back-patching would
> require manual fixups anyway.)

This is kind of why I think that reindenting the back branches is
unlikely to be productive: it only helps if you can get pgindent to do
the same thing on all branches, and I bet that's going to be tough.

Realistically, with merge.conflictstyle = diff3 (why is this not the
default?), resolving whitespace conflicts that occur when you try to
cherry-pick is typically not very difficult. But every time we
pgindent, especially with slightly different settings, we cause tools
like 'git blame' to return less useful answers. And that sucks.

We also risk breaking private patchsets that people are carrying - of
course, Advanced Server is one (very large) such patchset, but it's
hardly the only place where people are compiling a non-standard
distribution that has to be reconciled with upstream changes.

I'm not going to put up a huge fuss if we decide to go ahead with
this, but I still think it's a bad plan, especially with regarding to
existing branches that have not been re-indented in years. I bet it
won't save that much back-patching effort - maybe not any, on net -
and I bet it will inconvenience users and developers in various subtle
ways that we may not even hear about but which are still quite real.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Josh Berkus 2015-05-26 18:56:22 Re: ERROR: MultiXactId xxxx has not been created yet -- apparent wraparound
Previous Message Alvaro Herrera 2015-05-26 18:47:36 Re: ERROR: MultiXactId xxxx has not been created yet -- apparent wraparound