600536 - fixtabs and tamarin-commit-hook don't agree

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tools, defect)

Description

[Tommy Reilly]

the tamarin-commit-hook hook skips lines with just whitespace and fixtabs removes the whitespace

Comment 1

[Tommy Reilly]

Attachment: make the hook more picky to match fixtabs

Comment 2

[Tommy Reilly]

Attachment: run fixtabs on MMgc/[*.cpp,*.h]

Comment 3

[Lars T Hansen]

Comment on attachment 479384 [details] [diff] [review]
run fixtabs on MMgc/[*.cpp,*.h]

I'm inclined to R- this since we have three large outstanding patches on the MMgc code, namely, Simon's threading work, Brent's GCRef work, and my exact marking work.  But I'll hold off until I've attempted to apply my patches on top of this one, if it goes OK I may worry less.  Stay tuned.

Comment 4

[Edwin Smith]

Comment on attachment 479376 [details] [diff] [review]
make the hook more picky to match fixtabs

I think Chris put in the logic to skip empty lines, based on some pushback that IDE's that auto-indent, aggressively insert empty lines with trailing whitespace.  (XCode, I got my eye on you...)

However, we should make the scripts agree, and I'll vote in favor of fixtabs's more aggressive trailing whitespace removal.

aside: shame on me/us for not documenting the formatting standards.  but if we really want to relax this, then lets do it in both scripts.

Comment 5

[Tommy Reilly]

I could go either way, I was just trying to cleanup Brent's safegc patches for landing by using fixtabs...

Comment 6

[Tommy Reilly]

