On 2013-11-07 06:23:13 -0800, Kevin Grittner wrote:
> Andres Freund <andres@2ndquadrant.com> wrote:
>
> > Ok, I've attached a fix for this, which unfortunately is not all
> > that small.
> > Could either of you commit it?
>
> Unfortunately, I don't feel I have a good enough grasp of the
> caching/invalidation mechanism to be a committer for this
> particular patch,
That's understandable.
> but I think you could make it a lot easier to
> review by eliminating some of the whitespace changes. I applied
> your patch and then ran pgindent, which promptly undid some of the
> whitespace changes this patch makes, so there is really no excuse
> for those.
I'll try to produce a pgindent-clean version.
> I think this is one of those cases where the large
> chunk of code inside the new "else" block should not be indented
> with the initial patch -- a pgindent-based whitespace-only patch
> should follow so that the substantive changes here are easier to
> see in the initial patch.
I think commiting code with fundamentally broken indentation like that
is pretty much pointless though. Somebody who wants to look at the
actual changes without the whitespace noise can just use git diff -w/git
blame -w/... to eliminate those while viewing.
> Also, I *really* don't like the
> whitespace and comment placement here:
>
> /* check shared tables */
> if (reltablespace == GLOBALTABLESPACE_OID)
> {
> relid = RelationMapFilenodeToOid(relfilenode, true);
> }
>
> /*
> * Not a shared table, could either be a plain relation or a database
> * specific but nailed one, like e.g. pg_class.
> */
> else
> {
>
> ... which is what results from pgindent acting on this patch.
> Please move the "else" comment inside the opening brace for the
> "else".
Man, I *hate* pgindent. Imo the comment belongs to the outside, not the
inside since it's toplevel logic that matters. Anyway I'll try to clean
it up somehow.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
