Обсуждение: pgindent run next week?
We should do a pgindent run fairly soon, so that people with patches awaiting the next CF will have plenty of time to rebase them as necessary. I don't want to do it right this minute, to avoid making trouble for the several urgent patches we're trying to get done before Monday's beta1 wrap. But after the beta is tagged seems like it'd be a good time. Also, how do people feel about adopting the function prototype indenting change discussed in https://www.postgresql.org/message-id/flat/CAEepm%3D0P3FeTXRcU5B2W3jv3PgRVZ-kGUXLGfd42FFhUROO3ug%40mail.gmail.com ? The required change in pg_bsd_indent isn't quite done, but it could be done by next week. regards, tom lane
On Fri, May 17, 2019 at 10:29:46AM -0400, Tom Lane wrote: > We should do a pgindent run fairly soon, so that people with patches > awaiting the next CF will have plenty of time to rebase them as necessary. > I don't want to do it right this minute, to avoid making trouble for the > several urgent patches we're trying to get done before Monday's beta1 > wrap. But after the beta is tagged seems like it'd be a good time. > > Also, how do people feel about adopting the function prototype > indenting change discussed in > > https://www.postgresql.org/message-id/flat/CAEepm%3D0P3FeTXRcU5B2W3jv3PgRVZ-kGUXLGfd42FFhUROO3ug%40mail.gmail.com > > ? The required change in pg_bsd_indent isn't quite done, but it > could be done by next week. Yes, I think we are good with everything above. I am thinking you should do the run since you did the pg_indent modifications. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Hi, On 2019-05-17 10:29:46 -0400, Tom Lane wrote: > We should do a pgindent run fairly soon, so that people with patches > awaiting the next CF will have plenty of time to rebase them as > necessary. +1 > I don't want to do it right this minute, to avoid making trouble for the > several urgent patches we're trying to get done before Monday's beta1 > wrap. But after the beta is tagged seems like it'd be a good time. +1 > Also, how do people feel about adopting the function prototype > indenting change discussed in > https://www.postgresql.org/message-id/flat/CAEepm%3D0P3FeTXRcU5B2W3jv3PgRVZ-kGUXLGfd42FFhUROO3ug%40mail.gmail.com I think it'd be a huge improvement. I find it pretty annoying having to figure out the indentations to avoid unnecessary pgindent changes (after Thomas' explanation as to why it happens, I usually just add a linebreak after the return type, indent everything, and remove it). Would we want to also apply this to the back branches to avoid spurious conflicts? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2019-05-17 10:29:46 -0400, Tom Lane wrote: >> Also, how do people feel about adopting the function prototype >> indenting change discussed in >> https://www.postgresql.org/message-id/flat/CAEepm%3D0P3FeTXRcU5B2W3jv3PgRVZ-kGUXLGfd42FFhUROO3ug%40mail.gmail.com > I think it'd be a huge improvement. Yeah, that's probably the biggest remaining bug/issue in pgindent. > Would we want to also apply this to the back branches to avoid spurious > conflicts? I dunno, how far back are you thinking? I've occasionally wished we could reindent all the back branches to match HEAD, but realistically, people carrying out-of-tree patches would scream. regards, tom lane
On Fri, May 17, 2019 at 01:47:02PM -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-05-17 10:29:46 -0400, Tom Lane wrote: > >> Also, how do people feel about adopting the function prototype > >> indenting change discussed in > >> https://www.postgresql.org/message-id/flat/CAEepm%3D0P3FeTXRcU5B2W3jv3PgRVZ-kGUXLGfd42FFhUROO3ug%40mail.gmail.com > > > I think it'd be a huge improvement. > > Yeah, that's probably the biggest remaining bug/issue in pgindent. > > > Would we want to also apply this to the back branches to avoid spurious > > conflicts? > > I dunno, how far back are you thinking? I've occasionally wished we > could reindent all the back branches to match HEAD, but realistically, > people carrying out-of-tree patches would scream. My regular backpatch pain is SGML files. :-( -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Hi, On 2019-05-17 13:47:02 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Would we want to also apply this to the back branches to avoid spurious > > conflicts? > > I dunno, how far back are you thinking? I've occasionally wished we > could reindent all the back branches to match HEAD, but realistically, > people carrying out-of-tree patches would scream. I somehow thought we'd backpatched pgindent changes before, around when moving to the newer version of indent. But I think we might just have discussed that, and then didn't go for it... Not sure if a three-way merge wouldn't take care of many, but not all, the out-of-tree patch concerns. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2019-05-17 13:47:02 -0400, Tom Lane wrote:
>> I dunno, how far back are you thinking? I've occasionally wished we
>> could reindent all the back branches to match HEAD, but realistically,
>> people carrying out-of-tree patches would scream.
> I somehow thought we'd backpatched pgindent changes before, around when
> moving to the newer version of indent. But I think we might just have
> discussed that, and then didn't go for it...
Yeah, we talked about it but never actually did it.
> Not sure if a three-way merge wouldn't take care of many, but not all,
> the out-of-tree patch concerns.
I was wondering about "patch --ignore-whitespace" myself. In theory,
to the extent that our recent rounds of pgindent fixes just change
indentation, that would be able to cope (most of the time anyway).
But I don't think I'd want to just assume that without testing.
Anybody around here got large patches they're carrying against
back branches, that they could try reapplying after running
a newer version of pgindent?
regards, tom lane
> On May 17, 2019, at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Andres Freund <andres@anarazel.de> writes: >> On 2019-05-17 13:47:02 -0400, Tom Lane wrote: >>> I dunno, how far back are you thinking? I've occasionally wished we >>> could reindent all the back branches to match HEAD, but realistically, >>> people carrying out-of-tree patches would scream. > >> I somehow thought we'd backpatched pgindent changes before, around when >> moving to the newer version of indent. But I think we might just have >> discussed that, and then didn't go for it... > > Yeah, we talked about it but never actually did it. > >> Not sure if a three-way merge wouldn't take care of many, but not all, >> the out-of-tree patch concerns. > > I was wondering about "patch --ignore-whitespace" myself. In theory, > to the extent that our recent rounds of pgindent fixes just change > indentation, that would be able to cope (most of the time anyway). > But I don't think I'd want to just assume that without testing. > > Anybody around here got large patches they're carrying against > back branches, that they could try reapplying after running > a newer version of pgindent? I have forks of 9.1 and 9.5 that each amount to large changes against the public sources, though I consider those forks to be defunct. If you want me to run some particular version of pg_indent against the public sources of 9.1 and 9.5 and then try to merge the changed sources into my forks, I could give it a try. I'm not sure if this is the sort of thing you have in mind.... mark
Mark Dilger <hornschnorter@gmail.com> writes:
> On May 17, 2019, at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Anybody around here got large patches they're carrying against
>> back branches, that they could try reapplying after running
>> a newer version of pgindent?
> I have forks of 9.1 and 9.5 that each amount to large changes
> against the public sources, though I consider those forks to be
> defunct. If you want me to run some particular version of pg_indent
> against the public sources of 9.1 and 9.5 and then try to merge the
> changed sources into my forks, I could give it a try. I'm not
> sure if this is the sort of thing you have in mind....
9.1 is probably too far back to be interesting, but it'd be good
to try the experiment with your 9.5 fork.
Assuming you only want to do this once, I'd suggest waiting till
I push the function-prototype changes to the pg_bsd_indent repo,
and then use that along with the latest pgindent.
regards, tom lane
Andres Freund <andres@anarazel.de> writes: > On 2019-05-17 10:29:46 -0400, Tom Lane wrote: >> We should do a pgindent run fairly soon, so that people with patches >> awaiting the next CF will have plenty of time to rebase them as >> necessary. >> I don't want to do it right this minute, to avoid making trouble for the >> several urgent patches we're trying to get done before Monday's beta1 >> wrap. But after the beta is tagged seems like it'd be a good time. > +1 Hearing no objections, I'll plan on running pgindent tomorrow sometime. The new underlying pg_bsd_indent (2.1) is available now from https://git.postgresql.org/git/pg_bsd_indent.git if anyone wants to do further testing on it. (To use it with current pgindent, adjust the INDENT_VERSION value in that script. You don't really need to do anything else; the code rendered unnecessary by this change won't do anything.) > Would we want to also apply this to the back branches to avoid spurious > conflicts? I think we should hold off on any talk of that until we get some results from Mark Dilger (or anyone else) on how much pain it would cause for people carrying private patches. regards, tom lane
I wrote: > Hearing no objections, I'll plan on running pgindent tomorrow sometime. And done. > The new underlying pg_bsd_indent (2.1) is available now from > https://git.postgresql.org/git/pg_bsd_indent.git Please update your local copy if you have one. regards, tom lane
On Wed, May 22, 2019 at 10:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: > > Hearing no objections, I'll plan on running pgindent tomorrow sometime. > > And done. > > > The new underlying pg_bsd_indent (2.1) is available now from > > https://git.postgresql.org/git/pg_bsd_indent.git > > Please update your local copy if you have one. > > regards, tom lane > I cloned, built and used the new pg_bsd_indent to format my fork of PostgreSQL 11 (not the 9.1 or 9.5 forks I previously mentioned) and it caused me no problems whatsoever. I don't have a strong preference, but I would vote in favor of running pgindent on the back branches rather than against, since to the extent that I might need to move patches between forks of different versions, it will be easier to do if they have the same indentation. (In practice, this probably won't come up for me, since the older forks are defunct and unlikely to be patched by me.) mark
On 2019-05-21 23:46, Tom Lane wrote: >> Would we want to also apply this to the back branches to avoid spurious >> conflicts? > I think we should hold off on any talk of that until we get some results > from Mark Dilger (or anyone else) on how much pain it would cause for > people carrying private patches. In my experience, changes to function declarations in header files happen a lot in forks. So applying the pgindent change to backbranches would cause some trouble. On the other hand, it seems to me that patches that we backpatch between PostgreSQL branches should normally not touch function declarations in header files, since that would be an ABI break. So by not applying the pgindent change in backbranches we don't lose anything. And so it would be better to just leave things as they are. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> In my experience, changes to function declarations in header files
> happen a lot in forks. So applying the pgindent change to backbranches
> would cause some trouble.
> On the other hand, it seems to me that patches that we backpatch between
> PostgreSQL branches should normally not touch function declarations in
> header files, since that would be an ABI break. So by not applying the
> pgindent change in backbranches we don't lose anything. And so it would
> be better to just leave things as they are.
Maybe we could wait awhile and see how much pain we find in back-patching
across this change. I have to admit that the v10 pgindent changes have
not been as painful as I expected them to be, so maybe this round will
also prove to be just an annoyance not a major PITA for that.
Another thought is that, at least in principle, we could re-indent only
.c files not .h files in the back branches. But I'm not sure I believe
your argument that forks are more likely to touch exposed extern
declarations than local static declarations.
regards, tom lane
Em qua, 22 de mai de 2019 às 14:08, Tom Lane <tgl@sss.pgh.pa.us> escreveu: > > I wrote: > > Hearing no objections, I'll plan on running pgindent tomorrow sometime. > > And done. > > > The new underlying pg_bsd_indent (2.1) is available now from > > https://git.postgresql.org/git/pg_bsd_indent.git > > Please update your local copy if you have one. > I give it a try in a fork of PostgreSQL 10. The difference between v10 and my fork is not huge. The stats are 56 files changed, 2240 insertions(+), 203 deletions(-) and patch size is 139 Kb. I have conflicts in 3 of 19 .h files and 1 of 25 .c files. Like Mark, I don't have a strong preference, however, re-indent files would reduce developer time while preparing patches to back branches. -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento