blame: CRLF in the working tree and LF in the repo
A typicall setup under Windows:
core.eol is CRLF and a file is marked as "text" in .gitattributes.
After 4d4813a5 "git blame" no longer works as expected, every line is annotated as "Not Committed Yet", even though the working directory is clean.
commit 4d4813a5 removed the conversion in blame.c for all files, with or without CRLF in the repo.
Having files with CRLF in the repo and core.autocrlf=input is a temporary situation, the files should be normalized in the repo. Blaming them with "Not Committed Yet" is OK.
The solution is to revert commit 4d4813a5.
---
Reference:
https://github.com/git-for-windows/git/issues/105
Although the intention of 4d4813a5 is good, it breaks the usual EOL-handling for Windows.
Until we have a better solution, we suggest to revert it.
builtin/blame.c | 1 + t/t8003-blame-corner-cases.sh | 26 +++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-)
builtin/blame.c +1 -0
@@ -2348,6 +2348,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,if (strbuf_read (&buf,0 ,0 ) <0 )die_errno ("failed to read from stdin" ); } +convert_to_git (path, buf.buf, buf.len, &buf,0 ); origin->file.ptr = buf.buf; origin->file.size = buf.len;pretend_sha1_file (buf.buf, buf.len,OBJ_BLOB , origin->blob_sha1);
t/t8003-blame-corner-cases.sh +25 -1
@@ -191,7 +191,7 @@ test_expect_success 'indent of line numbers, ten lines' ' test $(grep -c " " actual) = 9 ' -test_expect_success 'blaming files with CRLF newlines' ' +test_expect_failure 'blaming files with CRLF newlines in repo, core.autoclrf=input' ' git config core.autocrlf false && printf "testcase\r\n" >crlffile && git add crlffile &&
@@ -199,5 +199,29 @@ test_expect_success 'blaming files with CRLF newlines' ' git -c core.autocrlf=input blame crlffile >actual && grep "A U Thor" actual ' +test_expect_success 'blaming files with CRLF newlines core.autocrlf=true' ' + test_create_repo blamerepo && + ( + cd blamerepo && + git config core.autocrlf true && + printf "testcase\r\n" >crlffile && + git add crlffile && + git commit -m TRUE && + git blame crlffile >actual && + grep "A U Thor" actual + ) +' + +test_expect_success 'blaming files with CRLF newlines core.autocrlf=false' ' + ( + cd blamerepo && + git config core.autocrlf false && + printf ".gitattributes text\r\n" >.gitattributes && + git add .gitattributes && + git commit -m FALSE && + git blame .gitattributes >actual && + grep "A U Thor" actual + ) +' test_done
Signature · 1 line
2.2.0.rc1.790.ge19fcd2
Hello,
thank you Torsten for the patch [I'm the reporter, but could not do it myself]
t/t8003-blame-corner-cases.sh test_expect_success 'indent of line numbers, ten lines' ' · line 194
Quoted 2 lines
t/t8003-blame-corner-cases.sh +1
-1-test_expect_success 'blaming files with CRLF newlines' ' +test_expect_failure 'blaming files with CRLF newlines in repo, core.autoclrf=input' '
Shouldn't the old test be rather removed?
It deals with an invalid situation.
I thought that having crlf in the repo is incorrect, so no wonder that it fails if the files in the working tree are changed to LF.
And changing the autocrlf transformation is effectively the same, no matter that the files _physically_ are the same as the files in the repo.
Have a nice day,
Stepan Kasal
Quoted 1 line · Torsten Bögershausen
A typicall setup under Windows:
s/typicall/typical/
Quoted 17 lines
core.eol is CRLF and a file is marked as "text" in .gitattributes.
After
4d4813a5"git blame" no longer works as expected, every line is annotated as "Not Committed Yet", even though the working directory is clean.commit
4d4813a5removed the conversion inblame.cfor all files, with or without CRLF in the repo.Having files with CRLF in the repo and core.autocrlf=input is a temporary situation, the files should be normalized in the repo. Blaming them with "Not Committed Yet" is OK.
The solution is to revert commit
4d4813a5.Reported-By Stepan Kasal <[email protected]>Signed-off-by Torsten Bögershausen <[email protected]>
Quoted 3 lines · Torsten Bögershausen
Although the intention of
4d4813a5is good, it breaks the usual EOL-handling for Windows.
Until we have a better solution, we suggest to revert it.
That makes it sound like you are proposing to rob Peter to pay Paul, but that is not how we do things around here. If both the case 4d4813a5 tried to solve and the issue reported by Stepan need to be satisfied, the current code will stay as-is until you can find a good solution to make both happy.
Having said that.
I suspect (I haven't looked very carefully for this round yet to be sure, though) that it may turn out that the commit you are proposing to revert was a misguided attempt to "fix" a non issue, or to break the behaviour to match a mistaken expectation. If that is the case then definitely the reversion is a good idea, and you should argue along that line of justification.
We'd just be fixing an old misguided and bad change in such a case.
Thanks.
Hello,
Quoted 2 lines · Junio C Hamano
[...] the commit you are proposing to revert [
4d4813a5] was a misguided attempt to "fix" a non issue, [...]
yes, it was this. So I propose to remove the whole commit, including the test case and add two new test cases.
Details:
Git does not support CRLF as the internal line separator. If you commit file in binary mode with CRLF, you are on your own.
If you then recode the file in the working tree to use LF, no wonder things break.
If you do it indirectly, by setting the file mode to "text", things break exactly the same way.
And that is the case that 4d4813a5 wanted to fix, cf the test case in it.
OTOH, the commit has broken the most recommended scenario for Windows:
LF in the repo, CRLF in the work tree.
Thanks,
Stepan
Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
Quoted 2 lines
Git does not support CRLF as the internal line separator. If you commit file in binary mode with CRLF, you are on your own.
When I commit my C source code files with CRLF into the repository (because I do not set any line ending options or configurations or any 'text' attributes or similar), do I then commit binary files or text files? Should I expect not to see any diffs?
-- Hannes
Quoted 8 lines · Johannes Sixt
Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
Quoted 2 lines
Git does not support CRLF as the internal line separator. If you commit file in binary mode with CRLF, you are on your own.
When I commit my C source code files with CRLF into the repository (because I do not set any line ending options or configurations or any 'text' attributes or similar), do I then commit binary files or text files? Should I expect not to see any diffs?
-- Hannes
You commit files with CRLF in the repo.
If you have CRLF in the working tree, things are as follows:
core.autocrlf=false : "Same as binary, no changes"
core.autocrlf=true : "Normalization is suppressed, (CRLF in repo), and therefore no changes.
core.autocrlf=input : "Normalization wanted, (CRLF in repo), normalization will be done
(and should be committed as soon as possible)
Am 27.04.2015 um 21:45 schrieb Torsten Bögershausen:
Quoted 12 lines · Johannes Sixt
Quoted 11 lines
Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
Quoted 2 lines
Git does not support CRLF as the internal line separator. If you commit file in binary mode with CRLF, you are on your own.
When I commit my C source code files with CRLF into the repository (because I do not set any line ending options or configurations or any 'text' attributes or similar), do I then commit binary files or text files? Should I expect not to see any diffs?
-- Hannes
Quoted 7 lines
You commit files with CRLF in the repo.
If you have CRLF in the working tree, things are as follows:core.autocrlf=false : "Same as binary, no changes"
core.autocrlf=true : "Normalization is suppressed, (CRLF in repo), and therefore no changes.
core.autocrlf=input : "Normalization wanted, (CRLF in repo), normalization will be done
(and should be committed as soon as possible)
I set none of these. But I do commit CRLF and expect to get CRLF back. Am I commiting binary files? Am I doing something that "Git does not support"? Am I "on [my] own"?
-- Hannes
Quoted 3 lines · Johannes Sixt
I set none of these. But I do commit CRLF and expect to get CRLF back. Am I commiting binary files? Am I doing something that "Git does not support"? Am I "on [my] own"?
I think these specific sentences are merely uninformed opinions; if I ignore and re-read what people said in the discussion, I think the thread as a whole makes sense.
Am 28.04.2015 um 21:52 schrieb Junio C Hamano:
Quoted 9 lines
Quoted 3 lines · Johannes Sixt
I set none of these. But I do commit CRLF and expect to get CRLF back. Am I commiting binary files? Am I doing something that "Git does not support"? Am I "on [my] own"?
I think these specific sentences are merely uninformed opinions; if I ignore and re-read what people said in the discussion, I think the thread as a whole makes sense.
Thanks for the clarification. Following the thread only superficially, I feared some behavior change (or even just a redefinition of what "is supported") is about to surface that impacts established workflows.
-- Hannes
Hello Hannes,
let me correct my previous statement:
Quoted 4 lines · Johannes Sixt
When I commit my C source code files with CRLF into the repository (because I do not set any line ending options or configurations or any 'text' attributes or similar), do I then commit binary files or text files? Should I expect not to see any diffs?
Of course, you can see diffs. The files are not binary in that sense.
Quoted 2 lines · Johannes Sixt
I set none of these. But I do commit CRLF and expect to get CRLF back. [...]
That works. You will not encounter any problem. (Supposing you do not change the line ending options, of course.)
Finally, let me explain my previous statement:
Quoted 2 lines
Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
Quoted 1 line
Git does not support CRLF as the internal line separator.
I'm often asked: "How do I set up git so that it uses CRLF in text files in the repository and checks them out with CRLF on Windows and with LF on unixy systems?"
My answer to that question always was that you cannot configure the internal line separator in git repo, it is always LF. Your only chance to support both line endings is to have LF in the repo and configure the Windows client to do the conversion.
Quoted 1 line
Quoted 1 line
If you commit file in binary mode with CRLF, you are on your own.
OK, scratch the word "binary". The files in the repo are actually text files. But each text line is contains one more char than you would think. From time to time, this lurks:
1) Does "git grep ';$' HEAD" find anything?
2) What about "git grep ';.$' HEAD" ?
Or "git grep `printf ';\r$' HEAD" ?
3) If you try things like
git diff HEAD^^..HEAD^ >outfile.diff
and then open outfile.diff with a suitable editor (e.g. vim), you can see an extra ^M at the end of some lines (the content ones).
This is why I tell users that they are on their own if they decide to use the setup you described.
Have a nice day,
Stepan
Quoted 8 lines · Junio C Hamano
I suspect (I haven't looked very carefully for this round yet to be sure, though) that it may turn out that the commit you are proposing to revert was a misguided attempt to "fix" a non issue, or to break the behaviour to match a mistaken expectation. If that is the case then definitely the reversion is a good idea, and you should argue along that line of justification.
We'd just be fixing an old misguided and bad change in such a case.
The original says this:
blame: correctly handle files regardless of autocrlf
If a file contained CRLF line endings in a repository with core.autocrlf=input, then blame always marked lines as "Not Committed Yet", even if they were unmodified. Don't attempt to convert the line endings when creating the fake commit so that blame works correctly regardless of the autocrlf setting.
But if autocrlf=input, then the end-user expectation is to keep the in-repository data with LF line endings. If your tip-of-the-tree commit incorrectly has CRLF line endings, and if you were going to commit what is in the working tree on top, you would be correcting that mistake by turning the in-repository data into a text file with LF line endings, so "Not Committed Yet" _is_ the correct behaviour.
So I think that the reverting that change is the right thing to do. It really was a change to break the behaviour to match a mistaken expectation, I would have to say.
Quoted 21 lines · Junio C Hamano
The original says this:
blame: correctly handle files regardless of autocrlf
If a file contained CRLF line endings in a repository with core.autocrlf=input, then blame always marked lines as "Not Committed Yet", even if they were unmodified. Don't attempt to convert the line endings when creating the fake commit so that blame works correctly regardless of the autocrlf setting.
Reported-by Ephrim Khong <[email protected]>Signed-off-by brian m. carlson <[email protected]>Signed-off-by Junio C Hamano <[email protected]>But if autocrlf=input, then the end-user expectation is to keep the in-repository data with LF line endings. If your tip-of-the-tree commit incorrectly has CRLF line endings, and if you were going to commit what is in the working tree on top, you would be correcting that mistake by turning the in-repository data into a text file with LF line endings, so "Not Committed Yet" _is_ the correct behaviour.
Quoted 3 lines
So I think that the reverting that change is the right thing to do. It really was a change to break the behaviour to match a mistaken expectation, I would have to say.
I don't have a strong opinion on whether or not this should be reverted, since I don't use Windows and therefore don't use CRLF or the respective options anywhere, nor am I very familiar with how they are supposed to function. Junio has articulated a good rationale for why it's broken, and I'm willing to go along with that.
I will say that perhaps it's worthwhile to write some documentation to explain how the CRLF translation works, as it seems that there's a lot of misunderstanding about it. I am, for the aforementioned reasons, not a good choice to write it.
Signature · 3 lines
brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
Quoted 31 lines · Junio C Hamano
Quoted 8 lines · Junio C Hamano
I suspect (I haven't looked very carefully for this round yet to be sure, though) that it may turn out that the commit you are proposing to revert was a misguided attempt to "fix" a non issue, or to break the behaviour to match a mistaken expectation. If that is the case then definitely the reversion is a good idea, and you should argue along that line of justification.
We'd just be fixing an old misguided and bad change in such a case.
The original says this:
blame: correctly handle files regardless of autocrlf
If a file contained CRLF line endings in a repository with core.autocrlf=input, then blame always marked lines as "Not Committed Yet", even if they were unmodified. Don't attempt to convert the line endings when creating the fake commit so that blame works correctly regardless of the autocrlf setting.
Reported-by Ephrim Khong <[email protected]>Signed-off-by brian m. carlson <[email protected]>Signed-off-by Junio C Hamano <[email protected]>But if autocrlf=input, then the end-user expectation is to keep the in-repository data with LF line endings. If your tip-of-the-tree commit incorrectly has CRLF line endings, and if you were going to commit what is in the working tree on top, you would be correcting that mistake by turning the in-repository data into a text file with LF line endings, so "Not Committed Yet" _is_ the correct behaviour.
Quoted 3 lines
So I think that the reverting that change is the right thing to do. It really was a change to break the behaviour to match a mistaken expectation, I would have to say.
Besides a better commit message (suggestions welcome), What do you think about the following test cases for a V2 patch ?
test_expect_success 'create blamerepo' '
test_create_repo blamerepo &&
(
cd blamerepo &&
printf "testcase\r\n" >crlffile &&
git -c core.autocrlf=false add crlffile &&
git commit -m "add files" &&
git -c core.autocrlf=false blame crlffile >crlfclean.txt
)'
test_expect_success 'blaming files with CRLF newlines in repo, core.autoclrf=input' '
(
cd blamerepo &&
git -c core.autocrlf=input blame crlffile >actual &&
grep "Not Committed Yet" actual
)'
test_expect_success 'blaming files with CRLF newlines core.autocrlf=true' '
(
cd blamerepo &&
git -c core.autocrlf=true blame crlffile >actual &&
test_cmp crlfclean.txt actual
)'
test_expect_success 'blaming files with CRLF newlines core.autocrlf=false' '
(
cd blamerepo &&
git -c core.autocrlf=false blame crlffile >actual &&
test_cmp crlfclean.txt actual
)'
Quoted 11 lines · Torsten Bögershausen
What do you think about the following test cases for a V2 patch ?
test_expect_success'create blamerepo' 'test_create_repo blamerepo && ( cd blamerepo && printf "testcase\r\n" >crlffile && git -c core.autocrlf=false add crlffile && git commit -m "add files" && git -c core.autocrlf=false blame crlffile >crlfclean.txt )
Quoted 7 lines
'
test_expect_success'blaming files with CRLF newlines in repo, core.autoclrf=input' '( cd blamerepo && git -c core.autocrlf=input blame crlffile >actual && grep "Not Committed Yet" actual
Are you interested in seeing just some of the lines to show up as "Not commited yet", or all of them? I think it would be the latter, so perhaps
! grep -v "Not Committed Yet" actual
or something?
Quoted 4 lines
)'
Two blank lines only here?
Quoted 7 lines
test_expect_success'blaming files with CRLF newlines core.autocrlf=true' '( cd blamerepo && git -c core.autocrlf=true blame crlffile >actual && test_cmp crlfclean.txt actual )'
OK
Quoted 7 lines
test_expect_success'blaming files with CRLF newlines core.autocrlf=false' '( cd blamerepo && git -c core.autocrlf=false blame crlffile >actual && test_cmp crlfclean.txt actual )'
Hmm, how's this blame invocation any different from the one done in the set-up step at the very beginning? In other words, I am not sure what kind of breakage could cause this step to fail.
I see there is no "git blame HEAD crlffile" that bypasses the fake latest commit altogether. Wouldn't that be the most appropriate thing to compare against (i.e. how to create crlfclean.txt in the set-up step)?
Quoted 34 lines · Junio C Hamano
Quoted 11 lines · Torsten Bögershausen
What do you think about the following test cases for a V2 patch ?
test_expect_success'create blamerepo' 'test_create_repo blamerepo && ( cd blamerepo && printf "testcase\r\n" >crlffile && git -c core.autocrlf=false add crlffile && git commit -m "add files" && git -c core.autocrlf=false blame crlffile >crlfclean.txt )Quoted 7 lines
'
test_expect_success'blaming files with CRLF newlines in repo, core.autoclrf=input' '( cd blamerepo && git -c core.autocrlf=input blame crlffile >actual && grep "Not Committed Yet" actualAre you interested in seeing just some of the lines to show up as "Not commited yet", or all of them? I think it would be the latter, so perhaps
! grep -v "Not Committed Yet" actual
or something?
Quoted 4 lines
)'
Two blank lines only here?
Quoted 8 lines
Quoted 7 lines
test_expect_success'blaming files with CRLF newlines core.autocrlf=true' '( cd blamerepo && git -c core.autocrlf=true blame crlffile >actual && test_cmp crlfclean.txt actual )'
OK
Interestingly this test doesn't pass on one of my systems, after having stripped t8003 to contain to only have the corner cases. When core.autocrlf is true, the converting should be suppressed:
convert.c/has_cr_in_index() should return 1, but doesn't.
data = read_blob_data_from_cache(path, &sz);
and data is NULL.
Some more digging has to be done here.
On the other hand we want to test blame on a file with LF in the repo and CRLF in the workspace as well.
So all in all I need to send a V2.
Quoted 8 lines
Quoted 8 lines
test_expect_success'blaming files with CRLF newlines core.autocrlf=false' '( cd blamerepo && git -c core.autocrlf=false blame crlffile >actual && test_cmp crlfclean.txt actual )'
Quoted 9 lines
Hmm, how's this blame invocation any different from the one done in the set-up step at the very beginning? In other words, I am not sure what kind of breakage could cause this step to fail.
I see there is no "git blame HEAD crlffile" that bypasses the fake latest commit altogether. Wouldn't that be the most appropriate thing to compare against (i.e. how to create crlfclean.txt in the set-up step)?
Jepp,
There is room for improvements.