Thread: Run pgindent now?
With feature freeze behind us, I'd like to propose that now is a good
time for a pgindent run. It's possible we'd need another one before
9.5 is branched off from HEAD, but a run now ought to take care of 95%
of the cleanup needed. I see a couple of advantages to doing it now:
1. Patches that are now postponed to 9.6 will need to be rebased against
pgindented sources anyway. Might as well give their authors more time
for that rather than less.
2. Code that matches the project layout conventions is easier to read
and review, IMO (not that I'm especially in love with the pgindent
layout, but I'm used to it). Also any issues like commit d678bde65
would be taken care of, which is an objective reason why reviewing is
easier.
The only significant downside I can think of is that, if we determine
that any of the recent feature additions are so sketchy that they need
to be reverted, having to undo just a portion of the pgindent commit
before reverting the feature commit would be a pain. But I don't think
we should optimize on the assumption that that will happen.
regards, tom lane
On Sat, May 16, 2015 at 5:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
With feature freeze behind us, I'd like to propose that now is a good
time for a pgindent run. It's possible we'd need another one before
9.5 is branched off from HEAD, but a run now ought to take care of 95%
of the cleanup needed. I see a couple of advantages to doing it now:
1. Patches that are now postponed to 9.6 will need to be rebased against
pgindented sources anyway. Might as well give their authors more time
for that rather than less.
2. Code that matches the project layout conventions is easier to read
and review, IMO (not that I'm especially in love with the pgindent
layout, but I'm used to it). Also any issues like commit d678bde65
would be taken care of, which is an objective reason why reviewing is
easier.
The only significant downside I can think of is that, if we determine
that any of the recent feature additions are so sketchy that they need
to be reverted, having to undo just a portion of the pgindent commit
before reverting the feature commit would be a pain. But I don't think
we should optimize on the assumption that that will happen.
+1, except I suggest we at least delay it until we have wrapped the new minor releases, to make sure we don't conflict with any backpatching there.
On Sat, May 16, 2015 at 11:58:59AM -0400, Tom Lane wrote: > With feature freeze behind us, I'd like to propose that now is a good > time for a pgindent run. It's possible we'd need another one before > 9.5 is branched off from HEAD, but a run now ought to take care of 95% > of the cleanup needed. I see a couple of advantages to doing it now: +1 to Magnus's suggestion of doing it after the minor release wrap. > The only significant downside I can think of is that, if we determine > that any of the recent feature additions are so sketchy that they need > to be reverted, having to undo just a portion of the pgindent commit > before reverting the feature commit would be a pain. But I don't think > we should optimize on the assumption that that will happen. Quite so; we'd have lost our way to be optimizing for that.
Magnus Hagander <magnus@hagander.net> writes:
> On Sat, May 16, 2015 at 5:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> With feature freeze behind us, I'd like to propose that now is a good
>> time for a pgindent run.
> +1, except I suggest we at least delay it until we have wrapped the new
> minor releases, to make sure we don't conflict with any backpatching there.
Sure, a couple of days won't make much difference.
regards, tom lane
On Sat, May 16, 2015 at 01:05:27PM -0400, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > On Sat, May 16, 2015 at 5:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> With feature freeze behind us, I'd like to propose that now is a good > >> time for a pgindent run. > > > +1, except I suggest we at least delay it until we have wrapped the new > > minor releases, to make sure we don't conflict with any backpatching there. > > Sure, a couple of days won't make much difference. There was talk last time of pgindent-ing head and all back branches, because a patch applied to head and back branches was historically only pgindented in head, meaning that any future patches in that area could not be easily backpatched. Do we want to do this? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, May 18, 2015 at 12:10 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Sat, May 16, 2015 at 01:05:27PM -0400, Tom Lane wrote: >> Magnus Hagander <magnus@hagander.net> writes: >> > On Sat, May 16, 2015 at 5:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> With feature freeze behind us, I'd like to propose that now is a good >> >> time for a pgindent run. >> >> > +1, except I suggest we at least delay it until we have wrapped the new >> > minor releases, to make sure we don't conflict with any backpatching there. >> >> Sure, a couple of days won't make much difference. > > There was talk last time of pgindent-ing head and all back branches, > because a patch applied to head and back branches was historically only > pgindented in head, meaning that any future patches in that area could > not be easily backpatched. > > Do we want to do this? I am personally not excited about that. I would rather leave the back-branches alone. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 18, 2015 at 12:10 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> There was talk last time of pgindent-ing head and all back branches,
>> because a patch applied to head and back branches was historically only
>> pgindented in head, meaning that any future patches in that area could
>> not be easily backpatched.
>>
>> Do we want to do this?
> I am personally not excited about that. I would rather leave the
> back-branches alone.
It would be awfully nice though if we didn't have to deal with random
cross-branch indenting differences. I've lost, maybe not years off my
life, but certainly weeks of not-very-pleasant make-work because of that.
I'm surprised you've not had the same experience.
If people were good about pgindenting patches meant to be back-patched
*before* they committed, it would not be such an issue, but they're not
very good about that.
Would it alleviate your concern any if we eased into this, like say only
apply the back-branch pgindent run to 9.5 and later branches? Then at
least I could foresee the end of that particular annoyance.
(BTW, one practical issue is where would we get typedef lists relevant
to the back branches. I'm not sure if the buildfarm infrastructure is
capable of collecting branch-specific data, or if we'd need to rather
than just using a union of all branches' typedefs.)
regards, tom lane
On Mon, May 18, 2015 at 06:53:00PM -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Mon, May 18, 2015 at 12:10 PM, Bruce Momjian <bruce@momjian.us> wrote: > >> There was talk last time of pgindent-ing head and all back branches, > >> because a patch applied to head and back branches was historically only > >> pgindented in head, meaning that any future patches in that area could > >> not be easily backpatched. > >> > >> Do we want to do this? > > > I am personally not excited about that. I would rather leave the > > back-branches alone. > > It would be awfully nice though if we didn't have to deal with random > cross-branch indenting differences. I've lost, maybe not years off my > life, but certainly weeks of not-very-pleasant make-work because of that. > I'm surprised you've not had the same experience. > > If people were good about pgindenting patches meant to be back-patched > *before* they committed, it would not be such an issue, but they're not > very good about that. I couldn't figure out why we were getting that code drift, but now that Tom has identified why it happens, it seems good that we fix it. > Would it alleviate your concern any if we eased into this, like say only > apply the back-branch pgindent run to 9.5 and later branches? Then at > least I could foresee the end of that particular annoyance. > > (BTW, one practical issue is where would we get typedef lists relevant > to the back branches. I'm not sure if the buildfarm infrastructure is > capable of collecting branch-specific data, or if we'd need to rather > than just using a union of all branches' typedefs.) Uh, I just happen to commit the typedef list file used for the pgindent run in src/tools/pgindent/typedefs.list, per branch, so we would just use the same file. If typedefs were added in a backbranch (unlikely), we probably wouldn't want to use them anyway. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, May 18, 2015 at 06:53:00PM -0400, Tom Lane wrote:
>> (BTW, one practical issue is where would we get typedef lists relevant
>> to the back branches. I'm not sure if the buildfarm infrastructure is
>> capable of collecting branch-specific data, or if we'd need to rather
>> than just using a union of all branches' typedefs.)
> Uh, I just happen to commit the typedef list file used for the pgindent
> run in src/tools/pgindent/typedefs.list, per branch, so we would just
> use the same file. If typedefs were added in a backbranch (unlikely),
> we probably wouldn't want to use them anyway.
Not sure why you think it's unlikely; a back-patched commit could easily
add one. And if it did, we'd want pgindent to treat it the same as in
HEAD, else the whole point of this is gone.
(Come to think of it, that argument means we *do* want to use the same
typedef list in every branch, if we're to do this at all.)
regards, tom lane
On Mon, May 18, 2015 at 07:10:00PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Mon, May 18, 2015 at 06:53:00PM -0400, Tom Lane wrote: > >> (BTW, one practical issue is where would we get typedef lists relevant > >> to the back branches. I'm not sure if the buildfarm infrastructure is > >> capable of collecting branch-specific data, or if we'd need to rather > >> than just using a union of all branches' typedefs.) > > > Uh, I just happen to commit the typedef list file used for the pgindent > > run in src/tools/pgindent/typedefs.list, per branch, so we would just > > use the same file. If typedefs were added in a backbranch (unlikely), > > we probably wouldn't want to use them anyway. > > Not sure why you think it's unlikely; a back-patched commit could easily > add one. And if it did, we'd want pgindent to treat it the same as in > HEAD, else the whole point of this is gone. Oh, good point. > (Come to think of it, that argument means we *do* want to use the same > typedef list in every branch, if we're to do this at all.) I am feeling either the head typedefs or the per-branch typedefs are close enough that either would be fine. If we go with the head typedefs, there will be some churn in the code layout, though, unrelated to the patches applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 05/18/2015 07:04 PM, Bruce Momjian wrote: > On Mon, May 18, 2015 at 06:53:00PM -0400, Tom Lane wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Mon, May 18, 2015 at 12:10 PM, Bruce Momjian <bruce@momjian.us> wrote: >>>> There was talk last time of pgindent-ing head and all back branches, >>>> because a patch applied to head and back branches was historically only >>>> pgindented in head, meaning that any future patches in that area could >>>> not be easily backpatched. >>>> >>>> Do we want to do this? >>> I am personally not excited about that. I would rather leave the >>> back-branches alone. >> It would be awfully nice though if we didn't have to deal with random >> cross-branch indenting differences. I've lost, maybe not years off my >> life, but certainly weeks of not-very-pleasant make-work because of that. >> I'm surprised you've not had the same experience. >> >> If people were good about pgindenting patches meant to be back-patched >> *before* they committed, it would not be such an issue, but they're not >> very good about that. > I couldn't figure out why we were getting that code drift, but now that > Tom has identified why it happens, it seems good that we fix it. > >> Would it alleviate your concern any if we eased into this, like say only >> apply the back-branch pgindent run to 9.5 and later branches? Then at >> least I could foresee the end of that particular annoyance. >> >> (BTW, one practical issue is where would we get typedef lists relevant >> to the back branches. I'm not sure if the buildfarm infrastructure is >> capable of collecting branch-specific data, or if we'd need to rather >> than just using a union of all branches' typedefs.) > Uh, I just happen to commit the typedef list file used for the pgindent > run in src/tools/pgindent/typedefs.list, per branch, so we would just > use the same file. If typedefs were added in a backbranch (unlikely), > we probably wouldn't want to use them anyway. > The buildfarm animals are perfectly capable of finding typedefs for each branch. They haven't been because the default configuration is only to collect them for HEAD. Changing this is easy, especially since I control five of the six members currently reporting typedefs successfully, and Tom controls the other one. I've currently set two of them to do run typedefs for all live branches. The other thing is that the server script that amalgamates them only looks at HEAD. That will need to change. We would probably want an amalgamated list, because there could have been symbols on old branches that were deleted in later branches. With luck the presence of false positives wouldn't matter. It usually doesn't seem to. cheers andrew
On 05/18/2015 08:06 PM, Andrew Dunstan wrote: > > On 05/18/2015 07:04 PM, Bruce Momjian wrote: >> On Mon, May 18, 2015 at 06:53:00PM -0400, Tom Lane wrote: >>> Robert Haas <robertmhaas@gmail.com> writes: >>>> On Mon, May 18, 2015 at 12:10 PM, Bruce Momjian <bruce@momjian.us> >>>> wrote: >>>>> There was talk last time of pgindent-ing head and all back branches, >>>>> because a patch applied to head and back branches was historically >>>>> only >>>>> pgindented in head, meaning that any future patches in that area >>>>> could >>>>> not be easily backpatched. >>>>> >>>>> Do we want to do this? >>>> I am personally not excited about that. I would rather leave the >>>> back-branches alone. >>> It would be awfully nice though if we didn't have to deal with random >>> cross-branch indenting differences. I've lost, maybe not years off my >>> life, but certainly weeks of not-very-pleasant make-work because of >>> that. >>> I'm surprised you've not had the same experience. >>> >>> If people were good about pgindenting patches meant to be back-patched >>> *before* they committed, it would not be such an issue, but they're not >>> very good about that. >> I couldn't figure out why we were getting that code drift, but now that >> Tom has identified why it happens, it seems good that we fix it. >>> Would it alleviate your concern any if we eased into this, like say >>> only >>> apply the back-branch pgindent run to 9.5 and later branches? Then at >>> least I could foresee the end of that particular annoyance. >>> >>> (BTW, one practical issue is where would we get typedef lists relevant >>> to the back branches. I'm not sure if the buildfarm infrastructure is >>> capable of collecting branch-specific data, or if we'd need to rather >>> than just using a union of all branches' typedefs.) >> Uh, I just happen to commit the typedef list file used for the pgindent >> run in src/tools/pgindent/typedefs.list, per branch, so we would just >> use the same file. If typedefs were added in a backbranch (unlikely), >> we probably wouldn't want to use them anyway. >> > > > > The buildfarm animals are perfectly capable of finding typedefs for > each branch. They haven't been because the default configuration is > only to collect them for HEAD. > > Changing this is easy, especially since I control five of the six > members currently reporting typedefs successfully, and Tom controls > the other one. > > I've currently set two of them to do run typedefs for all live branches. > > The other thing is that the server script that amalgamates them only > looks at HEAD. That will need to change. > > We would probably want an amalgamated list, because there could have > been symbols on old branches that were deleted in later branches. With > luck the presence of false positives wouldn't matter. It usually > doesn't seem to. > > OK, if you look at <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list> you will be able to see the state of things. It's not even remotely pretty, and I am going to fix that, but it works. As you will be able to see, a number of buildfarm members are reporting on typedefs on all the live branches. You can get the list for each branch by hitting the appropriate link (essentially '/cgi-bin/typedefs.pl?branch=$branch'). If you ask for 'ALL' as the branch it gives you the amalgamated list over all branches. If you don't specify a branch at all, it gives you HEAD (which is buildfarm spelling for master), since that's what it did previously. I can change the default to ALL if that's what people want. Tom, if you want to get dromedary reporting on all branches, just remove the "branches => [ 'HEAD' ]," from the config. Enjoy. cheers andrew
On Mon, May 18, 2015 at 6:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I am personally not excited about that. I would rather leave the >> back-branches alone. > > It would be awfully nice though if we didn't have to deal with random > cross-branch indenting differences. I've lost, maybe not years off my > life, but certainly weeks of not-very-pleasant make-work because of that. > I'm surprised you've not had the same experience. Well, there are a couple of things that worry me: - People rely on us to ship, in minor releases, only critical security and stability fixes. Re-indenting the code is neither, and people may not appreciate needless whitespace differences being shipped in the next branch. Anyone who diffs that tarball against the previous one is going to see a bunch of stuff in there that may make them nervous. - If pgindent doesn't handle every branch in exactly the same way, it's possible that this change could exacerbate differences instead of reducing them. I actually think this is quite a likely outcome. I personally have not found back-patching to have been significantly complicated by whitespace differences. There are certainly code differences that can make it quite miserable in some cases, but I cannot recall a case where there was an issue of this time due to erratic indenting in one branch that had meanwhile been fixed in another branch. I accept that your experience may be different, of course. > Would it alleviate your concern any if we eased into this, like say only > apply the back-branch pgindent run to 9.5 and later branches? Then at > least I could foresee the end of that particular annoyance. If we do this only beginning with 9.5, and if we can make the output 100% consistent across branches, and if we run it before EVERY minor release so that people don't see unrelated diffs between consecutive tarballs, then it would address my concerns. I wish that pgident could be made more automated, like by having it fully built into the tree so that you can type 'make indent', or by having a daemon that would automatically pgindent the main tree periodically (say, once a month, or when more than X number of lines/files have changed, whichever comes first). I still find it quite a hassle to set up and run. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom, if you want to get dromedary reporting on all branches, just
> remove the "branches => [ 'HEAD' ]," from the config.
dromedary is a pretty slow machine, so I'm going to pass on that unless
there's a good reason to think it would find typedefs your machines don't.
I rather doubt that --- our use of platform-dependent typedefs is fairly
small and stable, so it seems like checking HEAD should be sufficient.
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 18, 2015 at 6:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Would it alleviate your concern any if we eased into this, like say only
>> apply the back-branch pgindent run to 9.5 and later branches? Then at
>> least I could foresee the end of that particular annoyance.
> If we do this only beginning with 9.5, and if we can make the output
> 100% consistent across branches, and if we run it before EVERY minor
> release so that people don't see unrelated diffs between consecutive
> tarballs, then it would address my concerns.
To do it before every minor release would require re-indenting HEAD
as well (since the whole point is to keep HEAD and the back branches
consistent). I think we'd get too much push-back from developers
whose pending patches got broken. We can get away with reindenting
HEAD between development cycles, but probably not more often than that.
I'm not particularly concerned by the tarball-diff argument: running
diff with --ignore-spaces should mask most of the changes. Moreover,
assuming the code was properly indented at x.y.0 release time, any
changes applied by pgindent would only be within subsequent back-patches,
which hopefully are a very small part of the code. (Perhaps it would be
useful to do a trial indent on some old branch right now, just to see how
large the diffs are; then we'd have some actual facts in this argument...)
And lastly, committers who are bothered by the prospect of such changes
could take the time to reindent their back-patched changes before
committing in the first place. (FWIW, I usually do, and it's not hard
except in files that have been heavily mangled in HEAD.)
> I wish that pgident could be made more automated, like by having it
> fully built into the tree so that you can type 'make indent', or by
> having a daemon that would automatically pgindent the main tree
> periodically (say, once a month, or when more than X number of
> lines/files have changed, whichever comes first). I still find it
> quite a hassle to set up and run.
It is a pain. I have a shell script that fetches the typedef list
automatically, which helps. The main problem with a "make indent" target
is that only in Bruce's annual runs do we really want to let it loose on
the whole tree. In manual fixups, I only point it at the files I've
edited (and then, often, I have to remove some diffs in unrelated parts
of those files). I wish that could be a bit easier, though I'm not sure
how.
regards, tom lane
On Tue, May 19, 2015 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > To do it before every minor release would require re-indenting HEAD > as well (since the whole point is to keep HEAD and the back branches > consistent). I think we'd get too much push-back from developers > whose pending patches got broken. We can get away with reindenting > HEAD between development cycles, but probably not more often than that. I'm not convinced of that. If we did it more often, it might actually be less disruptive. > I'm not particularly concerned by the tarball-diff argument: running > diff with --ignore-spaces should mask most of the changes. Moreover, > assuming the code was properly indented at x.y.0 release time, any > changes applied by pgindent would only be within subsequent back-patches, > which hopefully are a very small part of the code. (Perhaps it would be > useful to do a trial indent on some old branch right now, just to see how > large the diffs are; then we'd have some actual facts in this argument...) That parenthetical idea sounds promising. > And lastly, committers who are bothered by the prospect of such changes > could take the time to reindent their back-patched changes before > committing in the first place. (FWIW, I usually do, and it's not hard > except in files that have been heavily mangled in HEAD.) Meh. With 10+ active committers, that's bound not to always work out. >> I wish that pgident could be made more automated, like by having it >> fully built into the tree so that you can type 'make indent', or by >> having a daemon that would automatically pgindent the main tree >> periodically (say, once a month, or when more than X number of >> lines/files have changed, whichever comes first). I still find it >> quite a hassle to set up and run. > > It is a pain. I have a shell script that fetches the typedef list > automatically, which helps. The main problem with a "make indent" target > is that only in Bruce's annual runs do we really want to let it loose on > the whole tree. In manual fixups, I only point it at the files I've > edited (and then, often, I have to remove some diffs in unrelated parts > of those files). I wish that could be a bit easier, though I'm not sure > how. Unless we reindent regularly, the problem with changes in unrelated parts of the file is not going away. Figuring out which files have been changed locally could probably be done with some sort of git-fu. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Tue, May 19, 2015 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > To do it before every minor release would require re-indenting HEAD > > as well (since the whole point is to keep HEAD and the back branches > > consistent). I think we'd get too much push-back from developers > > whose pending patches got broken. We can get away with reindenting > > HEAD between development cycles, but probably not more often than that. > > I'm not convinced of that. If we did it more often, it might actually > be less disruptive. I believe it's possible to mechanically rebase a patch over an indent run of the underlying branch with half a dozen commands or less. +1 for reindenting all branches before each minor release, FWIW. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Robert Haas wrote:
>> On Tue, May 19, 2015 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> To do it before every minor release would require re-indenting HEAD
>>> as well (since the whole point is to keep HEAD and the back branches
>>> consistent). I think we'd get too much push-back from developers
>>> whose pending patches got broken. We can get away with reindenting
>>> HEAD between development cycles, but probably not more often than that.
>> I'm not convinced of that. If we did it more often, it might actually
>> be less disruptive.
> I believe it's possible to mechanically rebase a patch over an indent
> run of the underlying branch with half a dozen commands or less. +1 for
> reindenting all branches before each minor release, FWIW.
Yeah? Can you show an example?
regards, tom lane
Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > I believe it's possible to mechanically rebase a patch over an indent
> > run of the underlying branch with half a dozen commands or less. +1 for
> > reindenting all branches before each minor release, FWIW.
>
> Yeah? Can you show an example?
So we have this:
---P----I----C'| \---C----I'
where P is the parent commit; I is the pgindent commit; C is your
change (applied to the unindented tree). What you need is to obtain C'
which is a copy of C that applies to I. You can do this by creating I'
which is a pgindent run over your patch, then diff that one to I.
I *think* this should work:
git checkout C
pgindent tree
git commit # yields I'
git diff I I' > C'
git checkout I
git apply C'
I spent a few minutes looking for a nontrivial patch to test this on,
couldn't find one; but the key is that you must be able to run pgindent
on your own using the same rules that Bruce's run would.
This shouldn't need human intervention at all, so should even be
possible to write a script for it and use it for git rebase -i -x this_script origin/master
for when you have a branch with several commits that you want to rebase
over an upstream pgindent.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> I believe it's possible to mechanically rebase a patch over an indent
>>> run of the underlying branch with half a dozen commands or less. +1 for
>>> reindenting all branches before each minor release, FWIW.
>> Yeah? Can you show an example?
> I *think* this should work:
> git checkout C
> pgindent tree
> git commit # yields I'
> git diff I I' > C'
> git checkout I
> git apply C'
> I spent a few minutes looking for a nontrivial patch to test this on,
> couldn't find one; but the key is that you must be able to run pgindent
> on your own using the same rules that Bruce's run would.
OK. So agreed, the blocking issue here is whether pgindent is
conveniently available to every patch submitter. Right now, it would
certainly be charitable to describe installing it as a PITA. I think
what we'd need to do is (1) include fully patched sources in our git
tree, and (2) build them by default (but not install them, probably)
so that we can flush out any portability issues.
I think it's too late to consider doing that for 9.5, but maybe we
could do it after the branch.
Another issue is whether there's a copyright problem if we include
modified BSD indent sources in our tree. I wouldn't think so but
we better check exactly how it's licensed.
We'd also want a more mechanical way of obtaining the right typedef list
to use. Although it probably couldn't be totally mechanized, because if
your patch adds new typedefs you'd want to manually add those names to
the list being used. Maybe there should be an optional local typedef
list separate from the automatically generated file. I guess in the
scenario you're describing, the most helpful thing would be if the
pgindent commit put the typedef list it had used into the tree, and then
we just use that (plus manual additions) when generating the I' commit.
regards, tom lane
On Fri, May 22, 2015 at 12:02:11PM -0400, Tom Lane wrote: > I guess in the > scenario you're describing, the most helpful thing would be if the > pgindent commit put the typedef list it had used into the tree, and then > we just use that (plus manual additions) when generating the I' commit. I have always added the typedef list I used as part of pgindent commit runs. Are we ready for a pgindent run? Back branches? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> Are we ready for a pgindent run? Back branches?
I think we could do it in HEAD, but it doesn't seem like we have consensus
about whether to touch the back branches. Suggest just HEAD for now and
we can continue to argue about the back branches.
regards, tom lane
On Sat, May 23, 2015 at 12:32:34PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Are we ready for a pgindent run? Back branches? > > I think we could do it in HEAD, but it doesn't seem like we have consensus > about whether to touch the back branches. Suggest just HEAD for now and > we can continue to argue about the back branches. pgindent run on HEAD and committed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 2015-05-23 21:36:50 -0400, Bruce Momjian wrote: > pgindent run on HEAD and committed. - if (IsA(node, Aggref) || IsA(node, GroupingFunc)) + if (IsA(node, Aggref) ||IsA(node, GroupingFunc)) There's a bunch of changes like this. Looks rather odd to me? I don't recall seing much code looking like that? Also, does somebody have an idea to keep pgindent from butchering inline asm like: /* * Perform cmpxchg and use the zero flag which it implicitly sets when * equal to measure the success. */ - __asm__ __volatile__( - " lock \n" - " cmpxchgl %4,%5 \n" - " setz %2 \n" -: "=a" (*expected), "=m"(ptr->value), "=q" (ret) -: "a" (*expected), "r" (newval), "m"(ptr->value) -: "memory", "cc"); + __asm__ __volatile__( + " lock \n" + " cmpxchgl %4,%5 \n" + " setz %2 \n" + : "=a"(*expected), "=m"(ptr->value), "=q"(ret) + : "a"(*expected), "r"(newval), "m"(ptr->value) + : "memory", "cc"); + return (bool) ret; Andres
On Sun, May 24, 2015 at 04:16:07AM +0200, Andres Freund wrote: > On 2015-05-23 21:36:50 -0400, Bruce Momjian wrote: > > pgindent run on HEAD and committed. > > - if (IsA(node, Aggref) || IsA(node, GroupingFunc)) > + if (IsA(node, Aggref) ||IsA(node, GroupingFunc)) > > There's a bunch of changes like this. Looks rather odd to me? I don't > recall seing much code looking like that? Wow, that is quite odd. I saw another case where it might have done this because the line was >80 characters without it, but not in this case. > Also, does somebody have an idea to keep pgindent from butchering inline > asm like: > /* > * Perform cmpxchg and use the zero flag which it implicitly sets when > * equal to measure the success. > */ > - __asm__ __volatile__( > - " lock \n" > - " cmpxchgl %4,%5 \n" > - " setz %2 \n" > -: "=a" (*expected), "=m"(ptr->value), "=q" (ret) > -: "a" (*expected), "r" (newval), "m"(ptr->value) > -: "memory", "cc"); > + __asm__ __volatile__( > + " lock \n" > + " cmpxchgl %4,%5 \n" > + " setz %2 \n" > + : "=a"(*expected), "=m"(ptr->value), "=q"(ret) > + : "a"(*expected), "r"(newval), "m"(ptr->value) > + : "memory", "cc"); > + > return (bool) ret; Ah, we have a file /pgtop/src/tools/pgindent/exclude_file_patterns which has excluded files, and s_lock.h is one of them. I think /include/port/atomics/arch-x86.h needs to be added, and the pgindent commit on the file reverted. Are there any other files with __asm__ lines like that? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Sun, May 24, 2015 at 04:16:07AM +0200, Andres Freund wrote:
>> - if (IsA(node, Aggref) || IsA(node, GroupingFunc))
>> + if (IsA(node, Aggref) ||IsA(node, GroupingFunc))
>>
>> There's a bunch of changes like this. Looks rather odd to me? I don't
>> recall seing much code looking like that?
> Wow, that is quite odd.
No, pgindent has *always* been wonky about lines that contain a typedef
name but are not variable declarations. I've gotten in the habit of
breaking IsA tests like these into two lines:
if (IsA(node, Aggref) || IsA(node, GroupingFunc))
just so that it doesn't look weird when pgindent gets done with it.
You can see similar weirdness around sizeof usages, if you look.
>> Also, does somebody have an idea to keep pgindent from butchering inline
>> asm like:
> Ah, we have a file /pgtop/src/tools/pgindent/exclude_file_patterns which
> has excluded files, and s_lock.h is one of them. I think
> /include/port/atomics/arch-x86.h needs to be added, and the pgindent
> commit on the file reverted.
Yeah, probably :-(
regards, tom lane
On Sat, May 23, 2015 at 11:37:30PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Sun, May 24, 2015 at 04:16:07AM +0200, Andres Freund wrote: > >> - if (IsA(node, Aggref) || IsA(node, GroupingFunc)) > >> + if (IsA(node, Aggref) ||IsA(node, GroupingFunc)) > >> > >> There's a bunch of changes like this. Looks rather odd to me? I don't > >> recall seing much code looking like that? > > > Wow, that is quite odd. > > No, pgindent has *always* been wonky about lines that contain a typedef > name but are not variable declarations. I've gotten in the habit of > breaking IsA tests like these into two lines: > > if (IsA(node, Aggref) || > IsA(node, GroupingFunc)) > > just so that it doesn't look weird when pgindent gets done with it. > You can see similar weirdness around sizeof usages, if you look. Oh, I checked if IsA was a typedef, but it isn't. I see now the problem is the Aggref typedef on the line. > >> Also, does somebody have an idea to keep pgindent from butchering inline > >> asm like: > > > Ah, we have a file /pgtop/src/tools/pgindent/exclude_file_patterns which > > has excluded files, and s_lock.h is one of them. I think > > /include/port/atomics/arch-x86.h needs to be added, and the pgindent > > commit on the file reverted. > > Yeah, probably :-( OK, will do. I am going to revert and exclude the entire include/port/atomics directory. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 05/23/2015 11:37 PM, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: >> On Sun, May 24, 2015 at 04:16:07AM +0200, Andres Freund wrote: >>> - if (IsA(node, Aggref) || IsA(node, GroupingFunc)) >>> + if (IsA(node, Aggref) ||IsA(node, GroupingFunc)) >>> >>> There's a bunch of changes like this. Looks rather odd to me? I don't >>> recall seing much code looking like that? >> Wow, that is quite odd. > No, pgindent has *always* been wonky about lines that contain a typedef > name but are not variable declarations. I've gotten in the habit of > breaking IsA tests like these into two lines: > > if (IsA(node, Aggref) || > IsA(node, GroupingFunc)) > > just so that it doesn't look weird when pgindent gets done with it. > You can see similar weirdness around sizeof usages, if you look. Well, that sounds like something we should try to patch, doesn't it? (No, I'm not volunteering.) cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 05/23/2015 11:37 PM, Tom Lane wrote:
>> No, pgindent has *always* been wonky about lines that contain a typedef
>> name but are not variable declarations.
> Well, that sounds like something we should try to patch, doesn't it?
> (No, I'm not volunteering.)
In the past, the main argument against changing pgindent has been that
it would cause reformatting of settled code. For example, when Bruce
twiddled its right margin limit for comments around 8.1, that caused
literally years worth of back-patching pain. If we can get to an
agreement on re-indenting back branches at the same time as master,
then this problem would go away, and I for one would get a lot more
enthusiastic about trying to improve pgindent rather than just working
around its oddities.
Not that I'm volunteering either right now.
regards, tom lane
On Sun, May 24, 2015 at 08:31:32AM -0400, Bruce Momjian wrote: > > >> Also, does somebody have an idea to keep pgindent from butchering inline > > >> asm like: > > > > > Ah, we have a file /pgtop/src/tools/pgindent/exclude_file_patterns which > > > has excluded files, and s_lock.h is one of them. I think > > > /include/port/atomics/arch-x86.h needs to be added, and the pgindent > > > commit on the file reverted. > > > > Yeah, probably :-( > > OK, will do. I am going to revert and exclude the entire > include/port/atomics directory. Done. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Sun, May 24, 2015 at 10:44:10AM -0400, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > On 05/23/2015 11:37 PM, Tom Lane wrote: > >> No, pgindent has *always* been wonky about lines that contain a typedef > >> name but are not variable declarations. > > > Well, that sounds like something we should try to patch, doesn't it? > > (No, I'm not volunteering.) > > In the past, the main argument against changing pgindent has been that > it would cause reformatting of settled code. For example, when Bruce > twiddled its right margin limit for comments around 8.1, that caused > literally years worth of back-patching pain. If we can get to an > agreement on re-indenting back branches at the same time as master, > then this problem would go away, and I for one would get a lot more > enthusiastic about trying to improve pgindent rather than just working > around its oddities. > > Not that I'm volunteering either right now. That is something we will need to fix in the BSD indent code. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Sat, May 23, 2015 at 12:32:34PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Are we ready for a pgindent run? Back branches? > > I think we could do it in HEAD, but it doesn't seem like we have consensus > about whether to touch the back branches. Suggest just HEAD for now and > we can continue to argue about the back branches. Here is a re-run of pgindent on 9.4: http://momjian.us/expire/pgindent-9.4.diff It is 5.7k line diff. I didn't do pre-9.4 because pgindent was slightly modified before the 9.4 run, so there would be additional changes in those back branches. If we wanted to do this on backbranches, I think we would create a diff file of the minor release just before running pgindent and stamping so users could see the non-pgindent content of the release. A crazy idea would be to release of just pgindent changes so we would not add pgindent changes into a fix release. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> If we wanted to do this on backbranches, I think we would create a diff
> file of the minor release just before running pgindent and stamping so
> users could see the non-pgindent content of the release.
What for? Those who want to see that can look at our git repo.
> A crazy idea
> would be to release of just pgindent changes so we would not add
> pgindent changes into a fix release.
I'm not entirely sure that I follow you here, but it doesn't sound very
workable --- what happens when we go to back-patch changes in an area
that was previously reindented? You can't have two separate versions
of a branch.
regards, tom lane
Bruce Momjian <bruce@momjian.us> writes: > Here is a re-run of pgindent on 9.4: > http://momjian.us/expire/pgindent-9.4.diff Some of those diffs would disappear if you'd used an up-to-date typedefs list ... not a lot, but some. That is rather a lot of diffs, but the thing I think people ought to take away from that is "this is the number of back-patch hazards we just introduced between HEAD and 9.4" --- because as of yesterday, HEAD looks like this not like what's in 9.4. What's more, those hazards are in code hunks that were back-patched bug fixes, which means that they are in areas that are more likely than average to need additional fixing. regards, tom lane
On 2015-05-20 11:47:15 -0400, Robert Haas wrote: > On Tue, May 19, 2015 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > To do it before every minor release would require re-indenting HEAD > > as well (since the whole point is to keep HEAD and the back branches > > consistent). I think we'd get too much push-back from developers > > whose pending patches got broken. We can get away with reindenting > > HEAD between development cycles, but probably not more often than that. > > I'm not convinced of that. If we did it more often, it might actually > be less disruptive. I personally would be fine with doing it more often. I do think that that'd require that a) make indent is provided *and* works in VPATH builds, b) pg_bsd_indent would have to be in src/tools.
* Andres Freund (andres@anarazel.de) wrote:
> On 2015-05-20 11:47:15 -0400, Robert Haas wrote:
> > On Tue, May 19, 2015 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > To do it before every minor release would require re-indenting HEAD
> > > as well (since the whole point is to keep HEAD and the back branches
> > > consistent). I think we'd get too much push-back from developers
> > > whose pending patches got broken. We can get away with reindenting
> > > HEAD between development cycles, but probably not more often than that.
> >
> > I'm not convinced of that. If we did it more often, it might actually
> > be less disruptive.
>
> I personally would be fine with doing it more often. I do think that
> that'd require that a) make indent is provided *and* works in VPATH
> builds, b) pg_bsd_indent would have to be in src/tools.
Yes, please...
Thanks!
Stephen
Andres Freund <andres@anarazel.de> writes:
> On 2015-05-20 11:47:15 -0400, Robert Haas wrote:
>> On Tue, May 19, 2015 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> To do it before every minor release would require re-indenting HEAD
>>> as well (since the whole point is to keep HEAD and the back branches
>>> consistent). I think we'd get too much push-back from developers
>>> whose pending patches got broken. We can get away with reindenting
>>> HEAD between development cycles, but probably not more often than that.
>> I'm not convinced of that. If we did it more often, it might actually
>> be less disruptive.
> I personally would be fine with doing it more often. I do think that
> that'd require that a) make indent is provided *and* works in VPATH
> builds, b) pg_bsd_indent would have to be in src/tools.
Yeah, this was already discussed upthread: doing it for every minor
release would require a significant jump in the availability of the tool.
I'm okay with working towards that, but it will take awhile to get there.
What we need to consider right now is whether to include back branches
in the existing practice of reindenting between development cycles.
This is somewhat urgent because we already did HEAD, so we have already
created a divergence from HEAD to 9.4 which is going to cause us pain
one way or the other. (It's worth noting for example that Bruce's
trial run of pgindent on 9.4 hit some of the code involved in the
fsync-the-whole-data-directory patch, which means that whatever we decide
to do about that is likely to stumble over pgindent diffs if we don't
re-indent the back branches. So I'm not talking about potential pain
in the vague future, I'm talking about this week.)
I remain in favor of doing a reindent on the back branches right now.
Changing the schedule to do it for every minor release is something to
consider in the future.
regards, tom lane
Tom Lane wrote: > What we need to consider right now is whether to include back branches > in the existing practice of reindenting between development cycles. > This is somewhat urgent because we already did HEAD, so we have already > created a divergence from HEAD to 9.4 which is going to cause us pain > one way or the other. (It's worth noting for example that Bruce's > trial run of pgindent on 9.4 hit some of the code involved in the > fsync-the-whole-data-directory patch, which means that whatever we decide > to do about that is likely to stumble over pgindent diffs if we don't > re-indent the back branches. So I'm not talking about potential pain > in the vague future, I'm talking about this week.) FWIW the multixact code is now slightly different between HEAD and 9.3/9.4, also. So if that needs further patches, they will be fun to backpatch. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, May 25, 2015 at 03:12:24PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Mon, May 25, 2015 at 03:03:00PM -0400, Tom Lane wrote: > >> As we discussed upthread, if we're trying to minimize cross-branch > >> pgindent differences then we probably need to use the same typedefs > >> list in all branches. I believe Andrew's already set up buildfarm > >> support for generating a unified typedef list for all active branches. > > > OK, but just consider that we are then introducing _new_ pgindent > > changes into back branches that have not been modified by patches at > > all. > > The point is for the back branches to absorb pgindent-induced changes that > have already happened in HEAD, so I'm not sure what you're getting at. My point is uses of new typedefs names added in HEAD which are not typedefs in the back branches will be pgindent'ed differently than before, e.g. back branches use a variable 'aaa' which is not a typedef in back branches, but if 'aaa' becomes a typedef in HEAD, references to 'aaa' in back branches will be adjusted by pgindent. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, May 25, 2015 at 03:20:47PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Mon, May 25, 2015 at 03:12:24PM -0400, Tom Lane wrote: > >> The point is for the back branches to absorb pgindent-induced changes that > >> have already happened in HEAD, so I'm not sure what you're getting at. > > > My point is uses of new typedefs names added in HEAD which are not > > typedefs in the back branches will be pgindent'ed differently than > > before, e.g. back branches use a variable 'aaa' which is not a typedef > > in back branches, but if 'aaa' becomes a typedef in HEAD, references to > > 'aaa' in back branches will be adjusted by pgindent. > > Right, and that's what we want, because the corresponding uses of 'aaa' > as a variable will also have been changed in HEAD. The point of this > exercise is to ensure that otherwise-equivalent code is indented the same > in all branches. OK, makes sense. You can see the old and 'all' diffs here: http://momjian.us/expire/ The diff size went from 5.7k to 5.3k lines. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, May 25, 2015 at 03:03:00PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Mon, May 25, 2015 at 01:10:04PM -0400, Tom Lane wrote: > >> Some of those diffs would disappear if you'd used an up-to-date typedefs > >> list ... not a lot, but some. > > > Uh, you mean a current 9.4.X typedef list? Should I try that? > > As we discussed upthread, if we're trying to minimize cross-branch > pgindent differences then we probably need to use the same typedefs > list in all branches. I believe Andrew's already set up buildfarm > support for generating a unified typedef list for all active branches. OK, but just consider that we are then introducing _new_ pgindent changes into back branches that have not been modified by patches at all. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, May 25, 2015 at 03:12:24PM -0400, Tom Lane wrote:
>> The point is for the back branches to absorb pgindent-induced changes that
>> have already happened in HEAD, so I'm not sure what you're getting at.
> My point is uses of new typedefs names added in HEAD which are not
> typedefs in the back branches will be pgindent'ed differently than
> before, e.g. back branches use a variable 'aaa' which is not a typedef
> in back branches, but if 'aaa' becomes a typedef in HEAD, references to
> 'aaa' in back branches will be adjusted by pgindent.
Right, and that's what we want, because the corresponding uses of 'aaa'
as a variable will also have been changed in HEAD. The point of this
exercise is to ensure that otherwise-equivalent code is indented the same
in all branches.
regards, tom lane
On Mon, May 25, 2015 at 09:00:25PM +0200, Andres Freund wrote: > On 2015-05-25 14:55:54 -0400, Bruce Momjian wrote: > > One issue I discussed is doing a pgindent-only release so users doing a > > diff would not have pgindent diffs mixed with fixes. > > I find a pgindent only release a pretty pointless goal. That's what git > is for. I said it was a crazy idea, of course. :-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, May 25, 2015 at 01:28:16PM -0400, Tom Lane wrote: > What we need to consider right now is whether to include back branches > in the existing practice of reindenting between development cycles. > This is somewhat urgent because we already did HEAD, so we have already > created a divergence from HEAD to 9.4 which is going to cause us pain > one way or the other. (It's worth noting for example that Bruce's > trial run of pgindent on 9.4 hit some of the code involved in the > fsync-the-whole-data-directory patch, which means that whatever we decide > to do about that is likely to stumble over pgindent diffs if we don't > re-indent the back branches. So I'm not talking about potential pain > in the vague future, I'm talking about this week.) One issue I discussed is doing a pgindent-only release so users doing a diff would not have pgindent diffs mixed with fixes. If we are going to do an fsync-fix-only release soon, adding pgindent diffs to that would be as minimal a mixing as we could hope for. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, May 25, 2015 at 12:49:41PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > If we wanted to do this on backbranches, I think we would create a diff > > file of the minor release just before running pgindent and stamping so > > users could see the non-pgindent content of the release. > > What for? Those who want to see that can look at our git repo. True. I was just considering people who want a comprehensive diff. > > A crazy idea > > would be to release of just pgindent changes so we would not add > > pgindent changes into a fix release. > > I'm not entirely sure that I follow you here, but it doesn't sound very > workable --- what happens when we go to back-patch changes in an area > that was previously reindented? You can't have two separate versions > of a branch. It would be a PG release, with number increment, which only has pgindent changes. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> FWIW the multixact code is now slightly different between HEAD and
> 9.3/9.4, also. So if that needs further patches, they will be fun to
> backpatch.
Well, sure, intentional cross-branch changes are always a hazard.
But pgindent diffs are a hazard that we could mechanically remove.
regards, tom lane
On 2015-05-25 14:55:54 -0400, Bruce Momjian wrote: > One issue I discussed is doing a pgindent-only release so users doing a > diff would not have pgindent diffs mixed with fixes. I find a pgindent only release a pretty pointless goal. That's what git is for.
Bruce Momjian wrote: > OK, makes sense. You can see the old and 'all' diffs here: > > http://momjian.us/expire/ Something is wrong. See aclchk.c changes. Also, sometime ago we changed pgindent rules so that dot-space-space is not turned into dot-tab in comments anymore, and many places were updated to change dot-tab to dot-space-space -- or something like that. This wasn't done in back branches (probably only 9.4 and back), but it's likely that we need to do something about that. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > FWIW the multixact code is now slightly different between HEAD and > > 9.3/9.4, also. So if that needs further patches, they will be fun to > > backpatch. > > Well, sure, intentional cross-branch changes are always a hazard. > But pgindent diffs are a hazard that we could mechanically remove. Sorry, I understand that -- I meant the multixact code is indented differently now. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, May 25, 2015 at 01:10:04PM -0400, Tom Lane wrote:
>> Some of those diffs would disappear if you'd used an up-to-date typedefs
>> list ... not a lot, but some.
> Uh, you mean a current 9.4.X typedef list? Should I try that?
As we discussed upthread, if we're trying to minimize cross-branch
pgindent differences then we probably need to use the same typedefs
list in all branches. I believe Andrew's already set up buildfarm
support for generating a unified typedef list for all active branches.
regards, tom lane
On Mon, May 25, 2015 at 01:10:04PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Here is a re-run of pgindent on 9.4: > > http://momjian.us/expire/pgindent-9.4.diff > > Some of those diffs would disappear if you'd used an up-to-date typedefs > list ... not a lot, but some. Uh, you mean a current 9.4.X typedef list? Should I try that? > That is rather a lot of diffs, but the thing I think people ought to take > away from that is "this is the number of back-patch hazards we just > introduced between HEAD and 9.4" --- because as of yesterday, HEAD looks > like this not like what's in 9.4. What's more, those hazards are in code > hunks that were back-patched bug fixes, which means that they are in areas > that are more likely than average to need additional fixing. Yes, that is very true. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, May 25, 2015 at 03:03:00PM -0400, Tom Lane wrote:
>> As we discussed upthread, if we're trying to minimize cross-branch
>> pgindent differences then we probably need to use the same typedefs
>> list in all branches. I believe Andrew's already set up buildfarm
>> support for generating a unified typedef list for all active branches.
> OK, but just consider that we are then introducing _new_ pgindent
> changes into back branches that have not been modified by patches at
> all.
The point is for the back branches to absorb pgindent-induced changes that
have already happened in HEAD, so I'm not sure what you're getting at.
regards, tom lane
Bruce Momjian wrote: > One issue I discussed is doing a pgindent-only release so users doing a > diff would not have pgindent diffs mixed with fixes. I doubt anyone is reading hand-generated diffs these days. Those that want to read diffs are much better served by looking at the git repo directly. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote: > Bruce Momjian wrote: > > > OK, makes sense. You can see the old and 'all' diffs here: > > > > http://momjian.us/expire/ > > Something is wrong. See aclchk.c changes. Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0 and 9.1, and the use of that as a typedef in 9.4 is certainly odd: - aclitem.ai_grantor = grantorId;+ aclitem. ai_grantor = grantorId; > Also, sometime ago we changed pgindent rules so that dot-space-space is > not turned into dot-tab in comments anymore, and many places were > updated to change dot-tab to dot-space-space -- or something like that. > This wasn't done in back branches (probably only 9.4 and back), but it's > likely that we need to do something about that. This was done in the backbranches with entab -m (only C comment periods). In fact, the only way we could improve pgindent in such a comprehensive way was to run this in back-branches. It was considered safer than running pgindent as it only affected C comments. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote:
>> Something is wrong. See aclchk.c changes.
> Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0
> and 9.1, and the use of that as a typedef in 9.4 is certainly odd:
> - aclitem.ai_grantor = grantorId;
> + aclitem. ai_grantor = grantorId;
Yeah. I think we might've gotten rid of that typedef partially in order
to fix this.
A different strategy we could consider is "use HEAD's typedef list
even in the back branches". This would in some situations lead to
inferior-looking results in the back branches, but that's probably better
than inferior results in HEAD. (In any case, we want the same typedef
list across all branches. Then anyplace where the results diverge, there
must have been non-pgindent code changes, so that back-patching would
require manual fixups anyway.)
A longer-term fix would be to make pgindent less stupid about this sort
of usage, but nobody's yet volunteered to dig into the guts of that code.
regards, tom lane
Bruce Momjian wrote: > On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote: > > Bruce Momjian wrote: > > > > > OK, makes sense. You can see the old and 'all' diffs here: > > > > > > http://momjian.us/expire/ > > > > Something is wrong. See aclchk.c changes. > > Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0 > and 9.1, and the use of that as a typedef in 9.4 is certainly odd: > > - aclitem.ai_grantor = grantorId; > + aclitem. ai_grantor = grantorId; Interesting. The #typedef line still appears up to 9.4; we only removed it in master after we branched from 9.4. I notice now that this function is mis-indented in 9.0 and 9.1 in exactly this way. Perhaps we can just accept that 9.2 - 9.4 are going to be identical to the older branches, and 9.5 and forward is going to look saner. Or we could rename the variable and re-indent the older branches if this is too bothersome. This doesn't happen anywhere else AFAICT. > > Also, sometime ago we changed pgindent rules so that dot-space-space is > > not turned into dot-tab in comments anymore, and many places were > > updated to change dot-tab to dot-space-space -- or something like that. > > This wasn't done in back branches (probably only 9.4 and back), but it's > > likely that we need to do something about that. > > This was done in the backbranches with entab -m (only C comment > periods). In fact, the only way we could improve pgindent in such a > comprehensive way was to run this in back-branches. It was considered > safer than running pgindent as it only affected C comments. Ah, I see now that you backpatched that. One less problem to care about. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tom Lane wrote: > A longer-term fix would be to make pgindent less stupid about this sort > of usage, but nobody's yet volunteered to dig into the guts of that code. We've discussed in the past that we could use something other than BSD's indent -- astyle has been mentioned. It seems that with suitable options, we could make the result very close to what we have now, and not be forced to deal with typedef lists and other nonsense. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> A longer-term fix would be to make pgindent less stupid about this sort
>> of usage, but nobody's yet volunteered to dig into the guts of that code.
> We've discussed in the past that we could use something other than BSD's
> indent -- astyle has been mentioned. It seems that with suitable
> options, we could make the result very close to what we have now, and
> not be forced to deal with typedef lists and other nonsense.
Meh. As far as I know, nobody's ever gotten decent-looking results from
other tools. Also, if we say "use astyle" then we're vulnerable to
different peoples' installations producing different results. There is
a whole lot to be said for bundling the tool into our source tree, if
we're going to recommend patching processes that depend on reproducible
results.
regards, tom lane
On Mon, May 25, 2015 at 06:48:47PM -0300, Alvaro Herrera wrote: > Bruce Momjian wrote: > > On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote: > > > Bruce Momjian wrote: > > > > > > > OK, makes sense. You can see the old and 'all' diffs here: > > > > > > > > http://momjian.us/expire/ > > > > > > Something is wrong. See aclchk.c changes. > > > > Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0 > > and 9.1, and the use of that as a typedef in 9.4 is certainly odd: > > > > - aclitem.ai_grantor = grantorId; > > + aclitem. ai_grantor = grantorId; > > Interesting. The #typedef line still appears up to 9.4; we only removed > it in master after we branched from 9.4. > > I notice now that this function is mis-indented in 9.0 and 9.1 in > exactly this way. Perhaps we can just accept that 9.2 - 9.4 are going > to be identical to the older branches, and 9.5 and forward is going to > look saner. Or we could rename the variable and re-indent the older > branches if this is too bothersome. This doesn't happen anywhere else > AFAICT. Ah, OK. Seems I didn't update the git typedefs.list file for every pgindent run so my quick cross-version grep failed, though I used the right version for each run. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, May 25, 2015 at 05:34:12PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote: > >> Something is wrong. See aclchk.c changes. > > > Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0 > > and 9.1, and the use of that as a typedef in 9.4 is certainly odd: > > > - aclitem.ai_grantor = grantorId; > > + aclitem. ai_grantor = grantorId; > > Yeah. I think we might've gotten rid of that typedef partially in order > to fix this. > > A different strategy we could consider is "use HEAD's typedef list > even in the back branches". This would in some situations lead to > inferior-looking results in the back branches, but that's probably better > than inferior results in HEAD. (In any case, we want the same typedef > list across all branches. Then anyplace where the results diverge, there > must have been non-pgindent code changes, so that back-patching would > require manual fixups anyway.) > > A longer-term fix would be to make pgindent less stupid about this sort > of usage, but nobody's yet volunteered to dig into the guts of that code. I assume a typedefs list is going to be a requirement of any decent C indenting tool. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 2015-05-25 19:01:28 -0400, Bruce Momjian wrote: > > A longer-term fix would be to make pgindent less stupid about this sort > > of usage, but nobody's yet volunteered to dig into the guts of that code. > > I assume a typedefs list is going to be a requirement of any decent C > indenting tool. Maybe I'm missing something major here, but why? Afaict it's just only used for formatting decisions that could be made without it just as well? Greetings, Andres Freund
On Tue, May 26, 2015 at 01:15:17AM +0200, Andres Freund wrote: > On 2015-05-25 19:01:28 -0400, Bruce Momjian wrote: > > > A longer-term fix would be to make pgindent less stupid about this sort > > > of usage, but nobody's yet volunteered to dig into the guts of that code. > > > > I assume a typedefs list is going to be a requirement of any decent C > > indenting tool. > > Maybe I'm missing something major here, but why? Afaict it's just only > used for formatting decisions that could be made without it just as well? Uh, well, formatting decisions is what pgindent does. It is about identifying variable declarations. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, May 26, 2015 at 01:15:17AM +0200, Andres Freund wrote:
>> Maybe I'm missing something major here, but why? Afaict it's just only
>> used for formatting decisions that could be made without it just as well?
> Uh, well, formatting decisions is what pgindent does. It is about
> identifying variable declarations.
It's conceivable that a tool could incorporate automatic identification
of typedef names ... but I wonder how well that would work in the face of
typedefs that are system-specific and exist nowhere in the system headers
of the machine the tool is being run on. We do certainly have code that
references such typedefs; conditionally of course, but pgindent has to
indent everything not only what gets compiled on the machine it runs on.
Again, this is about getting reproducible indentation results. We've
mostly been talking about the cross-PG-branch aspects of that, but it
also has to work across developer platforms.
regards, tom lane
On Mon, May 25, 2015 at 5:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Bruce Momjian <bruce@momjian.us> writes: >> On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote: >>> Something is wrong. See aclchk.c changes. > >> Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0 >> and 9.1, and the use of that as a typedef in 9.4 is certainly odd: > >> - aclitem.ai_grantor = grantorId; >> + aclitem. ai_grantor = grantorId; > > Yeah. I think we might've gotten rid of that typedef partially in order > to fix this. > > A different strategy we could consider is "use HEAD's typedef list > even in the back branches". This would in some situations lead to > inferior-looking results in the back branches, but that's probably better > than inferior results in HEAD. (In any case, we want the same typedef > list across all branches. Then anyplace where the results diverge, there > must have been non-pgindent code changes, so that back-patching would > require manual fixups anyway.) This is kind of why I think that reindenting the back branches is unlikely to be productive: it only helps if you can get pgindent to do the same thing on all branches, and I bet that's going to be tough. Realistically, with merge.conflictstyle = diff3 (why is this not the default?), resolving whitespace conflicts that occur when you try to cherry-pick is typically not very difficult. But every time we pgindent, especially with slightly different settings, we cause tools like 'git blame' to return less useful answers. And that sucks. We also risk breaking private patchsets that people are carrying - of course, Advanced Server is one (very large) such patchset, but it's hardly the only place where people are compiling a non-standard distribution that has to be reconciled with upstream changes. I'm not going to put up a huge fuss if we decide to go ahead with this, but I still think it's a bad plan, especially with regarding to existing branches that have not been re-indented in years. I bet it won't save that much back-patching effort - maybe not any, on net - and I bet it will inconvenience users and developers in various subtle ways that we may not even hear about but which are still quite real. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Realistically, with merge.conflictstyle = diff3 (why is this not the
> default?), resolving whitespace conflicts that occur when you try to
> cherry-pick is typically not very difficult.
Really? The problems I have generally come from places where pgindent
has changed the line breaks, not just horizontal spacing. I haven't
seen anything that copes with this, certainly not git.
regards, tom lane
Robert Haas wrote: > But every time we pgindent, especially with slightly different > settings, we cause tools like 'git blame' to return less useful > answers. And that sucks. I've wondered a few times whether there's a way to make pgindent commits "transparent" to git blame, i.e. blame their modified lines to whatever commits modified them immediately before. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/25/2015 05:34 PM, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: >> On Mon, May 25, 2015 at 04:52:38PM -0300, Alvaro Herrera wrote: >>> Something is wrong. See aclchk.c changes. >> Yes, this is what I was concerned about. "aclitem" was a typedef in 9.0 >> and 9.1, and the use of that as a typedef in 9.4 is certainly odd: >> - aclitem.ai_grantor = grantorId; >> + aclitem. ai_grantor = grantorId; > Yeah. I think we might've gotten rid of that typedef partially in order > to fix this. > > A different strategy we could consider is "use HEAD's typedef list > even in the back branches". This would in some situations lead to > inferior-looking results in the back branches, but that's probably better > than inferior results in HEAD. (In any case, we want the same typedef > list across all branches. Then anyplace where the results diverge, there > must have been non-pgindent code changes, so that back-patching would > require manual fixups anyway.) > > A longer-term fix would be to make pgindent less stupid about this sort > of usage, but nobody's yet volunteered to dig into the guts of that code. > > It looks to me like this says we should use the typedefs for each branch in any pgindent run for that branch, with the list being fetched just before each run, so it reflects any backpatches, bug fixes etc. The buildfarm collects these lists for all live branches now (or at least my animals do) and keeps them up to date. Anything else is likely to lead to false positives with results like that above. cheers andrew
On Tue, May 26, 2015 at 3:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
> Realistically, with merge.conflictstyle = diff3 (why is this not the
> default?), resolving whitespace conflicts that occur when you try to
> cherry-pick is typically not very difficult.
Really? The problems I have generally come from places where pgindent
has changed the line breaks, not just horizontal spacing. I haven't
seen anything that copes with this, certainly not git.
Iif pgindet were easy to run, committers could start complaining if patch submissions don't abide by pg coding style conventions.
Part of submitting a patch would be making sure that an "pgindent run" after the patch has been applied is still a no-op... A reviewer could easily check it, and a committer could easily squash the "pgindent run" result in if they wanted to be nice to a 1st time submitter...
If every patch were "pgindent clean", then you would never end up with these huge pgindent commits causing pain...
a.
On 2015-05-26 16:32:42 -0300, Alvaro Herrera wrote: > I've wondered a few times whether there's a way to make pgindent commits > "transparent" to git blame, i.e. blame their modified lines to whatever > commits modified them immediately before. You can make blame ignore whitespace changes with -w -- but that obviously doesn't help with rewrapped lines and such.
On 5/25/15 7:15 PM, Andres Freund wrote: > On 2015-05-25 19:01:28 -0400, Bruce Momjian wrote: >>> A longer-term fix would be to make pgindent less stupid about this sort >>> of usage, but nobody's yet volunteered to dig into the guts of that code. >> >> I assume a typedefs list is going to be a requirement of any decent C >> indenting tool. > > Maybe I'm missing something major here, but why? Afaict it's just only > used for formatting decisions that could be made without it just as well? AFAICT, the main reason is to decide whether * and & are binary infix or unary prefix operators. Otherwise, it wouldn't know whether to write char * foo; or the more customary char *foo; Now, running pgindent without a typedefs list also makes it do things like static int32 -makepol(QPRS_STATE *state) +makepol(QPRS_STATE * state) which, one might argue, it could figure out without a typedefs list. But then the formatting would be inconsistent between prototypes and variable declarations, which might drive people crazy. I don't know whether there is a better way than living with it, one way or the other (i.e., requiring a types list, or accepting slightly odd formatting).
On 2015-05-26 20:25:24 -0400, Peter Eisentraut wrote: > On 5/25/15 7:15 PM, Andres Freund wrote: > > On 2015-05-25 19:01:28 -0400, Bruce Momjian wrote: > >>> A longer-term fix would be to make pgindent less stupid about this sort > >>> of usage, but nobody's yet volunteered to dig into the guts of that code. > >> > >> I assume a typedefs list is going to be a requirement of any decent C > >> indenting tool. > > > > Maybe I'm missing something major here, but why? Afaict it's just only > > used for formatting decisions that could be made without it just as well? > > AFAICT, the main reason is to decide whether * and & are binary infix or > unary prefix operators. Otherwise, it wouldn't know whether to write > > char * foo; > > or the more customary > > char *foo; > > Now, running pgindent without a typedefs list also makes it do things like > > static int32 > -makepol(QPRS_STATE *state) > +makepol(QPRS_STATE * state) > > which, one might argue, it could figure out without a typedefs list. > But then the formatting would be inconsistent between prototypes and > variable declarations, which might drive people crazy. I don't know > whether there is a better way than living with it, one way or the other > (i.e., requiring a types list, or accepting slightly odd formatting). I actually think both are relatively easy to figure out without a typedef list. There's harder cases though, e.g. (char *) &foo in an expression is already more complicated. But really, the typedef list is the minor part what annoys me about pgindent. That it completely butchers so many constructs (e.g. function pointer typedefs, inline asm as extreme examples) is much worse. It's also neigh on impossible to predict/keep the indentation pgindent will use in many cases. Having to try to write code in a way that doesn't break the re-indentation tool, even if it'd otherwise be fine, is just absurd. Greetings, Andres Freund
On 5/25/15 5:51 PM, Alvaro Herrera wrote: > Tom Lane wrote: > >> A longer-term fix would be to make pgindent less stupid about this sort >> of usage, but nobody's yet volunteered to dig into the guts of that code. > > We've discussed in the past that we could use something other than BSD's > indent -- astyle has been mentioned. It seems that with suitable > options, we could make the result very close to what we have now, and > not be forced to deal with typedef lists and other nonsense. astyle looks like a decent tool, but it seems to me that it tends to leave things alone that it doesn't have an explicit rule about. So that would leave a lot of room for formatting randomness from different authors.
On Tue, May 26, 2015 at 3:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Realistically, with merge.conflictstyle = diff3 (why is this not the >> default?), resolving whitespace conflicts that occur when you try to >> cherry-pick is typically not very difficult. > > Really? The problems I have generally come from places where pgindent > has changed the line breaks, not just horizontal spacing. I haven't > seen anything that copes with this, certainly not git. Well, it's not fully automated, but if you set the setting above, and then cherry-pick, your merge conflicts will look something like this: <<<<<<< side A ||||||| original version ======= side B >>>>>>> Either side A or side B will be what changed in the patch you cherry-picked, and the other will be what changed in the branch in the meantime. I forget which is which. So you notice that one of the two sides differs from the original version only in terms of whitespace and delete that side, keeping the other side. Done. In general, the way you resolve these conflicts is by choosing the side that has fewer changes from the original version, noting how it differs from the original version, modifying the other side accordingly, and then deleting the other two versions. For example: <<<<<<< here we renamed the function! ||||||| original version ======= here we added an additional parameter to the function call! >>>>>>> So you either change side B to the new function name and remove side A, or else you change side A to pass the extra parameter and remove side B. In either case you remove the original version. This is obviously not zero effort. At the same time, it's not much effort, either. I resolve these kinds of mechanical conflicts all the time, and they don't take up much time or effort. If you have to deal with this kind of crap using "patch", it bites. If you use git but with the default conflictstyle, you don't get the "original version" part of the conflict, so it still bites. But after a modest amount of practice, resolving trivial conflicts with merge.conflictstyle=diff3 is pretty darn easy. Or at least, I have found it so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, May 26, 2015 at 04:32:42PM -0300, Alvaro Herrera wrote: > Robert Haas wrote: > > > But every time we pgindent, especially with slightly different > > settings, we cause tools like 'git blame' to return less useful > > answers. And that sucks. > > I've wondered a few times whether there's a way to make pgindent commits > "transparent" to git blame, i.e. blame their modified lines to whatever > commits modified them immediately before. I wonder if it might be a good idea to separate whitespace changes within a line from line breaks changes. You could do something like a script (it could be used as a git hook) that only enforces or warns about line-break style rules (which are easier to get right, I think), and have a mode of pgindent that only changes whitespace within a line and warns about line break style problems. So an author could be more or less on the hook to have acceptable line-breaks and fix that on their end and intra-line spacing could be fixed en mass with little impact. I can't figure out how painful this would be in practice. It's probably not worth it.... Garick
On Wed, May 27, 2015 at 02:31:07AM +0200, Andres Freund wrote: > But really, the typedef list is the minor part what annoys me about > pgindent. That it completely butchers so many constructs (e.g. function > pointer typedefs, inline asm as extreme examples) is much worse. It's > also neigh on impossible to predict/keep the indentation pgindent will > use in many cases. Having to try to write code in a way that doesn't > break the re-indentation tool, even if it'd otherwise be fine, is just > absurd. What does "break" mean here? Considering we are indenting 1.4M lines of code, skipping ASM files seems pretty minor. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 05/27/2015 11:53 AM, Bruce Momjian wrote: > On Wed, May 27, 2015 at 02:31:07AM +0200, Andres Freund wrote: >> But really, the typedef list is the minor part what annoys me about >> pgindent. That it completely butchers so many constructs (e.g. function >> pointer typedefs, inline asm as extreme examples) is much worse. It's >> also neigh on impossible to predict/keep the indentation pgindent will >> use in many cases. Having to try to write code in a way that doesn't >> break the re-indentation tool, even if it'd otherwise be fine, is just >> absurd. > What does "break" mean here? Considering we are indenting 1.4M lines of > code, skipping ASM files seems pretty minor. > That's not a bad bit of perspective. One thing that might ease some pain would a facility to tell pgindent to leave a block of code alone. That wouldn't be terribly hard to create. The perl code could save those blocks out in the pre_indent function, which would return them along with the source. they would then be passed to the post_indent function which would restore them. All we would need would be a pair or markers to delimit such blocks. Something like: /* PGINDENT_PRESERVE */ and /* PGINDENT_END_PRESERVE */ should do the trick. I imagine we could probably do this in 50 lines of perl or less. Worth the trouble? cheers andrew
Andres Freund <andres@anarazel.de> writes:
> But really, the typedef list is the minor part what annoys me about
> pgindent. That it completely butchers so many constructs (e.g. function
> pointer typedefs, inline asm as extreme examples) is much worse.
These are all things we might try to fix (where "fix" could include
"replace it with another tool") if the back-patching pain created by even
minor changes of the formatting rules weren't so great. But at this point
I despair of getting to consensus on a way to relieve that pain. Robert's
position seems to be that there is no such pain, which I beg to differ
with, but given that position he's naturally unwilling to accept any
invasive measures to alleviate it.
> It's
> also neigh on impossible to predict/keep the indentation pgindent will
> use in many cases. Having to try to write code in a way that doesn't
> break the re-indentation tool, even if it'd otherwise be fine, is just
> absurd.
Not clear to me why you need to "predict" anything ... just run the tool
and see what it does. Admittedly, we should do more to make it easy for
occasional contributors to use the tool, but I do not think it's
unreasonable to expect committers to have it set up and use it.
regards, tom lane
On 5/26/15 8:31 PM, Andres Freund wrote: > I actually think both are relatively easy to figure out without a > typedef list. There's harder cases though, e.g. (char *) &foo in an > expression is already more complicated. Well, if you know of a way to fix this, let's see it. Others have been trying for 20+ years.
On 2015-05-27 16:55:45 -0400, Peter Eisentraut wrote:
> On 5/26/15 8:31 PM, Andres Freund wrote:
> > I actually think both are relatively easy to figure out without a
> > typedef list. There's harder cases though, e.g. (char *) &foo in an
> > expression is already more complicated.
>
> Well, if you know of a way to fix this, let's see it. Others have been
> trying for 20+ years.
I don't think I need to. clang-format has apparently done pretty much
what I described:
typedef struct foo {int a} foo;
struct bar;
int frak(struct bar * barstar, foo * foostar, unknown * unknown)
{struct bar * barstar2;foo * foostar2;int * intstar2;unknown * unknown2;
pointless * operation;a = foostart * barstar2; x = (frak *) & blub;
}
=>
typedef struct foo
{int a
} foo;
struct bar;
int
frak(struct bar *barstar, foo *foostar, unknown *unknown)
{struct bar *barstar2;foo *foostar2;int *intstar2;unknown *unknown2;
pointless *operation;a = foostart * barstar2; x = (frak *) &blub;
}
Yes, it gets pointless * operation wrong. But boohoo.
On 5/27/15 2:55 PM, Tom Lane wrote: > These are all things we might try to fix (where "fix" could include > "replace it with another tool") if the back-patching pain created by even > minor changes of the formatting rules weren't so great. But at this point > I despair of getting to consensus on a way to relieve that pain. Robert's > position seems to be that there is no such pain, which I beg to differ > with, but given that position he's naturally unwilling to accept any > invasive measures to alleviate it. I don't understand how this can be such a major problem. Both the master branch and the back branch were once formatted by pgindent. The only way things can diverge is if a poorly formatted patch is committed to master, then backpatched, then pgindented in master but not in the back branches, and then another patch needs to be applied on top of that. The way to alleviate that problem is to make it easier and more likely that the initial commit matches the preferred format and does not need to be reindented later in major ways. But the current formatting standard makes that very difficult for a number of reasons, to the point that basically everyone has given up on it and lets pgindent sort it out later. (This is a circular problem to an extent, of course.) And even if we got to the point where all commits should be perfectly pgindented, it wouldn't work, because under the current workflow the updated typedef list isn't available until after the commit (on an unpredictable schedule). (This problem would also affect pgindent in back branches.) If we found a tool setup (either a new tool, or a different (pg)indent configuration) that addresses these problems and is future-proof, then I think we could have a useful discussion about whether we want to reformat the backbranches once or repeatedly or perhaps not. But I don't see such a tool, and I've looked. So with the current setup, I think reformatting master once a year and leaving the back branches alone is the best scenario.
On 05/27/2015 05:19 PM, Peter Eisentraut wrote: > > And even if we got to the point where all commits should be perfectly > pgindented, it wouldn't work, because under the current workflow the > updated typedef list isn't available until after the commit (on an > unpredictable schedule). (This problem would also affect pgindent in > back branches.) Up to date typedefs lists are available for all live branches all the time. That's something I have enabled with a fairly modest investment of time in response to this discussion. See <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list> Of course, there is a chicken and egg problem, but after all how many new typedefs are there likely to be in a given backpatched item? ISTM new typedefs are far more likely to appear from new development than from bug fixes. At a pinch you could add some yourself manually. cheers andrew
On 5/27/15 5:08 PM, Andres Freund wrote: > I don't think I need to. clang-format has apparently done pretty much > what I described: Well, that appears to work reasonably well in practice, which is all we can hope for. Unfortunately, clang-format makes a bit of a mess of some of our code, so it isn't quite ready. (There is hope, however. I recall significant improvements between 3.5 and 3.6.) For amusement, I have attached a .clang-format that I have been working on.
Attachment
On Tue, May 26, 2015 at 2:55 PM, Robert Haas <robertmhaas@gmail.com> wrote: > This is kind of why I think that reindenting the back branches is > unlikely to be productive: it only helps if you can get pgindent to do > the same thing on all branches, and I bet that's going to be tough. ...but having said that and thought about this a bit more, we could actually test that rather than guessing. Suppose somebody goes and writes a script which diffs the version of each file on master with the version of the same file, if it exists, on each stable branch. And then they indent each back-branch on a private branch and do the same thing again. And then they produce a report of every file where the number of lines that differ went up rather than down. Then, we could look at the files where things got worse and try to fix whatever the issue is. Of course, it would be nice to be even more fine-grained and try to figure out whether there are files where, although the file overall ended up with fewer differences, some individual places in the file diverged. Maybe somebody with sufficient git-fu could figure out how to do that. But even without that, it seems to me that with some work, we could probably measure how good an outcome we're actually going to get here. What I really don't want to do is apply the pgindent diff somewhat blindly, without really knowing how many cases we're improving and how many cases we're making worse. The number of times we've run pgindent and then realized later that it messed a bunch of stuff up is actually pretty high, and I find doing that to the back-branches particularly unexciting. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Jun 9, 2015 at 09:56:13PM -0400, Robert Haas wrote: > What I really don't want to do is apply the pgindent diff somewhat > blindly, without really knowing how many cases we're improving and how > many cases we're making worse. The number of times we've run pgindent > and then realized later that it messed a bunch of stuff up is actually > pretty high, and I find doing that to the back-branches particularly > unexciting. It is doing +1M lines of C code --- I assume it always will mess something up. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +