git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* git diff returns fatal error with core.safecrlf is set to true.
@ 2013-06-21 13:26 Yann Droneaud
  2013-06-21 15:44 ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Yann Droneaud @ 2013-06-21 13:26 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-21 13:26 git diff returns fatal error with core.safecrlf is set to true Yann Droneaud
@ 2013-06-21 15:44 ` Junio C Hamano
  2013-06-21 21:57   ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-06-21 15:44 UTC (permalink / raw)
  To: Yann Droneaud; +Cc: git

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-21 15:44 ` Junio C Hamano
@ 2013-06-21 21:57   ` Junio C Hamano
  2013-06-24 12:42     ` Yann Droneaud
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-06-21 21:57 UTC (permalink / raw)
  To: Yann Droneaud; +Cc: git

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;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-21 21:57   ` Junio C Hamano
@ 2013-06-24 12:42     ` Yann Droneaud
  2013-06-24 18:19       ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Yann Droneaud @ 2013-06-24 12:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-24 12:42     ` Yann Droneaud
@ 2013-06-24 18:19       ` Junio C Hamano
  2013-06-24 21:48         ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-06-24 18:19 UTC (permalink / raw)
  To: Yann Droneaud; +Cc: git

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-24 18:19       ` Junio C Hamano
@ 2013-06-24 21:48         ` Junio C Hamano
  2013-06-25 20:29           ` Torsten Bögershausen
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-06-24 21:48 UTC (permalink / raw)
  To: Yann Droneaud; +Cc: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-24 21:48         ` Junio C Hamano
@ 2013-06-25 20:29           ` Torsten Bögershausen
  2013-06-26 15:48             ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Torsten Bögershausen @ 2013-06-25 20:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Yann Droneaud, git

> +++ 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);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: git diff returns fatal error with core.safecrlf is set to true.
  2013-06-25 20:29           ` Torsten Bögershausen
@ 2013-06-26 15:48             ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2013-06-26 15:48 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: Yann Droneaud, git

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-06-26 15:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-21 13:26 git diff returns fatal error with core.safecrlf is set to true Yann Droneaud
2013-06-21 15:44 ` Junio C Hamano
2013-06-21 21:57   ` Junio C Hamano
2013-06-24 12:42     ` Yann Droneaud
2013-06-24 18:19       ` Junio C Hamano
2013-06-24 21:48         ` Junio C Hamano
2013-06-25 20:29           ` Torsten Bögershausen
2013-06-26 15:48             ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).