Re: pgindent run? - Mailing list pgsql-hackers

> > 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.
> 
> In FreeBSD we will simply refuse to apply patches that don't at least
> somewhat adhere to our coding standards.  Word of mouth keeps people
> submitting patches that are correct.

Yes, but most individuals can't be bothered to do that all the time, and
automated tools make it much better.

Also, we aren't too big on rejecting patches because they don't meet our
indentation standards.  After they get involved, they start to follow
it, but usually it takes a while for that to happen, and if you enforce
it right away, you risk losing that person.


> > 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.
> 
> This scares me to death, has indent done this before to you?

No, not really.  It is more the size of the diff that scares you.

> If it has, here's a nifty trick someone started doing on our project,
> whenever he came across some file that was terribly formatted (*) he'd
> compile it as is, then copy the object files somewhere else, reformat
> it and then recompile.  He'd then use the md5(1) command to verify
> that in reality nothing had changed.

Yes, I have considered taking the 'postgres' binary and doing a cmp
against the two versions.  I think I did that the first time I ran
pgindent.  You need to skip over the object timestamp headers, but other
than that, it should work.

> > 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.
> 
> This really goes without saying.  I think it would be cool to setup
> a site one day that explains the proper way to contribute to each
> project, I still occasionally get smacked upside the head for doing
> something like putting an RCS/CVS tag in the wrong spot in a file.
> :)

We do have the developers FAQ, which goes into some detail about it,
question #1.

> > 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.
> 
> yeah. :(


> > 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.
> 
> And how exactly does that make sense?  You guys have a long beta period,
> this give people that sit in thier own little corner of the code (OBDC
> for instance) stuck with a major conflict resolution after release when
> they go to add patches they held off on during beta.  You also suddenly
> make the code look completely forien to the contributor... what if he
> has a major issue with the style of pgindent?  It would make a lot more
> sense to explain this up front...
>   "Say, Alfred, I noticed you use two space indents, that's gross,
>    have you run your code through pgindent as explained in the
>    contributor's guide at http://www.postgresql.org/faq/cont....?"

But we have the FAQ item to explain the format.  I don't think we want
people personally formatting code to meet their style between releases
because it discourages others from looking at the code.  We sort of give
them their own format for the release cycle, then reign it back in just
before release.  Seems like a good balance to me.

> 
> > 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.
> 
> Well that's true, but how confident are you that the patch applied to
> the CVS version has the same effect as the -release version?

If there is no offset change, that means the lines are still in the
exact same spot, and no lines were added/removed above, and the code
around the patch hasn't been changed.  There is a chance that stuff
could slip in around that, but we have the beta test cycle and
reviewer's eyes to keep those in check.

--  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