Pt/native crlf (with updates) #88

Closed
wants to merge 2 commits into from

5 participants

@bricelam

This is another iteration on top of pull request #77 in an attempt to get issue #57 fixed.

@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>
8727c0c
@bricelam bricelam commented on an outdated diff Aug 16, 2013
t/t6038-merge-text-auto.sh
@@ -97,9 +97,9 @@ test_expect_success 'Merge addition of text=auto' '
test_expect_success 'Detect CRLF/LF conflict after setting text=auto' '
q_to_cr <<-\EOF >expected &&
<<<<<<<
- first line
- same line
- =======
+ first lineQ
+ same lineQ
+ =======Q
first lineQ
same lineQ
>>>>>>>

These changes don't feel right to me. It seems like the conflict file should preserve the line endings instead of normalize them all to crlf. Any thoughts?

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

Also, should these tests handle different native line endings? If so, what is the best way to make them conditional?

@buildhive

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

@bricelam

Hmm... @buildhive doesn't appear to have built the updated version of git before running the tests. I'm not sure what I'm doing wrong here. Is there any documentation on submitting pull request to this project?

@dscho
MSysGit - the development behind Git for Windows member

@bricelam actually, BuildHive builds on Linux. And it did it correctly: when I check out your crlf branch here, on Linux, and run the tests, I run into exactly the same problem:

*** t0026-eol-config.sh ***
ok 1 - setup
ok 2 - eol=lf puts LFs in normalized file
ok 3 - eol=crlf puts CRLFs in normalized file
ok 4 - autocrlf=true overrides eol=lf
ok 5 - autocrlf=true overrides unset eol
not ok 6 - eol native is crlf
#   
#   
#       rm -rf native_eol && mkdir native_eol &&
#       ( cd native_eol &&
#       printf "*.txt text
#   " > .gitattributes
#       printf "one
#   two
#   three
#   " > filedos.txt
#       printf "one
#   two
#   three
#   " > 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
#       )
#   
# failed 1 among 6 test(s)
1..6
make[2]: *** [t0026-eol-config.sh] Error 1

I think the reason is that your patch turns a failing test into a platform-specific (and therefore again failing) test. Remember: the Git code base is -- theoretically -- shared between all the platforms. It just does not make sense to maintain possibly-incompatible versions for the different platforms. The fact that Git for Windows has to maintain a couple of patches because they have not been accepted upstream in their current form is beside the point: the intention is actually to converge one day, when Git for Windows has enough active contributors that we can address all the nitpicks of upstream Git to get the patches into mainline.

Therefore we cannot change a failing test in such a way that it passes on Windows and fails everywhere else: most likely it means that the test is actually not precise enough. It probably assumes a certain native line ending convention, in which case it has to be either marked as such so it can be skipped if the native line ending is different than assumed or it has to be fixed not to assume a certain native line ending.

Also: you can force-push into the crlf branch. This will update this pull request, there is no need to make a new one.

Please let this not discourage you from working on this issue! I am positively surprised by your contribution. And please do not let the silence from my side from Monday through the beginning of next month discourage you, either, I will be offline, hoping all the while that other developers will step in and bring this pull request to a beautiful resolution.

Thanks!

@dscho
MSysGit - the development behind Git for Windows member

On Thu, 15 Aug 2013, Brice Lambson wrote:

Also, should these tests be handle different native line endings? If so, what is the best way to make them conditional?

There are some conditions you can specify, just git grep for NATIVE_EOL_IS_CRLF. This will show you how to define such conditions, and how to make them a pre-requisite for running a specific test (and skipping said test if the pre-requisite is not met).

@bricelam

@dscho I started to suspect today that it might be building on Linux today, thanks for confirming. I'm also delighted to hear that the goal is to converge into a single git codebase. I will definitely change the tests to be more platform agnostic then. :) Thank you for all your help and patience. Hopefully I can get another iteration out soon.

@patthoyts
MSysGit - the development behind Git for Windows member

This seems good to me - building this branch on my machine and running the tests has no failures other than t9903-bash-prompt 12-13 which fail anyway on the msys build. The only problem I see is that the commit message should explain as clearly as possibly why the change to append_cr is used and explain the change to 'Detect CRLF/LF conflict after setting text=auto'. Ultimately we would expect to forward these patches upstream and we need to conform to git's usual commit standards and be verbose in explaining the thinking behind changes. Particularly as crlf/lf issues are generally a bit tricky.

@bricelam

Thanks, Pat. I will update the tests to pass on both Linux and Windows, update my commit message per Git's guidelines, and update the pull request.

@bricelam bricelam MinGW: Update tests to handle a native eol of crlf
Some of the tests were written with the assumption that the native eol would always be lf. After defining NATIVE_CRLF on MinGW, these tests began failing. This change will update the tests to also handle a native eol of crlf.

Signed-off-by: Brice Lambson <bricelam@live.com>
635d6a0
@patthoyts
MSysGit - the development behind Git for Windows member

Merged by rebasing onto the 1.8.4 master.

@patthoyts patthoyts closed this Sep 13, 2013
@patthoyts patthoyts referenced this pull request Sep 13, 2013
Closed

Pt/native crlf #77

@maoueh

Tested with latest release, (msys) git 1.8.4 released few hours ago and it works like a charm. I think issue #57 can be close now that this PR has been merged into master.

Thanks @bricelam for the work on this issue.

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