Closed Bug 770286 Opened 7 years ago Closed 4 months ago

Add .gitattributes file

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: gps, Assigned: saschanaz)

References

Details

Attachments

(2 files)

We should probably have a .gitattributes file checked in to the root directory of mozilla-central to enforce conventions.

https://www.kernel.org/pub/software/scm/git/docs/gitattributes.html

At the very least, we should probably define EOL conventions so LF is preserved everywhere (I'm pretty sure checkouts on Windows using Mercurial use LF, so we should force LF everywhere).

We could also add in default diff arguments so patches are produced with Mozilla's preferred 8 lines of context.

And, if we want to be slightly sneaky, we could probably set some whitespace config parameters so patches with trailing whitespace are treated as errors. (This is the default git behavior, but some people may have it disabled.)
CC Hal, as he is working on m-c<->github mirroring
(In reply to Justin Wood (:Callek) from comment #1)
> CC Hal, as he is working on m-c<->github mirroring

Correction: we're supporting m-c -> git read only mirroring. All changes to m-c must be landed on hg.m.o

I believe this is simply a coding style issue for the dev community to reach consensus on and implement. It should have zero impact on either the CI system or hg -> git work.
Product: Core → Firefox Build System
Duplicate of this bug: 1546953

As of now this is not just a coding style issue. Building SpiderMonkey on Windows Subsystem for Linux fails when the local repository is cloned on Windows as CRLF autoconversion causes shell syntax error and Rust file checksum errors.

Adding needinfo to the relevant person...

Flags: needinfo?(hwine)

I'm no longer relevant here -- Connor?

Flags: needinfo?(hwine) → needinfo?(sheehan)

I'm not sure if the patch from comment 5 would fix the issue reported in comment 4, or if there's a better way to resolve that issue.

glandium, does this look okay to you?

As an aside, we're no longer accepting patches through Bugzilla. Please submit this through Phabricator.

Flags: needinfo?(sheehan) → needinfo?(mh+mozilla)

The premise in comment 0 doesn't seem to be true. AFAICT, committing a file with CRLF endings on Windows with mercurial keeps the file with CRLF endings in the repo. That's probably why we have some files that this in the tree.
The .gitattributes attached in comment 5 would make the git behavior nice, sure, but it would also be different from mercurial.

Flags: needinfo?(mh+mozilla)

(In reply to Mike Hommey [:glandium] from comment #9)

committing a file with CRLF endings on Windows with mercurial keeps the file with CRLF endings in the repo.

In that case I suggest two things:

  1. Turn off git autocrlf completely (* binary) so that it matches the mercurial behavior.
  2. Or, add a mercurial config to use LF on Windows.

I tried doing git checkout with * text=auto eol=lf and found the files that already added with CRLF in the repo won't lose their CR even with the settings. So I think my previous suggestion already matches mercurial checkout behavior. What do you think?

Flags: needinfo?(mh+mozilla)

I don't personally have a strong opinion on which particuliar behavior should be hardcoded, but whatever it is, we should make sure both mercurial and git do the same thing.

Flags: needinfo?(mh+mozilla)

Currently git for Windows converts LF to CRLF by default, so this change prevents that to match the mercurial behavior.

We could do * -text but it will break current git clones by detecting all already normalized files as uncommitted new changes. I found it too scary, I prefer we do * text=auto eol=lf to turn off the current unexpected LF-to-CRLF conversions first and then discuss about -text. Thoughts?

Assignee: nobody → saschanaz
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4762f5a28a72
Prevent git LF-to-CRLF autoconversion r=glandium

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.