284755 - The whitespace change to end all whitespace changes

Status: NEW

(Bugzilla :: Testing Suite, enhancement)

Description

[Marc Schumann [:Wurblzap]]

Whitespace-only changes are undesirable in patches, so it might be a good idea
to rename the 005no_tabs.t test to 005whitespace.t and have it check for
trailing whitespace on lines and trailing empty lines in files in addition to
its current work.

This will help people using editors that automatically remove such superfluous
whitespace, and it is bound to reduce review cycles.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch I

Despite the huge patch size (I need to split it in two), this is actually an
easy review. The only content change is in t/005whitespace.t (formerly known as
t/005no_tabs.t). The remaining changes are all whitespace-only changes to have
the code pass the new test :)

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch II

Comment 3

[Marc Schumann [:Wurblzap]]

For the record:

(15:29:24) LpSolit: hello Marc! I'm not sure your bug 284755 is a good idea :-/
(15:29:43) Wurblzap: I expected some discussion about it :)
(15:29:49) Wurblzap: That's why it's unconfirmed
(15:30:00) Wurblzap: What's your thoughts?
(15:30:14) LpSolit: Wurblzap: this will confuse bonsai and the historic of files
(15:30:28) Wurblzap: Because of the renaming?
(15:30:39) Wurblzap: Yes
(15:30:59) LpSolit: when you want to find which patch modified this or this part
of the code, you will always find 'bug 284755' ;)
(15:31:12) Wurblzap: Heh, true ;)
(15:31:30) LpSolit: Wurblzap: I use bonsai a lot to find regressions
(15:31:47) LpSolit: Wurblzap: it would make such searches very difficult
(15:31:59) Wurblzap: Old code will suddenly seem new
(15:32:05) LpSolit: Wurblzap: because your patch would hide the real ones
(15:32:22) Wurblzap: Yes, I'm aware. I mean, that's the reason we don't want
whitespace-only changes.
(15:32:30) Wurblzap: I'm trying to get rid of them for the future
(15:32:36) LpSolit: Wurblzap: each patch should clean the part they are working on
(15:32:58) LpSolit: but I would like a patch which fix them all
(15:33:05) LpSolit: *wouldn't*
(15:34:40) LpSolit: Wurblzap: if you fix whitespaces in the sub you are working
on, well why not... but not in an entire file
(15:35:03) LpSolit: and even less in the whole bugzilla directory ;)
(15:35:17) Wurblzap: Do you have an idea how to introduce a test?
(15:35:30) LpSolit: what kind of test??
(16:15:56) LpSolit: you want a test for all existing files or new patches only??
(16:16:21) Wurblzap: Well that's exactly the question -- how can we differentiate?
(16:16:40) Wurblzap: It would be good if we could test new contributions only
(16:16:58) LpSolit: yes, I was thinking about that too
(16:28:26) LpSolit: I like your idea about 005whitespace but it should only
apply to the new code, something that landfill could not do for example (because
that means that the trunk is already updated, so what means new here?)
(16:36:58) Wurblzap: LpSolit: Maybe checker-ins could have some trimming script
apply to patch files? Removing trailing whitespace from lines having a '+' as
the first character?
(16:37:45) LpSolit: sounds good
(16:38:06) LpSolit: kind of trim.pl script?
(16:39:15) LpSolit: or even better, we could write a script in the contrib
directory which checks your patch: perl trim.pl patch_to_check.diff
(16:39:56) Wurblzap: Hey good idea
(16:40:01) LpSolit: this way, everybody can test his own patches and makes life
easier for reviewers too
(16:41:04) LpSolit: and as you said, the script should only check lines
beginning with a +
(16:42:04) Wurblzap: I think both would be best -- check first, then trim
automatically if not good
(16:42:05) LpSolit: Wurblzap: could trailing whitespaces be required in
templates? Or is a newline equivalent to a whitespace?
(16:43:20) Wurblzap: There is no risk if it were indeed required. It could be
mentioned in the bug, and the checker-in would have to take care to keep the
trailing whitespace.
(16:43:32) LpSolit: automatically *but* with a message displayed to the user
saying that his patch has been altered
(16:43:35) Wurblzap: But I can't believe trailing whitespace is needed anywhere
(16:43:48) Wurblzap: LpSolit: Yes
(16:45:11) LpSolit: or do we prefer a two steps process? first check, then ask
the user (only if trailing whitespaces are detected) if he wants to remove these
useless whitespaces [y/n]
(16:47:31) Wurblzap: Or a param
(16:48:15) LpSolit: what kind of? trim.pl --auto
(16:48:47) Wurblzap: Yes, something like that

Comment 4

[Max Kanat-Alexander]

This will extend my development cycle, though, not shorten it -- I use an editor
that automatically indents me to where I ought to be. That's why there's
trailing whitepace. Sometimes it indents me and then I hit Enter again.

If I have to find every place that my patch does that, or if I have to run some
additional script on my patches every time I want to upload them, that will just
extend my development cycle. It will also make it difficult for non-core
developers to submit patches.

The only whitespace issues that I'm worried about are when people don't indent
things properly.

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 176273 [details] [diff] [review]
Patch I

Removing r? and leaving this as food for thought :)

Comment 6

[Shane H. W. Travis]

I have no issue with whitespace-only patches. I much prefer them to patches 
that fix something else, and as a side-effect go and fix up the whitespace on a 
bunch of unrelated code.

While I recognize LpSolit's concern that such a change would affect files 
globalle, that doesn't matter to me because hovering over the revision number 
clearly shows (from the checkin information) that it is a whitespace-only 
change. If something changes it after that point, this message will be 
overridden; if someone really needs to see the history of the file before the 
whitespace change, then simply use LXR/CVSBlame to pull out the version 
immediately prior to the whitespace fix. The links are all clearly marked, and 
it's not like people have to go digging, so I don't see it as that much of an 
impediment.

Having said that, however... I do agree with Max that the whitespace we should 
be most concerned about is that of indentation, and not trailing whitespace. If 
you're changing the line, and change the trailing whitespace in the process... 
so what? And if you're *not* changing the line, and your editor is removing 
trailing whitespace *anyway*, then you either need to get a different editor or 
teach your editor not to do that.