Pt/native crlf #77

Closed
wants to merge 1 commit into
from

5 participants

@patthoyts
MSysGit - the development behind Git for Windows member

This should handle issue #57. The earlier commit that set the build config NATIVE_CRLF = YesPlease didn't actually get propagated to the C compiler. This patch resolves that but the result has some additional test suite problems that require resolving before we merge this.

@patthoyts patthoyts Push the NATIVE_CRLF Makefile variable to C and added a test for native.
The previous patch correctly points out that the NATIVE_CRLF setting is
incorrectly set on Mingw git. However, the Makefile variable is not
propagated to the C preprocessor and results in no change. This patch
pushes the definition to the C code and adds a test to validate that
when core.eol as native is crlf, we actually normalize text files to this
line ending convention when core.autocrlf is false.

Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
b4e2f5e
@buildhive

MSysGit - the development behind Git for Windows » git #102 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@patthoyts
MSysGit - the development behind Git for Windows member

Updated with a rebased branch that was tried against 1.8.3 recently.

@buildhive

MSysGit - the development behind Git for Windows » git #103 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@Tvangeste

Any progress on this one? The current situation is rather ridiculous, the whole CRLF mess in git, with many settings and various ways to adjust things, which doesn't even work at the end, since (msys)git assumes that native EOL on Windows is LF!!! 8-)

@dscho
MSysGit - the development behind Git for Windows member

@Tvangeste if you wanted to be helpful, you were going about it in a counterproductive way.

If you want to see this happening, get down to the code, hammer it out, fix what needs to be fixed, and get it done.

If you want this pull request to die a cruel death without resolution, however, you could start insulting the people who actually do work on this project in their spare time.

@Tvangeste

@dscho, no disrespect intended, really. Sorry, if I offended anyone. I do work on open source projects in my spare time too, and I spent about two full days of my vacation time trying to figure out what's the best way to deal with CRLFs in our source base and why eol=native doesn't really work in gitattributes.

As far as I can see, the pull request has already been made, hence the question about its status and the reason why it not merged. The issue seems to be a rather important one. At least we have 3 configuration parameters related to CRLF, we have new way of doing things with gitattributes, so a lot of efforts has already been made in this area, and in the end, the very core functionality is completely broken.

@dscho
MSysGit - the development behind Git for Windows member

@Tvangeste Pat stated that there are test suite failures, and BuildHive agrees. That's the big blocker.

@bricelam

It looks like the failing test is an assertion that setting core.eol to 'native' results in a file with 'cr' line endings. This is, of course, now an invalid assertion on Windows.

@bricelam bricelam commented on the diff Aug 14, 2013
t/t0026-eol-config.sh
@@ -80,4 +94,22 @@ test_expect_success 'autocrlf=true overrides unset eol' '
test -z "$onediff" -a -z "$twodiff"
'
+test_expect_success NATIVE_EOL_IS_CRLF 'eol native is crlf' '
+
+ rm -rf native_eol && mkdir native_eol &&
+ ( cd native_eol &&
+ printf "*.txt text\n" > .gitattributes
+ printf "one\r\ntwo\r\nthree\r\n" > filedos.txt
+ printf "one\ntwo\nthree\n" > fileunix.txt
+ git init &&
+ git config core.autocrlf false &&
+ git config core.eol native &&
+ git add filedos.txt fileunix.txt &&
+ git commit -m "first" &&
+ rm file*.txt &&
+ git reset --hard HEAD &&
+ has_cr filedos.txt && has_cr fileunix.txt
@bricelam
bricelam added a line comment Aug 14, 2013

Shouldn't these be "! has_cr" assertions on Windows?

@bricelam
bricelam added a line comment Aug 15, 2013

Never mind; I'm still trying to wrap my head around all of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dscho
MSysGit - the development behind Git for Windows member

@bricelam as I stated in my comment on your commit, I do not think that this is the correct fix. This particular test case is executed under the NATIVE_EOL_IS_CRLF condition, so I would expect both filedos and fileunix to have carriage returns and line feeds in the local version.

@bricelam

Like @Tvangeste, I'm shocked this hasn't received more attention from someone with the ability to fix it. I'll continue trying to fumble my way through it, but it's all rather foreign coming from a heavy C#/VS/non-Linux background.

@bricelam

The good news is that it looks like all the failures are from tests that set text=auto. Hopefully they are just from assertions made invalid by this change. If so, I should be able to get a new pull request out pretty soon.

@patthoyts
MSysGit - the development behind Git for Windows member

merged as part of #88

@patthoyts patthoyts closed this Sep 13, 2013
@Tvangeste

Many thanks to everybody involved in this, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment