{"thread":{"id":"31850","subject":"Fix \"git diff --stat\" for interesting - but empty - file changes","startedAt":"2012-10-17T17:00:37Z","lastAt":"2012-10-17T19:35:48Z","messageCount":3,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"201430","messageId":"CA+55aFz88GPJcfMSqiyY+u0Cdm48bEyrsTGxHVJbGsYsDg=Q5w@mail.gmail.com","threadId":"31850","inReplyTo":null,"subject":"Fix \"git diff --stat\" for interesting - but empty - file changes","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-10-17T17:00:37Z","receivedAt":"2012-10-17T17:00:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"The behavior of \"git diff --stat\" is rather odd for files that have\nzero lines of changes: it will discount them entirely unless they were\nrenames.\n\nWhich means that the stat output will simply not show files that only\nhad \"other\" changes: they were created or deleted, or their mode was\nchanged.\n\nNow, those changes do show up in the summary, but so do renames, so\nthe diffstat logic is inconsistent. Why does it show renames with zero\nlines changed, but not mode changes or added files with zero lines\nchanged?\n\nSo change the logic to not check for \"is_renamed\", but for\n\"is_interesting\" instead, where \"interesting\" is judged to be any\naction but a pure data change (because a pure data change with zero\ndata changed really isn't worth showing, if we ever get one in our\ndiffpairs).\n\nSo if you did\n\n   chmod +x Makefile\n   git diff --stat\n\nbefore, it would show empty (\" 0 files changed\"), with this it shows\n\n Makefile | 0\n 1 file changed, 0 insertions(+), 0 deletions(-)\n\nwhich I think is a more correct diffstat (and then with \"--summary\" it\nshows *what* the metadata change to Makefile was - this is completely\nconsistent with our handling of renamed files).\n\nSide note: the old behavior was *really* odd. With no changes at all,\n\"git diff --stat\" output was empty. With just a chmod, it said \"0\nfiles changed\". No way is our legacy behavior sane.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nThis was triggered by kernel developers not noticing that they had\nadded zero-sized files, because those additions never showed up in the\ndiffstat.\n\nNOTE! This does break two of our tests, so we clearly did this on\npurpose, or at least tested for it. I just uncommented the subtests\nthat this makes irrelevant, and changed the output of another one.\n\nAnother test was simply buggy. It used \"git diff --root cmit\", and\nthought that would be the diff against root. It isn't, and never has\nbeen. It just happened to give the same (no file) output before.\nFixing --stat to show new files showed how buggy the test was. The\n\"--root\" thing matters for \"git show\" or \"git log\" (when showing a\nroot commit) and for \"git diff-tree\" with a single tree.\n\nMaybe we would *want* to make \"git diff --root <cmit>\" be the \"diff\nbetween root and cmit\", but that's not what it actually is.\n\nComments?\n\n\n diff.c                        | 25 +++++++++++++----------\n t/t4006-diff-mode.sh          | 46 +++++++++++++++++++++----------------------\n t/t4049-diff-stat-count.sh    |  3 ++-\n t/t4205-log-pretty-formats.sh |  4 ++--\n 4 files changed, 42 insertions(+), 36 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 35d3f073859a..95bbad66c686 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1300,6 +1300,7 @@ struct diffstat_t {\n \t\tunsigned is_unmerged:1;\n \t\tunsigned is_binary:1;\n \t\tunsigned is_renamed:1;\n+\t\tunsigned is_interesting:1;\n \t\tuintmax_t added, deleted;\n \t} **files;\n };\n@@ -1469,7 +1470,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tfor (i = 0; (i < count) && (i < data->nr); i++) {\n \t\tstruct diffstat_file *file = data->files[i];\n \t\tuintmax_t change = file->added + file->deleted;\n-\t\tif (!data->files[i]->is_renamed &&\n+\t\tif (!data->files[i]->is_interesting &&\n \t\t\t (change == 0)) {\n \t\t\tcount++; /* not shown == room for one more */\n \t\t\tcontinue;\n@@ -1590,7 +1591,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tuintmax_t deleted = data->files[i]->deleted;\n \t\tint name_len;\n \n-\t\tif (!data->files[i]->is_renamed &&\n+\t\tif (!data->files[i]->is_interesting &&\n \t\t\t (added + deleted == 0)) {\n \t\t\ttotal_files--;\n \t\t\tcontinue;\n@@ -1669,7 +1670,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tfor (i = count; i < data->nr; i++) {\n \t\tuintmax_t added = data->files[i]->added;\n \t\tuintmax_t deleted = data->files[i]->deleted;\n-\t\tif (!data->files[i]->is_renamed &&\n+\t\tif (!data->files[i]->is_interesting &&\n \t\t\t (added + deleted == 0)) {\n \t\t\ttotal_files--;\n \t\t\tcontinue;\n@@ -1697,7 +1698,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n \n \t\tif (data->files[i]->is_unmerged)\n \t\t\tcontinue;\n-\t\tif (!data->files[i]->is_renamed && (added + deleted == 0)) {\n+\t\tif (!data->files[i]->is_interesting && (added + deleted == 0)) {\n \t\t\ttotal_files--;\n \t\t} else if (!data->files[i]->is_binary) { /* don't count bytes */\n \t\t\tadds += added;\n@@ -2397,13 +2398,20 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t     struct diff_filespec *two,\n \t\t\t     struct diffstat_t *diffstat,\n \t\t\t     struct diff_options *o,\n-\t\t\t     int complete_rewrite)\n+\t\t\t     struct diff_filepair *p)\n {\n \tmmfile_t mf1, mf2;\n \tstruct diffstat_file *data;\n \tint same_contents;\n+\tint complete_rewrite = 0;\n+\n+\tif (!DIFF_PAIR_UNMERGED(p)) {\n+\t\tif (p->status == DIFF_STATUS_MODIFIED && p->score)\n+\t\t\tcomplete_rewrite = 1;\n+\t}\n \n \tdata = diffstat_add(diffstat, name_a, name_b);\n+\tdata->is_interesting = p->status != 0;\n \n \tif (!one || !two) {\n \t\tdata->is_unmerged = 1;\n@@ -3114,11 +3122,10 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n {\n \tconst char *name;\n \tconst char *other;\n-\tint complete_rewrite = 0;\n \n \tif (DIFF_PAIR_UNMERGED(p)) {\n \t\t/* unmerged */\n-\t\tbuiltin_diffstat(p->one->path, NULL, NULL, NULL, diffstat, o, 0);\n+\t\tbuiltin_diffstat(p->one->path, NULL, NULL, NULL, diffstat, o, p);\n \t\treturn;\n \t}\n \n@@ -3131,9 +3138,7 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n \tdiff_fill_sha1_info(p->one);\n \tdiff_fill_sha1_info(p->two);\n \n-\tif (p->status == DIFF_STATUS_MODIFIED && p->score)\n-\t\tcomplete_rewrite = 1;\n-\tbuiltin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite);\n+\tbuiltin_diffstat(name, other, p->one, p->two, diffstat, o, p);\n }\n \n static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex 3d4b1ba23f9e..05911492ca6d 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -32,28 +32,28 @@ test_expect_success 'prepare binary file' '\n \tgit commit -m binbin\n '\n \n-test_expect_success '--stat output after text chmod' '\n-\ttest_chmod -x rezrov &&\n-\techo \" 0 files changed\" >expect &&\n-\tgit diff HEAD --stat >actual &&\n-\ttest_i18ncmp expect actual\n-'\n-\n-test_expect_success '--shortstat output after text chmod' '\n-\tgit diff HEAD --shortstat >actual &&\n-\ttest_i18ncmp expect actual\n-'\n-\n-test_expect_success '--stat output after binary chmod' '\n-\ttest_chmod +x binbin &&\n-\techo \" 0 files changed\" >expect &&\n-\tgit diff HEAD --stat >actual &&\n-\ttest_i18ncmp expect actual\n-'\n-\n-test_expect_success '--shortstat output after binary chmod' '\n-\tgit diff HEAD --shortstat >actual &&\n-\ttest_i18ncmp expect actual\n-'\n+# test_expect_success '--stat output after text chmod' '\n+# \ttest_chmod -x rezrov &&\n+# \techo \" 0 files changed\" >expect &&\n+# \tgit diff HEAD --stat >actual &&\n+#\ttest_i18ncmp expect actual\n+# '\n+#\n+# test_expect_success '--shortstat output after text chmod' '\n+# \tgit diff HEAD --shortstat >actual &&\n+# \ttest_i18ncmp expect actual\n+# '\n+#\n+# test_expect_success '--stat output after binary chmod' '\n+# \ttest_chmod +x binbin &&\n+# \techo \" 0 files changed\" >expect &&\n+# \tgit diff HEAD --stat >actual &&\n+# \ttest_i18ncmp expect actual\n+# '\n+#\n+# test_expect_success '--shortstat output after binary chmod' '\n+# \tgit diff HEAD --shortstat >actual &&\n+# \ttest_i18ncmp expect actual\n+# '\n \n test_done\ndiff --git a/t/t4049-diff-stat-count.sh b/t/t4049-diff-stat-count.sh\nindex b41eb61ca8b1..7b3ef00533f7 100755\n--- a/t/t4049-diff-stat-count.sh\n+++ b/t/t4049-diff-stat-count.sh\n@@ -16,7 +16,8 @@ test_expect_success setup '\n \tcat >expect <<-\\EOF\n \t a | 1 +\n \t b | 1 +\n-\t 2 files changed, 2 insertions(+)\n+\t ...\n+\t 4 files changed, 2 insertions(+)\n \tEOF\n \tgit diff --stat --stat-count=2 >actual &&\n \ttest_i18ncmp expect actual\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 2c45de7aeaf2..98a43d457a3d 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -85,7 +85,7 @@ test_expect_success 'NUL termination' '\n \n test_expect_success 'NUL separation with --stat' '\n \tstat0_part=$(git diff --stat HEAD^ HEAD) &&\n-\tstat1_part=$(git diff --stat --root HEAD^) &&\n+\tstat1_part=$(git diff-tree --no-commit-id --stat --root HEAD^) &&\n \tprintf \"add bar\\n$stat0_part\\n\\0initial\\n$stat1_part\\n\" >expected &&\n \tgit log -z --stat --pretty=\"format:%s\" >actual &&\n \ttest_i18ncmp expected actual\n@@ -93,7 +93,7 @@ test_expect_success 'NUL separation with --stat' '\n \n test_expect_failure 'NUL termination with --stat' '\n \tstat0_part=$(git diff --stat HEAD^ HEAD) &&\n-\tstat1_part=$(git diff --stat --root HEAD^) &&\n+\tstat1_part=$(git diff-tree --no-commit-id --stat --root HEAD^) &&\n \tprintf \"add bar\\n$stat0_part\\n\\0initial\\n$stat1_part\\n\\0\" >expected &&\n \tgit log -z --stat --pretty=\"tformat:%s\" >actual &&\n \ttest_i18ncmp expected actual\n"},{"id":"201441","messageId":"7v4nltoswd.fsf@alter.siamese.dyndns.org","threadId":"31850","inReplyTo":"CA+55aFz88GPJcfMSqiyY+u0Cdm48bEyrsTGxHVJbGsYsDg=Q5w@mail.gmail.com","subject":"Re: Fix \"git diff --stat\" for interesting - but empty - file changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-17T18:28:02Z","receivedAt":"2012-10-17T18:28:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> So if you did\n>\n>    chmod +x Makefile\n>    git diff --stat\n>\n> before, it would show empty (\" 0 files changed\"), with this it shows\n>\n>  Makefile | 0\n>  1 file changed, 0 insertions(+), 0 deletions(-)\n>\n> which I think is a more correct diffstat (and then with \"--summary\" it\n> shows *what* the metadata change to Makefile was - this is completely\n> consistent with our handling of renamed files).\n>\n> Side note: the old behavior was *really* odd. With no changes at all,\n> \"git diff --stat\" output was empty. With just a chmod, it said \"0\n> files changed\". No way is our legacy behavior sane.\n>\n> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n> ---\n>\n> This was triggered by kernel developers not noticing that they had\n> added zero-sized files, because those additions never showed up in the\n> diffstat.\n> ...\n> Comments?\n\nI think listing a file whose content remain unchanged with 0 as the\nnumber of lines affected makes sense, and it will mesh well with\nDuy's\n\n  http://thread.gmane.org/gmane.comp.version-control.git/207749\n\nI first wondered if we would get a division-by-zero while scaling\nthe graph, but we do not scale smaller numbers up to fill the\ncolumns, so we should be safe.\n\nThese days, we omit 0 insertions and 0 deletions, so I am not sure\nwhat you should get for this case, though:\n\n>  Makefile | 0\n>  1 file changed, 0 insertions(+), 0 deletions(-)\n\nShould we just say \"1 file changed\"?\n"},{"id":"201445","messageId":"CA+55aFzChDPv3Vu5D9rXmXvQmxoxsxgZYVEyf9CaUmYP+Xs0OA@mail.gmail.com","threadId":"31850","inReplyTo":"7v4nltoswd.fsf@alter.siamese.dyndns.org","subject":"Re: Fix \"git diff --stat\" for interesting - but empty - file changes","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-10-17T19:35:48Z","receivedAt":"2012-10-17T19:35:48Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Oct 17, 2012 at 11:28 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I think listing a file whose content remain unchanged with 0 as the\n> number of lines affected makes sense, and it will mesh well with\n> Duy's\n>\n>   http://thread.gmane.org/gmane.comp.version-control.git/207749\n>\n> I first wondered if we would get a division-by-zero while scaling\n> the graph, but we do not scale smaller numbers up to fill the\n> columns, so we should be safe.\n\nNote that we should be safe for a totally different - and more\nfundamental - reason: the zero line case is by no means new. We've\nalways done it for the rename case.\n\n> These days, we omit 0 insertions and 0 deletions, so I am not sure\n> what you should get for this case, though:\n>\n>>  Makefile | 0\n>>  1 file changed, 0 insertions(+), 0 deletions(-)\n>\n> Should we just say \"1 file changed\"?\n\nIf that is what it does for the rename case, then yes. I think it\nshould fall out naturally.\n\n              Linus\n"}]}