[PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Torsten Bögershausen]

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.

Reported-By: Stepan Kasal <kasal@ucw.cz>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
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(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 06484c2..8d70623 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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);
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 32895e5..dcc9827 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -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
-- 
2.2.0.rc1.790.ge19fcd2

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Eric Sunshine]

On Sun, Apr 26, 2015 at 8:02 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> A typicall setup under Windows:

s/typicall/typical/

> 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.
>
> Reported-By: Stepan Kasal <kasal@ucw.cz>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Stepan Kasal]

Hello,

thank you Torsten for the patch [I'm the reporter, but could not do
it myself]

> -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

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> 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.

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.

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Stepan Kasal]

Hello,

On Sun, Apr 26, 2015 at 10:31:11PM -0700, Junio C Hamano wrote:
> [...] 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

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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 <dr.khong@gmail.com>
    Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

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.

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Johannes Sixt]

Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
> 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

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Torsten Bögershausen]

On 04/27/2015 07:47 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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 <dr.khong@gmail.com>
>     Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> 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.
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
    )
'

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Torsten Bögershausen]

On 04/27/2015 08:58 PM, Johannes Sixt wrote:
> Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
>> 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)
 

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]

On Mon, Apr 27, 2015 at 10:47:29AM -0700, Junio C Hamano wrote:
> 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 <dr.khong@gmail.com>
>     Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> 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.

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.
-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> 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

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?

>     )
> '
>
>

Two blank lines only here?

> 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

> 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)?

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Torsten Bögershausen]

On 28/04/15 09:28, Junio C Hamano wrote:
> Torsten Bögershausen<tboegi@web.de>  writes:
>
>> 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
> 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?
>
>>      )
>> '
>>
>>
> Two blank lines only here?
>
>> 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.





>
>> 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)?
>
Jepp,

There is room for improvements.

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Johannes Sixt]

Am 27.04.2015 um 21:45 schrieb Torsten Bögershausen:
> On 04/27/2015 08:58 PM, Johannes Sixt wrote:
>> Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
>>> 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)

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

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> 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.

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Johannes Sixt]

Am 28.04.2015 um 21:52 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> 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

Re: [PATCH/RFC] blame: CRLF in the working tree and LF in the repo

[Stepan Kasal]

Hello Hannes,

let me correct my previous statement:

On Mon, Apr 27, 2015 at 08:58:05PM +0200, Johannes Sixt wrote:
> 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.

Johannes Sixt <j6t@kdbg.org> writes:
> 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:
> Am 27.04.2015 um 08:11 schrieb Stepan Kasal:
>> 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.

>> 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