Open Bug 496703 Opened 12 years ago Updated 4 years ago

A commit hook should prevent files containing CRLF line endings to be added to the tree


(Developer Services :: Mercurial:, defect)

Not set


(Not tracked)


(Reporter: ehsan.akhgari, Unassigned)



(Whiteboard: [kanban:engops:] )


(1 file, 1 obsolete file)

We need to have a commit hook on which rejects commits with CRLF (Windows) line endings inside them.  We can look for a special token inside the commit message (such as "WITH CRLF LINES") to override this behavior in case CRLF line endings are really wanted (for example, in a test case.)
OS: Linux → All
Hardware: x86 → All
Product: → Release Engineering
Product: Release Engineering → Developer Services
This would still be very valuable, but looks like it got lost in time.
I'm still seeing crlf patches attached, and it's often very hard to notice the small red WINDOWS PATCH hint bugzilla puts in splinter.
We appear to have a lot of files not using LF. Look for yourself.

$ hg locate 'set:not binary() and not eol(unix)'
Whiteboard: [kanban:engops:]
This rejects only new files with CRLF line endings. The check can be bypassed with "INTENTIONAL CRLF" in the commit message.
Assignee: nobody → birunthan
Attachment #8508169 - Flags: review?(gps)
Added an extra test.
Attachment #8508169 - Attachment is obsolete: true
Attachment #8508169 - Flags: review?(gps)
Attachment #8508176 - Flags: review?(gps)
Duplicate of this bug: 752370
Comment on attachment 8508176 [details] [diff] [review]
Add commit hook to reject new files with CRLF line endings

Review of attachment 8508176 [details] [diff] [review]:

This is technically mostly solid.

I have concerns about the general approach of hard-coding logic like this in the server. That's a very firm and inflexible solution. Something that is guided by in-tree policies is a much better approach IMO. If you've ever used Subversion properties, you know what I am talking about.

At a previous company, I hacked up our server-side hooks to look for well-named files in source control which dictated settings. For example, you had a global policy file in your repository that said what file extensions could have CRLF line endings. This could be supplemented by per-directory files. The hooks looked at individual files and the policies that applied to it and made intelligent decisions based on policies that were flexible over time.

I'd like to eventually do something similar for Mozilla. I would likely also sneak module ownership and reviewer data in said files to make "find a reviewer" easier. But that's scope bloat.

Anyway, I'm kinda nervous about this approach. Before we deploy this, I'd like to see a listing of all files in mozilla-central this hook would currently fail.

::: hghooks/mozhghooks/
@@ +24,5 @@
> +    description = repo.changectx(changesets[-1]).description()
> +
> +    # Check 'a=release' to leave uplifts alone.
> +    if 'INTENTIONAL CRLF' in description or 'a=release' in description.lower():
> +        return 0

Limiting INTENTIONAL CRLF to the tip commit is not a good idea. If you e.g. land something to inbound, it is fine. But when you do the merge, it will fail because INTENTIONAL CRLF isn't in the merge commit.

We should honor INTENTIONAL CRLF in the individual commit as well.

@@ +28,5 @@
> +        return 0
> +
> +    invalid_paths = []
> +
> +    for changeset in reversed(changesets):

Why in reversed order? Going from smallest to highest is actually more efficient for Mercurial.

@@ +30,5 @@
> +    invalid_paths = []
> +
> +    for changeset in reversed(changesets):
> +        ctx = repo.changectx(changeset)
> +        for path in ctx.files():

I believe this will iterate over deleted files. I'm pretty sure something in this loop will trigger an exception due to accessing a file that doesn't exist in the current revision. I might be wrong. Please add tests that delete files with and without CRLF, just to be sure.

Also, some old Mercurial clients are buggy and don't send a proper ctx.files() to the server. This means your hook may not see all files changed by the commit. You shouldn't worry about this though. But I just thought I'd let you know.
Attachment #8508176 - Flags: review?(gps) → feedback+
Whiteboard: [kanban:engops:] → [kanban:engops:] [kanban:engops:]
Whiteboard: [kanban:engops:] [kanban:engops:] [kanban:engops:] [kanban:engops:] → [kanban:engops:] [kanban:engops:] [kanban:engops:] [kanban:engops:]
Whiteboard: [kanban:engops:] [kanban:engops:]
Whiteboard: [kanban:engops:]
Whiteboard: [kanban:engops:] → [kanban:engops:]
Assignee: birunthan → nobody
QA Contact: hwine
QA Contact: hwine → klibby
You need to log in before you can comment on or make changes to this bug.