1323575 - Add a commit hook that prevents unintentional line ending changes

Status: NEW

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Bug 1323521 landed a commit that rewrote all of the line endings in nsWindow.cpp, causing an autoland tree closure while we figured out what to do about it.

It was apparently caused by aklotz' editor noticing a couple lines with Windows endings and mass-changing the entire file. The original Windows line endings in that file came in from bug 1273091.

It would be nice if we had a commit hook that prevented this from happening in the future. Obviously with an override if said changes are actually wanted.

Comment 1

[Gregory Szorc [:gps]]

I'm generally in support of this.

Do note that hooks that look at file content are much more expensive than hooks that look at just metadata, like the set of files that changed. So this will impact push speed for large pushes.

As for implementation, I propose we examine non-binary files (files without a \0) and error if the count of \r\n in the new revision is greater than the count of \n (not preceded by \r) in the old revision. This will allow existing \r\n to be carried forward while stopping conversions from \n to \r\n.

There may be a few Windows files in the tree that expect \r\n line endings. We may have to establish a whitelist of extensions allowing this newline convention and/or a commit message magic word to allow these line endings through.

Comment 2

[Mike Hommey [:glandium]]

Corollary: mozreview doesn't show CRLFs being added. But if we reject them at push time to the reviewboard repo, then mozreview doesn't need to care about it.

Comment 3

[Gregory Szorc [:gps]]

We can also have the client look for this at push or event commit time. I'm all for failing fast. We will need defense in depth, as not everyone is using MozReview :/

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Is this still relevant?

Comment 5

[Connor Sheehan [:sheehan]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)

Is this still relevant?

It might be. I'm going to run a test and see how Phabricator displays such changes in it's diff view. If the line ending changes are picked up and displayed by Phabricator I would think most devs would notice and the error would be caught at review time. If Phabricator doesn't display any differences we should add some automation to avoid such changes.