Re: pgindent run? - Mailing list pgsql-hackers

* Bruce Momjian <pgman@candle.pha.pa.us> [010322 06:49] wrote:
> > * 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.

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.

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

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.

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

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.
:)

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

yeah. :(

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

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
codethrough pgindent as explained in the  contributor's guide at http://www.postgresql.org/faq/cont....?"
 

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

-- 
-Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]
Daemon News Magazine in your snail-mail! http://magazine.daemonnews.org/