Re: WTF?
On Wed, 25 Nov 2009, Dave Korn wrote: But does it, though? From http://gcc.gnu.org/svnwrite.html: [...] So, where are whitespace changes to non-comment parts of .c and .h source files covered? I think that there may be a bit of a common assumption that obvious extends somewhat further than the wording of the documentation actually implies - not just in the context of this incident, but the question has occurred to me in other cases too, and maybe now would be a good time to clear it up. So... On Wed, 25 Nov 2009, Kaveh R. Ghazi wrote: I agree the wording could be better. ...does one of you have a suggestion on how to improve the wording? The svnwrite.html page never was ment to be the law, more like documenting best practises and some rules of thumb, but of course improvements will be welcome. Gerald
Re: WTF?
On Thu, 2009-11-26 at 00:08 +0100, Richard Guenther wrote: On Thu, Nov 26, 2009 at 12:04 AM, Tom Tromey tro...@redhat.com wrote: Basile == Basile STARYNKEVITCH bas...@starynkevitch.net writes: Basile Of course, not every one has it (notably those working on non-linux Basile systems), but for those who have it, requiring that every C file Basile inside GCC has been automatically indented with GNU indent could Basile help. I looked at this once. GNU indent doesn't have all the features needed to make it correctly support the GCC coding style. I filed a bunch of GNU indent bug reports, but AFAIK none of them has ever been fixed. You could still do this if you didn't mind a coding style change at the same time. And of course, this will only help the .c and .h files, not everything else. Not to mention that I'd find this a quite pointless excercise. I trust humans much more than the little brain of GNU indent (or whatever you can code into a reasonable replacement). I don't think we need to go that far. An SVN pre-commit filter (default on, disabled by SVN attribute if needed) should instead just reject files that have trailing white space. R
Re: WTF?
On Wed, 2009-11-25 at 18:02 +0100, Michael Matz wrote: Hi, On Wed, 25 Nov 2009, Richard Guenther wrote: Remove trailing white spaces. WTF? Thankyouverymuch. This 1) wasn't posted or approved 2) is bad as it breaks svn blame 3) chokes all branches. What's up? Can someone please remove this revision from the subversion database on the server and fix things up? Someone with the appropriate rights needs to shutdown the svn server so that no additional commits can be done, then the revision files in db/revs/ and db/revprops/ starting with the wrong revision need to be removed, then db/current needs to be changed appropriately. PLEASE DO NOT DO THIS. No matter what the pain for a few individuals, doing this will break many mirrors and other users of the repository in completely irrecoverable ways. R.
Re: WTF?
On Wed, 2009-11-25 at 20:38 +0100, Richard Guenther wrote: If you can offer advice on how to teach quilt (which I belive uses patch) to ignore whitespace changes when applying patches then more power to you QUILT_PATCH_OPTS=-l
SVN pre-commit filter (Was: Re: WTF?)
Quoting Richard Earnshaw rearn...@arm.com: An SVN pre-commit filter (default on, disabled by SVN attribute if needed) should instead just reject files that have trailing white space. I think a better mechanism to deal with exceptions is to have a property that describes the current misformatting (or unusual formatting for some test cases / Makefiles). I.e. if a file is known to have N lines with trailing whitespace, M with spaces-for-tabs, K with spaces-hidden-in-front-of-tabs, and L with carriage return, a patch will be rejected if it increases N, M, K or L. If it decreases it, the count property can be auto-adjusted to prevent regressions. And if you really have to, you can adjust a property by hand before checking in a patch that increases one of the counts.
Re: SVN pre-commit filter (Was: Re: WTF?)
On Fri, 2009-11-27 at 09:40 -0500, Joern Rennecke wrote: Quoting Richard Earnshaw rearn...@arm.com: An SVN pre-commit filter (default on, disabled by SVN attribute if needed) should instead just reject files that have trailing white space. I think a better mechanism to deal with exceptions is to have a property that describes the current misformatting (or unusual formatting for some test cases / Makefiles). I.e. if a file is known to have N lines with trailing whitespace, M with spaces-for-tabs, K with spaces-hidden-in-front-of-tabs, and L with carriage return, a patch will be rejected if it increases N, M, K or L. If it decreases it, the count property can be auto-adjusted to prevent regressions. And if you really have to, you can adjust a property by hand before checking in a patch that increases one of the counts. Sounds like unnecessary over-engineering to me. 99% of files should have no trailing white space. The few that do probably have it for some good reason and policing this by explicit count is most likely pointless. R.
Re: SVN pre-commit filter (Was: Re: WTF?)
On Fri, 27 Nov 2009, Richard Earnshaw wrote: On Fri, 2009-11-27 at 09:40 -0500, Joern Rennecke wrote: Quoting Richard Earnshaw rearn...@arm.com: An SVN pre-commit filter (default on, disabled by SVN attribute if needed) should instead just reject files that have trailing white space. I think a better mechanism to deal with exceptions is to have a property that describes the current misformatting (or unusual formatting for some test cases / Makefiles). I.e. if a file is known to have N lines with trailing whitespace, M with spaces-for-tabs, K with spaces-hidden-in-front-of-tabs, and L with carriage return, a patch will be rejected if it increases N, M, K or L. If it decreases it, the count property can be auto-adjusted to prevent regressions. And if you really have to, you can adjust a property by hand before checking in a patch that increases one of the counts. Sounds like unnecessary over-engineering to me. 99% of files should have no trailing white space. The few that do probably have it for some good reason and policing this by explicit count is most likely pointless. Indeed, the obvious way to filter would be to disallow converting a file with good formatting into one with bad formatting, or adding new files with bad formatting, on trunk, with some general patterns for exceptions (binary files, generated files that bring in text from outside the GCC sources, etc.). If you need to commit a new file with bad formatting, edit the hooks before and after (or just before if it can be described by a new general pattern). If a file already has bad formatting, the hook wouldn't check changes to it (it would be possible to have multiple notions of bad, each checked independently). -- Joseph S. Myers jos...@codesourcery.com
Re: WTF?
On 11/26/2009 12:20 AM, Alexandre Oliva wrote: sed -i 's,[ \t]*$,,' probably won't work, if there are all-blanks lines being left alone in the patch (so the rx will match the patch markers too), but something slightly more elaborate preserving a fixed number of leading blanks dependng on the patch type (context or unified) should: Actually an empty line is considered by patch(1) the same way as a line containing only a space (or two spaces for context diffs), because there are many mailers that strip trailing whitespace. Paolo
Re: WTF?
On Thu, Nov 26, 2009 at 9:56 AM, Paolo Bonzini bonz...@gnu.org wrote: On 11/26/2009 12:20 AM, Alexandre Oliva wrote: sed -i 's,[ \t]*$,,' probably won't work, if there are all-blanks lines being left alone in the patch (so the rx will match the patch markers too), but something slightly more elaborate preserving a fixed number of leading blanks dependng on the patch type (context or unified) should: Actually an empty line is considered by patch(1) the same way as a line containing only a space (or two spaces for context diffs), because there are many mailers that strip trailing whitespace. Does svn rely on patch for merging and can it be taught to use -l? Richard.
Re: WTF?
Richard Guenther richard.guent...@gmail.com writes: Does svn rely on patch for merging No, it uses an internal diff3 algorithm. You can make it use an external diff3 program via the command line or the config file. and can it be taught to use -l? The internal diff/merge will ignore whitespace if you use -x -w. -- Philip
Re: WTF?
On 2009-11-25 17:49:39 +0100, Richard Guenther wrote: On Wed, 25 Nov 2009, Richard Kenner wrote: Why the latter? I agree with the problems this can cause, but if they can't be fixed by removing it from the database, why revert it? All things being equal, trailing blanks in fact aren't a good idea. Because it makes branch merging a nightmare and all patches people keep are hosed and won't apply anymore. Except if patches come from some web page (e.g. mailing-list archives) where trailing spaces are stripped out. In that case, patches will magically apply without any problem, whereas before the change they wouldn't. :) I've had such problems in the past with other projects, where I was told to apply some patch only available on the web, but it wouldn't apply since the source had trailing whitespace and the patch didn't (because some tool not specifically written to handle patches stripped such whitespace). -- Vincent Lefèvre vinc...@vinc17.net - Web: http://www.vinc17.net/ 100% accessible validated (X)HTML - Blog: http://www.vinc17.net/blog/ Work: CR INRIA - computer arithmetic / Arénaire project (LIP, ENS-Lyon)
Re: WTF?
On 2009-11-26 10:12:39 +1100, Ben Elliston wrote: On Wed, 2009-11-25 at 15:17 -0500, Kaveh R. GHAZI wrote: Finally, we have a process for reverting a patch. If anyone wants to revert some part, it needs to be followed. Otherwise *that* would be breaking the rules... Won't reverting the patch just attribute all of the lines to the username of the reverter? Probably not if this is done with svn cp from the previous revision (and the patch won't appear in the log anymore). But I don't know the side effects of such a svn cp on merges and so on. -- Vincent Lefèvre vinc...@vinc17.net - Web: http://www.vinc17.net/ 100% accessible validated (X)HTML - Blog: http://www.vinc17.net/blog/ Work: CR INRIA - computer arithmetic / Arénaire project (LIP, ENS-Lyon)
Re: WTF?
Hi, On Wed, 25 Nov 2009, Richard Kenner wrote: In my mind it's very simple: trailing whitespace poses exactly _no_ problem (except of being against the coding standard), It's against the coding standards for a very good reason, which is that it makes patching harder because you have lines that compare differently but look identical. Wow I like this creative twisting of arguments. So in order to not make patching harder in theory you make patching harder in practice?! Brilliant. So removing them, while making some patches harder to apply, makes others easier into the future. Except if you produce patches by hand this change doesn't make the slightest thing easier in the future. patch and diff will consume and generate applyable .diff files no matter if the material did or didn't contain trailing white space as long as this isn't changed between producing and applying the patch, hence future patch processing is not affected in any way. I also didn't want to know how to make patch work around this commit by loosening it's apply algorithm, neither did I want to know how to mold the patches I have into a form that now applies again. The point is That I Don't Want To Work Around useless unapproved mass changes. I'm also surprised by the hair splitting and bike shedding if the patch was or wasn't obvious. To any reasonable person it must be clear that due to the ripple effect it has it is not obvious and hence has to be reverted already just out of policy reasons. Then we perhaps can discuss further about the merits of the change, and then, after reaching agreement, commit it again; but only then. I'll note that conveniently nobody of the supporters of this change answered Richards question about the positive effects of the change and the seeming absence of real GCC bugs so that we can waste our time on this type of thing. Ciao, Michael.
Re: WTF?
On 11/25/09 16:51, Lu, Hongjiu wrote: Sorry for that. I did send an email and though it was an obvious fix. I guess the patch may be too big, 400K with bz2. Did you get a reject message? If so, that should have caused you to stop and think a little... Jeff
Re: WTF?
Michael Matz wrote: I Don't Want To Work Around useless unapproved mass changes. Hey, nor do I. (In fact, I don't even like having to work around useful approved mass changes, merging is always dull and tedious work. But I think of it as just something that goes with the territory.) To any reasonable person it must be clear I'm a reasonable person. It is not clear to me. the seeming absence of real GCC bugs so that we can waste our time on this type of thing. Actually, I made a quick judgement call when Richard G. first posted about this, and decided that the quickest and most efficient thing I could possibly do to deal with this problem using the least effort and wasting the least time would be to just not care and simply get on with my work. If this thread goes on much longer, there will presumably come a crossover point at which it would have been more worth your while too cheers, DaveK
RE: WTF?
Did you get a reject message? If so, that should have caused you to stop and think a little... I sent: Date: Wed, 25 Nov 2009 02:53:21 -0800 From: H.J. Lu hongjiu...@intel.com To: gcc-patc...@gcc.gnu.org Subject: PATCH: Remove trailing white spaces User-Agent: Mutt/1.5.19 (2009-01-05) Hi, I am checking in this patch to remove trailing white spaces. I didn't get any reject. But apparently it never showed up. H.J.
Re: WTF?
On Wed, 25 Nov 2009, Richard Guenther wrote: Author: hjl Date: Wed Nov 25 10:55:54 2009 New Revision: 154645 URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=154645 Log: Remove trailing white spaces. WTF? Thankyouverymuch. This 1) wasn't posted or approved 2) is bad as it breaks svn blame 3) chokes all branches. What's up? Can someone please remove this revision from the subversion database on the server and fix things up? If that's not possible at least the revision should be reverted. Richard.
Re: WTF?
Can someone please remove this revision from the subversion database on the server and fix things up? If that's not possible at least the revision should be reverted. Why the latter? I agree with the problems this can cause, but if they can't be fixed by removing it from the database, why revert it? All things being equal, trailing blanks in fact aren't a good idea.
Re: WTF?
On Wed, 25 Nov 2009, Richard Kenner wrote: Can someone please remove this revision from the subversion database on the server and fix things up? If that's not possible at least the revision should be reverted. Why the latter? I agree with the problems this can cause, but if they can't be fixed by removing it from the database, why revert it? All things being equal, trailing blanks in fact aren't a good idea. Because it makes branch merging a nightmare and all patches people keep are hosed and won't apply anymore. And because it wasn't approved and earlier suggestions were shot down quickly. Richard.
Re: WTF?
On 11/25/09 09:51, Richard Kenner wrote: Can someone please remove this revision from the subversion database on the server and fix things up? If that's not possible at least the revision should be reverted. Why the latter? I agree with the problems this can cause, but if they can't be fixed by removing it from the database, why revert it? All things being equal, trailing blanks in fact aren't a good idea. I agree with Kenner here. We've encouraged removal of trailing whitespace in the past and I don't why this should be any different. It does make things marginally harder when dealing with branches, but the right way to deal with that problem is to avoid trailing whitespace in the source tree to begin with. The problem as I see it is HJ installed the patch without sending it to the list. Jeff
Re: WTF?
Hi, On Wed, 25 Nov 2009, Richard Guenther wrote: Remove trailing white spaces. WTF? Thankyouverymuch. This 1) wasn't posted or approved 2) is bad as it breaks svn blame 3) chokes all branches. What's up? Can someone please remove this revision from the subversion database on the server and fix things up? Someone with the appropriate rights needs to shutdown the svn server so that no additional commits can be done, then the revision files in db/revs/ and db/revprops/ starting with the wrong revision need to be removed, then db/current needs to be changed appropriately. Currently there are only two more revisions on top of the white space one which don't conflict with it (they touch only stuff in config/arm) so it might optionally be possible to move those additional revs just one earlier instead of removing them. I don't know how well checkouts that already refer to the broken revisions will cope with this. This should be done quickly in order to not get too many revs on top of the wrong one. Ciao, Michael.
Re: WTF?
Hi, On Wed, 25 Nov 2009, Jeff Law wrote: On 11/25/09 09:51, Richard Kenner wrote: Can someone please remove this revision from the subversion database on the server and fix things up? If that's not possible at least the revision should be reverted. Why the latter? I agree with the problems this can cause, but if they can't be fixed by removing it from the database, why revert it? All things being equal, trailing blanks in fact aren't a good idea. I agree with Kenner here. We've encouraged removal of trailing whitespace in the past Not as bulk changes that don't do anything else, rather only as part of changes that touch the relevant parts anyway. and I don't why this should be any different. It does make things marginally harder when dealing with branches, And local patches. Basically _no_ patch will apply anymore as HJ changed every single file. That's not something marginally harder, it's terrible pointless thankless work created by a single 500 KB svn commit. svn blame will now also point to HJ most of the time, wonderful! but the right way to deal with that problem is to avoid trailing whitespace in the source tree to begin with. In an ideal world, yes. But it's not a clever idea to burn down the house just because the walls have some dots nobody cares about. Ciao, Michael.
Re: WTF?
And local patches. Basically _no_ patch will apply anymore as HJ changed every single file. That's an exaggeration since only a few lines in each file were change. The vast majority of outstanding patches won't be affected. In an ideal world, yes. But it's not a clever idea to burn down the house just because the walls have some dots nobody cares about. I'm not saying that what he did is a good idea, and I don't think Jeff is either. I support trying to remove it from the server. My point is that if this CAN'T be done, reverting it won't help all the problems and the lines should indeed be fixed.
Re: WTF?
On 11/25/09 10:09, Michael Matz wrote: Hi, On Wed, 25 Nov 2009, Jeff Law wrote: On 11/25/09 09:51, Richard Kenner wrote: Can someone please remove this revision from the subversion database on the server and fix things up? If that's not possible at least the revision should be reverted. Why the latter? I agree with the problems this can cause, but if they can't be fixed by removing it from the database, why revert it? All things being equal, trailing blanks in fact aren't a good idea. I agree with Kenner here. We've encouraged removal of trailing whitespace in the past Not as bulk changes that don't do anything else, rather only as part of changes that touch the relevant parts anyway. Yes we have allowed this kind of change in-bulk without changing anything else. In many ways I prefer the bulk change -- once done you don't have to worry about it again for a long time. Of course one could claim HJ's timing is terrible. and I don't why this should be any different. It does make things marginally harder when dealing with branches, And local patches. Basically _no_ patch will apply anymore as HJ changed every single file. That's not something marginally harder, it's terrible pointless thankless work created by a single 500 KB svn commit. svn blame will now also point to HJ most of the time, wonderful! A horrible exaggeration. Patches should be applying just fine, though you might need the magic switch to allow whitespace differences sometimes. I could believe automatic merging gets mucked up depending on how it's implemented within svn/svnmerge.But it's also a manageable problem. As for svn blame calling out HJ, well, there's a certain poetic justice in that. In an ideal world, yes. But it's not a clever idea to burn down the house just because the walls have some dots nobody cares about. I know other projects have done this kind of thing, so it can't be that terribly difficult to implement. Hell, a poor mans way would be a nightly script to check out the tree and removing the trailing whitespace. So I continue to maintain the real problem here is HJ just blasted in his patch without posting it or even warning anyone about it. That's bad. Really bad. jeff
Re: WTF?
On Wed, 25 Nov 2009, Jeff Law wrote: In many ways I prefer the bulk change -- once done you don't have to worry about it again for a long time. Of course one could claim HJ's timing is terrible. Doing it right at the end of branch-to-trunk merges for a release (which is where we are right now, just after merges from Graphite) is probably the optimal timing in terms of minimising the amount of branches that will need fixing up. The problem is doing it without warning or consensus. But I think hastily removing SVN revisions (so breaking all subsequent checkouts of trunk) is much worse than hastily checking in such a bulk change. -- Joseph S. Myers jos...@codesourcery.com
Re: WTF?
Not as bulk changes that don't do anything else, rather only as part of changes that touch the relevant parts anyway. Yes we have allowed this kind of change in-bulk without changing anything else. In many ways I prefer the bulk change -- once done you don't have to worry about it again for a long time. Also, it becomes hard to understand a change if it combines substantive and whitespace changes. I think a good rule is that they should ALWAYS be separate.
Re: WTF?
Michael Matz wrote: Someone with the appropriate rights needs to shutdown the svn server so that no additional commits can be done, then the revision files in db/revs/ and db/revprops/ starting with the wrong revision need to be removed, then db/current needs to be changed appropriately. Or... we could just suck it up. cheers, DaveK
Re: WTF?
Joseph S. Myers wrote: Doing it right at the end of branch-to-trunk merges for a release (which is where we are right now, just after merges from Graphite) is probably the optimal timing in terms of minimising the amount of branches that will need fixing up. The problem is doing it without warning or consensus. Yep. Given that, maybe we should make a global whitespace cleanup an official part of the release branching process? It would be nice to keep it from all building up again, and we could avoid the pain if it was regular and predictable. But I think hastily removing SVN revisions (so breaking all subsequent checkouts of trunk) is much worse than hastily checking in such a bulk change. I think the risks of manually editing a vcs database outweigh the pain of having a few irritating merges to do. cheers, DaveK
Re: WTF?
Hi, On Wed, 25 Nov 2009, Richard Kenner wrote: And local patches. Basically _no_ patch will apply anymore as HJ changed every single file. That's an exaggeration since only a few lines in each file were change. The top 12 from the 2.2 MB patch: tree-vect-loop.c | 386 ++-- tree-vect-slp.c| 394 ++-- ipa-type-escape.c | 432 +++--- tree-vect-data-refs.c | 462 +++ df-problems.c | 552 +- tree-data-ref.c| 558 +- tree-scalar-evolution.c| 576 +- df-scan.c | 580 +- tree-vect-stmts.c | 618 ++-- ipa-struct-reorg.c | 706 +++ sel-sched-ir.c | 732 +++ sel-sched.c| 1376 ++- The vast majority of outstanding patches won't be affected. Do you offer fixing my patches in case I'm becoming too annoyed to fix the conflicts? I guess not, but I have some queued for stage 1, which don't apply anymore. As for us having allowed such massive changes in the past: e.g. http://gcc.gnu.org/ml/gcc/2007-03/msg00417.html (and the surrounding thread) or http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00561.html say something different. In my mind it's very simple: trailing whitespace poses exactly _no_ problem (except of being against the coding standard), hence removing them should be weighed fairly aggressively against any disadvantage. Basically if even just one patch in the wild could possibly be broken by a whitespace change, that wouldn't also be broken by non-whitespace, then it simply should not be done. But I don't want to discuss whitespace policies actually, I want that HJ follows patch submission policies, and I want this checkin be reverted or removed. Ciao, Michael.
Re: WTF?
In my mind it's very simple: trailing whitespace poses exactly _no_ problem (except of being against the coding standard), It's against the coding standards for a very good reason, which is that it makes patching harder because you have lines that compare differently but look identical. So removing them, while making some patches harder to apply, makes others easier into the future. Like others, my feeling is that the biggest problem here is the timing and lack of notice.
Re: WTF?
On Wed, 25 Nov 2009, Richard Kenner wrote: And local patches. Basically _no_ patch will apply anymore as HJ changed every single file. That's an exaggeration since only a few lines in each file were change. The vast majority of outstanding patches won't be affected. In an ideal world, yes. But it's not a clever idea to burn down the house just because the walls have some dots nobody cares about. I'm not saying that what he did is a good idea, and I don't think Jeff is either. I support trying to remove it from the server. My point is that if this CAN'T be done, reverting it won't help all the problems and the lines should indeed be fixed. That's not an option. That would basically tell you that you can get away with breaking the rules if you just take the blame. And I just checked and none of my 12 local patches I queued for stage1 apply anymore. And as usual there are big hunks that now are broken. And patch doesn't have an option to ignore whitespace changes. The patch HJ installed is 100% useless and so should be reverted in some way or another. Richard.
Re: WTF?
On Wed, 25 Nov 2009, Dave Korn wrote: Joseph S. Myers wrote: Doing it right at the end of branch-to-trunk merges for a release (which is where we are right now, just after merges from Graphite) is probably the optimal timing in terms of minimising the amount of branches that will need fixing up. The problem is doing it without warning or consensus. Yep. Given that, maybe we should make a global whitespace cleanup an official part of the release branching process? It would be nice to keep it from all building up again, and we could avoid the pain if it was regular and predictable. What is the _point_ in doing this? Is there _any_ positive effect of removing trailing whitespace? Does it looks like there are no bugs to fix in gcc so that the pointless task of removing trailing whitespace looks appealing?? Richard.
Re: WTF?
On Wed, 25 Nov 2009, Richard Kenner wrote: In my mind it's very simple: trailing whitespace poses exactly _no_ problem (except of being against the coding standard), It's against the coding standards for a very good reason, which is that it makes patching harder because you have lines that compare differently but look identical. So removing them, while making some patches harder to apply, makes others easier into the future. Like others, my feeling is that the biggest problem here is the timing and lack of notice. But different timing or a notice wouldn't fix one of my no longer applying patches. If you can offer advice on how to teach quilt (which I belive uses patch) to ignore whitespace changes when applying patches then more power to you - the only tool that seems to be able to ignore whitespace changes is diff. Richard.
Re: WTF?
On Wed, 25 Nov 2009, Dave Korn wrote: Joseph S. Myers wrote: Doing it right at the end of branch-to-trunk merges for a release (which is where we are right now, just after merges from Graphite) is probably the optimal timing in terms of minimising the amount of branches that will need fixing up. The problem is doing it without warning or consensus. Yep. Given that, maybe we should make a global whitespace cleanup an official part of the release branching process? It would be nice to keep it from all building up again, and we could avoid the pain if it was regular and predictable. What is the _point_ in doing this? Is there _any_ positive effect of removing trailing whitespace? Does it looks like there are no bugs to fix in gcc so that the pointless task of removing trailing whitespace looks appealing?? Gentlemen, a point of order please. Many people read these lists and we place high regard on the quality of the GCC project as well as the people involved. A personal slight as well as an error may happen from time to time but this is simply the nature of working as a software engineer. Let us not degrade into a base argument with words traded and thus undermine the great name that GCC and open source has. I am sure that this issue can be resolved without any damage done to the most precious resource we have, good people that work together openly and with respect and dignity. Having said that, I firmly would defend any of you as truely awesome engineers and good people that work to benefit the state of mankind. -- Dennis Clarke http://www.blastwave.org/ dcla...@opensolaris.ca - Email related to the open source Solaris dcla...@blastwave.org - Email related to open source for Solaris
Re: WTF?
On Wed, 25 Nov 2009, Richard Guenther wrote: On Wed, 25 Nov 2009, Richard Kenner wrote: In my mind it's very simple: trailing whitespace poses exactly _no_ problem (except of being against the coding standard), It's against the coding standards for a very good reason, which is that it makes patching harder because you have lines that compare differently but look identical. So removing them, while making some patches harder to apply, makes others easier into the future. Like others, my feeling is that the biggest problem here is the timing and lack of notice. But different timing or a notice wouldn't fix one of my no longer applying patches. If you can offer advice on how to teach quilt (which I belive uses patch) to ignore whitespace changes when applying patches then more power to you - the only tool that seems to be able to ignore whitespace changes is diff. Btw, I'd be happy with a commit hook that forces you to fix whitespace in the area you patch (basically just make sure there is no trailing whitespace in ^[+ ] lines of a unified diff (maybe even only in ^+ lines to not cause continuous rejects if you fixup the ^ lines in your patch and drag in more context). Anyone willing to implement that? I still see no point in fixing up the existing source just for the sake of it. Richard.
Re: WTF?
On Wed, Nov 25, 2009 at 08:31:27PM +0100, Richard Guenther wrote: And patch doesn't have an option to ignore whitespace changes. Sure it does. -l (for loose, or --ignore-whitespace). QUILT_PATCH_OPTS for quilt. -- Daniel Jacobowitz CodeSourcery
Re: WTF?
Richard Guenther wrote:
Btw, I'd be happy with a commit hook that forces you to fix
whitespace in the area you patch (basically just make sure there
is no trailing whitespace in ^[+ ] lines of a unified diff
(maybe even only in ^+ lines to not cause continuous rejects
if you fixup the ^ lines in your patch and drag in more context).
Then, I might suggest using GNU indent as the commit hook.
Of course, not every one has it (notably those working on non-linux systems), but for those who have it, requiring that
every C file inside GCC has been automatically indented with GNU indent could help. It certainly could have helped me
for most of the patches I have submitted (but I am too afraid of running indent on a GCC file by myself; it probably
would change lot of things outside of my patch).
But I confess that I don't know much about svn commit machinery.
Regards.
--
Basile STARYNKEVITCH http://starynkevitch.net/Basile/
email: basileatstarynkevitchdotnet mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***
Re: WTF?
On 11/25/09 12:55, Basile STARYNKEVITCH wrote: Richard Guenther wrote: Btw, I'd be happy with a commit hook that forces you to fix whitespace in the area you patch (basically just make sure there is no trailing whitespace in ^[+ ] lines of a unified diff (maybe even only in ^+ lines to not cause continuous rejects if you fixup the ^ lines in your patch and drag in more context). Then, I might suggest using GNU indent as the commit hook. Of course, not every one has it (notably those working on non-linux systems), but for those who have it, requiring that every C file inside GCC has been automatically indented with GNU indent could help. It certainly could have helped me for most of the patches I have submitted (but I am too afraid of running indent on a GCC file by myself; it probably would change lot of things outside of my patch). But I confess that I don't know much about svn commit machinery. I'd support something along these lines as well. Regardless of whether or not one likes the GNU formatting standards, having a standard is a good thing. jeff
Re: WTF?
On 11/25/09 12:41, Dennis Clarke wrote: Gentlemen, a point of order please. Many people read these lists and we place high regard on the quality of the GCC project as well as the people involved. A personal slight as well as an error may happen from time to time but this is simply the nature of working as a software engineer. Let us not degrade into a base argument with words traded and thus undermine the great name that GCC and open source has. I am sure that this issue can be resolved without any damage done to the most precious resource we have, good people that work together openly and with respect and dignity. Having said that, I firmly would defend any of you as truely awesome engineers and good people that work to benefit the state of mankind. I don't see anything in this discussion that's out of the ordinary for this group of engineers. We've had (and will certainly have) far more heated discussions on more important issues. Jeff
Re: WTF?
On Wed, 25 Nov 2009, Richard Guenther wrote: That's not an option. That would basically tell you that you can get away with breaking the rules if you just take the blame. And I just checked and none of my 12 local patches I queued for stage1 apply anymore. And as usual there are big hunks that now are broken. And patch doesn't have an option to ignore whitespace changes. The patch HJ installed is 100% useless and so should be reverted in some way or another. I think we need to take a deep breath and relax. First of all, HJ didn't need approval for this patch. Whether it's useful or not, it aligns with our stated coding standards and it clearly qualifies as obvious under the Free for all category in our checkin policies. Second, stage3 is probably the best time for such a patch. Third, it's possible HJ *did* post it to gcc-patches, and it bounced because of the size. Let's give him the benefit of the doubt and hear what he has to say before anyone goes postal over this. Finally, we have a process for reverting a patch. If anyone wants to revert some part, it needs to be followed. Otherwise *that* would be breaking the rules... --Kaveh
Re: WTF?
Jeff Law wrote: On 11/25/09 12:41, Dennis Clarke wrote: Gentlemen, a point of order please. Let us not degrade into a base argument I don't see anything in this discussion that's out of the ordinary for this group of engineers. We've had (and will certainly have) far more heated discussions on more important issues. I'd just like to add that I didn't take any offense at the quoted text(*) from Richard G's response to me, nor see it as even intemperate or harsh; it's nothing more than a bit of minor hyperbole to emphasise the point, a bog-standard rhetorical technique in entirely everyday use. cheers, DaveK -- (*) - which you left unattributed and may appear in your out-of-context quote to be something that I wrote.
Re: WTF?
Kaveh R. GHAZI wrote: I think we need to take a deep breath and relax. First of all, HJ didn't need approval for this patch. Whether it's useful or not, it aligns with our stated coding standards and it clearly qualifies as obvious under the Free for all category in our checkin policies. But does it, though? From http://gcc.gnu.org/svnwrite.html: Free for all The following changes can be made by everyone with SVN write access: Fixes for obvious typos in ChangeLog files, docs, web pages, comments and similar stuff. Just check in the fix and copy it to gcc-patches. We don't want to get overly anal-retentive about checkin policies. Similarly, no outside approval is needed to revert a patch that you checked in. Importing files maintained outside the tree from their official versions. Creating and using a branch for development, including outside the parts of the compiler one maintains, provided that changes on the branch have copyright assignments on file. Merging such developments back to the mainline still needs approval in the usual way. So, where are whitespace changes to non-comment parts of .c and .h source files covered? I think that there may be a bit of a common assumption that obvious extends somewhat further than the wording of the documentation actually implies - not just in the context of this incident, but the question has occurred to me in other cases too, and maybe now would be a good time to clear it up. cheers, DaveK
Re: WTF?
On Wed, 25 Nov 2009, Dave Korn wrote: Kaveh R. GHAZI wrote: I think we need to take a deep breath and relax. First of all, HJ didn't need approval for this patch. Whether it's useful or not, it aligns with our stated coding standards and it clearly qualifies as obvious under the Free for all category in our checkin policies. But does it, though? From http://gcc.gnu.org/svnwrite.html: Free for all The following changes can be made by everyone with SVN write access: Fixes for obvious typos in ChangeLog files, docs, web pages, comments and similar stuff. Just check in the fix and copy it to gcc-patches. We don't want to get overly anal-retentive about checkin policies. Similarly, no outside approval is needed to revert a patch that you checked in. Importing files maintained outside the tree from their official versions. Creating and using a branch for development, including outside the parts of the compiler one maintains, provided that changes on the branch have copyright assignments on file. Merging such developments back to the mainline still needs approval in the usual way. So, where are whitespace changes to non-comment parts of .c and .h source files covered? I think that there may be a bit of a common assumption that obvious extends somewhat further than the wording of the documentation actually implies - not just in the context of this incident, but the question has occurred to me in other cases too, and maybe now would be a good time to clear it up. The change certainly didn't fall under the obvious rule because of its size. One might argue that 'and similar stuff' covers coding-style changes, but here again I'd fear of a certain kind of people going wild and follow the coding-style by word rather than existing practice in GCC or even the code around their changes. So, I would say even obvious patches should be posted for review with the usual (but not documented) will checkin tomorrow if there are no complaints style disclaimer. Richard.
Re: WTF?
From: Dave Korn dave.korn.cyg...@googlemail.com But does it, though? From http://gcc.gnu.org/svnwrite.html: Free for all The following changes can be made by everyone with SVN write access: Fixes for obvious typos in ChangeLog files, docs, web pages, comments and similar stuff. Just check in the fix and copy it to gcc-patches. We don't want to get overly anal-retentive about checkin policies. Incorrect whitespace is a typo. I.e. an error in typed material.
Re: WTF?
From: Richard Guenther rguent...@suse.de The change certainly didn't fall under the obvious rule because of its size. One might argue that 'and similar stuff' covers coding-style changes, but here again I'd fear of a certain kind of people going wild and follow the coding-style by word rather than existing practice in GCC or even the code around their changes. So, I would say even obvious patches should be posted for review with the usual (but not documented) will checkin tomorrow if there are no complaints style disclaimer. Given its size, I agree that would have been the sensible thing to do.
Re: WTF?
Fixes for obvious typos in ChangeLog files, docs, web pages, comments and similar stuff. Just check in the fix and copy it to gcc-patches. We don't want to get overly anal-retentive about checkin policies. Incorrect whitespace is a typo. I.e. an error in typed material. Yes, but it's not in ChangeLog files, docs, web pages, comments and similar stuff. I suspect the web page in question needs to be updated to more accurately reflect current standard practice. It appears wrong to me on more counts than just this one (my understanding has always been that no approval is needed to fix typos, whether in code or comments, but this page says only the latter).
Re: WTF?
From: Richard Kenner ken...@vlsi1.ultra.nyu.edu I suspect the web page in question needs to be updated to more accurately reflect current standard practice. It appears wrong to me on more counts than just this one (my understanding has always been that no approval is needed to fix typos, whether in code or comments, but this page says only the latter). I agree the wording could be better. Unlike HJ's patch, many so-called obvious checkins actually have code-gen consequences. (Gasp!) Okay, okay, those tend to be one-liners. ;-) --Kaveh
Re: WTF?
On 11/25/2009 08:38 PM, Richard Guenther wrote:
If you can offer advice on how to teach quilt
(which I belive uses patch) to ignore whitespace changes when
applying patches then more power to you - the only tool that
seems to be able to ignore whitespace changes is diff.
sed -i '/^-/s/[ \t]*$//' SOMETHING/*.patch
?
With git, I suggest
git checkout -b branch-wp origin/master
git format-patch --stdout origin/mas...@{1}..branch | \
sed '/^-/s/[ \t]*$//' | git am
Paolo
Re: WTF?
On 11/25/2009 06:45 PM, Dave Korn wrote: Michael Matz wrote: Someone with the appropriate rights needs to shutdown the svn server so that no additional commits can be done, then the revision files in db/revs/ and db/revprops/ starting with the wrong revision need to be removed, then db/current needs to be changed appropriately. Or... we could just suck it up. Definitely I support making the repository readonly. Paolo
Re: WTF?
Basile == Basile STARYNKEVITCH bas...@starynkevitch.net writes: Basile Of course, not every one has it (notably those working on non-linux Basile systems), but for those who have it, requiring that every C file Basile inside GCC has been automatically indented with GNU indent could Basile help. I looked at this once. GNU indent doesn't have all the features needed to make it correctly support the GCC coding style. I filed a bunch of GNU indent bug reports, but AFAIK none of them has ever been fixed. You could still do this if you didn't mind a coding style change at the same time. And of course, this will only help the .c and .h files, not everything else. Tom
Re: WTF?
On Thu, Nov 26, 2009 at 12:04 AM, Tom Tromey tro...@redhat.com wrote: Basile == Basile STARYNKEVITCH bas...@starynkevitch.net writes: Basile Of course, not every one has it (notably those working on non-linux Basile systems), but for those who have it, requiring that every C file Basile inside GCC has been automatically indented with GNU indent could Basile help. I looked at this once. GNU indent doesn't have all the features needed to make it correctly support the GCC coding style. I filed a bunch of GNU indent bug reports, but AFAIK none of them has ever been fixed. You could still do this if you didn't mind a coding style change at the same time. And of course, this will only help the .c and .h files, not everything else. Not to mention that I'd find this a quite pointless excercise. I trust humans much more than the little brain of GNU indent (or whatever you can code into a reasonable replacement). Richard.
Re: WTF?
On Wed, 2009-11-25 at 15:17 -0500, Kaveh R. GHAZI wrote: Finally, we have a process for reverting a patch. If anyone wants to revert some part, it needs to be followed. Otherwise *that* would be breaking the rules... Won't reverting the patch just attribute all of the lines to the username of the reverter? Losing the svn blame facility is a serious regression to my mind. Ben
Re: WTF?
On Nov 25, 2009, Daniel Jacobowitz d...@false.org wrote:
On Wed, Nov 25, 2009 at 08:31:27PM +0100, Richard Guenther wrote:
And patch doesn't have an option to ignore whitespace changes.
Sure it does. -l (for loose, or --ignore-whitespace).
QUILT_PATCH_OPTS for quilt.
And even without this, given that the patch under discussion was
mechanical, it ought to be possible to change all other patches just as
mechanically.
sed -i 's,[ \t]*$,,' probably won't work, if there are all-blanks lines
being left alone in the patch (so the rx will match the patch markers
too), but something slightly more elaborate preserving a fixed number of
leading blanks dependng on the patch type (context or unified) should:
context diffs: /../{s,^..,:,;s,[ \t]*$,,;s,^\(..\):,\1,;}
unified diffs: /./ {s,^.,:,; s,[ \t]*$,,;s,^\(.\):,\1,; }
--
Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist Red Hat Brazil Compiler Engineer
Re: WTF?
On Wed, 25 Nov 2009, Tom Tromey wrote: Basile == Basile STARYNKEVITCH bas...@starynkevitch.net writes: Basile Of course, not every one has it (notably those working on non-linux Basile systems), but for those who have it, requiring that every C file Basile inside GCC has been automatically indented with GNU indent could Basile help. I looked at this once. GNU indent doesn't have all the features needed to make it correctly support the GCC coding style. I filed a bunch of GNU indent bug reports, but AFAIK none of them has ever been fixed. That's why I suggested patching it - not waiting for GNU indent maintainers to do so. Since this discussion shows people don't like code formatting patches, the onus is on the person wishing to make the formatting more uniform to make the GCC changes to do so as small as possible, which means following the existing documented or majority style (which should be the same thing in general) wherever a style is documented or can be discerned from the code. -- Joseph S. Myers jos...@codesourcery.com
Re: WTF?
Richard Guenther wrote:
On Thu, Nov 26, 2009 at 12:04 AM, Tom Tromey tro...@redhat.com wrote:
Basile == Basile STARYNKEVITCH bas...@starynkevitch.net writes:
Basile Of course, not every one has it (notably those working on non-linux
Basile systems), but for those who have it, requiring that every C file
Basile inside GCC has been automatically indented with GNU indent could
Basile help.
I looked at this once. GNU indent doesn't have all the features needed
to make it correctly support the GCC coding style. I filed a bunch of
GNU indent bug reports, but AFAIK none of them has ever been fixed.
You could still do this if you didn't mind a coding style change at the
same time. And of course, this will only help the .c and .h files, not
everything else.
Not to mention that I'd find this a quite pointless excercise. I trust
humans much more than the little brain of GNU indent (or whatever
you can code into a reasonable replacement).
I don't understand that point. Do you believe that removing trailing whitespace cannot be automatized? Could you explain
why?
Of course, we might have very clever rules for whitespaces (I don't claim to understand all of GCC coding styles). My
opinion is that, if the rules about spaces are too clever to be easily handled by a small piece of software, we should
better change these rules to simplify them.
Again, I am only speaking of whitespaces! Are they so important inside C code that we could not in principle agree that
tools put them better than humans?
If our rules for whitespaces are so complex (and perhaps they are), shouldn't we aim to simplify them? We could decide
that GNU indent -the current version- is doing a good enough job for us.
If feel that all this flamewar is non-sensical, since we only speak of spaces!!!
And I have no understanding about the design process of our whitespace rules?
Who decided them and when?
Why would the GCC code written in C become less readable if we decided to GNU indent it? There might be some corner
cases which could be handled by clever /* *INDENT-OFF* */ comments.
If we decided to use a software tool to indent our code, all the reviewers would have less (unininteresting) work, I
mean correcting the whitespaces. Since human reviewers are the bottleneck in the development of GCC, shouldn't a tiny
improvement to help them be considered? I am sure that Diego Novillo alone spent more than an hour of his valuable time
to improve my spaces in my code.
Wouldn't be a simple rule like always run indent on GCC source files written in
C be much more efficient for everyone?
Why should in 2010 whitespaces (especially trailing ones) be corrected by
humans?
I don't really want to patch GNU indent (unless it would be easy, which it is probably not). I just want to push the
idea that have more tools to help code being reviewed is valuable. Of course, I cannot decide that alone. I am just
throwing a proposal for discussion.
Wouldn't it be better to change a bit our coding rules, to make tools like indent better working for them us, and to
have reviewers put their effort on something more interesting than spaces!!!
Regards.
--
Basile STARYNKEVITCH http://starynkevitch.net/Basile/
email: basileatstarynkevitchdotnet mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***
RE: WTF?
Sorry for that. I did send an email and though it was an obvious fix. I guess the patch may be too big, 400K with bz2. H.J. -Original Message- From: Richard Guenther [mailto:rguent...@suse.de] Sent: Wednesday, November 25, 2009 8:39 AM To: gcc@gcc.gnu.org Cc: Lu, Hongjiu Subject: WTF? Author: hjl Date: Wed Nov 25 10:55:54 2009 New Revision: 154645 URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=154645 Log: Remove trailing white spaces. WTF? Thankyouverymuch. This 1) wasn't posted or approved 2) is bad as it breaks svn blame 3) chokes all branches. What's up? Richard.
Re: WTF?
FWIW, I have been using git to maintain my patches. I created a branch for each patch. Update and merge are almost automatic. It works quite well for me. H.J. On Wed, Nov 25, 2009 at 3:51 PM, Lu, Hongjiu hongjiu...@intel.com wrote: Sorry for that. I did send an email and though it was an obvious fix. I guess the patch may be too big, 400K with bz2. H.J. -Original Message- From: Richard Guenther [mailto:rguent...@suse.de] Sent: Wednesday, November 25, 2009 8:39 AM To: gcc@gcc.gnu.org Cc: Lu, Hongjiu Subject: WTF? Author: hjl Date: Wed Nov 25 10:55:54 2009 New Revision: 154645 URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=154645 Log: Remove trailing white spaces. WTF? Thankyouverymuch. This 1) wasn't posted or approved 2) is bad as it breaks svn blame 3) chokes all branches. What's up? Richard. -- H.J.
Re: WTF?
Quoting Basile STARYNKEVITCH bas...@starynkevitch.net: Why would the GCC code written in C become less readable if we decided to GNU indent it? There might be some corner cases which could be handled by clever /* *INDENT-OFF* */ comments. GNU indent becomes so confused by some constructs in GCC that the result becomes unreadable in places; sometimes it won't even compile anymore. Besides, even if we had a working indent for GCC, we wouldn't want all of our code to be reformatted. Ideally indent would only reformat what is actually formatted incorrectly, not things that you can validly format in various different ways. If we decided to use a software tool to indent our code, all the reviewers would have less (unininteresting) work, I mean correcting the whitespaces. Having diffs balloon to a multiple of their current size because indent has no common sense whatsoever how to incorporate a code change without reformatting the world won't help.
