Thread: pgindent run?
With RC1 nearing, when should I run pgindent? This is usually the time I do it. -- 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
> With RC1 nearing, when should I run pgindent? This is usually the time > I do it. Does the silence mean I should pick a date to run this? -- 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
> Bruce Momjian writes: > > > With RC1 nearing, when should I run pgindent? This is usually the time > > I do it. > > Are there any severely mis-indented files? Not sure. I think there are some. It doesn't do anything unless there is mis-indenting, so it is pretty safe and has always been done in the past. It obviously only affects new changes since the last run. -- 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
On Wed, 21 Mar 2001, Bruce Momjian wrote: > > With RC1 nearing, when should I run pgindent? This is usually the time > > I do it. > > Does the silence mean I should pick a date to run this? Since I'm going to end up re-rolling RC1, do a run tonight on her, so that any problems that arise from pgindent this time can be caught with those testing RC1 ...
Bruce Momjian writes: > With RC1 nearing, when should I run pgindent? This is usually the time > I do it. Are there any severely mis-indented files? -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
> On Wed, 21 Mar 2001, Bruce Momjian wrote: > > > > With RC1 nearing, when should I run pgindent? This is usually the time > > > I do it. > > > > Does the silence mean I should pick a date to run this? > > Since I'm going to end up re-rolling RC1, do a run tonight on her, so that > any problems that arise from pgindent this time can be caught with those > testing RC1 ... Good idea. It is well tested, but you never know. Peter, this is the optimial time to do it because no one has any outstanding patches at this point. Seems this is the only good time. Unless someone says otherwise, I will do the run tonight. -- 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
Bruce Momjian writes: > Peter, this is the optimial time to do it because no one has any > outstanding patches at this point. Seems this is the only good time. Actually, I have quite a few outstanding patches. I got screwed by this last time around as well. But I understand that this might be the best time. -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
> Bruce Momjian writes: > > > Peter, this is the optimial time to do it because no one has any > > outstanding patches at this point. Seems this is the only good time. > > Actually, I have quite a few outstanding patches. I got screwed by this > last time around as well. But I understand that this might be the best > time. That you are holding? Yes, I have a few to at my new Unapplied Patches web page: http://candle.pha.pa.us/cgi-bin/pgpatches The good news is that these will apply fine to 7.2 unless they touch an area that needed indenting. The problem of not doing it is that the code starts to look different after a while and takes on a chaotic feel. This is probably the time when there are the fewest oustanding patches, I guess. -- 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
OK, I am going to have dinner and then get started on the pgindent run. I have also noticed we have some comments like: /* ---- * one word * ---- */ that look funny in a few places. I propose: /* one word */ to be consistent. > With RC1 nearing, when should I run pgindent? This is usually the time > I do it. > > -- > 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, Pennsylvania 19026 > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://www.postgresql.org/search.mpl > -- 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
On Wed, 21 Mar 2001, Bruce Momjian wrote: > > OK, I am going to have dinner and then get started on the pgindent run. > > I have also noticed we have some comments like: > > /* ---- > * one word > * ---- > */ > > that look funny in a few places. I propose: > > /* one word */ > > to be consistent. to be consistent with what ... ? isn't: /* ----------* comment* ----------*/ the standard? > > > > With RC1 nearing, when should I run pgindent? This is usually the time > > I do it. > > > > -- > > 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, Pennsylvania 19026 > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 6: Have you searched our list archives? > > > > http://www.postgresql.org/search.mpl > > > > > -- > 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, Pennsylvania 19026 > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy Systems Administrator @ hub.org primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
> On Wed, 21 Mar 2001, Bruce Momjian wrote: > > > > > OK, I am going to have dinner and then get started on the pgindent run. > > > > I have also noticed we have some comments like: > > > > /* ---- > > * one word > > * ---- > > */ > > > > that look funny in a few places. I propose: > > > > /* one word */ > > > > to be consistent. > > to be consistent with what ... ? isn't: > > /* ---------- > * comment > * ---------- > */ > > the standard? Sorry. It has been a while since I studied this. The issue is the dashes, not the block comments. /* --- is needed for multi-line comment where you want to preserve the layout, but in other cases, it prevents comment layout and looks kind of heavy. I eyeball each change to make sure it is clean so: /* --- * test * --- */ becomes the cleaner: /* * test */ This makes the comment easier to read. -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> With RC1 nearing, when should I run pgindent? This is usually the time
>> I do it.
> Does the silence mean I should pick a date to run this?
If you're going to do it before the release, I think you should do it
*before* we wrap RC1. I've said before and will say again that I think
it's utter folly to run pgindent at the conclusion of the test cycle.
I've been around this project for three major release cycles and we have
seen errors introduced by pgindent in two of them. I don't trust
pgindent to be bug-free and I don't believe you should either.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> With RC1 nearing, when should I run pgindent? This is usually the time > >> I do it. > > > Does the silence mean I should pick a date to run this? > > If you're going to do it before the release, I think you should do it > *before* we wrap RC1. I've said before and will say again that I think > it's utter folly to run pgindent at the conclusion of the test cycle. > I've been around this project for three major release cycles and we have > seen errors introduced by pgindent in two of them. I don't trust > pgindent to be bug-free and I don't believe you should either. OK, running now. Should I run it at another time or never? -- 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
>> Are there any severely mis-indented files?
There are some new contrib modules that are nowhere close to our
indent conventions; also a good deal of foreign-key-related stuff
in the parser that needs to be cleaned up. So we should run it.
I've always felt that it'd be smarter to run pgindent at the start
of a development cycle, not the end, but I've been unable to convince
Bruce of that ...
regards, tom lane
> >> Are there any severely mis-indented files? > > There are some new contrib modules that are nowhere close to our > indent conventions; also a good deal of foreign-key-related stuff > in the parser that needs to be cleaned up. So we should run it. > > I've always felt that it'd be smarter to run pgindent at the start > of a development cycle, not the end, but I've been unable to convince > Bruce of that ... Hey, I am open to whatever people want to do. Just remember that we accumulate lots of patches/development during the slow time before development, and those patches become harder to apply. Peter E has some already. -- 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
* Bruce Momjian <pgman@candle.pha.pa.us> [010321 22:11]: > > >> Are there any severely mis-indented files? > > > > There are some new contrib modules that are nowhere close to our > > indent conventions; also a good deal of foreign-key-related stuff > > in the parser that needs to be cleaned up. So we should run it. > > > > I've always felt that it'd be smarter to run pgindent at the start > > of a development cycle, not the end, but I've been unable to convince > > Bruce of that ... > > Hey, I am open to whatever people want to do. Just remember that we > accumulate lots of patches/development during the slow time before > development, and those patches become harder to apply. Peter E has some > already. How about: 1) just AFTER release 2) just BEFORE Beta LER > > -- > 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, Pennsylvania 19026 > > ---------------------------(end of broadcast)--------------------------- > TIP 3: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: ler@lerctr.org US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749
On Wed, 21 Mar 2001, Bruce Momjian wrote: > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > >> With RC1 nearing, when should I run pgindent? This is usually the time > > >> I do it. > > > > > Does the silence mean I should pick a date to run this? > > > > If you're going to do it before the release, I think you should do it > > *before* we wrap RC1. I've said before and will say again that I think > > it's utter folly to run pgindent at the conclusion of the test cycle. > > I've been around this project for three major release cycles and we have > > seen errors introduced by pgindent in two of them. I don't trust > > pgindent to be bug-free and I don't believe you should either. > > OK, running now. Should I run it at another time or never? I'll put my vote on Tom's side of things ... run if after the release, right at the start of the next development cycle, so that any bugs that crop up aren't just as we are trying to release ... Hell, maybe once then and once *just* as we are going into first beta of a release ... Tom?
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Hey, I am open to whatever people want to do. Just remember that we
> accumulate lots of patches/development during the slow time before
> development, and those patches become harder to apply. Peter E has some
> already.
Why not start a devel cycle by (a) branching the tree, (b) applying
all held-over patches, and then (c) running pgindent?
I'd probably wait a week or so between (a) and (c) to let people push
in whatever they have pending. But in general it seems a lot safer
to pgindent at the front end of the cycle not the back end.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Hey, I am open to whatever people want to do. Just remember that we > > accumulate lots of patches/development during the slow time before > > development, and those patches become harder to apply. Peter E has some > > already. > > Why not start a devel cycle by (a) branching the tree, (b) applying > all held-over patches, and then (c) running pgindent? If people can get their patches in all at one time, that would work. The only problem there is that people who supply patches against 7.1 will not match the 7.2 tree, and we get those patches from people for months. -- 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
> > If people can get their patches in all at one time, that would work. > > The only problem there is that people who supply patches against 7.1 > > will not match the 7.2 tree, and we get those patches from people for > > months. > > and those patches should only be applied to the v7.1 branch ... we are > suggesting (or, at least, I am) is that you pgindent *HEAD* after we've > branched off v7.1 ... > > ... that way, we go into the new dev cycle "clean", but we doon't mess up > the *STABLE* tree ... But we get patches from 7.0.X now that get applied to 7.1. All our developers are not working on CVS trees. Many use their main trees and send in the occasional patch. -- 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
On Wed, 21 Mar 2001, Bruce Momjian wrote: > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > Hey, I am open to whatever people want to do. Just remember that we > > > accumulate lots of patches/development during the slow time before > > > development, and those patches become harder to apply. Peter E has some > > > already. > > > > Why not start a devel cycle by (a) branching the tree, (b) applying > > all held-over patches, and then (c) running pgindent? > > If people can get their patches in all at one time, that would work. > The only problem there is that people who supply patches against 7.1 > will not match the 7.2 tree, and we get those patches from people for > months. and those patches should only be applied to the v7.1 branch ... we are suggesting (or, at least, I am) is that you pgindent *HEAD* after we've branched off v7.1 ... ... that way, we go into the new dev cycle "clean", but we doon't mess up the *STABLE* tree ...
On Wed, 21 Mar 2001, Bruce Momjian wrote: > > > If people can get their patches in all at one time, that would work. > > > The only problem there is that people who supply patches against 7.1 > > > will not match the 7.2 tree, and we get those patches from people for > > > months. > > > > and those patches should only be applied to the v7.1 branch ... we are > > suggesting (or, at least, I am) is that you pgindent *HEAD* after we've > > branched off v7.1 ... > > > > ... that way, we go into the new dev cycle "clean", but we doon't mess up > > the *STABLE* tree ... > > But we get patches from 7.0.X now that get applied to 7.1. All our > developers are not working on CVS trees. Many use their main trees and > send in the occasional patch. 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? its not something that I've ever seen required on other projects I've worked on ... in general, most projects seem to require that a submit'd patch from an older release be at least tested on the newest CVS, and with nightly snapshots being created as it is, I really don't see why such a requirement is a bad thing ...
> 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? its not something that I've ever seen required on > other projects I've worked on ... in general, most projects seem to > require that a submit'd patch from an older release be at least tested on > the newest CVS, and with nightly snapshots being created as it is, I > really don't see why such a requirement is a bad thing ... In an ideal world, people would test on CVS but in reality, the patches are usually pretty small and if they fix the problem, we apply them. Seems like a lot of work just to avoid pgindent. -- 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
On Wed, 21 Mar 2001, Bruce Momjian wrote: > > 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? its not something that I've ever seen required on > > other projects I've worked on ... in general, most projects seem to > > require that a submit'd patch from an older release be at least tested on > > the newest CVS, and with nightly snapshots being created as it is, I > > really don't see why such a requirement is a bad thing ... > > In an ideal world, people would test on CVS but in reality, the patches > are usually pretty small and if they fix the problem, we apply them. > > Seems like a lot of work just to avoid pgindent. If they are small, then why is pgindent required? And if they are large, is it too much to ask that the person submitting tests the patch to make sure its even applicable in the newest snapshot?
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.
regards, tom lane
> > > 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? its not something that I've ever seen required on > > > other projects I've worked on ... in general, most projects seem to > > > require that a submit'd patch from an older release be at least tested on > > > the newest CVS, and with nightly snapshots being created as it is, I > > > really don't see why such a requirement is a bad thing ... > > > > In an ideal world, people would test on CVS but in reality, the patches > > are usually pretty small and if they fix the problem, we apply them. > > > > Seems like a lot of work just to avoid pgindent. > > If they are small, then why is pgindent required? And if they are large, > is it too much to ask that the person submitting tests the patch to make > sure its even applicable in the newest snapshot? The problem is that the small ones don't apply cleanly if they don't match the indenting in the source. -- 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
> 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. -- 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
On Thu, 22 Mar 2001, Bruce Momjian wrote: > > > > 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? its not something that I've ever seen required on > > > > other projects I've worked on ... in general, most projects seem to > > > > require that a submit'd patch from an older release be at least tested on > > > > the newest CVS, and with nightly snapshots being created as it is, I > > > > really don't see why such a requirement is a bad thing ... > > > > > > In an ideal world, people would test on CVS but in reality, the patches > > > are usually pretty small and if they fix the problem, we apply them. > > > > > > Seems like a lot of work just to avoid pgindent. > > > > If they are small, then why is pgindent required? And if they are large, > > is it too much to ask that the person submitting tests the patch to make > > sure its even applicable in the newest snapshot? > > The problem is that the small ones don't apply cleanly if they don't > match the indenting in the source. but ... if they are small, manually merging isn't that big of a deal ... and if anyone else has been working in that code since release, there is a chance it won't mergef cleanly ... Quite frankly, I'm for pgindent after branch and before beta ...
> > The problem is that the small ones don't apply cleanly if they don't > > match the indenting in the source. > > but ... if they are small, manually merging isn't that big of a deal ... > and if anyone else has been working in that code since release, there is a > chance it won't mergef cleanly ... Yes, they can be manually merged, but that is much more error-prone than pgindent itself, at least with me patching them. :-) Yes, I agree there is a risk. I was quite scared the first time I ran it on the tree and did the commit. At this point, there are very few changes to it, so I feel a little better, and the stuff gets caught somehow if there is a problem. -- 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
* 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. 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. 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. 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. 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. just my usual 20 cents. :) -- -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org] Daemon News Magazine in your snail-mail! http://magazine.daemonnews.org/
> * 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
Alfred Perlstein <bright@wintelcom.net> writes:
> 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 think this is a good reason for *not* applying pgindent on an
incremental basis, but only once per release cycle. That at least
lets cvs annotate be useful within a cycle.
regards, tom lane
> 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. I would liken running pgindent to having a nice looking store or website. No one is going to go to a website or a store only because it looks nice, but having it look nice does keep people coming back. -- 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
> Alfred Perlstein <bright@wintelcom.net> writes: > > 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 think this is a good reason for *not* applying pgindent on an > incremental basis, but only once per release cycle. That at least > lets cvs annotate be useful within a cycle. What about having it happen on every CVS commit? -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Alfred Perlstein <bright@wintelcom.net> writes:
> 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 think this is a good reason for *not* applying pgindent on an
>> incremental basis, but only once per release cycle. That at least
>> lets cvs annotate be useful within a cycle.
> What about having it happen on every CVS commit?
Try that and you'll get all-out war. It's tough enough keeping in sync
with the repository already.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Alfred Perlstein <bright@wintelcom.net> writes: > > 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 think this is a good reason for *not* applying pgindent on an > >> incremental basis, but only once per release cycle. That at least > >> lets cvs annotate be useful within a cycle. > > > What about having it happen on every CVS commit? > > Try that and you'll get all-out war. It's tough enough keeping in sync > with the repository already. Oh, well. I tried. -- 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
Bruce Momjian writes: > > >> Are there any severely mis-indented files? > > > > There are some new contrib modules that are nowhere close to our > > indent conventions; also a good deal of foreign-key-related stuff > > in the parser that needs to be cleaned up. So we should run it. > > > > I've always felt that it'd be smarter to run pgindent at the start > > of a development cycle, not the end, but I've been unable to convince > > Bruce of that ... > > Hey, I am open to whatever people want to do. Just remember that we > accumulate lots of patches/development during the slow time before > development, and those patches become harder to apply. Peter E has some > already. This argument seems irrelevant when given the choice of before release or after start of new cycle. From an analytical point of view (and slightly idealized), these are limits approaching the same points in time, from opposite sides. Thus, the second choice seems the infinitely better option. -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
> > Hey, I am open to whatever people want to do. Just remember that we > > accumulate lots of patches/development during the slow time before > > development, and those patches become harder to apply. Peter E has some > > already. > > This argument seems irrelevant when given the choice of before release or > after start of new cycle. From an analytical point of view (and slightly > idealized), these are limits approaching the same points in time, from > opposite sides. Thus, the second choice seems the infinitely better > option. Yes, the bigger problem is people running our most recent stable release not matching the current CVS sources. I think early beta is the time to do this next time. That has the fewest patches crossing over time. In 7.1, we had the WAL patches still being worked on until near the end, but once Tom put that mega-patch in last week, we could have done it then. -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I think early beta is the time to
> do this next time. That has the fewest patches crossing over time.
That would work too, particularly if you give people a few days' notice.
("Get your patches in now, or expect to have to reformat...")
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I think early beta is the time to
> > do this next time. That has the fewest patches crossing over time.
>
> That would work too, particularly if you give people a few days' notice.
> ("Get your patches in now, or expect to have to reformat...")
Yes, I did try that in a previous release and got the, "Oh, I am still
working on X." I will be more insistent in the future that we get this
done earlier.
-- 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
* 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/
> > 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
* Bruce Momjian <pgman@candle.pha.pa.us> [010322 07:12] wrote: > > 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. > > I would liken running pgindent to having a nice looking store or > website. No one is going to go to a website or a store only because it > looks nice, but having it look nice does keep people coming back. I'm not saying I don't like pgindent, I'm saying I don't like pgindent's effect on the CVS history. -- -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org] Instead of asking why a piece of software is using "1970s technology," start asking why software is ignoring 30 years of accumulated wisdom.
> * Bruce Momjian <pgman@candle.pha.pa.us> [010322 07:12] wrote: > > > 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. > > > > I would liken running pgindent to having a nice looking store or > > website. No one is going to go to a website or a store only because it > > looks nice, but having it look nice does keep people coming back. > > I'm not saying I don't like pgindent, I'm saying I don't like > pgindent's effect on the CVS history. How do we get around this problem? Also, I now remember that the problem with CVS auto-pgindenting, as Tom mentioned, is that once you cvs commit, you would have to cvs update again because your source tree wouldn't match cvs anymore. -- 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
Bruce Momjian wrote: > > 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. > I see now the following comment in interfaces/odbc/statement.c. Though it's mine(probably), it's hard for me to read. Please tell me how to prevent pgindent from changing comments. /* * Basically we don't have to begin a transaction in autocommit mode * because Postgres backend runs in autocomit mode.We issue "BEGIN" * in the following cases. 1) we use declare/fetch and the statement * is SELECT (because declare/fetchmust be called in a transaction). * 2) we are in autocommit off state and the statement isn't of type * OTHER.*/ regards, Hiroshi Inoue
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Please tell me how to prevent pgindent from changing
> comments.
Put dashes at the start and end of the comment block, eg
/*---------- * comment here *---------- */
I'm not sure exactly how many dashes are needed --- I usually use ten as
shown above.
regards, tom lane
> Bruce Momjian wrote: > > > > 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. > > > > I see now the following comment in interfaces/odbc/statement.c. > Though it's mine(probably), it's hard for me to read. > Please tell me how to prevent pgindent from changing > comments. > > /* > * Basically we don't have to begin a transaction in autocommit mode > * because Postgres backend runs in autocomit mode. We issue "BEGIN" > * in the following cases. 1) we use declare/fetch and the statement > * is SELECT (because declare/fetch must be called in a transaction). > * 2) we are in autocommit off state and the statement isn't of type > * OTHER. > */ Sorry that happened. It is mentioned in the developer's FAQ that the dashes prevent wrapping. I know it is hard to remember even if you know it, and I would be glad to fix any comments that look bad. -- 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
Greetings, I have been following along with the thread and would just like to say a few paragraphs. Can't the contributors themselves run pgindent on the files which they have changed _just_ before creating the patch which is to be contributed? Thus, patching a "pure" pgindented file from cvs with another "pure" file created loacally will allow for the contributor to work with their own ideosyncratic indentation style, yet at the same time keep the pgindentification of the cvs tree "pure". Otherwise the alternative is to insist that people code according to the project's standards, which is really just another application of the slogans; "When in Rome do as Rome does." and "There is no "I" in "TEAM." If there is one thing the Mozilla project has done well, it is the creation of all the tools to make cooperative coding simpler. As the PostgreSQL grows in complexity and following you might find that these tools are helpful. -- Sincerely etc., NAME Christopher SawtellCELL PHONE 021 257 4451ICQ UIN 45863470EMAIL csawtell @ xtra . co . nzCNOTES ftp://ftp.funet.fi/pub/languages/C/tutorials/sawtell_C.tar.gz -->> Please refrain from using HTML or WORD attachments in e-mails to me <<--
Christopher Sawtell <csawtell@xtra.co.nz> writes:
> Can't the contributors themselves run pgindent on the files which they
> have changed _just_ before creating the patch which is to be contributed?
That would require everyone to have a working copy of BSD indent (gnu
indent does not behave the same, btw). Doesn't seem real workable.
It'd be nice if we had a more portable/widespread tool, but that's not
a very high priority, at least not for yours truly.
regards, tom lane