Closed Bug 1015779 Opened 6 years ago Closed 6 years ago

Remove trailing white space across mail/

Categories

(Thunderbird :: General, defect)

x86
All
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 32.0

People

(Reporter: jsbruner, Assigned: jsbruner)

References

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
So while I was working on a personal project I came up with this command to remove trailing whitespace from my project files, but realized we could probably use it to cleanup around mail/ as well.

Here's that command: find . -name "*.css" | xargs gsed -i 's/[ \t]*$//'

gsed == GNU sed
Assignee: nobody → josiah
Status: NEW → ASSIGNED
This is the cleanup for C++.

Joshua, for the review really just take a look at the command, unless of course want to go through every line. :)

C++ actually did the best in regards to not having much trailing white space.
Attachment #8428423 - Flags: review?(Pidgeot18)
Attached patch Remove trailing whitespace - JS (obsolete) β€” β€” Splinter Review
Mike, same deal as Joshua, just take a peak at the patch and focus on the command instead for the review. This should be pretty easy review-wise.

JS did the worst white space wise.
Attachment #8428424 - Flags: review?(mconley)
UL version.
Attachment #8428425 - Flags: review?(mconley)
ML.
Attachment #8428426 - Flags: review?(mconley)
C headers.
Attachment #8428427 - Flags: review?(Pidgeot18)
I just CC'd people I know may be affected by this change. Just as a heads up that you may have to un-bitrot your patches.
I'll probably have to unbitrot these patches before landing, so don't r- for such a reason.
I welcome such cleanup but I am sure some reviewers may frown upon such unnecessary noise ;)
Hm, yes, as you loose history, or make it hard to get to at least. Some history is more valuable than other of course...
You don't really loose a lot of history though. One commit per file isn't really that big a deal I think. Unless there's some other history you're taking about besides a file's hg commit log.

I imagine quite a bit of this is a matter of preference. Personally I'd prefer to not have to deal with the white space (I frequently either have to remove white space when writing my own patches, or they are mentioned during review, even if I didn't actually add it. And I do the same when reviewing others)

So I myself would rather not have to worry about that anymore, and just take the little bit of hg history cloudiness. But I suppose Joshua and Mike can make the call.
You don't loose history, you lose history. :-)

English grammar aside, both hg and git let you follow blame while ignoring whitespace [CVS didn't,  but it's in the minority of VCSen]. The standard web interfaces for hg and git don't give you this option, but we fully control DXR and it's not unfeasible to add this kind of option to DXR at some point in the future. As one of the relatively few people on this bug that regularly does deep-dives on history of source code [to CVS 1.1 era], I'm not horribly bothered by whitespace-only changes, particularly if the commit message clearly indicates that it is whitespace-only.

(In reply to Josiah Bruner [:JosiahOne] from comment #1)
> Here's that command: find . -name "*.css" | xargs gsed -i 's/[ \t]*$//'

A few things to keep in mind:
1. Not all file formats are whitespace-agnostic. Makefiles are extremely bad offenders in this regard, but email files also have very meaningful CRLF and trailing whitespace that should not be obliterated. Arguably some of our textual resources for tests fall under the same category.
2. Different files have different coding styles. Sometimes even different files with the same extensions (yay for our "follow prevailing style" exception :-/ ).

I personally kind of prefer doing a single pass with a smart formatting tool (e.g., clang-format) to make all of our code conform to style guidelines, but that option never seems to get off the ground.
Yes I was talking about hg blame. I didn't know about the hg annotate -w option, seems silly that's not the default in the web ui.
Attachment #8428427 - Flags: review?(Pidgeot18) → review+
Attachment #8428423 - Flags: review?(Pidgeot18) → review+
Attachment #8428424 - Flags: review?(mconley) → review+
Attachment #8428425 - Flags: review?(mconley) → review+
Attachment #8428426 - Flags: review?(mconley) → review+
Updated for trunk.
Attachment #8428424 - Attachment is obsolete: true
Attachment #8435478 - Flags: review+
Flags: in-testsuite-
Depends on: 1530955
No longer depends on: 1530955
See Also: → 1530955
You need to log in before you can comment on or make changes to this bug.