(In reply to comment #3)
> I'm inclined to R- this since we have three large outstanding patches on the
> MMgc code, namely, Simon's threading work, Brent's GCRef work, and my exact
> marking work.  But I'll hold off until I've attempted to apply my patches on
> top of this one, if it goes OK I may worry less.  Stay tuned.

I'm happy to sit on it and revisit when we're ready to land the safegc patches

Comment 7

[Edwin Smith]

fwiw, when/if the time comes, I've had good results with merging whitespace churn this way:

1. run fixtabs on your branch
2. commit
3. run fixtabs on mainline (if not already done)
4. commit
5. hg  merge.  the hg internal merge utility is decent about ignoring whitespace.  i have this in my ~/.hgrc:
[ui]
merge = internal:merge

Comment 8

[Tommy Reilly]

played with xcode you can prevent it from indenting on newline by selecting "Return" from the auto indent characters.

Comment 9

[Tommy Reilly]

actually xcode will still put in 4 spaces if you hit tab on a empty line.   Since I'm ambivalent and it will cause less churn to just relax fixtabs I'm proposing that now.   arguably something call fixtabs shouldn't be doing anything else.

Comment 10

[Tommy Reilly]

Attachment: don't strip lines with just spaces in fixtabs

New approach, relax fixtabs

Comment 11

[Tommy Reilly]

if we'd like we can view this as a temporary measure and if we want to remove these lines with just spaces we can update fixtabs and the commit hook together at some later time, for now make them coincide and don't cause any churn (fixtabs with my patch doesn't change anything in core or mmgc)

Comment 12

[Edwin Smith]

Comment on attachment 479487 [details] [diff] [review]
don't strip lines with just spaces in fixtabs

I think fixtabs and the mercurial hook should enforce no-trailing-whitespace as well as no-tabs, as implemented in fixtabs.  Maybe we should dust off whatever code formatting spec we have, and put it in docs (but at least the spec would say no tabs, 4-space indent, no trailing whitespace).

Feel free to outvote me on a more relaxed format standard; I don't feel strongly enough to go against a majority opinion.

In any case, +1 to consistency and renaming fixtabs if necessary to reflect what it really does (fixwhitespace).

Comment 13

[Tommy Reilly]

I agree with you but its clear to me from the # of instances of this that a # of people are using editors that do it (xcode).   Here's the diffstat from running fixtabs in core, nanojit and MMgc.


treilly-MacBookPro:tr-mq treilly$ utils/fixtabs core/*.h core/*.cpp MMgc/*.cpp MMgc/*.h nanojit/*.cpp nanojit/*.h
treilly-MacBookPro:tr-mq treilly$ hg diff | diffstat
 MMgc/FixedAlloc-inlines.h      |    6 +-
 MMgc/FixedAlloc.cpp            |    6 +-
 MMgc/FixedAlloc.h              |   34 +++++++--------
 MMgc/FixedMalloc.cpp           |   10 ++--
 MMgc/FixedMalloc.h             |   16 +++----
 MMgc/GC.cpp                    |   10 ++--
 MMgc/GC.h                      |    4 -
 MMgc/GCAlloc-inlines.h         |    6 +-
 MMgc/GCAlloc.cpp               |   18 ++++----
 MMgc/GCAlloc.h                 |    6 +-
 MMgc/GCHashtable-inlines.h     |    2 
 MMgc/GCHashtable.h             |    8 +--
 MMgc/GCHeap.cpp                |    6 +-
 MMgc/GCHeap.h                  |    4 -
 MMgc/GCMemoryProfiler.cpp      |   10 ++--
 MMgc/GCObject.h                |    2 
 MMgc/GCPolicyManager-inlines.h |    8 +--
 MMgc/GCPolicyManager.cpp       |   24 +++++------
 MMgc/GCPolicyManager.h         |    2 
 MMgc/GCStack.cpp               |    2 
 MMgc/GCWeakRef.h               |    2 
 MMgc/MMgc.h                    |    2 
 MMgc/Shared-inlines.h          |    2 
 core/ArrayClass.cpp            |   14 +++---
 core/AvmCore.cpp               |    6 +-
 core/AvmCore.h                 |    2 
 core/CodegenLIR.cpp            |    2 
 core/DomainMgr.h               |    2 
 core/Interpreter.cpp           |    4 -
 core/MethodInfo-inlines.h      |    2 
 core/Traits.cpp                |    2 
 core/XMLParser16.cpp           |    2 
 core/avmfeatures.h             |   22 +++++-----
 core/avmplus.h                 |    2 
 core/peephole.cpp              |   88 ++++++++++++++++++++---------------------
 35 files changed, 169 insertions(+), 169 deletions(-)


nj is clean, core ain't bad, MMgc is pretty bad.

Comment 14

[Edwin Smith]

core/avmfeatures.h is a bug; it is a generated file, no excuse for whitespace mixups there.  I thought I fixed it, but apparently not.

Comment 15

[Tommy Reilly]

(In reply to comment #14)
> core/avmfeatures.h is a bug; it is a generated file, no excuse for whitespace
> mixups there.  I thought I fixed it, but apparently not.

Indeed: https://bugzilla.mozilla.org/show_bug.cgi?id=600653

Comment 16

[Lars T Hansen]

(In reply to comment #13)
> I agree with you but its clear to me from the # of instances of this that a #
> of people are using editors that do it (xcode).

I use Xcode on Mac with standard auto-indent and I'm not likely to stop doing that - I hate what it does with blank lines but that's just a FOL.  I'll oppose any coding standard that prohibits non-empty blank lines as that standard would make me do a fair amount of busywork for absolutely no tangible benefit whatsoever.  (How's that for bias.)

Meta-point: legislate where it matters, stay silent where it doesn't.  Disallowing tabs in indentation clearly matters; whether lines end with blanks or not does not; whether blank lines end with blanks is a distraction.

Comment 17

[Lars T Hansen]

Comment on attachment 479487 [details] [diff] [review]
don't strip lines with just spaces in fixtabs

I'm fine with this but obviously the R- from Ed counts.

Comment 18

[Tommy Reilly]

http://hg.mozilla.org/tamarin-redux/rev/97dfed717ac2

Took Edwin up on his offer to outvote him.   Already spent more time on this than its worth.

Comment 19

[Edwin Smith]

I'm fine with this, but please document the decision.

Comment 20

[Tommy Reilly]

sure, a comment in fixtabs or is there a better place?

Comment 21

[Edwin Smith]

minimal: comments in fixtabs and the commit hook
better: Start a skeliton "code format" document under /doc, suitable for publishing to html (doxygen or our ad-hoc wiki syntax).

Comment 22

[Tommy Reilly]

Attachment: strip some trailing whitespace

Strip trailing whitespace only on lines that aren't pure whitespace.  Now fixtabs and the commit check are actually symmetric and you can run fixtabs w/o fear of creating changes on all those xcode pure whitespace lines.

Comment 23

[Lars T Hansen]

Comment on attachment 495515 [details] [diff] [review]
strip some trailing whitespace

Looks credible.

Comment 24

[Tamarin Bot]

changeset: 5631:d3bb662e20d4
user:      Tommy Reilly <[e-mail]>
summary:   Bug 600536 - restore eol whitespace stripping to fixtabs w/o touching pure whitespace lines Xcode likes to add (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/d3bb662e20d4

Comment 25

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 495515 [details] [diff] [review]
strip some trailing whitespace

Best of all would be if the code (i.e. line-matching regexps) were identical in both fixtabs and the commit hook (so that it would be trivial to verify that the behavior is the same.  But the logical structure of the commit hook isn't just a regexp (instead its using strip with a if branch), so achieving that would require more work than this is worth.