Skip to content

Conversation

@tinganho
Copy link
Contributor

Just figured out how to fix the new line problem we had.

We could leave tests folder ignoring normalization:

tests/**/*          crlf=false 

But that only works if no other overrides it so all other attributes must be more selective

src/**/*.json       eol=crlf
src/**/*.ts         eol=crlf
scripts/**/*.cmd    eol=crlf
scripts/**/*.js     eol=crlf
scripts/**/*.js.map eol=crlf
scripts/**/*.json   eol=crlf
scripts/**/*.ts     eol=crlf

In order for git to convert LF to CRLF it also need to have clean files with CRLF. So I updated all the files under src. And I got this warning outputted if I try to add LF:

warning: LF will be replaced by CRLF in src/compiler/emitter.
The file will have its original line endings in your working directory

Otherwise, I think git tries to be smart and not normalize files to prevent an explosion from happening.

I just tried everything above and it works for me on a Mac with:

git --version
git version 2.3.2 (Apple Git-55)

@DanielRosenwasser
Copy link
Member

I actually have a commit for this here: 52e6b7f

Mainly I did this because some on the team would prefer we whitelist instead of blacklist files. I'm generally rethinking it now.

@tinganho
Copy link
Contributor Author

@DanielRosenwasser I think that solution turns off all normalization even under src. With eol=crlf we can convert LF to CRLF.

@tinganho tinganho force-pushed the gitattributes branch 2 times, most recently from e9be8ea to 55b4cbe Compare May 15, 2015 06:19
@DanielRosenwasser
Copy link
Member

@tinganho why would that be? I thought later entries in .gitattributes got priority.

Also, I thought the idea was that we wanted autocrlf for src.

@tinganho
Copy link
Contributor Author

+src//*.json text
+src/
/*.ts text

I believe the text attribute normalize to LF. At least according to their man page:

When a text file is normalized, its line endings are converted to LF in the repository.

Also, I thought the idea was that we wanted autocrlf for src.

The autocrlf can have three values? false, true and input. But also looking at this table. There can still be mixed crlf and lf line endings for all three values:
http://stackoverflow.com/questions/1249932/git-1-6-4-beta-on-windows-msysgit-unix-or-dos-line-termination/1250133#1250133

@mhegazy
Copy link
Contributor

mhegazy commented May 19, 2015

@DanielRosenwasser can you give this a test and see what we need to do here.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 19, 2015

@tinganho and @DanielRosenwasser do we still need this?

@tinganho
Copy link
Contributor Author

I think if we want LF converted into CRLF then this is still relevant. Though there exists -text attribute now in .gitattributes. It will treat everything as binary files and not do any conversion/normalization at all. Most non-windows repo just have the text attribute to convert CRLF to LF. This just the opposite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be mistaken, but I don't think this is a valid flag.

@DanielRosenwasser
Copy link
Member

Here's still my idealized scenario:

  1. Everything is -text unless otherwise specified
  2. Everything in src is text.
  3. Everything in scripts is text.
  4. All top-level text files are text.
  5. doc/docx/pdf files have astextplain for diffing.

This PR addresses a different view: that the text should just always be CRLF, which some on the team do agree with, but I personally don't. I'd prefer we didn't go down that route.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 19, 2015

My question is more about the source issue. We have got regular issues about line ending until we added "-text". i have not seen any since. is this still an issue we need to fix?

if so, @DanielRosenwasser 's proposal seem to have less movig pieces, and i would just enforce this on src.

@tinganho
Copy link
Contributor Author

I think @DanielRosenwasser proposal is to have LF now in src and scripts. I personally like LF more than CRLF.

http://git-scm.com/docs/gitattributes

@tinganho
Copy link
Contributor Author

I'm also have not been able to set overriding rules in gittattributes. I'm not sure why. If you see my PR I set everything explicitly.

@DanielRosenwasser
Copy link
Member

I'm also have not been able to set overriding rules in gittattributes. I'm not sure why.

Same, this is why I haven't been able to get this moving. What OS are you running on @tinganho?

@tinganho
Copy link
Contributor Author

Mac OS.

On Sat, Jun 20, 2015, 09:27 Daniel Rosenwasser notifications@github.com
wrote:

I'm also have not been able to set overriding rules in gittattributes. I'm
not sure why.

Same, this is why I haven't been able to get this moving. What OS are you
running on @tinganho https://github.com/tinganho?


Reply to this email directly or view it on GitHub
#3174 (comment)
.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 23, 2015

so what are we doing with change at this point?

@DanielRosenwasser
Copy link
Member

I think it would be appropriate to close this and open up a new PR some point down the line to accomplish what I've described.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 23, 2015

closing then. thanks

@mhegazy mhegazy closed this Jun 23, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5d470fd on tinganho:gitattributes into ** on Microsoft:master**.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants