{"thread":{"id":"45166","subject":"git diff --quiet exits with 1 on clean tree with CRLF conversions","startedAt":"2017-02-17T21:34:19Z","lastAt":"2017-03-04T19:59:19Z","messageCount":29,"participants":["Mike Crowe","Junio C Hamano","Torsten Bögershausen","tboegi@web.de","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"311939","messageId":"20170217212633.GA24937@mcrowe.com","threadId":"45166","inReplyTo":null,"subject":"git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-02-17T21:26:33Z","receivedAt":"2017-02-17T21:34:19Z","isPatch":false,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"If \"git diff --quiet\" finds it necessary to compare actual file contents,\nand a file requires CRLF conversion, then it incorrectly exits with an exit\ncode of 1 even if there have been no changes.\n\nThe patch below adds a test file that shows the problem.\n\nThe first test of diff without --quiet correctly has an exit status of zero\non both invocations.\n\nThe second test of diff with --quiet has an exit code of zero on the first\ninvocation, but an exit code of one on the second invocation. Further\ninvocations (not included in the test) may yield an exit code of 1. Calling\n\"git status\" always fixes things.\n\n(The touching with \"tomorrow\" was my attempt to avoid the sleep, but that\ndidn't seem to help - it appears that time must pass in order to ensure\nthat the cache is not used.)\n\nThe culprit would appear to be in diff_filespec_check_stat_unmatch where it\nassumes that the files are different if their sizes are different:\n\n\tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n\t !DIFF_FILE_VALID(p->two) ||\n\t (p->one->oid_valid && p->two->oid_valid) ||\n\t (p->one->mode != p->two->mode) ||\n\t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n\t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n\t (p->one->size != p->two->size) ||\n\t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n\t\tp->skip_stat_unmatch_result = 1;\n\nIn the failing case p->one->size == 14 and p->two->size == 12.\n\nMike.\n\ndiff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh\nnew file mode 100755\nindex 0000000..a108dfb\n--- /dev/null\n+++ b/t/t4063-diff-converted.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2017 Mike Crowe\n+#\n+\n+test_description='git diff with files that require CRLF conversion'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo \"* text=auto\" > .gitattributes &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" > crlf.txt &&\n+\tgit add .gitattributes crlf.txt &&\n+\tgit commit -m \"initial\"\n+'\n+test_expect_success 'noisy diff works on file that requires CRLF conversion' '\n+\tgit status >/dev/null &&\n+\tgit diff >/dev/null &&\n+\tsleep 1 &&\n+\ttouch --date=tomorrow crlf.txt &&\n+\tgit diff >/dev/null\n+'\n+# The sleep is required for reasons I don't fully understand\n+test_expect_success 'quiet diff works on file that requires CRLF conversion with no changes' '\n+\tgit status &&\n+\tgit diff --quiet &&\n+\tsleep 1 &&\n+\ttouch --date=tomorrow crlf.txt &&\n+\tgit diff --quiet\n+'\n+\n+test_done\n"},{"id":"311944","messageId":"xmqqr32wqxr6.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170217212633.GA24937@mcrowe.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T22:05:17Z","receivedAt":"2017-02-17T22:05:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Crowe writes:\n\n> If \"git diff --quiet\" finds it necessary to compare actual file contents,\n> and a file requires CRLF conversion, then it incorrectly exits with an exit\n> code of 1 even if there have been no changes.\n>\n> The patch below adds a test file that shows the problem.\n\nIf \"git diff\" does not show any output and \"git diff --exit-code\" or\n\"git diff --quiet\" says there are differences, then it is a bug.\n\nI would however have expected that any culprit would involve a code\nthat says \"under QUICK option, we do not have to bother doing\nthis\". The part you quoted:\n\n> \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n> \t !DIFF_FILE_VALID(p->two) ||\n> \t (p->one->oid_valid && p->two->oid_valid) ||\n> \t (p->one->mode != p->two->mode) ||\n> \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n> \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n> \t (p->one->size != p->two->size) ||\n> \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n> \t\tp->skip_stat_unmatch_result = 1;\n\nis used by \"git diff\" with and without \"--quiet\", afacr, so I\nsuspect that the bug lies somewhere else.\n"},{"id":"311951","messageId":"20170217221958.GA12163@mcrowe.com","threadId":"45166","inReplyTo":"xmqqr32wqxr6.fsf@gitster.mtv.corp.google.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-02-17T22:19:58Z","receivedAt":"2017-02-17T22:29:16Z","isPatch":false,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Friday 17 February 2017 at 14:05:17 -0800, Junio C Hamano wrote:\n> Mike Crowe writes:\n> \n> > If \"git diff --quiet\" finds it necessary to compare actual file contents,\n> > and a file requires CRLF conversion, then it incorrectly exits with an exit\n> > code of 1 even if there have been no changes.\n> >\n> > The patch below adds a test file that shows the problem.\n> \n> If \"git diff\" does not show any output and \"git diff --exit-code\" or\n> \"git diff --quiet\" says there are differences, then it is a bug.\n> \n> I would however have expected that any culprit would involve a code\n> that says \"under QUICK option, we do not have to bother doing\n> this\". The part you quoted:\n> \n> > \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n> > \t !DIFF_FILE_VALID(p->two) ||\n> > \t (p->one->oid_valid && p->two->oid_valid) ||\n> > \t (p->one->mode != p->two->mode) ||\n> > \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n> > \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n> > \t (p->one->size != p->two->size) ||\n> > \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n> > \t\tp->skip_stat_unmatch_result = 1;\n> \n> is used by \"git diff\" with and without \"--quiet\", afacr, so I\n> suspect that the bug lies somewhere else.\n\nI can't say that I really understand the code fully, but it appears that\nthe first pass generates a queue of files that contain differences. The\nresult of the quiet diff comes from the size of that queue,\ndiff_queued_diff.nr, being non-zero in diffcore_std. I'm assuming that the\nresult of the noisy diff comes from actually comparing the files.\n\nBut, I've only spent a short while looking so I may have got the wrong end\nof the stick.\n\nThanks.\n\nMike.\n"},{"id":"312154","messageId":"20170220153322.GA8352@mcrowe.com","threadId":"45166","inReplyTo":"20170217221958.GA12163@mcrowe.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-02-20T15:33:22Z","receivedAt":"2017-02-20T15:33:32Z","isPatch":false,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Friday 17 February 2017 at 22:19:58 +0000, Mike Crowe wrote:\n> On Friday 17 February 2017 at 14:05:17 -0800, Junio C Hamano wrote:\n> > Mike Crowe writes:\n> > \n> > > If \"git diff --quiet\" finds it necessary to compare actual file contents,\n> > > and a file requires CRLF conversion, then it incorrectly exits with an exit\n> > > code of 1 even if there have been no changes.\n> > >\n> > > The patch below adds a test file that shows the problem.\n> > \n> > If \"git diff\" does not show any output and \"git diff --exit-code\" or\n> > \"git diff --quiet\" says there are differences, then it is a bug.\n> > \n> > I would however have expected that any culprit would involve a code\n> > that says \"under QUICK option, we do not have to bother doing\n> > this\". The part you quoted:\n> > \n> > > \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n> > > \t !DIFF_FILE_VALID(p->two) ||\n> > > \t (p->one->oid_valid && p->two->oid_valid) ||\n> > > \t (p->one->mode != p->two->mode) ||\n> > > \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n> > > \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n> > > \t (p->one->size != p->two->size) ||\n> > > \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n> > > \t\tp->skip_stat_unmatch_result = 1;\n> > \n> > is used by \"git diff\" with and without \"--quiet\", afacr, so I\n> > suspect that the bug lies somewhere else.\n> \n> I can't say that I really understand the code fully, but it appears that\n> the first pass generates a queue of files that contain differences. The\n> result of the quiet diff comes from the size of that queue,\n> diff_queued_diff.nr, being non-zero in diffcore_std. I'm assuming that the\n> result of the noisy diff comes from actually comparing the files.\n> \n> But, I've only spent a short while looking so I may have got the wrong end\n> of the stick.\n\nTricking Git into checking the actual file contents (by passing\n--ignore-space-change for example) is sufficient to ensure that the exit\nstatus is never 1 when it should be zero. (Of course that option has other\nunwanted effects in this case.)\n\nI think that if there's a risk that file contents will undergo conversion\nthen this should force the diff to check the file contents. Something like:\n\ndiff --git a/diff.c b/diff.c\nindex 051761b..bee1662 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3413,13 +3413,14 @@ void diff_setup_done(struct diff_options *options)\n \t/*\n \t * Most of the time we can say \"there are changes\"\n \t * only by checking if there are changed paths, but\n-\t * --ignore-whitespace* options force us to look\n-\t * inside contents.\n+\t * --ignore-whitespace* options or text conversion\n+\t * force us to look inside contents.\n \t */\n \n \tif (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n \t DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n-\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n+\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) ||\n+\t DIFF_OPT_TST(options, ALLOW_TEXTCONV))\n \t\tDIFF_OPT_SET(options, DIFF_FROM_CONTENTS);\n \telse\n \t\tDIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);\n\nThis solves the problem for me and my test case now passes. Unfortunately\nit breaks the 'removing and adding subproject' test case in\nt3040-subprojects-basic at the line:\n\n test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD\n\npresumably because after the rename has been detected the file contents are\nidentical. :( A rename of a single file appears to still be handled\ncorrectly.\n\nMike.\n"},{"id":"312170","messageId":"xmqqlgt0imhe.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170220153322.GA8352@mcrowe.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-20T21:25:01Z","receivedAt":"2017-02-20T21:25:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Crowe writes:\n\n> I think that if there's a risk that file contents will undergo conversion\n> then this should force the diff to check the file contents. Something like:\n>\n> diff --git a/diff.c b/diff.c\n> index 051761b..bee1662 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3413,13 +3413,14 @@ void diff_setup_done(struct diff_options *options)\n> \t/*\n> \t * Most of the time we can say \"there are changes\"\n> \t * only by checking if there are changed paths, but\n> -\t * --ignore-whitespace* options force us to look\n> -\t * inside contents.\n> +\t * --ignore-whitespace* options or text conversion\n> +\t * force us to look inside contents.\n> \t */\n> \n> \tif (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n> \t DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n> -\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n> +\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) ||\n> +\t DIFF_OPT_TST(options, ALLOW_TEXTCONV))\n> \t\tDIFF_OPT_SET(options, DIFF_FROM_CONTENTS);\n> \telse\n> \t\tDIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);\n\nThanks.\n\nYou may be on the right track to find FROM_CONTENTS bit, but\nI think TEXTCONV bit is a red-herring.\n\nThis part of diff.c caught my attention, while thinking about this\ntopic:\n\n\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n\t DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n\t DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n\t\t/*\n\t\t * run diff_flush_patch for the exit status. setting\n\t\t * options->file to /dev/null should be safe, because we\n\t\t * aren't supposed to produce any output anyway.\n\t\t */\n\nand the body of this \"if\" statement loops over q->queue[]. It is\nabout the \"even though we prefer not having to format the patch\nbecause we are doing --quiet, we need to see if contents of one and\ntwo that we _know_ are different are made into the same thing when\nwe compare them while ignoring various forms of whitespace changes\".\nSo one and two that are removed in earlier step because they were\ntruly identical may not be penalized if you flip FROM_CONTENTS bit\nearly on.\n\nHaving said that, DIFF_FROM_CONTENTS is about all paths this options\nstructure governs, not just paths that have eol conversion defined.\nWhen you say \"diff --ignore-whitespace-change\", that applies to all\npaths. The eol conversion is specified per-path, so ideally the\nFROM_CONTENTS bit should be flipped if and only if one or more of\nthe paths would need the conversion (i.e. does the helper function\nwould_convert_to_git() say \"yes\" to the path?). I however suspect\nthat necessary information to do so (i.e. \"which paths we are\nlooking at?\") has not been generated yet at the point of the code\nyou quoted. setup comes (and must come) very early, and then\nq->queue[] is populated by different front-end functions that\ncompare trees, the index, and the working tree, depending on the\n\"git diff\" option or \"git diff-{tree,index,files}\" plumbing command,\nand you can ask \"would one of these paths need conversion?\" only\nafter q->queue[] is populated. Hmm.....\n\nAnother thing is that ALLOW_TEXTCONV is not a right bit to check for\nyour example. It is \"do we use textconv filters to turn binary\nfiles into a (phony) text representation before comparing?\". People\nuse the mechanism to throw JPEG photos in Git and have textconv\nfilter to extract only EXIF data, and \"diff --textconv\" would let us\ntextually compare only the EXIF data (which is the only human\nreadable part of the contents anyway). \n\nIt might be a good idea to also flip FROM_CONTENTS bit under \"diff\n--textconv\", and ...\n\n> This solves the problem for me and my test case now passes.\n\n... but I suspect that you were misled to think it fixes your issue,\nonly because \"--textconv\" is by default enabled for \"git diff\" and\n\"git log\" (see \"git diff --help\"). I think you are saying that if\nyou always set FROM_CONTENTS bit on, you get what you want. But\nthat is to be expected and it unfortunately penalizes everybody by\nturning an obvious optimization permanently off.\n\nAlso \"--textconv\" is not on by default for the plumbing \"git\ndiff-index\" command and its friends, so it is likely that \"git\ndiff-index HEAD\" with your change will still not work as you expect.\n\nA cheap (from code-change point of view) band-aid might be to flip\nFROM_CONTENTS on if we know the repository has _some_ paths that\nneed eol conversion, even when the particular \"diff\" we are taking\ndoes not involve any eol conversion (e.g. \"is core.crlf set?\").\nWhile it may be better than \"if we are porcelain (aka ALLOW_TEXTCONV\nis set), unconditionally flip FROM_CONTENTS on\", it is not ideal,\neither.\n\nThis almost makes me suspect that the place that checks lengths of\none and two in order to refrain from running more expensive content\ncomparison you found earlier need to ask would_convert_to_git()\nbefore taking the short-cut, something along the lines of this (for\nillustration purposes only, not even compile-tested). The \"almost\"\ncomes to me because I do not offhand know the performance implications\nof making calls to would_convert_to_git() here.\n\n diff.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 051761be40..094d5913da 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n \t * differences.\n \t *\n \t * 2. At this point, the file is known to be modified,\n-\t * with the same mode and size, and the object\n-\t * name of one side is unknown. Need to inspect\n-\t * the identical contents.\n+\t * with the same mode and size, the object\n+\t * name of one side is unknown, or size comparison\n+\t * cannot be depended upon. Need to inspect the \n+\t * contents.\n \t */\n \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n \t !DIFF_FILE_VALID(p->two) ||\n@@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n \t (p->one->mode != p->two->mode) ||\n \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n-\t (p->one->size != p->two->size) ||\n+\n+\t /* \n+\t * only if eol and other conversions are not involved,\n+\t * we can say that two contents of different sizes\n+\t * cannot be the same without checking their contents.\n+\t */\n+\t (!would_convert_to_git(p->one->path) &&\n+\t !would_convert_to_git(p->two->path) &&\n+\t (p->one->size != p->two->size)) ||\n+\n \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n \t\tp->skip_stat_unmatch_result = 1;\n \treturn p->skip_stat_unmatch_result;\n\n\n"},{"id":"312639","messageId":"20170225153230.GA30565@mcrowe.com","threadId":"45166","inReplyTo":"xmqqlgt0imhe.fsf@gitster.mtv.corp.google.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-02-25T15:32:30Z","receivedAt":"2017-02-25T15:32:39Z","isPatch":false,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Monday 20 February 2017 at 13:25:01 -0800, Junio C Hamano wrote:\n> This almost makes me suspect that the place that checks lengths of\n> one and two in order to refrain from running more expensive content\n> comparison you found earlier need to ask would_convert_to_git()\n> before taking the short-cut, something along the lines of this (for\n> illustration purposes only, not even compile-tested). The \"almost\"\n> comes to me because I do not offhand know the performance implications\n> of making calls to would_convert_to_git() here.\n> \n> diff.c | 18 ++++++++++++++----\n> 1 file changed, 14 insertions(+), 4 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 051761be40..094d5913da 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n> \t * differences.\n> \t *\n> \t * 2. At this point, the file is known to be modified,\n> -\t * with the same mode and size, and the object\n> -\t * name of one side is unknown. Need to inspect\n> -\t * the identical contents.\n> +\t * with the same mode and size, the object\n> +\t * name of one side is unknown, or size comparison\n> +\t * cannot be depended upon. Need to inspect the \n> +\t * contents.\n> \t */\n> \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n> \t !DIFF_FILE_VALID(p->two) ||\n> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n> \t (p->one->mode != p->two->mode) ||\n> \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n> \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n> -\t (p->one->size != p->two->size) ||\n> +\n> +\t /* \n> +\t * only if eol and other conversions are not involved,\n> +\t * we can say that two contents of different sizes\n> +\t * cannot be the same without checking their contents.\n> +\t */\n> +\t (!would_convert_to_git(p->one->path) &&\n> +\t !would_convert_to_git(p->two->path) &&\n> +\t (p->one->size != p->two->size)) ||\n> +\n> \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n> \t\tp->skip_stat_unmatch_result = 1;\n> \treturn p->skip_stat_unmatch_result;\n> \n> \n\nThanks for investigating this. I think you are correct that I was misguided\nin my previous \"fix\". However, your change above does fix the problem for\nme.\n\nIt looks like the main cost of convert_to_git is in convert_attrs which\nends up doing various path operations in attr.c. After that, both\napply_filter and crlf_to_git return straight away if there's nothing to do.\n\nI experimented several times with running \"git diff -quiet\" after touching\nevery file in Git's own worktree and any difference in total time was lost\nin the noise.\n\nI've further improved my test case. Tests 3 and 4 fail without the above\nchange but pass with it. Unfortunately I'm still unable to get those tests\nto fail without the above fix unless the sleeps are present. I've tried\nusing the \"touch -r .datetime\" technique from racy-git.txt but it doesn't\nhelp. It seems that I'm unable to stop Git from using its cache without\nsleeping. :(\n\ndiff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh\nnew file mode 100755\nindex 0000000..31a730d\n--- /dev/null\n+++ b/t/t4063-diff-converted.sh\n@@ -0,0 +1,44 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2017 Mike Crowe\n+#\n+# These tests ensure that files changing line endings in the presence\n+# of .gitattributes to indicate that line endings should be ignored\n+# don't cause 'git diff' or 'git diff --quiet' to think that they have\n+# been changed.\n+#\n+# The sleeps are necessary to reproduce the problem for reasons that I\n+# don't understand.\n+\n+test_description='git diff with files that require CRLF conversion'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo \"* text=auto\" > .gitattributes &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" > crlf.txt &&\n+\tprintf \"Hello\\nWorld\\n\" > lf.txt &&\n+\tgit add .gitattributes crlf.txt lf.txt &&\n+\tgit commit -m \"initial\" && echo three\n+'\n+test_expect_success 'noisy diff works on file that requires CRLF conversion' '\n+\tgit status >/dev/null &&\n+\tgit diff >/dev/null &&\n+\tsleep 1 &&\n+\ttouch crlf.txt lf.txt &&\n+\tgit diff >/dev/null\n+'\n+test_expect_success 'quiet diff works on file that requires CRLF conversion with no changes' '\n+\tgit status &&\n+\tgit diff --quiet &&\n+\tsleep 1 &&\n+\ttouch crlf.txt lf.txt &&\n+\tgit diff --quiet\n+'\n+\n+test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n+\tprintf \"Hello\\nWorld\\n\" > crlf.txt &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" > lf.txt &&\n+\tgit diff --quiet\n+'\n+test_done\n\n\nMike.\n"},{"id":"312806","messageId":"xmqqefyjwfql.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170225153230.GA30565@mcrowe.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T20:17:22Z","receivedAt":"2017-02-28T00:40:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten, you've been quite active in fixing various glitches around\nthe EOL conversion in the latter half of last year. Have any\nthoughts to share on this topic?\n\nThanks.\n\nMike Crowe writes:\n\n> On Monday 20 February 2017 at 13:25:01 -0800, Junio C Hamano wrote:\n>> This almost makes me suspect that the place that checks lengths of\n>> one and two in order to refrain from running more expensive content\n>> comparison you found earlier need to ask would_convert_to_git()\n>> before taking the short-cut, something along the lines of this (for\n>> illustration purposes only, not even compile-tested). The \"almost\"\n>> comes to me because I do not offhand know the performance implications\n>> of making calls to would_convert_to_git() here.\n>> \n>> diff.c | 18 ++++++++++++++----\n>> 1 file changed, 14 insertions(+), 4 deletions(-)\n>> \n>> diff --git a/diff.c b/diff.c\n>> index 051761be40..094d5913da 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n>> \t * differences.\n>> \t *\n>> \t * 2. At this point, the file is known to be modified,\n>> -\t * with the same mode and size, and the object\n>> -\t * name of one side is unknown. Need to inspect\n>> -\t * the identical contents.\n>> +\t * with the same mode and size, the object\n>> +\t * name of one side is unknown, or size comparison\n>> +\t * cannot be depended upon. Need to inspect the \n>> +\t * contents.\n>> \t */\n>> \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n>> \t !DIFF_FILE_VALID(p->two) ||\n>> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n>> \t (p->one->mode != p->two->mode) ||\n>> \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n>> \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n>> -\t (p->one->size != p->two->size) ||\n>> +\n>> +\t /* \n>> +\t * only if eol and other conversions are not involved,\n>> +\t * we can say that two contents of different sizes\n>> +\t * cannot be the same without checking their contents.\n>> +\t */\n>> +\t (!would_convert_to_git(p->one->path) &&\n>> +\t !would_convert_to_git(p->two->path) &&\n>> +\t (p->one->size != p->two->size)) ||\n>> +\n>> \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n>> \t\tp->skip_stat_unmatch_result = 1;\n>> \treturn p->skip_stat_unmatch_result;\n>> \n>> \n>\n> Thanks for investigating this. I think you are correct that I was misguided\n> in my previous \"fix\". However, your change above does fix the problem for\n> me.\n>\n> It looks like the main cost of convert_to_git is in convert_attrs which\n> ends up doing various path operations in attr.c. After that, both\n> apply_filter and crlf_to_git return straight away if there's nothing to do.\n>\n> I experimented several times with running \"git diff -quiet\" after touching\n> every file in Git's own worktree and any difference in total time was lost\n> in the noise.\n>\n> I've further improved my test case. Tests 3 and 4 fail without the above\n> change but pass with it. Unfortunately I'm still unable to get those tests\n> to fail without the above fix unless the sleeps are present. I've tried\n> using the \"touch -r .datetime\" technique from racy-git.txt but it doesn't\n> help. It seems that I'm unable to stop Git from using its cache without\n> sleeping. :(\n>\n> diff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh\n> new file mode 100755\n> index 0000000..31a730d\n> --- /dev/null\n> +++ b/t/t4063-diff-converted.sh\n> @@ -0,0 +1,44 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2017 Mike Crowe\n> +#\n> +# These tests ensure that files changing line endings in the presence\n> +# of .gitattributes to indicate that line endings should be ignored\n> +# don't cause 'git diff' or 'git diff --quiet' to think that they have\n> +# been changed.\n> +#\n> +# The sleeps are necessary to reproduce the problem for reasons that I\n> +# don't understand.\n> +\n> +test_description='git diff with files that require CRLF conversion'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\techo \"* text=auto\" > .gitattributes &&\n> +\tprintf \"Hello\\r\\nWorld\\r\\n\" > crlf.txt &&\n> +\tprintf \"Hello\\nWorld\\n\" > lf.txt &&\n> +\tgit add .gitattributes crlf.txt lf.txt &&\n> +\tgit commit -m \"initial\" && echo three\n> +'\n> +test_expect_success 'noisy diff works on file that requires CRLF conversion' '\n> +\tgit status >/dev/null &&\n> +\tgit diff >/dev/null &&\n> +\tsleep 1 &&\n> +\ttouch crlf.txt lf.txt &&\n> +\tgit diff >/dev/null\n> +'\n> +test_expect_success 'quiet diff works on file that requires CRLF conversion with no changes' '\n> +\tgit status &&\n> +\tgit diff --quiet &&\n> +\tsleep 1 &&\n> +\ttouch crlf.txt lf.txt &&\n> +\tgit diff --quiet\n> +'\n> +\n> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n> +\tprintf \"Hello\\nWorld\\n\" > crlf.txt &&\n> +\tprintf \"Hello\\r\\nWorld\\r\\n\" > lf.txt &&\n> +\tgit diff --quiet\n> +'\n> +test_done\n>\n>\n> Mike.\n"},{"id":"312864","messageId":"d98aa589-3e08-249d-0c88-72dbcee1a568@web.de","threadId":"45166","inReplyTo":"xmqqefyjwfql.fsf@gitster.mtv.corp.google.com","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-02-28T18:06:44Z","receivedAt":"2017-02-28T18:15:37Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-02-27 21:17, Junio C Hamano wrote:\n\n> Torsten, you've been quite active in fixing various glitches around\n> the EOL conversion in the latter half of last year. Have any\n> thoughts to share on this topic?\n> \n> Thanks.\n\nSorry for the delay, being too busy with other things.\nI followed the discussion, but didn't have good things to contribute.\nI am not an expert in diff.c, but there seems to be a bug, thanks everybody\nfor digging.\n\n\n\nBack to business:\n\nMy understanding is that git diff --quiet should be quiet, when\ngit add will not do anything (but the file is \"touched\".\nThe touched means that Git will detect e.g a new mtime or inode\nor file size when doing lstat().\n\nmtime is tricky, inodes are problematic under Windows.\nWhat is easy to change is the file length.\nI don't thing that we need a test file with LF, nor do we need\nthe sleep, touch or anything.\nWould the the following work ?\n(This is copy-paste, so the TABs may be corrupted)\n\n\n#!/bin/sh\n#\n# Copyright (c) 2017 Mike Crowe\n#\n# These tests ensure that files changing line endings in the presence\n# of .gitattributes to indicate that line endings should be ignored\n# don't cause 'git diff' or 'git diff --quiet' to think that they have\n# been changed.\n\ntest_description='git diff with files that require CRLF conversion'\n\n. ./test-lib.sh\n\ntest_expect_success setup '\n\techo \"* text=auto\" > .gitattributes &&\n\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n\tgit add .gitattributes crlf.txt lf.txt &&\n\tgit commit -m \"initial\"\n'\n\ntest_expect_success 'quiet diff works on file with line-ending change that has\nno effect on repository' '\n\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n\tgit status &&\n\tgit diff --quiet\n'\n\ntest_done\n\n\n\n\n\n"},{"id":"312912","messageId":"xmqqshmyhtnu.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"d98aa589-3e08-249d-0c88-72dbcee1a568@web.de","subject":"Re: git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T21:50:13Z","receivedAt":"2017-02-28T22:00:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 2017-02-27 21:17, Junio C Hamano wrote:\n>\n>> Torsten, you've been quite active in fixing various glitches around\n>> the EOL conversion in the latter half of last year. Have any\n>> thoughts to share on this topic?\n>> \n>> Thanks.\n>\n> Sorry for the delay, being too busy with other things.\n> I followed the discussion, but didn't have good things to contribute.\n> I am not an expert in diff.c, but there seems to be a bug, thanks everybody\n> for digging.\n>\n> Back to business:\n>\n> My understanding is that git diff --quiet should be quiet, when\n> git add will not do anything.\n\nYes, I think that is a sensible criterion. What I was interested to\nhear from you the most was to double check if Mike's expectation is\nreasonable. Earlier we had a lengthy discussion on what to do when\nconvert-to-git and convert-to-working-tree conversions do not round\ntrip, and I was wondering if this was one of those cases.\n\n"},{"id":"312961","messageId":"20170301170444.14274-1-tboegi@web.de","threadId":"45166","inReplyTo":"xmqqshmyhtnu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2017-03-01T17:04:44Z","receivedAt":"2017-03-01T18:40:35Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Junio C Hamano \n\ngit diff --quiet may take a short-cut to see if a file is changed\nin the working tree:\nWhenever the file size differs from what is recorded in the index,\nthe file is assumed to be changed and git diff --quiet returns\nexit with code 1\n\nThis shortcut must be suppressed whenever the line endings are converted\nor a filter is in use.\nThe attributes say \"* text=auto\" and a file has\n\"Hello\\nWorld\\n\" in the index with a length of 12.\nThe file in the working tree has \"Hello\\r\\nWorld\\r\\n\" with a length of 14.\n(Or even \"Hello\\r\\nWorld\\n\").\nIn this case \"git add\" will not do any changes to the index, and\n\"git diff -quiet\" should exit 0.\n\nAdd calls to would_convert_to_git() before blindly saying that a different\nsize means different content.\n\nReported-By: Mike Crowe \nSigned-off-by: Torsten Bögershausen \n---\nThis is what I can come up with, collecting all the loose ends.\nI'm not sure if Mike wan't to have the Reported-By with a\nSigned-off-by ?\nThe other question is, if the commit message summarizes the discussion\nwell enough ?\n\ndiff.c | 18 ++++++++++++++----\n t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 4 deletions(-)\n create mode 100755 t/t0028-diff-converted.sh\n\ndiff --git a/diff.c b/diff.c\nindex 051761b..c264758 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n \t * differences.\n \t *\n \t * 2. At this point, the file is known to be modified,\n-\t * with the same mode and size, and the object\n-\t * name of one side is unknown. Need to inspect\n-\t * the identical contents.\n+\t * with the same mode and size, the object\n+\t * name of one side is unknown, or size comparison\n+\t * cannot be depended upon. Need to inspect the\n+\t * contents.\n \t */\n \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n \t !DIFF_FILE_VALID(p->two) ||\n@@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n \t (p->one->mode != p->two->mode) ||\n \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n-\t (p->one->size != p->two->size) ||\n+\n+\t /*\n+\t * only if eol and other conversions are not involved,\n+\t * we can say that two contents of different sizes\n+\t * cannot be the same without checking their contents.\n+\t */\n+\t (!would_convert_to_git(p->one->path) &&\n+\t !would_convert_to_git(p->two->path) &&\n+\t (p->one->size != p->two->size)) ||\n+\n \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n \t\tp->skip_stat_unmatch_result = 1;\n \treturn p->skip_stat_unmatch_result;\ndiff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh\nnew file mode 100755\nindex 0000000..3d5ab95\n--- /dev/null\n+++ b/t/t0028-diff-converted.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2017 Mike Crowe\n+#\n+# These tests ensure that files changing line endings in the presence\n+# of .gitattributes to indicate that line endings should be ignored\n+# don't cause 'git diff' or 'git diff --quiet' to think that they have\n+# been changed.\n+\n+test_description='git diff with files that require CRLF conversion'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo \"* text=auto\" >.gitattributes &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n+\tgit add .gitattributes crlf.txt &&\n+\tgit commit -m \"initial\"\n+'\n+\n+test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n+\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n+\tgit status &&\n+\tgit diff --quiet\n+'\n+\n+test_done\n-- \n2.10.0\n\n"},{"id":"312978","messageId":"xmqqr32gg0o6.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170301170444.14274-1-tboegi@web.de","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T21:14:01Z","receivedAt":"2017-03-01T21:18:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Junio C Hamano \n>\n> git diff --quiet may take a short-cut to see if a file is changed\n> in the working tree:\n> Whenever the file size differs from what is recorded in the index,\n> the file is assumed to be changed and git diff --quiet returns\n> exit with code 1\n>\n> This shortcut must be suppressed whenever the line endings are converted\n> or a filter is in use.\n> The attributes say \"* text=auto\" and a file has\n> \"Hello\\nWorld\\n\" in the index with a length of 12.\n> The file in the working tree has \"Hello\\r\\nWorld\\r\\n\" with a length of 14.\n> (Or even \"Hello\\r\\nWorld\\n\").\n> In this case \"git add\" will not do any changes to the index, and\n> \"git diff -quiet\" should exit 0.\n\nThe thing I find the most disturbing is that at this point in the\nflow, p->one->size and p->two->size are supposed to be the sizes of\nthe blob object, not the contents of the file on the working tree.\nIOW, p->two->size being 14 in the above example sounds like pointing\nat a different bug, if it is 14. \n\nThe early return in diff_populate_filespec(), where it does\n\n\ts->size = xsize_t(st.st_size);\n\t...\n\tif (size_only)\n\t\treturn 0;\n\nway before it runs convert_to_git(), may be the real culprit.\n\nI am wondering if the real fix would be to do this, instead of the\ntwo extra would_convert_to_git() call there in the patch you sent.\nThe result seems to still pass the new test in your patch.\n\nThanks for helping.\n\n diff.c | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8c78fce49d..dc51dceb44 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n-\t\tif (size_only)\n+\n+\t\t/*\n+\t\t * Even if the caller would be happy with getting\n+\t\t * only the size, we cannot return early at this\n+\t\t * point if the path requires us to run the content\n+\t\t * conversion.\n+\t\t */\n+\t\tif (!would_convert_to_git(s->path) && size_only)\n \t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * Note: this check uses xsize_t(st.st_size) that may\n+\t\t * not be the true size of the blob after it goes\n+\t\t * through convert_to_git(). This may not strictly be\n+\t\t * correct, but the whole point of big_file_threashold\n+\t\t * and is_binary check is that we want to avoid\n+\t\t * opening the file and inspecting the contents, so\n+\t\t * this is probably fine.\n+\t\t */\n \t\tif ((flags & CHECK_BINARY) &&\n \t\t s->size > big_file_threshold && s->is_binary == -1) {\n \t\t\ts->is_binary = 1;\n"},{"id":"312979","messageId":"20170301212535.GA6878@mcrowe.com","threadId":"45166","inReplyTo":"20170301170444.14274-1-tboegi@web.de","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-03-01T21:25:35Z","receivedAt":"2017-03-01T21:27:37Z","isPatch":true,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Wednesday 01 March 2017 at 18:04:44 +0100, tboegi@web.de wrote:\n> From: Junio C Hamano \n> \n> git diff --quiet may take a short-cut to see if a file is changed\n> in the working tree:\n> Whenever the file size differs from what is recorded in the index,\n> the file is assumed to be changed and git diff --quiet returns\n> exit with code 1\n> \n> This shortcut must be suppressed whenever the line endings are converted\n> or a filter is in use.\n> The attributes say \"* text=auto\" and a file has\n> \"Hello\\nWorld\\n\" in the index with a length of 12.\n> The file in the working tree has \"Hello\\r\\nWorld\\r\\n\" with a length of 14.\n> (Or even \"Hello\\r\\nWorld\\n\").\n> In this case \"git add\" will not do any changes to the index, and\n> \"git diff -quiet\" should exit 0.\n> \n> Add calls to would_convert_to_git() before blindly saying that a different\n> size means different content.\n> \n> Reported-By: Mike Crowe \n> Signed-off-by: Torsten Bögershausen \n> ---\n> This is what I can come up with, collecting all the loose ends.\n> I'm not sure if Mike wan't to have the Reported-By with a\n> Signed-off-by ?\n> The other question is, if the commit message summarizes the discussion\n> well enough ?\n> \n> diff.c | 18 ++++++++++++++----\n> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 41 insertions(+), 4 deletions(-)\n> create mode 100755 t/t0028-diff-converted.sh\n> \n> diff --git a/diff.c b/diff.c\n> index 051761b..c264758 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n> \t * differences.\n> \t *\n> \t * 2. At this point, the file is known to be modified,\n> -\t * with the same mode and size, and the object\n> -\t * name of one side is unknown. Need to inspect\n> -\t * the identical contents.\n> +\t * with the same mode and size, the object\n> +\t * name of one side is unknown, or size comparison\n> +\t * cannot be depended upon. Need to inspect the\n> +\t * contents.\n> \t */\n> \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n> \t !DIFF_FILE_VALID(p->two) ||\n> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n> \t (p->one->mode != p->two->mode) ||\n> \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n> \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n> -\t (p->one->size != p->two->size) ||\n> +\n> +\t /*\n> +\t * only if eol and other conversions are not involved,\n> +\t * we can say that two contents of different sizes\n> +\t * cannot be the same without checking their contents.\n> +\t */\n> +\t (!would_convert_to_git(p->one->path) &&\n> +\t !would_convert_to_git(p->two->path) &&\n> +\t (p->one->size != p->two->size)) ||\n> +\n> \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n> \t\tp->skip_stat_unmatch_result = 1;\n> \treturn p->skip_stat_unmatch_result;\n> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh\n> new file mode 100755\n> index 0000000..3d5ab95\n> --- /dev/null\n> +++ b/t/t0028-diff-converted.sh\n> @@ -0,0 +1,27 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2017 Mike Crowe\n> +#\n> +# These tests ensure that files changing line endings in the presence\n> +# of .gitattributes to indicate that line endings should be ignored\n> +# don't cause 'git diff' or 'git diff --quiet' to think that they have\n> +# been changed.\n> +\n> +test_description='git diff with files that require CRLF conversion'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\techo \"* text=auto\" >.gitattributes &&\n> +\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n> +\tgit add .gitattributes crlf.txt &&\n> +\tgit commit -m \"initial\"\n> +'\n> +\n> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n> +\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n> +\tgit status &&\n> +\tgit diff --quiet\n> +'\n> +\n> +test_done\n\nHi Torsten,\n\nThanks for investigating this.\n\nI think that you've simplified the test to the point where it doesn't\nentirely prove the fix. Although you test the case where the file has\nchanged size, you don't test the case where it hasn't.\n\nUnfortunately that was the part of my test that could only reproduce the\nproblem with the sleeps. Maybe someone who understands how the cache works\nfully could explain an alternative way to force the cache not to be used.\n\nAlso, I think I've found a behaviour change with this fix. Consider:\n\n echo \"* text=auto\" >.gitattributes\n printf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt\n git add .gitattributes crlf.txt\n git commit -m \"initial\"\n\n printf \"\\r\\n\" >>crlf.txt\n\nWith the above patch, both \"git diff\" and \"git diff --quiet\" report that\nthere are no changes. Previously Git would report the extra newline\ncorrectly.\n\nMike.\n"},{"id":"312985","messageId":"xmqqa894fyst.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"xmqqr32gg0o6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T21:54:26Z","receivedAt":"2017-03-01T22:00:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Now I thought about it through a bit more thoroughly, I think this\nis the right approach, so here is my (tenative) final version.\n\nI seem to be getty really rusty---after all the codepaths involved\nare practically all my code and I should have noticed the real\nculprit during my first attempt X-<.\n\nThanks for helping.\n\n-- >8 --\nSubject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()\n\nCallers of diff_populate_filespec() can choose to ask only for the\nsize of the blob without grabbing the blob data, and the function,\nafter running lstat() when the filespec points at a working tree\nfile, returns by copying the value in size field of the stat\nstructure into the size field of the filespec when this is the case.\n\nHowever, this short-cut cannot be taken if the contents from the\npath needs to go through convert_to_git(), whose resulting real blob\ndata may be different from what is in the working tree file.\n\nAs \"git diff --quiet\" compares the .size fields of filespec\nstructures to skip content comparison, this bug manifests as a\nfalse \"there are differences\" for a file that needs eol conversion,\nfor example.\n\nReported-by: Mike Crowe \nHelped-by: Torsten Bögershausen \nSigned-off-by: Junio C Hamano \n---\n diff.c | 19 ++++++++++++++++++-\n t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+), 1 deletion(-)\n create mode 100755 t/t0028-diff-converted.sh\n\ndiff --git a/diff.c b/diff.c\nindex 8c78fce49d..dc51dceb44 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n-\t\tif (size_only)\n+\n+\t\t/*\n+\t\t * Even if the caller would be happy with getting\n+\t\t * only the size, we cannot return early at this\n+\t\t * point if the path requires us to run the content\n+\t\t * conversion.\n+\t\t */\n+\t\tif (!would_convert_to_git(s->path) && size_only)\n \t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * Note: this check uses xsize_t(st.st_size) that may\n+\t\t * not be the true size of the blob after it goes\n+\t\t * through convert_to_git(). This may not strictly be\n+\t\t * correct, but the whole point of big_file_threashold\n+\t\t * and is_binary check being that we want to avoid\n+\t\t * opening the file and inspecting the contents, this\n+\t\t * is probably fine.\n+\t\t */\n \t\tif ((flags & CHECK_BINARY) &&\n \t\t s->size > big_file_threshold && s->is_binary == -1) {\n \t\t\ts->is_binary = 1;\ndiff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh\nnew file mode 100755\nindex 0000000000..3d5ab9565b\n--- /dev/null\n+++ b/t/t0028-diff-converted.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2017 Mike Crowe\n+#\n+# These tests ensure that files changing line endings in the presence\n+# of .gitattributes to indicate that line endings should be ignored\n+# don't cause 'git diff' or 'git diff --quiet' to think that they have\n+# been changed.\n+\n+test_description='git diff with files that require CRLF conversion'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo \"* text=auto\" >.gitattributes &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n+\tgit add .gitattributes crlf.txt &&\n+\tgit commit -m \"initial\"\n+'\n+\n+test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n+\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n+\tgit status &&\n+\tgit diff --quiet\n+'\n+\n+test_done\n-- \n2.12.0-319-gc5f21175ee\n\n"},{"id":"313007","messageId":"xmqq60jsefuq.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170301212535.GA6878@mcrowe.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T23:29:01Z","receivedAt":"2017-03-01T23:29:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Crowe writes:\n\n> With the above patch, both \"git diff\" and \"git diff --quiet\" report that\n> there are no changes. Previously Git would report the extra newline\n> correctly.\n\nI sent an updated one that (I think) fixes the real issue, which the\nextra would_convert_to_git() calls added in the older iteration to\ndiff_filespec_check_stat_unmatch() were merely papering over.\n\nIt would be nice to see if it fixes the issue for you.\n\nThanks.\n\n\n"},{"id":"313055","messageId":"20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net","threadId":"45166","inReplyTo":"xmqqa894fyst.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-02T08:53:13Z","receivedAt":"2017-03-02T09:00:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 01, 2017 at 01:54:26PM -0800, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()\n\nThanks, this is well-explained, and the new comments in the code really\nhelp.\n\nI wondered if we should be checking would_convert_to_git() in\nreuse_worktree_file(), but we already do. It's just that we may still\nend up in this code-path when we're _actually_ diffing the working tree\nfile, not just trying to optimize.\n\n> diff --git a/diff.c b/diff.c\n> index 8c78fce49d..dc51dceb44 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n> \t\t\ts->should_free = 1;\n> \t\t\treturn 0;\n> \t\t}\n> -\t\tif (size_only)\n> +\n> +\t\t/*\n> +\t\t * Even if the caller would be happy with getting\n> +\t\t * only the size, we cannot return early at this\n> +\t\t * point if the path requires us to run the content\n> +\t\t * conversion.\n> +\t\t */\n> +\t\tif (!would_convert_to_git(s->path) && size_only)\n> \t\t\treturn 0;\n\nThe would_convert_to_git() function is a little expensive (it may have\nto do an attribute lookup). It may be worth swapping the two halves of\nthe conditional here to get the short-circuit.\n\nIt may not matter much in practice, though, because in the !size_only\ncase we'd make the same query lower a few lines later (and in theory\nexpensive bits of the attr lookup are cached).\n\n> +\n> +\t\t/*\n> +\t\t * Note: this check uses xsize_t(st.st_size) that may\n> +\t\t * not be the true size of the blob after it goes\n> +\t\t * through convert_to_git(). This may not strictly be\n> +\t\t * correct, but the whole point of big_file_threashold\n\ns/threashold/threshold/\n\n> +\t\t * and is_binary check being that we want to avoid\n> +\t\t * opening the file and inspecting the contents, this\n> +\t\t * is probably fine.\n> +\t\t */\n> \t\tif ((flags & CHECK_BINARY) &&\n> \t\t s->size > big_file_threshold && s->is_binary == -1) {\n> \t\t\ts->is_binary = 1;\n\nI'm trying to think how this \"not strictly correct\" could bite us. For\nline-ending conversion, I'd say that the before/after are going to be\napproximately the same size. But what about something like LFS? If I\nhave a 600MB file that convert_to_git() filters into a short LFS\npointer, I think this changes the behavior. Before, we would diff the\npointer file, but now we'll get \"binary file changed\".\n\nI wonder if we should take the opposite approach, and ignore\nbig_file_threshold for converted files. One assumes that such gigantic\nfiles are binary, and therefore do not have line endings to convert. And\nany filtering has a reasonable chance of condensing them to something\nmuch smaller.\n\nI dunno. I'm sure somebody has some horrific 500MB-filtering example\nthat can prove me wrong.\n\n-Peff\n"},{"id":"313073","messageId":"20170302142056.GB7821@mcrowe.com","threadId":"45166","inReplyTo":"xmqqa894fyst.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-03-02T14:20:56Z","receivedAt":"2017-03-02T14:59:04Z","isPatch":true,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Wednesday 01 March 2017 at 13:54:26 -0800, Junio C Hamano wrote:\n> Now I thought about it through a bit more thoroughly, I think this\n> is the right approach, so here is my (tenative) final version.\n> \n> I seem to be getty really rusty---after all the codepaths involved\n> are practically all my code and I should have noticed the real\n> culprit during my first attempt X-<.\n> \n> Thanks for helping.\n> \n> -- >8 --\n> Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()\n> \n> Callers of diff_populate_filespec() can choose to ask only for the\n> size of the blob without grabbing the blob data, and the function,\n> after running lstat() when the filespec points at a working tree\n> file, returns by copying the value in size field of the stat\n> structure into the size field of the filespec when this is the case.\n> \n> However, this short-cut cannot be taken if the contents from the\n> path needs to go through convert_to_git(), whose resulting real blob\n> data may be different from what is in the working tree file.\n> \n> As \"git diff --quiet\" compares the .size fields of filespec\n> structures to skip content comparison, this bug manifests as a\n> false \"there are differences\" for a file that needs eol conversion,\n> for example.\n> \n> Reported-by: Mike Crowe \n> Helped-by: Torsten Bögershausen \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 19 ++++++++++++++++++-\n> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 45 insertions(+), 1 deletion(-)\n> create mode 100755 t/t0028-diff-converted.sh\n> \n> diff --git a/diff.c b/diff.c\n> index 8c78fce49d..dc51dceb44 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n> \t\t\ts->should_free = 1;\n> \t\t\treturn 0;\n> \t\t}\n> -\t\tif (size_only)\n> +\n> +\t\t/*\n> +\t\t * Even if the caller would be happy with getting\n> +\t\t * only the size, we cannot return early at this\n> +\t\t * point if the path requires us to run the content\n> +\t\t * conversion.\n> +\t\t */\n> +\t\tif (!would_convert_to_git(s->path) && size_only)\n> \t\t\treturn 0;\n> +\n> +\t\t/*\n> +\t\t * Note: this check uses xsize_t(st.st_size) that may\n> +\t\t * not be the true size of the blob after it goes\n> +\t\t * through convert_to_git(). This may not strictly be\n> +\t\t * correct, but the whole point of big_file_threashold\n> +\t\t * and is_binary check being that we want to avoid\n> +\t\t * opening the file and inspecting the contents, this\n> +\t\t * is probably fine.\n> +\t\t */\n> \t\tif ((flags & CHECK_BINARY) &&\n> \t\t s->size > big_file_threshold && s->is_binary == -1) {\n> \t\t\ts->is_binary = 1;\n\nThis patch solves the problem for me. Including my tests where the file\nsize doesn't change but the file has been touched. It also doesn't have the\nside effect of failing to report the extra trailing newline that the\noriginal fix suffered from.\n\nAll the solutions presented so far do cause a small change in behaviour\nwhen using git diff --quiet: they may now cause warning messages like:\n\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n\nto be emitted (unless of course core.safecrlf=false.) I think this is an\nunavoidable side-effect of doing the job properly but it might be worth\nmentioning.\n\n> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh\n> new file mode 100755\n> index 0000000000..3d5ab9565b\n> --- /dev/null\n> +++ b/t/t0028-diff-converted.sh\n> @@ -0,0 +1,27 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2017 Mike Crowe\n> +#\n> +# These tests ensure that files changing line endings in the presence\n> +# of .gitattributes to indicate that line endings should be ignored\n> +# don't cause 'git diff' or 'git diff --quiet' to think that they have\n> +# been changed.\n> +\n> +test_description='git diff with files that require CRLF conversion'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\techo \"* text=auto\" >.gitattributes &&\n> +\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n> +\tgit add .gitattributes crlf.txt &&\n> +\tgit commit -m \"initial\"\n> +'\n> +\n> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n> +\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n> +\tgit status &&\n> +\tgit diff --quiet\n> +'\n> +\n> +test_done\n\nAs I said before, this doesn't actually test the case when the file sizes\nmatch. However, given the way that the code has changed the actual file\nsizes are not compared, so perhaps this doesn't matter.\n\nThanks for all your help investigating this.\n\nMike.\n"},{"id":"313075","messageId":"20170302153817.GC7821@mcrowe.com","threadId":"45166","inReplyTo":"d98aa589-3e08-249d-0c88-72dbcee1a568@web.de","subject":"git status reports file modified when only line-endings have changed (was git diff --quiet exits with 1 on clean tree with CRLF conversions)","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-03-02T15:38:17Z","receivedAt":"2017-03-02T15:44:53Z","isPatch":false,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Tuesday 28 February 2017 at 19:06:44 +0100, Torsten Bögershausen wrote:\n> My understanding is that git diff --quiet should be quiet, when\n> git add will not do anything (but the file is \"touched\".\n> The touched means that Git will detect e.g a new mtime or inode\n> or file size when doing lstat().\n\nDoes the same apply to \"git status\"?\n\nIf so, then whilst investigating the \"git diff --quiet\" problems in this\nthread I've found a similar bug with \"git status\". It reports the file has\nmodifications even if only the line-endings have changed, and issuing \"git\nadd\" causes the perceived modification to disappear.\n\nIt can be very confusing for users if \"git status\" reports a modification\nbut for \"git diff\" to claim that the files are identical.\n\nThis bug is still reproducible even with the fix from\nhttps://public-inbox.org/git/xmqqshmyhtnu.fsf@gitster.mtv.corp.google.com/T/#m67cbfad1f2efe721f0c2afac2a1523b743bb57ca\n\nHere's the test case. Test 3 is the part that currently fails:\n\ncommit de5f3f1d9161cdd46342689abe38a046fc71850e\nAuthor: Mike Crowe \nDate: Sat Feb 25 09:28:37 2017 +0000\n\n status: Add tests for status output when file line endings change\n\ndiff --git a/t/t7518-status-eol-change.sh b/t/t7518-status-eol-change.sh\nnew file mode 100755\nindex 0000000..e18186f\n--- /dev/null\n+++ b/t/t7518-status-eol-change.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2017 Mike Crowe\n+#\n+\n+test_description='git status with files that require CRLF conversion'\n+\n+. ./test-lib.sh\n+\n+cat >expected_no_change < .gitattributes &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" > crlf.txt &&\n+\tprintf \"expected_no_change\\nactual\\n\" > .gitignore &&\n+\tgit add .gitignore .gitattributes crlf.txt &&\n+\tgit commit -m \"initial\"\n+'\n+test_expect_success 'git status reports no change if file regenerated' '\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" > crlf.txt &&\n+\tgit status >actual &&\n+\ttest_cmp expected_no_change actual\n+'\n+test_expect_success 'git status reports no change if line endings change' '\n+\tprintf \"Hello\\nWorld\\n\" > crlf.txt &&\n+\tgit status >actual &&\n+\ttest_cmp expected_no_change actual\n+'\n+test_expect_success 'git status reports no change if line ending change is staged' '\n+\tgit add crlf.txt &&\n+\tgit status >actual &&\n+\ttest_cmp expected_no_change actual\n+'\n+test_done\n"},{"id":"313082","messageId":"5d92d3b8-f438-9be5-9742-22f8cd8fe03d@web.de","threadId":"45166","inReplyTo":"20170301212535.GA6878@mcrowe.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-02T18:17:00Z","receivedAt":"2017-03-02T18:24:37Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-03-01 22:25, Mike Crowe wrote:\n> On Wednesday 01 March 2017 at 18:04:44 +0100, tboegi@web.de wrote:\n>> From: Junio C Hamano \n>>\n>> git diff --quiet may take a short-cut to see if a file is changed\n>> in the working tree:\n>> Whenever the file size differs from what is recorded in the index,\n>> the file is assumed to be changed and git diff --quiet returns\n>> exit with code 1\n>>\n>> This shortcut must be suppressed whenever the line endings are converted\n>> or a filter is in use.\n>> The attributes say \"* text=auto\" and a file has\n>> \"Hello\\nWorld\\n\" in the index with a length of 12.\n>> The file in the working tree has \"Hello\\r\\nWorld\\r\\n\" with a length of 14.\n>> (Or even \"Hello\\r\\nWorld\\n\").\n>> In this case \"git add\" will not do any changes to the index, and\n>> \"git diff -quiet\" should exit 0.\n>>\n>> Add calls to would_convert_to_git() before blindly saying that a different\n>> size means different content.\n>>\n>> Reported-By: Mike Crowe \n>> Signed-off-by: Torsten Bögershausen \n>> ---\n>> This is what I can come up with, collecting all the loose ends.\n>> I'm not sure if Mike wan't to have the Reported-By with a\n>> Signed-off-by ?\n>> The other question is, if the commit message summarizes the discussion\n>> well enough ?\n>>\n>> diff.c | 18 ++++++++++++++----\n>> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++\n>> 2 files changed, 41 insertions(+), 4 deletions(-)\n>> create mode 100755 t/t0028-diff-converted.sh\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 051761b..c264758 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n>> \t * differences.\n>> \t *\n>> \t * 2. At this point, the file is known to be modified,\n>> -\t * with the same mode and size, and the object\n>> -\t * name of one side is unknown. Need to inspect\n>> -\t * the identical contents.\n>> +\t * with the same mode and size, the object\n>> +\t * name of one side is unknown, or size comparison\n>> +\t * cannot be depended upon. Need to inspect the\n>> +\t * contents.\n>> \t */\n>> \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n>> \t !DIFF_FILE_VALID(p->two) ||\n>> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n>> \t (p->one->mode != p->two->mode) ||\n>> \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n>> \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n>> -\t (p->one->size != p->two->size) ||\n>> +\n>> +\t /*\n>> +\t * only if eol and other conversions are not involved,\n>> +\t * we can say that two contents of different sizes\n>> +\t * cannot be the same without checking their contents.\n>> +\t */\n>> +\t (!would_convert_to_git(p->one->path) &&\n>> +\t !would_convert_to_git(p->two->path) &&\n>> +\t (p->one->size != p->two->size)) ||\n>> +\n>> \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n>> \t\tp->skip_stat_unmatch_result = 1;\n>> \treturn p->skip_stat_unmatch_result;\n>> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh\n>> new file mode 100755\n>> index 0000000..3d5ab95\n>> --- /dev/null\n>> +++ b/t/t0028-diff-converted.sh\n>> @@ -0,0 +1,27 @@\n>> +#!/bin/sh\n>> +#\n>> +# Copyright (c) 2017 Mike Crowe\n>> +#\n>> +# These tests ensure that files changing line endings in the presence\n>> +# of .gitattributes to indicate that line endings should be ignored\n>> +# don't cause 'git diff' or 'git diff --quiet' to think that they have\n>> +# been changed.\n>> +\n>> +test_description='git diff with files that require CRLF conversion'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success setup '\n>> +\techo \"* text=auto\" >.gitattributes &&\n>> +\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n>> +\tgit add .gitattributes crlf.txt &&\n>> +\tgit commit -m \"initial\"\n>> +'\n>> +\n>> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n>> +\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n>> +\tgit status &&\n>> +\tgit diff --quiet\n>> +'\n>> +\n>> +test_done\n> \n> Hi Torsten,\n> \n> Thanks for investigating this.\n> \n> I think that you've simplified the test to the point where it doesn't\n> entirely prove the fix. Although you test the case where the file has\n> changed size, you don't test the case where it hasn't.\n> \n> Unfortunately that was the part of my test that could only reproduce the\n> problem with the sleeps. Maybe someone who understands how the cache works\n> fully could explain an alternative way to force the cache not to be used.\n> \n> Also, I think I've found a behaviour change with this fix. Consider:\n> \n> echo \"* text=auto\" >.gitattributes\n> printf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt\nThat should give\n\"Hello\\nWorld\\n\" in the index:\n\ngit add .gitattributes crlf.txt\nwarning: CRLF will be replaced by LF in ttt/crlf.txt.\nThe file will have its original line endings in your working directory.\ntb@mac:/tmp/ttt> git commit -m \"initial\"\n[master (root-commit) 354f657] initial\n 2 files changed, 3 insertions(+)\n create mode 100644 ttt/.gitattributes\n create mode 100644 ttt/crlf.txt\ntb@mac:/tmp/ttt> git ls-files --eol\ni/lf w/lf attr/text=auto .gitattributes\ni/lf w/crlf attr/text=auto crlf.txt\ntb@mac:/tmp/ttt>\n\n> git add .gitattributes crlf.txt\n> git commit -m \"initial\"\n> \n> printf \"\\r\\n\" >>crlf.txt\n> \n> With the above patch, both \"git diff\" and \"git diff --quiet\" report that\n> there are no changes. Previously Git would report the extra newline\n> correctly.\nWait a second.\nWhich extra newline \"correctly\" ?\n\nThe \"git diff\" command is about the changes which will be done to the index.\nRegardless if you have any of these in the working tree on disk:\n\n\"Hello\\nWorld\\n\"\n\"Hello\\nWorld\\r\\n\"\n\"Hello\\r\\nWorld\\n\"\n\"Hello\\r\\nWorld\\r\\n\"\n\n\"git status\" and \"git diff --quiet\"\nshould not report any changes.\n\nSo I don't know if there is a mis-understanding about \"git diff\" on your side,\nor if I miss something.\n\n\n"},{"id":"313083","messageId":"xmqqbmtjcyug.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170302142056.GB7821@mcrowe.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-02T18:33:59Z","receivedAt":"2017-03-02T18:34:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Crowe writes:\n\n> All the solutions presented so far do cause a small change in behaviour\n> when using git diff --quiet: they may now cause warning messages like:\n>\n> warning: CRLF will be replaced by LF in crlf.txt.\n> The file will have its original line endings in your working directory.\n\nThat is actually a good thing, I think. As the test modifies a file\nthat originally has \"Hello\\r\\nWorld\\r\\n\" in it to this:\n\n>> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n>> +\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n\nIf you did \"git add\" at this point, you would get the same warning,\nbecause the lack of CR on the second line could well be a mistake\nyou may want to notice and fix before going forward. Otherwise\nyou'd be losing information that _might_ matter to you (i.e. the\nfact that the first line had CRLF while the second had LF) and it is\nthe whole point of safe_crlf setting.\n\nI also think it is a good thing if \"git status\" reported this path\nas modified for the same reason (I didn't actually check if that is\nthe case).\n"},{"id":"313087","messageId":"xmqqmvd3d0ru.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-02T17:52:21Z","receivedAt":"2017-03-02T19:09:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/diff.c b/diff.c\n>> index 8c78fce49d..dc51dceb44 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n>> \t\t\ts->should_free = 1;\n>> \t\t\treturn 0;\n>> \t\t}\n>> -\t\tif (size_only)\n>> +\n>> +\t\t/*\n>> +\t\t * Even if the caller would be happy with getting\n>> +\t\t * only the size, we cannot return early at this\n>> +\t\t * point if the path requires us to run the content\n>> +\t\t * conversion.\n>> +\t\t */\n>> +\t\tif (!would_convert_to_git(s->path) && size_only)\n>> \t\t\treturn 0;\n>\n> The would_convert_to_git() function is a little expensive (it may have\n> to do an attribute lookup). It may be worth swapping the two halves of\n> the conditional here to get the short-circuit.\n\nYes. I think it makes sense.\n\n>> +\n>> +\t\t/*\n>> +\t\t * Note: this check uses xsize_t(st.st_size) that may\n>> +\t\t * not be the true size of the blob after it goes\n>> +\t\t * through convert_to_git(). This may not strictly be\n>> +\t\t * correct, but the whole point of big_file_threashold\n>\n> s/threashold/threshold/\n\nThanks. I felt there was something wrong and looked at the line\nthree times but somehow failed to spot exactly what was wrong ;-)\n\n>\n>> +\t\t * and is_binary check being that we want to avoid\n>> +\t\t * opening the file and inspecting the contents, this\n>> +\t\t * is probably fine.\n>> +\t\t */\n>> \t\tif ((flags & CHECK_BINARY) &&\n>> \t\t s->size > big_file_threshold && s->is_binary == -1) {\n>> \t\t\ts->is_binary = 1;\n>\n> I'm trying to think how this \"not strictly correct\" could bite us. \n\nNote that the comment is just documenting what I learned and thought\nwhile working on an unrelated thing that happened to be sitting next\nto it.\n\nNobody asks \"I am OK without the contents i.e. size-only\" and \"Can\nyou see if this is binary?\" at the same time (and if a caller did,\nit would never have got is_binary with the original code). s->size\nis still a copy of st.size at this point of the code (we have not\nactually updated it to the size of the real blob, which happens a\nbit later in the flow of this codepath where we actually slurp the\nthing in and run the conversion). So with or without this patch,\nthere shouldn't be any change in the behaviour wrt CHECK_BINARY.\n\n> For\n> line-ending conversion, I'd say that the before/after are going to be\n> approximately the same size. But what about something like LFS? If I\n> have a 600MB file that convert_to_git() filters into a short LFS\n> pointer, I think this changes the behavior. Before, we would diff the\n> pointer file, but now we'll get \"binary file changed\".\n\nTo be quite honest, I do not think this code should cater to LFS or\nany other conversion hack. They all install their own diff driver\nand they can tell diff_filespec_is_binary() if the thing is binary\nor not without falling back to this heuristics codepath.\n\nWhat is done here *is* inconsistent with what is done on the other\nside of if/else, that compares big_file_threshold with in-git size,\nnot in-working-tree size. That may want to be addressed, but I am\nnot sure if it is worth it.\n"},{"id":"313088","messageId":"20170302191201.nhhucl44dwmcqkb4@sigill.intra.peff.net","threadId":"45166","inReplyTo":"xmqqmvd3d0ru.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-02T19:12:01Z","receivedAt":"2017-03-02T19:13:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 02, 2017 at 09:52:21AM -0800, Junio C Hamano wrote:\n\n> >> +\t\t * and is_binary check being that we want to avoid\n> >> +\t\t * opening the file and inspecting the contents, this\n> >> +\t\t * is probably fine.\n> >> +\t\t */\n> >> \t\tif ((flags & CHECK_BINARY) &&\n> >> \t\t s->size > big_file_threshold && s->is_binary == -1) {\n> >> \t\t\ts->is_binary = 1;\n> >\n> > I'm trying to think how this \"not strictly correct\" could bite us. \n> \n> Note that the comment is just documenting what I learned and thought\n> while working on an unrelated thing that happened to be sitting next\n> to it.\n\nYeah, sorry, this is obviously not a blocker to your patch. I'm just\nwondering if there is more work needed.\n\n> To be quite honest, I do not think this code should cater to LFS or\n> any other conversion hack. They all install their own diff driver\n> and they can tell diff_filespec_is_binary() if the thing is binary\n> or not without falling back to this heuristics codepath.\n\nYeah, you're right, I was just being silly. Whatever configured the\nfilter already has an opportunity to give us this knowledge in a better\nway, and we should rely on that.\n\n-Peff\n"},{"id":"313089","messageId":"xmqqwpc7bjgi.fsf_-_@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net","subject":"[PATCH v2] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-02T18:51:41Z","receivedAt":"2017-03-02T19:14:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Callers of diff_populate_filespec() can choose to ask only for the\nsize of the blob without grabbing the blob data, and the function,\nafter running lstat() when the filespec points at a working tree\nfile, returns by copying the value in size field of the stat\nstructure into the size field of the filespec when this is the case.\n\nHowever, this short-cut cannot be taken if the contents from the\npath needs to go through convert_to_git(), whose resulting real blob\ndata may be different from what is in the working tree file.\n\nAs \"git diff --quiet\" compares the .size fields of filespec\nstructures to skip content comparison, this bug manifests as a\nfalse \"there are differences\" for a file that needs eol conversion,\nfor example.\n\nReported-by: Mike Crowe \nHelped-by: Torsten Bögershausen \nSigned-off-by: Junio C Hamano \n---\n\n * With \"test size_only to avoid more expensive would_convert call\"\n fix applied. Also the new test is now in t4xxx that it belongs\n to.\n\n diff.c | 19 ++++++++++++++++++-\n t/t4035-diff-quiet.sh | 9 +++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 059123c5dc..37e60ca601 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2783,8 +2783,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n-\t\tif (size_only)\n+\n+\t\t/*\n+\t\t * Even if the caller would be happy with getting\n+\t\t * only the size, we cannot return early at this\n+\t\t * point if the path requires us to run the content\n+\t\t * conversion.\n+\t\t */\n+\t\tif (size_only && !would_convert_to_git(s->path))\n \t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * Note: this check uses xsize_t(st.st_size) that may\n+\t\t * not be the true size of the blob after it goes\n+\t\t * through convert_to_git(). This may not strictly be\n+\t\t * correct, but the whole point of big_file_threshold\n+\t\t * and is_binary check being that we want to avoid\n+\t\t * opening the file and inspecting the contents, this\n+\t\t * is probably fine.\n+\t\t */\n \t\tif ((flags & CHECK_BINARY) &&\n \t\t s->size > big_file_threshold && s->is_binary == -1) {\n \t\t\ts->is_binary = 1;\ndiff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\nindex 461f4bb583..2f1737fcef 100755\n--- a/t/t4035-diff-quiet.sh\n+++ b/t/t4035-diff-quiet.sh\n@@ -152,4 +152,13 @@ test_expect_success 'git diff --quiet ignores stat-change only entries' '\n \ttest_expect_code 1 git diff --quiet\n '\n \n+test_expect_success 'git diff --quiet on a path that need conversion' '\n+\techo \"crlf.txt text=auto\" >.gitattributes &&\n+\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n+\tgit add .gitattributes crlf.txt &&\n+\n+\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n+\tgit diff --quiet crlf.txt\n+'\n+\n test_done\n-- \n2.12.0-352-gb05ccab5eb\n"},{"id":"313091","messageId":"4e53ca81-3328-349b-3612-a672ddc6a949@web.de","threadId":"45166","inReplyTo":"20170302142056.GB7821@mcrowe.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-02T18:20:08Z","receivedAt":"2017-03-02T19:19:58Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-03-02 15:20, Mike Crowe wrote:\n> ll the solutions presented so far do cause a small change in behaviour\n> when using git diff --quiet: they may now cause warning messages like:\n> \n> warning: CRLF will be replaced by LF in crlf.txt.\n> The file will have its original line endings in your working directory.\nAh,\nthat is not ideal.\nI can have a look at it later (or due to the weekend)\n\n"},{"id":"313096","messageId":"20170302200356.GA31318@mcrowe.com","threadId":"45166","inReplyTo":"xmqqbmtjcyug.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-03-02T20:03:56Z","receivedAt":"2017-03-02T20:04:10Z","isPatch":true,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"On Thursday 02 March 2017 at 10:33:59 -0800, Junio C Hamano wrote:\n> Mike Crowe writes:\n> \n> > All the solutions presented so far do cause a small change in behaviour\n> > when using git diff --quiet: they may now cause warning messages like:\n> >\n> > warning: CRLF will be replaced by LF in crlf.txt.\n> > The file will have its original line endings in your working directory.\n> \n> That is actually a good thing, I think. As the test modifies a file\n> that originally has \"Hello\\r\\nWorld\\r\\n\" in it to this:\n> \n> >> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n> >> +\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n> \n> If you did \"git add\" at this point, you would get the same warning,\n> because the lack of CR on the second line could well be a mistake\n> you may want to notice and fix before going forward. Otherwise\n> you'd be losing information that _might_ matter to you (i.e. the\n> fact that the first line had CRLF while the second had LF) and it is\n> the whole point of safe_crlf setting.\n\nWell, there is an argument that it's not very \"--quiet\" to emit this\nmessage. Especially when it didn't used to come out. However, I can\nunderstand that the message is useful if the line endings have changed\ndespite this.\n\nHowever, I can make the message appear from \"git diff --quiet\" even if the\nline endings have not changed. I merely need to touch a file where the line\nendings do not match the canonical representation in the repository first.\nUpon subsequent invocations of \"git diff --quiet\" the message does not come\nout. (Note that in this may not be reproducible in a script without\nsleeps.)\n\nPerhaps this interactive log will make things clearer:\n\n $ git init\n Initialized empty Git repository in /tmp/test/.git/\n $ echo \"* text=auto\" >.gitattributes\n $ printf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt\n $ git add .gitattributes crlf.txt\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n\nThe message was expected and useful there.\n\n $ git commit -m \"initial\"\n [master (root-commit) c3fb5a5] initial\n 2 files changed, 3 insertions(+)\n create mode 100644 .gitattributes\n create mode 100644 crlf.txt\n $ git diff --quiet\n $ touch crlf.txt\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n\nI didn't change the file - I just touched it. Why did the message come out here?\n\n $ git diff --quiet\n\nBut then it didn't here. Which is correct?\n\n $ printf \"Hello\\r\\nWorld\\n\" >crlf.txt\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n\nIf the line endings have genuinely changed then the message comes out every\ntime...\n\n $ git add crlf.txt\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n\n...until the file is added to the index. That's probably the right thing to\ndo.\n\n $ git diff --quiet\n $ touch crlf.txt\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n $ git diff --quiet\n\nBut once the file has been added the previous behaviour of only emitting\nthe message on the first time after the touch occurs.\n\n $ printf \"Hello\\r\\nWorld\\n\" >crlf.txt\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n $ git diff --quiet\n $ printf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n $ git diff --quiet\n warning: CRLF will be replaced by LF in crlf.txt.\n The file will have its original line endings in your working directory.\n\nHopefully that makes things a bit clearer.\n\nMike.\n"},{"id":"313197","messageId":"20170303170142.GA14150@mcrowe.com","threadId":"45166","inReplyTo":"5d92d3b8-f438-9be5-9742-22f8cd8fe03d@web.de","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Mike Crowe","fromEmail":"mac@mcrowe.com","sentAt":"2017-03-03T17:01:42Z","receivedAt":"2017-03-03T17:01:53Z","isPatch":true,"sender":{"key":"mac@mcrowe.com","avatar":"https://avatars.githubusercontent.com/u/93615?v=4"},"body":"Hi Torsten,\n\nYour patch has been superseded, but I thought I ought to answer your\nquestions rather than leave them hanging.\n\nOn Thursday 02 March 2017 at 19:17:00 +0100, Torsten Bögershausen wrote:\n> On 2017-03-01 22:25, Mike Crowe wrote:\n> > On Wednesday 01 March 2017 at 18:04:44 +0100, tboegi@web.de wrote:\n> >> From: Junio C Hamano \n> >>\n> >> git diff --quiet may take a short-cut to see if a file is changed\n> >> in the working tree:\n> >> Whenever the file size differs from what is recorded in the index,\n> >> the file is assumed to be changed and git diff --quiet returns\n> >> exit with code 1\n> >>\n> >> This shortcut must be suppressed whenever the line endings are converted\n> >> or a filter is in use.\n> >> The attributes say \"* text=auto\" and a file has\n> >> \"Hello\\nWorld\\n\" in the index with a length of 12.\n> >> The file in the working tree has \"Hello\\r\\nWorld\\r\\n\" with a length of 14.\n> >> (Or even \"Hello\\r\\nWorld\\n\").\n> >> In this case \"git add\" will not do any changes to the index, and\n> >> \"git diff -quiet\" should exit 0.\n> >>\n> >> Add calls to would_convert_to_git() before blindly saying that a different\n> >> size means different content.\n> >>\n> >> Reported-By: Mike Crowe \n> >> Signed-off-by: Torsten Bögershausen \n> >> ---\n> >> This is what I can come up with, collecting all the loose ends.\n> >> I'm not sure if Mike wan't to have the Reported-By with a\n> >> Signed-off-by ?\n> >> The other question is, if the commit message summarizes the discussion\n> >> well enough ?\n> >>\n> >> diff.c | 18 ++++++++++++++----\n> >> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++\n> >> 2 files changed, 41 insertions(+), 4 deletions(-)\n> >> create mode 100755 t/t0028-diff-converted.sh\n> >>\n> >> diff --git a/diff.c b/diff.c\n> >> index 051761b..c264758 100644\n> >> --- a/diff.c\n> >> +++ b/diff.c\n> >> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n> >> \t * differences.\n> >> \t *\n> >> \t * 2. At this point, the file is known to be modified,\n> >> -\t * with the same mode and size, and the object\n> >> -\t * name of one side is unknown. Need to inspect\n> >> -\t * the identical contents.\n> >> +\t * with the same mode and size, the object\n> >> +\t * name of one side is unknown, or size comparison\n> >> +\t * cannot be depended upon. Need to inspect the\n> >> +\t * contents.\n> >> \t */\n> >> \tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n> >> \t !DIFF_FILE_VALID(p->two) ||\n> >> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n> >> \t (p->one->mode != p->two->mode) ||\n> >> \t diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||\n> >> \t diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||\n> >> -\t (p->one->size != p->two->size) ||\n> >> +\n> >> +\t /*\n> >> +\t * only if eol and other conversions are not involved,\n> >> +\t * we can say that two contents of different sizes\n> >> +\t * cannot be the same without checking their contents.\n> >> +\t */\n> >> +\t (!would_convert_to_git(p->one->path) &&\n> >> +\t !would_convert_to_git(p->two->path) &&\n> >> +\t (p->one->size != p->two->size)) ||\n> >> +\n> >> \t !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n> >> \t\tp->skip_stat_unmatch_result = 1;\n> >> \treturn p->skip_stat_unmatch_result;\n> >> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh\n> >> new file mode 100755\n> >> index 0000000..3d5ab95\n> >> --- /dev/null\n> >> +++ b/t/t0028-diff-converted.sh\n> >> @@ -0,0 +1,27 @@\n> >> +#!/bin/sh\n> >> +#\n> >> +# Copyright (c) 2017 Mike Crowe\n> >> +#\n> >> +# These tests ensure that files changing line endings in the presence\n> >> +# of .gitattributes to indicate that line endings should be ignored\n> >> +# don't cause 'git diff' or 'git diff --quiet' to think that they have\n> >> +# been changed.\n> >> +\n> >> +test_description='git diff with files that require CRLF conversion'\n> >> +\n> >> +. ./test-lib.sh\n> >> +\n> >> +test_expect_success setup '\n> >> +\techo \"* text=auto\" >.gitattributes &&\n> >> +\tprintf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt &&\n> >> +\tgit add .gitattributes crlf.txt &&\n> >> +\tgit commit -m \"initial\"\n> >> +'\n> >> +\n> >> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '\n> >> +\tprintf \"Hello\\r\\nWorld\\n\" >crlf.txt &&\n> >> +\tgit status &&\n> >> +\tgit diff --quiet\n> >> +'\n> >> +\n> >> +test_done\n> >\n\n[snip]\n\n> > Also, I think I've found a behaviour change with this fix. Consider:\n> > \n> > echo \"* text=auto\" >.gitattributes\n> > printf \"Hello\\r\\nWorld\\r\\n\" >crlf.txt\n> That should give\n> \"Hello\\nWorld\\n\" in the index:\n> \n> git add .gitattributes crlf.txt\n> warning: CRLF will be replaced by LF in ttt/crlf.txt.\n> The file will have its original line endings in your working directory.\n> tb@mac:/tmp/ttt> git commit -m \"initial\"\n> [master (root-commit) 354f657] initial\n> 2 files changed, 3 insertions(+)\n> create mode 100644 ttt/.gitattributes\n> create mode 100644 ttt/crlf.txt\n> tb@mac:/tmp/ttt> git ls-files --eol\n> i/lf w/lf attr/text=auto .gitattributes\n> i/lf w/crlf attr/text=auto crlf.txt\n> tb@mac:/tmp/ttt>\n> \n> > git add .gitattributes crlf.txt\n> > git commit -m \"initial\"\n> > \n> > printf \"\\r\\n\" >>crlf.txt\n> > \n> > With the above patch, both \"git diff\" and \"git diff --quiet\" report that\n> > there are no changes. Previously Git would report the extra newline\n> > correctly.\n> Wait a second.\n> Which extra newline \"correctly\" ?\n\nThe extra newline I appended with the printf \"\\r\\n\" >> crlf.txt\n\n> The \"git diff\" command is about the changes which will be done to the index.\n> Regardless if you have any of these in the working tree on disk:\n> \n> \"Hello\\nWorld\\n\"\n> \"Hello\\nWorld\\r\\n\"\n> \"Hello\\r\\nWorld\\n\"\n> \"Hello\\r\\nWorld\\r\\n\"\n> \n> \"git status\" and \"git diff --quiet\"\n> should not report any changes.\n\nBut I didn't have any of those. I ended up with:\n\n \"Hello\\nWorld\\n\"\n\nin the index, and\n\n \"Hello\\r\\nWorld\\r\\n\\r\\n\"\n\nin the working tree, but the extra newline was not reported by git diff.\n\n> So I don't know if there is a mis-understanding about \"git diff\" on your side,\n> or if I miss something.\n\nI don't think it matters any more since Junio's patch didn't suffer from\nthis problem.\n\nThanks.\n\nMike.\n"},{"id":"313209","messageId":"ae2b144a-5e39-8178-5161-1d8eb673b6f0@web.de","threadId":"45166","inReplyTo":"20170302200356.GA31318@mcrowe.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-03T17:02:56Z","receivedAt":"2017-03-03T17:35:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Understood, thanks for the explanation.\n\nquiet is not quite any more..\n\nDoes the following fix help ?\n\n--- a/diff.c\n+++ b/diff.c\n@@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,\nunsigned int flags)\n enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL\n ? SAFE_CRLF_WARN\n : safe_crlf);\n+ if (size_only)\n+ crlf_warn = SAFE_CRLF_FALSE;\n\n\n"},{"id":"313211","messageId":"xmqq37eu2qxl.fsf@junio-linux.mtv.corp.google.com","threadId":"45166","inReplyTo":"ae2b144a-5e39-8178-5161-1d8eb673b6f0@web.de","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T17:47:18Z","receivedAt":"2017-03-03T17:49:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Understood, thanks for the explanation.\n>\n> quiet is not quite any more..\n>\n> Does the following fix help ?\n>\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,\n> unsigned int flags)\n> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL\n> ? SAFE_CRLF_WARN\n> : safe_crlf);\n> + if (size_only)\n> + crlf_warn = SAFE_CRLF_FALSE;\n\nIf you were to go this route, it may be sufficient to change its\ninitialization from WARN to FALSE _unconditionally_, because this\nfunction uses the convert_to_git() only to _show_ the differences by\ncomputing canonical form out of working tree contents, and the\nconversion is not done to _write_ into object database to create a\nnew object.\n\nHaving size_only here is not a sign of getting --quiet passed from\nthe command line, by the way.\n"},{"id":"313260","messageId":"9c9eeb35-e1c1-ec46-1d85-ef6a05886880@web.de","threadId":"45166","inReplyTo":"xmqq37eu2qxl.fsf@junio-linux.mtv.corp.google.com","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-04T06:25:52Z","receivedAt":"2017-03-04T06:58:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-03-03 18:47, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n> \n>> Understood, thanks for the explanation.\n>>\n>> quiet is not quite any more..\n>>\n>> Does the following fix help ?\n>>\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,\n>> unsigned int flags)\n>> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL\n>> ? SAFE_CRLF_WARN\n>> : safe_crlf);\n>> + if (size_only)\n>> + crlf_warn = SAFE_CRLF_FALSE;\n> \n> If you were to go this route, it may be sufficient to change its\n> initialization from WARN to FALSE _unconditionally_, because this\n> function uses the convert_to_git() only to _show_ the differences by\n> computing canonical form out of working tree contents, and the\n> conversion is not done to _write_ into object database to create a\n> new object.\nHm, since when (is it not used) ?\n\nI thought that it is needed to support the safecrlf handling introduced in\n21e5ad50fc5e7277c74cfbb3cf6502468e840f86\nAuthor: Steffen Prohaska \nDate: Wed Feb 6 12:25:58 2008 +0100\n\n safecrlf: Add mechanism to warn about irreversible crlf conversions\n\n-------------\nThe SAFE_CRLF_FAIL was converted into WARN here:\ncommit 5430bb283b478991a979437a79e10dcbb6f20e28\nAuthor: Junio C Hamano \nDate: Mon Jun 24 14:35:04 2013 -0700\n\n diff: demote core.safecrlf=true to core.safecrlf=warn\n\n Otherwise the user will not be able to start to guess where in the\n contents in the working tree the offending unsafe CR lies.\n------------\n\nMy understanding is that we don't want to break the safecrlf feature,\nbut after applying\n\ndiff --git a/diff.c b/diff.c\nindex a628ac3a95..a05d88dd9f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2820,12 +2820,10 @@ int diff_populate_filespec(struct diff_filespec *s,\nunsigned int flags)\n int size_only = flags & CHECK_SIZE_ONLY;\n int err = 0;\n /*\n- * demote FAIL to WARN to allow inspecting the situation\n- * instead of refusing.\n+ * Don't use FAIL or WARN as this code is not called when _writing_\n+ * into object database to create a new object.\n */\n- enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL\n- ? SAFE_CRLF_WARN\n- : safe_crlf);\n+ enum safe_crlf crlf_warn = SAFE_CRLF_FALSE;\n\n\nNone of the test cases in t0020--t0027 fails or complain about missing warnings.\nDoes this all means that, looking back, 5430bb283b478991 could have been more\naggressive and could have used SAFE_CRLF_FALSE ?\nAnd we can do this change now?\n\n(If the answer is yes, we don't need to deal with the problem below)\n> Having size_only here is not a sign of getting --quiet passed from\n> the command line, by the way.\n> \n\n"},{"id":"313283","messageId":"xmqq4lz87r01.fsf@gitster.mtv.corp.google.com","threadId":"45166","inReplyTo":"9c9eeb35-e1c1-ec46-1d85-ef6a05886880@web.de","subject":"Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-04T19:59:10Z","receivedAt":"2017-03-04T19:59:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>>> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL\n>>> ? SAFE_CRLF_WARN\n>>> : safe_crlf);\n>>> + if (size_only)\n>>> + crlf_warn = SAFE_CRLF_FALSE;\n>> \n>> If you were to go this route, it may be sufficient to change its\n>> initialization from WARN to FALSE _unconditionally_, because this\n>> function uses the convert_to_git() only to _show_ the differences by\n>> computing canonical form out of working tree contents, and the\n>> conversion is not done to _write_ into object database to create a\n>> new object.\n>\n> Hm, since when (is it not used) ?\n\nSince forever, but my statement above said \"this function\", which may\nhave confused you, where it could have said diff_populate_filespec().\n\nSurely it is possible for somebody to diff_populate_filespec(s, 0)\nand then call hash_sha1_file(s->data, s->size, \"blob\", ...) to write\nthe data into the object database to create a new object. But that\nsounds really crazy, no?\n\n> The SAFE_CRLF_FAIL was converted into WARN here:\n> commit 5430bb283b478991a979437a79e10dcbb6f20e28\n> Author: Junio C Hamano \n> Date: Mon Jun 24 14:35:04 2013 -0700\n>\n> diff: demote core.safecrlf=true to core.safecrlf=warn\n\nYes.\n\n> Does this all means that, looking back, 5430bb283b478991 could have been more\n> aggressive and could have used SAFE_CRLF_FALSE ?\n\nThat is pretty much the statement, to which you said \"since when\",\nsuspects.\n\n> And we can do this change now?\n\nI am not sure. The conversion the safe-crlf code does is unsafe and\nit is a disservice to users not to warn whenever we notice they are\nrisking information loss. Maybe time they run \"git diff\" is not a\ngood time to warn, as they may not be actually adding the file\nas-is, but if warning against information loss at \"git diff\" time is\nimportant enough, the I think that should not be squelched by the\n\"--quiet\" option, which is about \"do not show the patch text\noutput\". It should not be taken as \"do not diagnose any errors\".\n\n"}]}