WTF?

WTF?

[Richard Guenther]

Author: hjl
Date: Wed Nov 25 10:55:54 2009
New Revision: 154645

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=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?

[Richard Guenther]

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=gcc&view=rev&rev=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?

[Richard Kenner]

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

[Richard Guenther]

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?

[Vincent Lefevre]

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

[Jeff Law]

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?

[Michael Matz]

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?

[Richard Kenner]

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

[Michael Matz]

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?

[Richard Kenner]

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

[Richard Guenther]

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?

[Richard Guenther]

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?

[Basile STARYNKEVITCH]

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: basile<at>starynkevitch<dot>net 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?

[Jeff Law]

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?

[Tom Tromey]

>>>>> "Basile" == Basile STARYNKEVITCH <basile@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?

[Richard Guenther]

On Thu, Nov 26, 2009 at 12:04 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Basile" == Basile STARYNKEVITCH <basile@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?

[Basile STARYNKEVITCH]

Richard Guenther wrote:
> On Thu, Nov 26, 2009 at 12:04 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Basile" == Basile STARYNKEVITCH <basile@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: basile<at>starynkevitch<dot>net 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?

[Joern Rennecke]

Quoting Basile STARYNKEVITCH <basile@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.

Re: WTF?

[Richard Earnshaw]

On Thu, 2009-11-26 at 00:08 +0100, Richard Guenther wrote:
> On Thu, Nov 26, 2009 at 12:04 AM, Tom Tromey <tromey@redhat.com> wrote:
> >>>>>> "Basile" == Basile STARYNKEVITCH <basile@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

SVN pre-commit filter (Was: Re: WTF?)

[Joern Rennecke]

Quoting Richard Earnshaw <rearnsha@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?)

[Richard Earnshaw]

On Fri, 2009-11-27 at 09:40 -0500, Joern Rennecke wrote:
> Quoting Richard Earnshaw <rearnsha@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?)

[Joseph S. Myers]

On Fri, 27 Nov 2009, Richard Earnshaw wrote:

> On Fri, 2009-11-27 at 09:40 -0500, Joern Rennecke wrote:
> > Quoting Richard Earnshaw <rearnsha@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
joseph@codesourcery.com

Re: WTF?

[Joseph S. Myers]

On Wed, 25 Nov 2009, Tom Tromey wrote:

> >>>>> "Basile" == Basile STARYNKEVITCH <basile@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
joseph@codesourcery.com

Re: WTF?

[Paolo Bonzini]

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/master@{1}..branch | \
   sed  '/^-/s/[ \t]*$//' | git am

Paolo

Re: WTF?

[Peter Zijlstra]

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"

Re: WTF?

[Michael Matz]

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?

[Dave Korn]

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?

[Richard Guenther]

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?

[Daniel Jacobowitz]

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?

[Alexandre Oliva]

On Nov 25, 2009, Daniel Jacobowitz <drow@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 fighter    http://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?

[Paolo Bonzini]

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?

[Richard Guenther]

On Thu, Nov 26, 2009 at 9:56 AM, Paolo Bonzini <bonzini@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?

[Philip Martin]

Richard Guenther <richard.guenther@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?

[Kaveh R. GHAZI]

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?

[Dave Korn]

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?

[Richard Guenther]

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?

[Kaveh R. Ghazi]

From: "Richard Guenther" <rguenther@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?

[Kaveh R. Ghazi]

From: "Dave Korn" <dave.korn.cygwin@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?

[Richard Kenner]

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

[Kaveh R. Ghazi]

From: "Richard Kenner" <kenner@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?

[Gerald Pfeifer]

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: [annoyed grunt]?

[Dave Korn]

Kaveh R. Ghazi wrote:
> From: "Dave Korn" <dave.korn.cygwin@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.
> 

  Agreed, but what I'm not at all certain is whether it counts as *in*
"ChangeLog files, docs, web pages, comments and similar stuff", specifically
when it's at the end of a line of functional C code in a source file.

    cheers,
      DaveK

Re: [annoyed grunt]?

[Kaveh R. Ghazi]

From: "Dave Korn" <dave.korn.cygwin@googlemail.com>

>  Agreed, but what I'm not at all certain is whether it counts as *in*
> "ChangeLog files, docs, web pages, comments and similar stuff", 
> specifically
> when it's at the end of a line of functional C code in a source file.

Since whitespace in C has no syntactic effect, certainly these changes 
qualify under "and similar stuff".

Re: [annoyed grunt]?

[Dave Korn]

Kaveh R. Ghazi wrote:
> From: "Dave Korn" <dave.korn.cygwin@googlemail.com>
> 
>>  Agreed, but what I'm not at all certain is whether it counts as *in*
>> "ChangeLog files, docs, web pages, comments and similar stuff",
>> specifically
>> when it's at the end of a line of functional C code in a source file.
> 
> Since whitespace in C has no syntactic effect, 

  Heh.  /Almost/ no syntactic effect.

> certainly these changes qualify under "and similar stuff".

  That's a valid point of view, but I don't think it's unambiguously what the
intent of the wording implies.  Which is why I suggested clearing it all up
now, and I certainly don't object to these sorts of changes being included in
the obvious category.

    cheers,
      DaveK

Re: WTF?

[Ben Elliston]

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?

[Vincent Lefevre]

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

[Jeff Law]

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?

[Joseph S. Myers]

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
joseph@codesourcery.com

Re: WTF?

[Dave Korn]

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?

[Richard Guenther]

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: trivial trailing whitespace issue

[Jeff Law]

On 11/25/09 12:33, Richard Guenther wrote:
> 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??
>    
I agree there is little value in removing the trailing whitespace.  It 
makes reviewing changes marginally easier when you don't have to wade 
through gratutious whitespace changes.

Ultimately I think there are X issues here.

   1. What to do in the immediate term with the repo.  I've got no 
strong opinions here.

   2. What to do longer term.  I'd like to see us fix the whitespace 
stuff then use hooks or a cron job to make sure they don't come back 
again in the future.

   3. What is a suitable consequence for this gaffe by HJ?


Jeff

Re: trivial trailing whitespace issue

[Basile STARYNKEVITCH]

Jeff Law wrote:
> 
> Ultimately I think there are X issues here.
> 
>   1. What to do in the immediate term with the repo.  I've got no strong 
> opinions here.
I do understand most of the opinions, but I would rather prefer that we don't revert the trailing whitespace patch.
I definitely can live with them, and they probably cleaned up a bit the code.
> 
>   2. What to do longer term.  I'd like to see us fix the whitespace 
> stuff then use hooks or a cron job to make sure they don't come back 
> again in the future.
Agreed. I believe that we might want to have some automated tools (at least in contrib, like we have gcc_update there 
already) to help us (in particular newbies) about purely layout issue (including whitespaces).

> 
>   3. What is a suitable consequence for this gaffe by HJ?
I hope nothing serious would happen to HJ. He definitely wanted to help the GCC community. And perhaps, if he did ask 
permission (which he probably should have) to remove whitespaces, it would have triggered tons of useless discussions 
(like it does now).

My wish would be that every C source file in GCC would be indent-ed by GNU indent with --gnu option. But since this has 
not been done, I suppose it is (sadly) not realistic. I don't understand all the issues involved (but I do admit that 
many of my small patches have been kindly corrected, in particular by Diego Novillo, for "typographical layout" issues 
[lack of space, ...].

Perhaps in a few years we would have more automated tools to help coding inside GCC. I could even dream of specialized 
plugins for that (for the common case when building the compiler under Linux at least).

Regards.

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: trivial trailing whitespace issue

[Joseph S. Myers]

On Wed, 25 Nov 2009, Basile STARYNKEVITCH wrote:

> My wish would be that every C source file in GCC would be indent-ed by GNU
> indent with --gnu option. But since this has not been done, I suppose it is
> (sadly) not realistic. I don't understand all the issues involved (but I do

There are various deviations of GCC style from what GNU indent does.  If 
you want to make this a reality, I advise enhancing GNU indent to add 
options to make it follow the style used in GCC as closely as possible, 
while adding parentheses (etc.) to GCC where the present source makes 
proper indenting intrinsically hard for indent to do.  Enhancing GNU 
indent to follow the GCC style is the way to minimise the controversy 
involved in any mass reindentation patch.  Then you can write a commit 
hook, and test it in a copy of the repository, that reindents modified 
files (compiler source only, list of exceptions for generated files, not 
touching testcases) and gives error on commit if the reindentation is not 
the identity operation.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: trivial trailing whitespace issue

