{"thread":{"id":"34244","subject":"git diff returns fatal error with core.safecrlf is set to true.","startedAt":"2013-06-21T13:26:51Z","lastAt":"2013-06-26T15:48:53Z","messageCount":8,"participants":["Yann Droneaud","Junio C Hamano","Torsten Bögershausen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"221593","messageId":"6a3d8a2b19a859d8c969ee434e1d6a89@meuh.org","threadId":"34244","inReplyTo":null,"subject":"git diff returns fatal error with core.safecrlf is set to true.","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2013-06-21T13:26:51Z","receivedAt":"2013-06-21T13:26:51Z","isPatch":false,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"Hi,\n\nFollowing my previous email \"Tracking vendor release with Git\" [1][2],\nand the advice from Git users/developers, I'm trying to use \n.gitattributes\nto handle CRLF/LF conversion.\n\nWhile testing the behavor of Git regarding CRLF handling,\nwhen core.safecrlf is set to true, I've found that \"git diff\" is \nreturning\n\"fatal: CRLF would be replaced by LF\" without returning any kind of \ndiff.\n\nThis make me wonder if its the correct behavor for git diff to (only) \nfail:\nIt should be fatal for git add / git commit ( / git cherry-pick / ... \n?),\nbut non fatal for git diff.\n\nAccording to the documentation git-config(5) [3]:\n\"Git will verify if a command modifies a file in the work tree either \ndirectly or indirectly\"\nI don't thing \"git diff\" is an operation that could modify a file.\n\nRegards.\n\n1. <1370970410-7935-1-git-send-email-ydroneaud@opteya.com>\n2. \n \n3. https://www.kernel.org/pub/software/scm/git/docs/git-config.html\n\n-- \nYann Droneaud\nOPTEYA\n"},{"id":"221601","messageId":"7vobazo4ds.fsf@alter.siamese.dyndns.org","threadId":"34244","inReplyTo":"6a3d8a2b19a859d8c969ee434e1d6a89@meuh.org","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T15:44:15Z","receivedAt":"2013-06-21T15:44:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Droneaud writes:\n\n> While testing the behavor of Git regarding CRLF handling,\n> when core.safecrlf is set to true, I've found that \"git diff\" is\n> returning\n> \"fatal: CRLF would be replaced by LF\" without returning any kind of\n> diff.\n>\n> This make me wonder if its the correct behavor for git diff to (only)\n> fail:\n> It should be fatal for git add / git commit ( / git cherry-pick /\n> ... ?),\n> but non fatal for git diff.\n\nYeah, I agree.\n\nThis is a diff between something and the working tree file, right?\nIt needs to convert from the working tree representation to the\ncanonical repository representation before doing the actual\ncomparison, and most likely the same helper function that is reused\nfor the check-in codepath, which needs to error out, is erroring out\nafter finding an input in your working tree that cannot safely\nround-trip between LF/CRLF world.\n\nThe helper may want to learn a way to be told to demote that error\nto a warning.\n"},{"id":"221640","messageId":"7vip17ktyz.fsf@alter.siamese.dyndns.org","threadId":"34244","inReplyTo":"7vobazo4ds.fsf@alter.siamese.dyndns.org","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T21:57:24Z","receivedAt":"2013-06-21T21:57:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The helper may want to learn a way to be told to demote that error\n> to a warning.\n\nPerhaps something like this?\n\n diff.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex f0b3e7c..9b4f3ac 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2677,6 +2677,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n int diff_populate_filespec(struct diff_filespec *s, int size_only)\n {\n \tint err = 0;\n+\tenum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL\n+\t\t\t\t ? safe_crlf\n+\t\t\t\t : SAFE_CRLF_WARN);\n+\n \tif (!DIFF_FILE_VALID(s))\n \t\tdie(\"internal error: asking to populate invalid file.\");\n \tif (S_ISDIR(s->mode))\n@@ -2732,7 +2736,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t/*\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, crlf_warn)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\n"},{"id":"221787","messageId":"b8e932cba326588db09ebd0986913ac2@meuh.org","threadId":"34244","inReplyTo":"7vip17ktyz.fsf@alter.siamese.dyndns.org","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Yann Droneaud","fromEmail":"ydroneaud@opteya.com","sentAt":"2013-06-24T12:42:12Z","receivedAt":"2013-06-24T12:42:12Z","isPatch":false,"sender":{"key":"ydroneaud@opteya.com","avatar":"https://avatars.githubusercontent.com/u/881377?v=4"},"body":"Hi,\n\nLe 21.06.2013 23:57, Junio C Hamano a écrit :\n> Junio C Hamano writes:\n>\n>> The helper may want to learn a way to be told to demote that error\n>> to a warning.\n>\n> Perhaps something like this?\n>\n\nThanks for the patch.\n\nI run my test again, eg. run git diff after a rebase failure (see my \nother mail about core.safecrlf),\nI'm able to run git diff a get a meaningful output:\n\n# git version 1.8.1.4\nfatal: CRLF would be replaced by LF in test.\n\n# git version 1.8.3.1.741.g635527f.dirty (eg. next + your patch)\nwarning: CRLF will be replaced by LF in test.\nThe file will have its original line endings in your working directory.\ndiff --git a/test b/test\nindex b043836..63ba10f 100644\n--- a/test\n+++ b/test\n@@ -1,4 +1,4 @@\n-Hello World 1\n-Hello World 2\n-Hello World 3\n+Hello World 1\n+Hello World 2\n+Hello World 3\n Hello World 4\n\\ No newline at end of file\n\nIt seems better. The removed lines have CRLF EOL while the added line \nhave LF line ending characters.\n\nTested-By: Yann Droneaud \n\n\nRegards.\n\n-- \nYann Droneaud\nOPTEYA\n"},{"id":"221869","messageId":"7vbo6v9xrr.fsf@alter.siamese.dyndns.org","threadId":"34244","inReplyTo":"b8e932cba326588db09ebd0986913ac2@meuh.org","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T18:19:52Z","receivedAt":"2013-06-24T18:19:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Droneaud writes:\n\n> Hi,\n>\n> Le 21.06.2013 23:57, Junio C Hamano a écrit :\n>> Junio C Hamano writes:\n>>\n>>> The helper may want to learn a way to be told to demote that error\n>>> to a warning.\n>>\n>> Perhaps something like this?\n>>\n>\n> Thanks for the patch.\n\nCare to turn it into an appliable patch with tests?\n\nThanks.\n"},{"id":"221890","messageId":"7vli5z6uyq.fsf@alter.siamese.dyndns.org","threadId":"34244","inReplyTo":"7vbo6v9xrr.fsf@alter.siamese.dyndns.org","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T21:48:45Z","receivedAt":"2013-06-24T21:48:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Care to turn it into an appliable patch with tests?\n\nIn the meantime, here is a quick-and-dirty one. I am not proud of\nit; it was just something to keep in 'pu' let it gets lost.\n\nA better replacement is very much welcomed.\n\n-- >8 --\nSubject: [PATCH] diff: demote core.safecrlf=true to core.safecrlf=warn\n\nOtherwise the user will not be able to start to guess where in the\ncontents in the working tree the offending unsafe CR lies.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 6 +++++-\n t/t0020-crlf.sh | 8 ++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 32142db..155857c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2647,6 +2647,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n int diff_populate_filespec(struct diff_filespec *s, int size_only)\n {\n \tint err = 0;\n+\tenum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL\n+\t\t\t\t ? safe_crlf\n+\t\t\t\t : SAFE_CRLF_WARN);\n+\n \tif (!DIFF_FILE_VALID(s))\n \t\tdie(\"internal error: asking to populate invalid file.\");\n \tif (S_ISDIR(s->mode))\n@@ -2702,7 +2706,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t/*\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, crlf_warn)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 1a8f44c..e526184 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -81,6 +81,14 @@ test_expect_success 'safecrlf: print warning only once' '\n \ttest $(git add doublewarn 2>&1 | grep \"CRLF will be replaced by LF\" | wc -l) = 1\n '\n \n+\n+test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\tgit diff HEAD\n+'\n+\n+\n test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '\n \tgit config core.autocrlf false &&\n \tgit config core.safecrlf false &&\n-- \n1.8.3.1-771-gb7c3f25\n"},{"id":"221969","messageId":"51C9FDC0.5020709@web.de","threadId":"34244","inReplyTo":"7vli5z6uyq.fsf@alter.siamese.dyndns.org","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-06-25T20:29:52Z","receivedAt":"2013-06-25T20:29:52Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"> +++ b/diff.c\n> @@ -2647,6 +2647,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n> int diff_populate_filespec(struct diff_filespec *s, int size_only)\n> {\n> \tint err = 0;\n> +\tenum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL\n> +\t\t\t\t ? safe_crlf\n> +\t\t\t\t : SAFE_CRLF_WARN);\n\nThanks, \nDoes it makes sense to write it the other way around?\n\nenum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL \n ? SAFE_CRLF_WARN \n : safe_crlf);\n"},{"id":"222021","messageId":"7vsj04yisa.fsf@alter.siamese.dyndns.org","threadId":"34244","inReplyTo":"51C9FDC0.5020709@web.de","subject":"Re: git diff returns fatal error with core.safecrlf is set to true.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-26T15:48:53Z","receivedAt":"2013-06-26T15:48:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> +++ b/diff.c\n>> @@ -2647,6 +2647,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n>> int diff_populate_filespec(struct diff_filespec *s, int size_only)\n>> {\n>> \tint err = 0;\n>> +\tenum safe_crlf crlf_warn = (safe_crlf != SAFE_CRLF_FAIL\n>> +\t\t\t\t ? safe_crlf\n>> +\t\t\t\t : SAFE_CRLF_WARN);\n>\n> Thanks, \n> Does it makes sense to write it the other way around?\n>\n> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL \n> ? SAFE_CRLF_WARN \n> : safe_crlf);\n\nI didn't see much difference either way, but between \"FAIL needs to\nbe demoted to WARN, everything else goes as-is\" and the original \"We\ndo not care about anything other than FAIL, so use it as-is, but\ndemote FAIL to WARN\", yours look shorter. Will replace.\n"}]}