threads / patch / 39200

v4blame: CRLF in the working tree and LF in the repo

Subject: [PATCH v4] blame: CRLF in the working tree and LF in the repo

## tl;dr

4 messages between May 1, 2015 and May 3, 2015. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Torsten Bögershausen· May 1, 2015, 11:49 UTC · lore

A typical setup under Windows: core.eol is CRLF and a file is marked as "text" in .gitattributes, or core.autocrlf is true

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>
---
Changes since V3:
 more test cases
 Sorry for the spam
builtin/blame.c               |  1 +
 t/t8003-blame-corner-cases.sh | 48 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 43 insertions(+), 6 deletions(-)
Show changes to 2 files +43 −6

builtin/blame.c, t/t8003-blame-corner-cases.sh

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..8af36a6 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -191,13 +191,49 @@ test_expect_success 'indent of line numbers, ten lines' '
 	test $(grep -c "  " actual) = 9
 '
 
-test_expect_success 'blaming files with CRLF newlines' '
-	git config core.autocrlf false &&
+test_expect_success 'setup crlf files' '
 	printf "testcase\r\n" >crlffile &&
-	git add crlffile &&
-	git commit -m testcase &&
-	git -c core.autocrlf=input blame crlffile >actual &&
-	grep "A U Thor" actual
+	printf "testcase\n" >lffile &&
+	git -c core.autocrlf=false add lffile crlffile &&
+	git commit -m "add files" &&
+	git -c core.autocrlf=false blame HEAD -- crlffile >crlfclean.txt
+	printf "testcase\r\n" >crlffile &&
+	git -c core.autocrlf=false blame HEAD -- lffile >lfclean.txt
+	printf "testcase\r\n" >lffile
+	#Keep lffile with CRLF in worktree
+'
+
+test_expect_success 'blame file with CRLF in repo core.autocrlf=false' '
+	git -c core.autocrlf=false blame crlffile >crlf_repo_false &&
+	test_cmp crlfclean.txt crlf_repo_false
+'
+
+#has_cr_in_index() should suppress the normalization, see convert.c
+#but read_blob_data_from_cache() returns NULL
+test_expect_failure 'blame file with CRLF in repo core.autocrlf=true' '
+	git -c core.autocrlf=true blame crlffile >crlf_repo_true &&
+	test_cmp crlfclean.txt crlf_repo_true
+'
+
+test_expect_success 'blame file with CRLF in WS core.autocrlf=true' '
+	git -c core.autocrlf=true blame lffile >lf_repo_true &&
+	test_cmp lfclean.txt lf_repo_true
+'
+
+test_expect_success 'blame file with CRLF in WS core.autocrlf=input' '
+	git -c core.autocrlf=input blame lffile >lf_repo_input &&
+	test_cmp lfclean.txt lf_repo_input
+'
+
+test_expect_success 'blame file with CRLF in WS core.autocrlf=false' '
+	git -c core.autocrlf=false blame lffile >lf_repo_false &&
+	grep "Not Committed Yet" lf_repo_false
+'
+
+test_expect_success 'blame file with CRLF in WS core.autocrlf=false attributes' '
+	printf "lffile text\r\n" >.gitattributes &&
+	git -c core.autocrlf=false blame lffile >lf_repo_attr_text &&
+	test_cmp lfclean.txt lf_repo_attr_text
 '
 
 test_done
-- 
2.2.0.rc1.790.ge19fcd2
Junio C Hamano· May 1, 2015, 17:13 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
> A typical setup under Windows:
> core.eol is CRLF and a file is marked as "text" in .gitattributes,
> or core.autocrlf is true

A full sentence (a proper prose) is easier to read. The above is unclear if that is what you are unilaterally declaring, a statement of fact, or something else (see what I queued based on the previous round on 'pu').

Show 12 quoted lines
> 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.
Do these two (or three) patches separately:
 - One patch to revert it and do nothing else other than justify the
   revert in the log message; and
 - Another patch that demonstrates failures Kasal found so that we
   won't repeat the mistake.
 - Optionally, yet another patch that demonstrates the current
   breakage that exists with or without the reversion.

The former two may be combined into one, but the last one should be a separate patch, as it does not have anything to do with this reversion.

In other words, there shouldn't be a new test-expect-failure in a revert of an existing commit (unless that commit removed that test-expect-failure in the first place, claiming that it fixed something, that is). If you are documenting a failure that has not been tested in our test suite, do so in a separate patch.

Thanks.
Show 83 quoted lines
> Reported-By: Stepan Kasal <kasal@ucw.cz>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> Changes since V3:
>  more test cases
>  Sorry for the spam
>
> builtin/blame.c               |  1 +
>  t/t8003-blame-corner-cases.sh | 48 +++++++++++++++++++++++++++++++++++++------
>  2 files changed, 43 insertions(+), 6 deletions(-)
>
> 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..8af36a6 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -191,13 +191,49 @@ test_expect_success 'indent of line numbers, ten lines' '
>  	test $(grep -c "  " actual) = 9
>  '
>  
> -test_expect_success 'blaming files with CRLF newlines' '
> -	git config core.autocrlf false &&
> +test_expect_success 'setup crlf files' '
>  	printf "testcase\r\n" >crlffile &&
> -	git add crlffile &&
> -	git commit -m testcase &&
> -	git -c core.autocrlf=input blame crlffile >actual &&
> -	grep "A U Thor" actual
> +	printf "testcase\n" >lffile &&
> +	git -c core.autocrlf=false add lffile crlffile &&
> +	git commit -m "add files" &&
> +	git -c core.autocrlf=false blame HEAD -- crlffile >crlfclean.txt
> +	printf "testcase\r\n" >crlffile &&
> +	git -c core.autocrlf=false blame HEAD -- lffile >lfclean.txt
> +	printf "testcase\r\n" >lffile
> +	#Keep lffile with CRLF in worktree
> +'
> +
> +test_expect_success 'blame file with CRLF in repo core.autocrlf=false' '
> +	git -c core.autocrlf=false blame crlffile >crlf_repo_false &&
> +	test_cmp crlfclean.txt crlf_repo_false
> +'
> +
> +#has_cr_in_index() should suppress the normalization, see convert.c
> +#but read_blob_data_from_cache() returns NULL
> +test_expect_failure 'blame file with CRLF in repo core.autocrlf=true' '
> +	git -c core.autocrlf=true blame crlffile >crlf_repo_true &&
> +	test_cmp crlfclean.txt crlf_repo_true
> +'
> +
> +test_expect_success 'blame file with CRLF in WS core.autocrlf=true' '
> +	git -c core.autocrlf=true blame lffile >lf_repo_true &&
> +	test_cmp lfclean.txt lf_repo_true
> +'
> +
> +test_expect_success 'blame file with CRLF in WS core.autocrlf=input' '
> +	git -c core.autocrlf=input blame lffile >lf_repo_input &&
> +	test_cmp lfclean.txt lf_repo_input
> +'
> +
> +test_expect_success 'blame file with CRLF in WS core.autocrlf=false' '
> +	git -c core.autocrlf=false blame lffile >lf_repo_false &&
> +	grep "Not Committed Yet" lf_repo_false
> +'
> +
> +test_expect_success 'blame file with CRLF in WS core.autocrlf=false attributes' '
> +	printf "lffile text\r\n" >.gitattributes &&
> +	git -c core.autocrlf=false blame lffile >lf_repo_attr_text &&
> +	test_cmp lfclean.txt lf_repo_attr_text
>  '
>  
>  test_done
Torsten Bögershausen· May 1, 2015, 18:57 UTC · re: Junio C Hamano · lore

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

On 2015-05-01 19.13, Junio C Hamano wrote:
Show 10 quoted lines
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> A typical setup under Windows:
>> core.eol is CRLF and a file is marked as "text" in .gitattributes,
>> or core.autocrlf is true
> 
> A full sentence (a proper prose) is easier to read.  The above is
> unclear if that is what you are unilaterally declaring, a statement
> of fact, or something else (see what I queued based on the previous
> round on 'pu').

OK, let's go for that, thanks for amending. And is there a chance to correct my mis-spelling ?

s/A typicall/A typical/ 
Junio C Hamano· May 3, 2015, 02:02 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
> And is there a chance to correct my mis-spelling ?
>
> s/A typicall/A typical/ 

Well, I didn't intend to keep an amended old version, especially when you are actively working on polishing it to submit an updated series. Please make sure you have that correction in the rerolled version, which separates revert and other things.

← back to recent threads