[Basile STARYNKEVITCH]

Joseph S. Myers wrote:
> On Wed, 25 Nov 2009, Basile STARYNKEVITCH wrote:
> 
>> My wish would be that every C source file in GCC would be indent-ed by GNU
>> indent with --gnu option. But since this has not been done, I suppose it is
>> (sadly) not realistic. I don't understand all the issues involved (but I do
> 
> There are various deviations of GCC style from what GNU indent does.  If 
> you want to make this a reality, I advise enhancing GNU indent to add 
> options to make it follow the style used in GCC as closely as possible,

I would imagine that patching GNU indent (in its source code form) requires a copyright assignment specific to it (and 
uncovered by a, or at least "mine", GCC copyright assigment to FSF). If this is the case, I [Basile] won't do it 
(because getting a copyright assignemnt is so painful to me).

Perhaps a simpler alternative would be to revise the GCC coding standards so that they be compatible with GNU ident 
behavior. I have no idea of how to push that idea. I even don't understand the procedure for changing them, and who 
decides such change. And I don't understand how painful would it be to require all GCC C source to be GNU indent-ed. I 
would imagine that smarter people have already discussed & rejected that trivial idea.

But it is a pity that in nearly 2010, whitespace issues are still manually handled, and trigger such big discussions. In 
our time, computers should be good enough to handle whitespace :-) better than humans manually do.

Regards.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: trivial trailing whitespace issue

[Joern Rennecke]

Quoting Basile STARYNKEVITCH <basile@starynkevitch.net>:
> I would imagine that patching GNU indent (in its source code form)
> requires a copyright assignment specific to it (and uncovered by a, or
> at least "mine", GCC copyright assigment to FSF). If this is the case,
> I [Basile] won't do it (because getting a copyright assignemnt is so
> painful to me).

If you put a patch against GNU indent in contrib to format GCC-style,
wouldn't that then be a part of GCC and thus covered?

If that is the case, then you can post afterwards this piece of FSF-owned
GCC code to the indent maintainers...

Re: trivial trailing whitespace issue

[Basile STARYNKEVITCH]

Joern Rennecke wrote:
> Quoting Basile STARYNKEVITCH <basile@starynkevitch.net>:
>> I would imagine that patching GNU indent (in its source code form)
>> requires a copyright assignment specific to it (and uncovered by a, or
>> at least "mine", GCC copyright assigment to FSF). If this is the case,
>> I [Basile] won't do it (because getting a copyright assignemnt is so
>> painful to me).
> 
> If you put a patch against GNU indent in contrib to format GCC-style,
> wouldn't that then be a part of GCC and thus covered?
> 
> If that is the case, then you can post afterwards this piece of FSF-owned
> GCC code to the indent maintainers...

I have no idea if that could work, and I prefer doing software tricks (which I am supposed to be able to understand, 
being a software person) to doing "legal" tricks (which I don't understand, not being a lawyer).

Regards.


-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: trivial trailing whitespace issue

[Michael Matz]

Hi,

On Wed, 25 Nov 2009, Basile STARYNKEVITCH wrote:

> >   1. What to do in the immediate term with the repo.  I've got no strong
> > opinions here.
> 
> I do understand most of the opinions, but I would rather prefer that we 
> don't revert the trailing whitespace patch. I definitely can live with 
> them, and they probably cleaned up a bit the code.

How, for goodness sake, can pure trailing whitespace changes, which by 
necessity you can't even _see_ in an editor (except if you activate modes 
that explicitely show them e.g. in different color) clean up code?  Have 
you looked at the patch or fallout from it?  If not, why do you speak in 
favor of it?


Ciao,
Michael.

Re: trivial trailing whitespace issue

[Basile STARYNKEVITCH]

Michael Matz wrote:
> Hi,
> 
> On Wed, 25 Nov 2009, Basile STARYNKEVITCH wrote:
> 
>>>   1. What to do in the immediate term with the repo.  I've got no strong
>>> opinions here.
>> I do understand most of the opinions, but I would rather prefer that we 
>> don't revert the trailing whitespace patch. I definitely can live with 
>> them, and they probably cleaned up a bit the code.
> 
> How, for goodness sake, can pure trailing whitespace changes, which by 
> necessity you can't even _see_ in an editor (except if you activate modes 
> that explicitely show them e.g. in different color) clean up code?  Have 
> you looked at the patch or fallout from it?  If not, why do you speak in 
> favor of it?

I was speaking in favor of the patch because it apparently make the GCC code more conformant to GCC coding styles.


Regards.


-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: trivial trailing whitespace issue

[Mark Mielke]

On 11/26/2009 08:18 AM, Michael Matz wrote:
> How, for goodness sake, can pure trailing whitespace changes, which by
> necessity you can't even _see_ in an editor (except if you activate modes
> that explicitely show them e.g. in different color) clean up code?  Have
> you looked at the patch or fallout from it?  If not, why do you speak in
> favor of it?

I'm an outside observer with professional ties to SCM.

This issue is similar in scope to TAB vs SPACE for indenting. Whether 
you can see it or not is not indicative of whether it has downstream 
effects or not.

Generally, whitespace on the end of lines is bad, and cleanup eventually 
becomes necessary. It is "bad" because people will accidentally change 
lines without realizing them, and these changed lines show up in patches 
and annotations. This goes both ways - a lot of people browse code in vi,
and accidentally add space to the end of lines they don't even intend to 
change. Or, maybe their editor automatically strips whitespace at end of 
line when they re-indent. Who knows? Same happens with TAB changed to 
SPACE or back and forth. Once these start to accumulate - they create 
downstream consequences that multiply. You can't see it, but every 
program that the file passes through *does* see them.

Is cleanup in bulk the right approach? You either clean up over time or 
you do it in bulk. In my own projects, some of the designers I worked 
with had horrible habits in this regard. Every second line would have 
whitespace on the end. The longer it is left, the worse the effect.

As software architect for our project, I notified the team I was doing a 
cleanup, and did it in bulk. Due to the notification, everything went 
smooth.

It sounds like the idea should have been proposed and accepted. 
Everybody would have been ready for it, and nobody would be upset. Oh well.

It's been entertaining. gcc@gcc.gnu.org is normally pretty dull to 
read... :-)

Cheers,
mark

-- 
Mark Mielke<mark@mielke.cc>

Re: trivial trailing whitespace issue

[Andrew Haley]

Jeff Law wrote:

> Ultimately I think there are X issues here.
> 
>    1. What to do in the immediate term with the repo.  I've got no 
> strong opinions here.
> 
>    2. What to do longer term.  I'd like to see us fix the whitespace 
> stuff then use hooks or a cron job to make sure they don't come back 
> again in the future.
> 
>    3. What is a suitable consequence for this gaffe by HJ?

Force him to read all of this thread?  That should be punishment
enough...

Andrew.

Re: trivial trailing whitespace issue

[Sebastian Huber]

Hi!

If whitespace is an issue for some of you, why don't you fix this in a generic way?

1. Identify all modules in which whitespace at the end of a line is a problem.
2. Identify all branches of these modules.
3. Identify all lines with the whitespace problem in all modules and branches.
4. Identify the current author of each of these lines.
5. Fix the whitespace and commit the changes on behalf of the current line author.

This does not affect blame. It does not make merging more difficult.  Existing
patches may be affected by this.

Have a nice day!

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: WTF?

[Richard Kenner]

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

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?

[Dave Korn]

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?

[Paolo Bonzini]

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?

[Richard Earnshaw]

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

[Dave Korn]

Richard Earnshaw wrote:
> 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.

  I'm also deeply leery of any notion of manually editing an RCS repository,
it's fairly high on my list of "don't ever do this even if you do think it's
simple and safe and you can get away with it because of the terrible
consequences if you're wrong".

  However I don't think it's going to happen, given that it's been a couple of
days now and a whole load of commits have gone in on top.  If I for one second
thought that it had been going to happen, I would have objected.  Very loudly.

    cheers,
      DaveK

Re: doh?

[Michael Matz]

Hi,

On Fri, 27 Nov 2009, Dave Korn wrote:

> > PLEASE DO NOT DO THIS.
> 
>   However I don't think it's going to happen,

Yes, it's probably not going to happen; neither the requested revert.  
But now I at least know a strategy how to sneak in controversial patches.

> given that it's been a couple of days now and a whole load of commits 
> have gone in on top.  If I for one second thought that it had been going 
> to happen, I would have objected.  Very loudly.

I also complained loudly, I think.  Nobody is interested.


Ciao,
Michael.

Re: doh?

[Dave Korn]

Michael Matz wrote:
> Hi,
> 
> On Fri, 27 Nov 2009, Dave Korn wrote:
> 
>>> PLEASE DO NOT DO THIS.
>>   However I don't think it's going to happen,
> 
> Yes, it's probably not going to happen; neither the requested revert.  
> But now I at least know a strategy how to sneak in controversial patches.

  No, that is a false inference.  Nothing about the way we've decided to
handle this situation sets a precedent that we would be obliged to follow in
any future instance and I think there's a fairly strong consensus that this
patch wasn't suitable for the free-for-all rule owing to its scale.

>> given that it's been a couple of days now and a whole load of commits 
>> have gone in on top.  If I for one second thought that it had been going 
>> to happen, I would have objected.  Very loudly.
> 
> I also complained loudly, I think.  Nobody is interested.

  Everyone agrees that it was a bad idea for this patch to go in the way it
did, the only point on which there is any difference of opinion is what we
should do about it, and there, yes; you and Paolo are pretty much the only
people who feel that it should have been backed out, where I and others feel
that it's not worth the second helping of pain that doing so would cause and
that we should proceed not by doing anything in this particular case but by
trying to make sure it doesn't happen again in the future.

    cheers,
      DaveK

Re: doh?

[Paolo Bonzini]

On 11/27/2009 03:21 PM, Dave Korn wrote:
> you and Paolo are pretty much the only
> people who feel that it should have been backed out

Uh?  I said that the repository should have been made readonly if there 
was a concrete possibility of backing out the patch, be it with svn cp 
(which we already did a couple of times that trunk disappeared ;-) and 
svn blame works great) or by manual editing.

Paolo

Re: doh?

[Dave Korn]

Paolo Bonzini wrote:
> On 11/27/2009 03:21 PM, Dave Korn wrote:
>> you and Paolo are pretty much the only
>> people who feel that it should have been backed out
> 
> Uh?  I said that the repository should have been made readonly if there
> was a concrete possibility of backing out the patch, be it with svn cp
> (which we already did a couple of times that trunk disappeared ;-) and
> svn blame works great) or by manual editing.

  Well, if you say that's what you meant then that is what you meant and I
must have read more into your post than you intended, but I didn't see any of
those caveats in it(*) so I think my confusion is understandable!

    cheers,
      DaveK

-- 
(*) - http://gcc.gnu.org/ml/gcc/2009-11/msg00725.html

Re: doh?

[Joern Rennecke]

Quoting Michael Matz <matz@suse.de>:
> Yes, it's probably not going to happen; neither the requested revert.
> But now I at least know a strategy how to sneak in controversial patches.

I don't think the patch was controversial in the changes it made to the
code per se, only in the side effects that its check in has on source
code control.  (Plus, how can you be sure that such a large patch is safe?
There are a few rare situations in which trailing whitespace is required.)
I.e. for any date you pick for a check-in, there will be unmerged branches
and patches that will be affected.
Plus the ongoing effects on svn blame.  I wish there was a  
--ignore-space-change option for svn blame - that would also be useful  
to 'see through' ordinary
patches that change indentation levels due to changes in control flow, but
given how resource hungry the current svn blame is, I suppose we have to
wait till the repository fits into RAM before this is feasible.

RE: WTF?

[Lu, Hongjiu]

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:rguenther@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=gcc&view=rev&rev=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?

[H.J. Lu]

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.lu@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:rguenther@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=gcc&view=rev&rev=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?

[Jeff Law]

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?

[Lu, Hongjiu]

> >
> 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.lu@intel.com>
To: gcc-patches@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.