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

From: Junio C Hamano <[email-masked]>
Date: Fri, 01 May 2015 10:13:56 -0700

Torsten Bögershausen <[email-masked]> 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').

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

> Reported-By: Stepan Kasal <[email-masked]>
> Signed-off-by: Torsten Bögershausen <[email-masked]>
> ---
> 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