Re: pgindent run? - Mailing list pgsql-hackers

> * Bruce Momjian <pgman@candle.pha.pa.us> [010321 21:14] wrote:
> > > The Hermit Hacker <scrappy@hub.org> writes:
> > > > and most times, those have to be merged into the source tree due to
> > > > extensive changes anyway ... maybe we should just get rid of the use of
> > > > pgindent altogether?
> > > 
> > > I think pgindent is a good thing; the style of different parts of the
> > > code would vary too much without it.  I'm only unhappy about the risk
> > > issues of running it at this late stage of the release cycle.
> > 
> > This is the usual?discussion.  Some like it, some don't like the risk,
> > some don't like the timing.  I don't think we ever came up with a better
> > time than before RC, though I think we could do it a little earlier in
> > beta if people were not holding patches during that period.  It is the
> > beta patching folks that we have the most control over.
> 
> It seems that you guys are dead set on using this pgindent tool,
> this is cool, we'd probably use some indentation tool on the FreeBSD
> sources if there was one that met our code style(9) guidelines.

You don't notice the value of pgindent until you have some code that
hasn't been run through it.  For example, ODBC was not run through until
this release, and I had a terrible time trying to understand the code
because it didn't _look_ like the rest of the code.  Now that pgindent
is run, it looks more normal, and I am sure that will encourage more
people to get in and make changes.

It gives more of a "I know where I am" feeling to the code.  It
certainly doesn't make anything possible that wasn't possible before,
but it does encourage people, and that is pretty powerful.

I can't tell you how many times I have had to fix someone's contributed
code that hadn't been through pgindent yet, and the problems I had
trying to understand it.  I have even copied the file, pgindented it,
read through the copy, then went back and fixed the original code.  (I
can't run pgindent during development cycle, only when I have 100%
control over the code and outstanding patches people may have.)

As far as FreeBSD, I guarantee you will see major benefits to community
participation by running the script.  You will have to hand-review all
the changes after the first run to make sure it didn't wack out some
wierd piece of code, but after that you will be pretty OK.  The only
issue is that the person who takes this on is a taking a major risk of
exposure to ridicule if it fails.

I remember doing it the first time, and being really scared I would
lethally brake the code.  Indenting is one of those efforts that has
this _big_ risk componient when it is performed, and you get the small,
steady benefit of doing it for months after.

> With that said, I really scares the crud out of me to see those massive
> pg_indent runs right before you guys do a release.
> 
> It would make a lot more sense to force a pgindent run after applying
> each patch.  This way you don't loose the history.


Yes, we have considered that.  The problem there is that sometimes
people supply a patch, do some more work on their production source,
then supply other patches to fix new problems.  If we pgindent for every
CVS commit, we then are changing the supplied patch, which means any new
patches that person sends do not match their previous patch, and we get
into hand edits again.

I know we ask for context diff's, but anytime a patch applies with some
offset, if the offset is large, I have to make sure there wasn't some
other identical context of code that may have been found by the patch
program and applied incorrectly.

A silent patch apply is safe;  if it reports a large offset, I have to
investigate.

> You want to be upset with yourself Bruce?  Go into a directory and type:
> 
> cvs annotate <any file that's been pgindented>
> 
> cvs annotate is a really, really handy tool, unfortunetly these
> indent runs remove this very useful tool as well as do a major job
> of obfuscating the code changes.

I have never seen that feature.  I don't even see it in my cvs manual
page.  It is great, and yes, I clearly wack that out for pgindent runs.
Maybe pgindent for every commit is the way to go.

> It's not like you guys have a massive devel team with new people each
> week that have a steep committer learning curve ahead of them, making
> pgindent as patches are applied should work.

I imagine we can get CVS to do that automatically.  The number of patch
on top of another patch is pretty rare and it would solve the other
stated problems.


> There's also the argument that a developer's pgindent may force a
> contributor to resolve conflicts, while this is true, it's also
> true that you guys expect diffs to be in context format, comments
> to be in english, function prototypes to be new style, etc, etc..
> 
> I think contributors can deal with this.

If someone submits a massive patch, and we apply it, all patches after
that that they give us will not apply cleanly because they still have
the old format.  The other argument for not doing pgindent on cvs commit
is that if someone is working in an area of the code, they should be
able to format that code as they like to see it.  They may be working in
there for months.  Only during release are is their _style_ removed.

On a side note, the idea of having people submit patches only against
current CVS seems bad to me.  If people are running production machines
and they develop a patch and test it there, I want the patch that works
on their machine and can make sure it applies here.  Having them
download CVS and do the merge themselves seems really risky, especially
because they probably can't test the CVS in production.  The CVS may not
even run properly.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026