* 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 related [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 related [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 related [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 public 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).