git diff returns fatal error with core.safecrlf is set to true.

git diff returns fatal error with core.safecrlf is set to true.

[Yann Droneaud]

Hi,

Following my previous email "Tracking vendor release with Git" [1][2],
and the advice from Git users/developers, I'm trying to use 
.gitattributes
to handle CRLF/LF conversion.

While testing the behavor of Git regarding CRLF handling,
when core.safecrlf is set to true, I've found that "git diff" is 
returning
"fatal: CRLF would be replaced by LF" without returning any kind of 
diff.

This make me wonder if its the correct behavor for git diff to (only) 
fail:
It should be fatal for git add / git commit ( / git cherry-pick / ... 
?),
but non fatal for git diff.

According to the documentation git-config(5) [3]:
"Git will verify if a command modifies a file in the work tree either 
directly or indirectly"
I don't thing "git diff" is an operation that could modify a file.

Regards.

1. <1370970410-7935-1-git-send-email-ydroneaud@opteya.com>
2. <http://thread.gmane.org/gmane.comp.version-control.git/227466>
    <http://marc.info/?l=git&m=137097069115462&w=2>
3. https://www.kernel.org/pub/software/scm/git/docs/git-config.html

-- 
Yann Droneaud
OPTEYA

Re: git diff returns fatal error with core.safecrlf is set to true.

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> While testing the behavor of Git regarding CRLF handling,
> when core.safecrlf is set to true, I've found that "git diff" is
> returning
> "fatal: CRLF would be replaced by LF" without returning any kind of
> diff.
>
> This make me wonder if its the correct behavor for git diff to (only)
> fail:
> It should be fatal for git add / git commit ( / git cherry-pick /
> ... ?),
> but non fatal for git diff.

Yeah, I agree.

This is a diff between something and the working tree file, right?
It needs to convert from the working tree representation to the
canonical repository representation before doing the actual
comparison, and most likely the same helper function that is reused
for the check-in codepath, which needs to error out, is erroring out
after finding an input in your working tree that cannot safely
round-trip between LF/CRLF world.

The helper may want to learn a way to be told to demote that error
to a warning.

Re: git diff returns fatal error with core.safecrlf is set to true.

[Junio C Hamano]

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

> The helper may want to learn a way to be told to demote that error
> to a warning.

Perhaps something like this?

 diff.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index f0b3e7c..9b4f3ac 100644
--- a/diff.c
+++ b/diff.c
@@ -2677,6 +2677,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
 int diff_populate_filespec(struct diff_filespec *s, int size_only)
 {
 	int err = 0;
+	enum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL
+				    ? safe_crlf
+				    : SAFE_CRLF_WARN);
+
 	if (!DIFF_FILE_VALID(s))
 		die("internal error: asking to populate invalid file.");
 	if (S_ISDIR(s->mode))
@@ -2732,7 +2736,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		/*
 		 * Convert from working tree format to canonical git format
 		 */
-		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, crlf_warn)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;

Re: git diff returns fatal error with core.safecrlf is set to true.

[Yann Droneaud]

Hi,

Le 21.06.2013 23:57, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The helper may want to learn a way to be told to demote that error
>> to a warning.
>
> Perhaps something like this?
>

Thanks for the patch.

I run my test again, eg. run git diff after a rebase failure (see my 
other mail about core.safecrlf),
I'm able to run git diff a get a meaningful output:

# git version 1.8.1.4
fatal: CRLF would be replaced by LF in test.

# git version 1.8.3.1.741.g635527f.dirty (eg. next + your patch)
warning: CRLF will be replaced by LF in test.
The file will have its original line endings in your working directory.
diff --git a/test b/test
index b043836..63ba10f 100644
--- a/test
+++ b/test
@@ -1,4 +1,4 @@
-Hello World 1
-Hello World 2
-Hello World 3
+Hello World 1
+Hello World 2
+Hello World 3
  Hello World 4
\ No newline at end of file

It seems better. The removed lines have CRLF EOL while the added line 
have LF line ending characters.

Tested-By: Yann Droneaud <ydroneaud@opteya.com>


Regards.

-- 
Yann Droneaud
OPTEYA

Re: git diff returns fatal error with core.safecrlf is set to true.

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> Hi,
>
> Le 21.06.2013 23:57, Junio C Hamano a écrit :
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> The helper may want to learn a way to be told to demote that error
>>> to a warning.
>>
>> Perhaps something like this?
>>
>
> Thanks for the patch.

Care to turn it into an appliable patch with tests?

Thanks.

Re: git diff returns fatal error with core.safecrlf is set to true.

[Junio C Hamano]

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

> Care to turn it into an appliable patch with tests?

In the meantime, here is a quick-and-dirty one.  I am not proud of
it; it was just something to keep in 'pu' let it gets lost.

A better replacement is very much welcomed.

-- >8 --
Subject: [PATCH] diff: demote core.safecrlf=true to core.safecrlf=warn

Otherwise the user will not be able to start to guess where in the
contents in the working tree the offending unsafe CR lies.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c          | 6 +++++-
 t/t0020-crlf.sh | 8 ++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 32142db..155857c 100644
--- a/diff.c
+++ b/diff.c
@@ -2647,6 +2647,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
 int diff_populate_filespec(struct diff_filespec *s, int size_only)
 {
 	int err = 0;
+	enum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL
+				    ? safe_crlf
+				    : SAFE_CRLF_WARN);
+
 	if (!DIFF_FILE_VALID(s))
 		die("internal error: asking to populate invalid file.");
 	if (S_ISDIR(s->mode))
@@ -2702,7 +2706,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		/*
 		 * Convert from working tree format to canonical git format
 		 */
-		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, crlf_warn)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 1a8f44c..e526184 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -81,6 +81,14 @@ test_expect_success 'safecrlf: print warning only once' '
 	test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | wc -l) = 1
 '
 
+
+test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+	git diff HEAD
+'
+
+
 test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
 	git config core.autocrlf false &&
 	git config core.safecrlf false &&
-- 
1.8.3.1-771-gb7c3f25

Re: git diff returns fatal error with core.safecrlf is set to true.

[Torsten Bögershausen]

> +++ b/diff.c
> @@ -2647,6 +2647,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
>  int diff_populate_filespec(struct diff_filespec *s, int size_only)
>  {
>  	int err = 0;
> +	enum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL
> +				    ? safe_crlf
> +				    : SAFE_CRLF_WARN);

Thanks, 
Does it makes sense to write it the other way around?

enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL 
                           ? SAFE_CRLF_WARN 
                           : safe_crlf);

Re: git diff returns fatal error with core.safecrlf is set to true.

[Junio C Hamano]

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

>> +++ b/diff.c
>> @@ -2647,6 +2647,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
>>  int diff_populate_filespec(struct diff_filespec *s, int size_only)
>>  {
>>  	int err = 0;
>> +	enum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL
>> +				    ? safe_crlf
>> +				    : SAFE_CRLF_WARN);
>
> Thanks, 
> Does it makes sense to write it the other way around?
>
> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL 
>                            ? SAFE_CRLF_WARN 
>                            : safe_crlf);

I didn't see much difference either way, but between "FAIL needs to
be demoted to WARN, everything else goes as-is" and the original "We
do not care about anything other than FAIL, so use it as-is, but
demote FAIL to WARN", yours look shorter.  Will replace.