{"thread":{"id":"44906","subject":"\"git diff --ignore-space-change --stat\" lists files with only whitespace differences as \"changed\"","startedAt":"2017-01-18T02:18:19Z","lastAt":"2017-01-18T23:32:02Z","messageCount":5,"participants":["Matt McCutchen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"309609","messageId":"1484704915.2096.16.camel@mattmccutchen.net","threadId":"44906","inReplyTo":null,"subject":"\"git diff --ignore-space-change --stat\" lists files with only whitespace differences as \"changed\"","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2017-01-18T02:01:55Z","receivedAt":"2017-01-18T02:18:19Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"A bug report: I noticed that \"git diff --ignore-space-change --stat\"\nlists files with only whitespace differences as having changed with 0\ndiffering lines.  This is inconsistent with the behavior without --\nstat, which doesn't list such files at all.  (Same behavior with all\nthe --ignore*space* flags.)  I can reproduce this with the current\n\"next\", af746e4.  Quick test case:\n\necho ' ' >test1 && echo '  ' >test2 &&\ngit diff --stat --no-index --ignore-space-change test1 test2\n\nThis caused me some inconvenience in the following scenario: I was\nreading a commit diff that had a bulk license change in all files\ncombined with code changes.  I attempted to revert the bulk license\nchange locally using \"sed\" to more easily read the code diff, but my\nreversion left some whitespace diffs where the original files had\ninconsistent whitespace.  So the diffstat after my reversion was\ncluttered with these \"0\" entries.\n\nRegards,\nMatt\n"},{"id":"309617","messageId":"20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net","threadId":"44906","inReplyTo":"1484704915.2096.16.camel@mattmccutchen.net","subject":"Re: \"git diff --ignore-space-change --stat\" lists files with only whitespace differences as \"changed\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-18T11:17:05Z","receivedAt":"2017-01-18T11:19:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:\n\n> A bug report: I noticed that \"git diff --ignore-space-change --stat\"\n> lists files with only whitespace differences as having changed with 0\n> differing lines.  This is inconsistent with the behavior without --\n> stat, which doesn't list such files at all.  (Same behavior with all\n> the --ignore*space* flags.)  I can reproduce this with the current\n> \"next\", af746e4.  Quick test case:\n\nHmm. This is pretty easy to do naively, but the special-casing for\naddition/deletion (which I think we _do_ need, and which certainly we\nfail t4205 without) makes me feel dirty. I'd worry there are other\ncases, too (perhaps renames?). And I also notice that the\nbinary-diffstat code path just above my changes explicitly creates 0/0\ndiffstats, but I'm not even sure how one would trigger that.\n\nSo I dunno. A sensible rule to me is \"iff -p would show a diff header,\nthen --stat should mention it\". I think we'd want to somehow extract the\nlogic from builtin_diff() and reuse it.\n\n---\ndiff --git a/diff.c b/diff.c\nindex e2eb6d66a..57ff5c1dc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2105,17 +2105,20 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o\n \tgather_dirstat(options, &dir, changed, \"\", 0);\n }\n \n+static void free_diffstat_file(struct diffstat_file *f)\n+{\n+\tif (f->name != f->print_name)\n+\t\tfree(f->print_name);\n+\tfree(f->name);\n+\tfree(f->from_name);\n+\tfree(f);\n+}\n+\n static void free_diffstat_info(struct diffstat_t *diffstat)\n {\n \tint i;\n-\tfor (i = 0; i < diffstat->nr; i++) {\n-\t\tstruct diffstat_file *f = diffstat->files[i];\n-\t\tif (f->name != f->print_name)\n-\t\t\tfree(f->print_name);\n-\t\tfree(f->name);\n-\t\tfree(f->from_name);\n-\t\tfree(f);\n-\t}\n+\tfor (i = 0; i < diffstat->nr; i++)\n+\t\tfree_diffstat_file(diffstat->files[i]);\n \tfree(diffstat->files);\n }\n \n@@ -2603,6 +2606,23 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tif (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n \t\t\t\t  &xpp, &xecfg))\n \t\t\tdie(\"unable to generate diffstat for %s\", one->path);\n+\n+\t\t/*\n+\t\t * Omit diffstats where nothing changed. Even if\n+\t\t * !same_contents, this might be the case due to ignoring\n+\t\t * whitespace changes, etc.\n+\t\t *\n+\t\t * But note that we special-case additions and deletions,\n+\t\t * as adding an empty file, for example, is still of interest.\n+\t\t */\n+\t\tif (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {\n+\t\t\tstruct diffstat_file *file =\n+\t\t\t\tdiffstat->files[diffstat->nr - 1];\n+\t\t\tif (!file->added && !file->deleted) {\n+\t\t\t\tfree_diffstat_file(file);\n+\t\t\t\tdiffstat->nr--;\n+\t\t\t}\n+\t\t}\n \t}\n \n \tdiff_free_filespec_data(one);\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 289806d0c..2805db411 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -736,7 +736,7 @@ test_expect_success 'checkdiff allows new blank lines' '\n \n cat <<EOF >expect\n EOF\n-test_expect_success 'whitespace-only changes not reported' '\n+test_expect_success 'whitespace-only changes not reported (diff)' '\n \tgit reset --hard &&\n \techo >x \"hello world\" &&\n \tgit add x &&\n@@ -746,6 +746,12 @@ test_expect_success 'whitespace-only changes not reported' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'whitespace-only changes not reported (diffstat)' '\n+\t# reuse state from previous test\n+\tgit diff --stat -b >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat <<EOF >expect\n diff --git a/x b/z\n similarity index NUM%\n"},{"id":"309667","messageId":"20170118210821.xugr6jnvzgoxpynb@sigill.intra.peff.net","threadId":"44906","inReplyTo":"xmqqvatc3x3r.fsf@gitster.mtv.corp.google.com","subject":"Re: \"git diff --ignore-space-change --stat\" lists files with only whitespace differences as \"changed\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-18T21:08:21Z","receivedAt":"2017-01-18T21:08:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So I dunno. A sensible rule to me is \"iff -p would show a diff header,\n> > then --stat should mention it\".\n> \n> True but tricky (you need a better definition of \"a diff header\").\n> \n> In addition to a new and deleted file, does a file whose executable\n> bit was flipped need mention?  If so, then \"diff --git\" is the diff\n> header in the above.  Otherwise \"@@ ... @@\", iow, \"iff -p would show\n> any hunk\".\n> \n> I think the patch implements the latter, which I think is sensible.\n\nI would think the former is more sensible (and is what my patch is\nworking towards). Doing:\n\n  >empty\n  git add empty\n  git diff --cached\n\nshows a \"diff --git\" header, but no hunk. I think it should show a\ndiffstat (and does with my patch).\n\nI was thinking the rule should be something like:\n\n  if (p->status == DIFF_STATUS_MODIFIED &&\n      !file->added && !file->deleted))\n\nand otherwise include the entry, since it would be an add, delete,\nrename, etc, which carries useful information.\n\nThough a pure rename would not hit this code path at all, I would think\n(it would not trigger \"!same_contents\"). And a rename where there was a\nwhitespace only change probably _should_ be omitted from \"-b\".\n\nDitto for a pure mode change, I think. We do not run the contents\nthrough diff at all, so it does not hit this code path.\n\n-Peff\n"},{"id":"309680","messageId":"xmqqvatc3x3r.fsf@gitster.mtv.corp.google.com","threadId":"44906","inReplyTo":"20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net","subject":"Re: \"git diff --ignore-space-change --stat\" lists files with only whitespace differences as \"changed\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-18T20:57:12Z","receivedAt":"2017-01-18T21:52:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I dunno. A sensible rule to me is \"iff -p would show a diff header,\n> then --stat should mention it\".\n\nTrue but tricky (you need a better definition of \"a diff header\").\n\nIn addition to a new and deleted file, does a file whose executable\nbit was flipped need mention?  If so, then \"diff --git\" is the diff\nheader in the above.  Otherwise \"@@ ... @@\", iow, \"iff -p would show\nany hunk\".\n\nI think the patch implements the latter, which I think is sensible.\n\n> +\t\t/*\n> +\t\t * Omit diffstats where nothing changed. Even if\n> +\t\t * !same_contents, this might be the case due to ignoring\n> +\t\t * whitespace changes, etc.\n> +\t\t *\n> +\t\t * But note that we special-case additions and deletions,\n> +\t\t * as adding an empty file, for example, is still of interest.\n> +\t\t */\n> +\t\tif (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {\n> +\t\t\tstruct diffstat_file *file =\n> +\t\t\t\tdiffstat->files[diffstat->nr - 1];\n> +\t\t\tif (!file->added && !file->deleted) {\n> +\t\t\t\tfree_diffstat_file(file);\n> +\t\t\t\tdiffstat->nr--;\n> +\t\t\t}\n> +\t\t}\n>  \t}\n"},{"id":"309702","messageId":"xmqqtw8w2ewj.fsf@gitster.mtv.corp.google.com","threadId":"44906","inReplyTo":"20170118210821.xugr6jnvzgoxpynb@sigill.intra.peff.net","subject":"Re: \"git diff --ignore-space-change --stat\" lists files with only whitespace differences as \"changed\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-18T22:15:40Z","receivedAt":"2017-01-18T23:32:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > So I dunno. A sensible rule to me is \"iff -p would show a diff header,\n>> > then --stat should mention it\".\n>> \n>> True but tricky (you need a better definition of \"a diff header\").\n>> \n>> In addition to a new and deleted file, does a file whose executable\n>> bit was flipped need mention?  If so, then \"diff --git\" is the diff\n>> header in the above.  Otherwise \"@@ ... @@\", iow, \"iff -p would show\n>> any hunk\".\n>> \n>> I think the patch implements the latter, which I think is sensible.\n>\n> I would think the former is more sensible (and is what my patch is\n> working towards).\n\nDoh (yes, \"diff --git\" was what I meant).  As a mode-flipping patch\ndoes not have hunk but does show the header, it wants to be included\nin \"git diff --stat\" output, I would think, independent of -b issue.\nIn fact\n\n\tchmod +x README.md\n\tgit diff --stat\n\ndoes show a 0-line diffstat.\n\n\n> Doing:\n>\n>   >empty\n>   git add empty\n>   git diff --cached\n>\n> shows a \"diff --git\" header, but no hunk. I think it should show a\n> diffstat (and does with my patch).\n>\n> I was thinking the rule should be something like:\n>\n>   if (p->status == DIFF_STATUS_MODIFIED &&\n>       !file->added && !file->deleted))\n>\n> and otherwise include the entry, since it would be an add, delete,\n> rename, etc, which carries useful information.\n>\n> Though a pure rename would not hit this code path at all, I would think\n> (it would not trigger \"!same_contents\"). And a rename where there was a\n> whitespace only change probably _should_ be omitted from \"-b\".\n>\n> Ditto for a pure mode change, I think. We do not run the contents\n> through diff at all, so it does not hit this code path.\n>\n> -Peff\n"}]}