threads / discuss / 34244

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

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

## tl;dr

8 messages between Jun 21, 2013 and Jun 26, 2013.

replies: 7people: 3as markdown or json

Yann Droneaud· Jun 21, 2013, 13:26 UTC · lore
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
Junio C Hamano· Jun 21, 2013, 15:44 UTC · re: Yann Droneaud · lore

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

Yann Droneaud <ydroneaud@opteya.com> writes:
Show 11 quoted lines
> 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.

Junio C Hamano· Jun 21, 2013, 21:57 UTC · re: Junio C Hamano · lore

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

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;
Yann Droneaud· Jun 24, 2013, 12:42 UTC · re: Junio C Hamano · lore

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

Hi,
Le 21.06.2013 23:57, Junio C Hamano a écrit :
Show 7 quoted lines
> 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
Junio C Hamano· Jun 24, 2013, 18:19 UTC · re: Yann Droneaud · lore

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

Yann Droneaud <ydroneaud@opteya.com> writes:
Show 12 quoted lines
> 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.
Junio C Hamano· Jun 24, 2013, 21:48 UTC · re: Junio C Hamano · lore

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

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
Torsten Bögershausen· Jun 25, 2013, 20:29 UTC · re: Junio C Hamano · lore

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

Show 8 quoted lines
> +++ 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);
Junio C Hamano· Jun 26, 2013, 15:48 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
Show 15 quoted lines
>> +++ 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.

← back to recent threads