{"thread":{"id":"30381","subject":"[PATCH 0/4] report chmod'ed binary files the same as text files","startedAt":"2012-05-01T17:10:11Z","lastAt":"2012-05-03T11:45:30Z","messageCount":11,"participants":["Zbigniew Jędrzejewski-Szmek","Junio C Hamano","Johannes Sixt","Martin Mares"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"190434","messageId":"1335892215-21331-1-git-send-email-zbyszek@in.waw.pl","threadId":"30381","inReplyTo":null,"subject":"[PATCH 0/4] report chmod'ed binary files the same as text files","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T17:10:11Z","receivedAt":"2012-05-01T17:10:11Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"This patch series fixes a small discrepancy between the way that\ntext files and binary files are treated. Reported by Martin Mareš in [1].\nFirt patch is cleanup, second describes current behaviour, third does\nthe change, and fourth is a bonus micro-opt. \n\n[1] http://article.gmane.org/gmane.comp.version-control.git/179361\n\nZbigniew Jędrzejewski-Szmek (4):\n  test: modernize style of t4006\n  tests: check --[short]stat output after chmod\n  diff --stat: report chmoded binary files like text files\n  diff --stat: do not run diff on indentical files\n\n diff.c               |   30 +++++++++++++----------\n t/t4006-diff-mode.sh |   65 ++++++++++++++++++++++++++++++++++++--------------\n 2 files changed, 65 insertions(+), 30 deletions(-)\n\n-- \n1.7.10.539.g288dd\n"},{"id":"190435","messageId":"1335892215-21331-2-git-send-email-zbyszek@in.waw.pl","threadId":"30381","inReplyTo":"1335892215-21331-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH 1/4] test: modernize style of t4006","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T17:10:12Z","receivedAt":"2012-05-01T17:10:12Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n t/t4006-diff-mode.sh |   32 +++++++++++++++-----------------\n 1 file changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex ff8c2f7..c8f5180 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -8,23 +8,21 @@ test_description='Test mode change diffs.\n '\n . ./test-lib.sh\n \n-test_expect_success \\\n-    'setup' \\\n-    'echo frotz >rezrov &&\n-     git update-index --add rezrov &&\n-     tree=`git write-tree` &&\n-     echo $tree'\n-\n-test_expect_success \\\n-    'chmod' \\\n-    'test_chmod +x rezrov &&\n-     git diff-index $tree >current'\n-\n-sed -e 's/\\(:100644 100755\\) \\('\"$_x40\"'\\) \\2 /\\1 X X /' <current >check\n-echo \":100644 100755 X X M\trezrov\" >expected\n+test_expect_success 'setup' '\n+\techo frotz >rezrov &&\n+\tgit update-index --add rezrov &&\n+\ttree=`git write-tree` &&\n+\techo $tree\n+'\n \n-test_expect_success \\\n-    'verify' \\\n-    'test_cmp expected check'\n+# $_x40 is defined in test-lib.sh\n+sed_script='s/\\(:100644 100755\\) \\('\"$_x40\"'\\) \\2 /\\1 X X /'\n+test_expect_success 'chmod' '\n+\ttest_chmod +x rezrov &&\n+\tgit diff-index $tree >current &&\n+\tsed -e \"$sed_script\" <current >check &&\n+\techo \":100644 100755 X X M\trezrov\" >expected &&\n+\ttest_cmp expected check\n+'\n \n test_done\n-- \n1.7.10.539.g288dd\n"},{"id":"190436","messageId":"1335892215-21331-3-git-send-email-zbyszek@in.waw.pl","threadId":"30381","inReplyTo":"1335892215-21331-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH 2/4] tests: check --[short]stat output after chmod","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T17:10:13Z","receivedAt":"2012-05-01T17:10:13Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"Add a test to check 'diff --stat' output with a text file after chmod,\nand the same for a binary file. This demonstrates that text and binary\nfiles are treated differently, which can be misleading.\n\nWhile at it, duplicate the tests to check --shortstat output too.\n\nReported-by: Martin Mareš <mj@ucw.cz>\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n t/t4006-diff-mode.sh |   37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex c8f5180..a81c095 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -25,4 +25,41 @@ test_expect_success 'chmod' '\n \ttest_cmp expected check\n '\n \n+test_expect_success 'prepare binary file' '\n+\tgit commit -m rezrov &&\n+\tdd if=/dev/zero of=binbin bs=1024 count=1 &&\n+\tgit add binbin &&\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_cmp expect actual\n+'\n+\n+test_expect_success '--shortstat output after text chmod' '\n+\tgit diff HEAD --shortstat >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--stat output after binary chmod' '\n+\ttest_chmod +x binbin &&\n+\tcat >expect <<-EOF &&\n+\t binbin |  Bin 1024 -> 1024 bytes\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\tEOF\n+\tgit diff HEAD --stat >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--shortstat output after binary chmod' '\n+\tcat >expect <<-EOF &&\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\tEOF\n+\tgit diff HEAD --shortstat >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.10.539.g288dd\n"},{"id":"190437","messageId":"1335892215-21331-4-git-send-email-zbyszek@in.waw.pl","threadId":"30381","inReplyTo":"1335892215-21331-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH 3/4] diff --stat: report chmoded binary files like text files","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T17:10:14Z","receivedAt":"2012-05-01T17:10:14Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"Binary files chmoded without content change were reported as if they\nwere rewritten. At the same time, text files in the same situation\nwere reported as \"unchanged\". Let's treat binary files like text files\nhere, and simply say that they are unchanged.\n\nFor text files, we knew that they were unchanged if the numbers of\nlines added and deleted were both 0. For binary files this metric does\nnot make sense and is not calculated, so a new way of conveying this\ninformation is needed. A new flag is_unchanged is added in struct\ndiffstat_t that is set if the contents of both files are identical.\nFor consistency, this new flag is used both for text files and binary\nfiles.\n\nOutput of --shortstat is modified in the same way.\n\nReported-by: Martin Mareš <mj@ucw.cz>\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n diff.c               |   28 +++++++++++++++++-----------\n t/t4006-diff-mode.sh |    8 +-------\n 2 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7da16c9..6eb2946 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1299,6 +1299,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_unchanged:1;\n \t\tuintmax_t added, deleted;\n \t} **files;\n };\n@@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\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\t\t (change == 0)) {\n+\t\t    data->files[i]->is_unchanged) {\n \t\t\tcount++; /* not shown == room for one more */\n \t\t\tcontinue;\n \t\t}\n@@ -1565,7 +1566,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tint name_len;\n \n \t\tif (!data->files[i]->is_renamed &&\n-\t\t\t (added + deleted == 0)) {\n+\t\t    data->files[i]->is_unchanged) {\n \t\t\ttotal_files--;\n \t\t\tcontinue;\n \t\t}\n@@ -1587,8 +1588,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tif (data->files[i]->is_binary) {\n \t\t\tfprintf(options->file, \"%s\", line_prefix);\n \t\t\tshow_name(options->file, prefix, name, len);\n-\t\t\tfprintf(options->file, \"  Bin \");\n-\t\t\tfprintf(options->file, \"%s%\"PRIuMAX\"%s\",\n+\t\t\tfprintf(options->file, \"  Bin\");\n+\t\t\tif (data->files[i]->is_unchanged) {\n+\t\t\t\tfprintf(options->file, \"\\n\");\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tfprintf(options->file, \" %s%\"PRIuMAX\"%s\",\n \t\t\t\tdel_c, deleted, reset);\n \t\t\tfprintf(options->file, \" -> \");\n \t\t\tfprintf(options->file, \"%s%\"PRIuMAX\"%s\",\n@@ -1661,16 +1666,15 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n \t\treturn;\n \n \tfor (i = 0; i < data->nr; i++) {\n-\t\tif (!data->files[i]->is_binary &&\n-\t\t    !data->files[i]->is_unmerged) {\n-\t\t\tint added = data->files[i]->added;\n-\t\t\tint deleted= data->files[i]->deleted;\n+\t\tif (!data->files[i]->is_unmerged) {\n \t\t\tif (!data->files[i]->is_renamed &&\n-\t\t\t    (added + deleted == 0)) {\n+\t\t\t    data->files[i]->is_unchanged) {\n \t\t\t\ttotal_files--;\n+\t\t\t} else if (data->files[i]->is_binary) {\n+\t\t\t\t; /* do nothing */\n \t\t\t} else {\n-\t\t\t\tadds += added;\n-\t\t\t\tdels += deleted;\n+\t\t\t\tadds += data->files[i]->added;\n+\t\t\t\tdels += data->files[i]->deleted;\n \t\t\t}\n \t\t}\n \t}\n@@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\treturn;\n \t}\n \n+\tdata->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;\n+\n \tif (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {\n \t\tdata->is_binary = 1;\n \t\tdata->added = diff_filespec_size(two);\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex a81c095..e85a1d6 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -46,18 +46,12 @@ test_expect_success '--shortstat output after text chmod' '\n \n test_expect_success '--stat output after binary chmod' '\n \ttest_chmod +x binbin &&\n-\tcat >expect <<-EOF &&\n-\t binbin |  Bin 1024 -> 1024 bytes\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\tEOF\n+\techo \" 0 files changed\" >expect &&\n \tgit diff HEAD --stat >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '--shortstat output after binary chmod' '\n-\tcat >expect <<-EOF &&\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\tEOF\n \tgit diff HEAD --shortstat >actual &&\n \ttest_cmp expect actual\n '\n-- \n1.7.10.539.g288dd\n"},{"id":"190438","messageId":"1335892215-21331-5-git-send-email-zbyszek@in.waw.pl","threadId":"30381","inReplyTo":"1335892215-21331-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH 4/4] diff --stat: do not run diff on indentical files","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T17:10:15Z","receivedAt":"2012-05-01T17:10:15Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"If sha1's are equal, then there's no point in performing the diff.\n\nIn a very unscientific test:\n\ngit init &&\n  dd if=/dev/urandom bs=1M count=30 | hexdump >file1 &&\n  git add file1 && git commit -m 'add file' &&\n  git mv file1 file1-moved && chmod +x file1-moved &&\n  command time git diff --stat\n\n(before) git diff --stat  2.00s user 0.31s system 99% cpu 2.323 total\n(after)  git diff --stat  0.80s user 0.10s system 98% cpu 0.913 total\n\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n diff.c |    2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6eb2946..7cb9893 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2398,7 +2398,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->added = count_lines(two->data, two->size);\n \t}\n \n-\telse {\n+\telse if (!data->is_unchanged) {\n \t\t/* Crazy xdl interfaces.. */\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n-- \n1.7.10.539.g288dd\n"},{"id":"190454","messageId":"7vzk9r93ym.fsf@alter.siamese.dyndns.org","threadId":"30381","inReplyTo":"1335892215-21331-2-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 1/4] test: modernize style of t4006","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-01T18:00:17Z","receivedAt":"2012-05-01T18:00:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:\n\n> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n> ---\n>  t/t4006-diff-mode.sh |   32 +++++++++++++++-----------------\n>  1 file changed, 15 insertions(+), 17 deletions(-)\n\nStyle update is welcome, but shouldn't the assignment to sed_script\nbe done in the second test if it is the only user?  If you are going to\nadd more tests at the end, then it should be away from the second test to\nmake it clear that it is not part of it.\n\nThanks.\n\n> diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\n> index ff8c2f7..c8f5180 100755\n> --- a/t/t4006-diff-mode.sh\n> +++ b/t/t4006-diff-mode.sh\n> @@ -8,23 +8,21 @@ test_description='Test mode change diffs.\n>  '\n>  . ./test-lib.sh\n>  \n> -test_expect_success \\\n> -    'setup' \\\n> -    'echo frotz >rezrov &&\n> -     git update-index --add rezrov &&\n> -     tree=`git write-tree` &&\n> -     echo $tree'\n> -\n> -test_expect_success \\\n> -    'chmod' \\\n> -    'test_chmod +x rezrov &&\n> -     git diff-index $tree >current'\n> -\n> -sed -e 's/\\(:100644 100755\\) \\('\"$_x40\"'\\) \\2 /\\1 X X /' <current >check\n> -echo \":100644 100755 X X M\trezrov\" >expected\n> +test_expect_success 'setup' '\n> +\techo frotz >rezrov &&\n> +\tgit update-index --add rezrov &&\n> +\ttree=`git write-tree` &&\n> +\techo $tree\n> +'\n>  \n> -test_expect_success \\\n> -    'verify' \\\n> -    'test_cmp expected check'\n> +# $_x40 is defined in test-lib.sh\n> +sed_script='s/\\(:100644 100755\\) \\('\"$_x40\"'\\) \\2 /\\1 X X /'\n> +test_expect_success 'chmod' '\n> +\ttest_chmod +x rezrov &&\n> +\tgit diff-index $tree >current &&\n> +\tsed -e \"$sed_script\" <current >check &&\n> +\techo \":100644 100755 X X M\trezrov\" >expected &&\n> +\ttest_cmp expected check\n> +'\n>  \n>  test_done\n"},{"id":"190459","messageId":"7vvckf92pp.fsf@alter.siamese.dyndns.org","threadId":"30381","inReplyTo":"1335892215-21331-4-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-01T18:27:14Z","receivedAt":"2012-05-01T18:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:\n\n> Binary files chmoded without content change were reported as if they\n> were rewritten. At the same time, text files in the same situation\n> were reported as \"unchanged\". Let's treat binary files like text files\n> here, and simply say that they are unchanged.\n>\n> For text files, we knew that they were unchanged if the numbers of\n> lines added and deleted were both 0. For binary files this metric does\n> not make sense and is not calculated, so a new way of conveying this\n> information is needed. A new flag is_unchanged is added in struct\n> diffstat_t that is set if the contents of both files are identical.\n> For consistency, this new flag is used both for text files and binary\n> files.\n>\n> Output of --shortstat is modified in the same way.\n>\n> Reported-by: Martin Mareš <mj@ucw.cz>\n> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n> ---\n>  diff.c               |   28 +++++++++++++++++-----------\n>  t/t4006-diff-mode.sh |    8 +-------\n>  2 files changed, 18 insertions(+), 18 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 7da16c9..6eb2946 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1299,6 +1299,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_unchanged:1;\n\nThe name is somewhat misleading, as a filepair that consists of two blobs\nwith the same contents with different mode bits is still \"changed\", and\nyou are trying to say that they have the same contents.\n\n> @@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\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\t\t (change == 0)) {\n> +\t\t    data->files[i]->is_unchanged) {\n\nI am not sure if all these hunks are needed.  If you are going to show\nonly \"  Bin\\n\" for a filepair with the same binary contents, perhaps it is\nsimpler to set added/deleted fields of such a filepair to 0?  Then most of\nthe hunks in this patch can disappear, no?\n\n> @@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n>  \t\treturn;\n>  \t}\n>  \n> +\tdata->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;\n\nPlease write it as \"!hashcmp(a, b)\", not \"hashcmp(a, b) == 0\".\n\nIn any case, how about doing it like this instead?\n\n diff.c               |   38 +++++++++++++++++++++++---------------\n t/t4006-diff-mode.sh |    8 +-------\n 2 files changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 22288b0..338ef41 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1583,8 +1583,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tif (data->files[i]->is_binary) {\n \t\t\tfprintf(options->file, \"%s\", line_prefix);\n \t\t\tshow_name(options->file, prefix, name, len);\n-\t\t\tfprintf(options->file, \"  Bin \");\n-\t\t\tfprintf(options->file, \"%s%\"PRIuMAX\"%s\",\n+\t\t\tfprintf(options->file, \"  Bin\");\n+\t\t\tif (!added && !deleted) {\n+\t\t\t\tputc('\\n', options->file);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tfprintf(options->file, \" %s%\"PRIuMAX\"%s\",\n \t\t\t\tdel_c, deleted, reset);\n \t\t\tfprintf(options->file, \" -> \");\n \t\t\tfprintf(options->file, \"%s%\"PRIuMAX\"%s\",\n@@ -1657,17 +1661,16 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n \t\treturn;\n \n \tfor (i = 0; i < data->nr; i++) {\n-\t\tif (!data->files[i]->is_binary &&\n-\t\t    !data->files[i]->is_unmerged) {\n-\t\t\tint added = data->files[i]->added;\n-\t\t\tint deleted= data->files[i]->deleted;\n-\t\t\tif (!data->files[i]->is_renamed &&\n-\t\t\t    (added + deleted == 0)) {\n-\t\t\t\ttotal_files--;\n-\t\t\t} else {\n-\t\t\t\tadds += added;\n-\t\t\t\tdels += deleted;\n-\t\t\t}\n+\t\tint added = data->files[i]->added;\n+\t\tint deleted= data->files[i]->deleted;\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\t\ttotal_files--;\n+\t\t} else {\n+\t\t\tadds += added;\n+\t\t\tdels += deleted;\n \t\t}\n \t}\n \tif (options->output_prefix) {\n@@ -2377,8 +2380,13 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \tif (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {\n \t\tdata->is_binary = 1;\n-\t\tdata->added = diff_filespec_size(two);\n-\t\tdata->deleted = diff_filespec_size(one);\n+\t\tif (!hashcmp(one->sha1, two->sha1)) {\n+\t\t\tdata->added = 0;\n+\t\t\tdata->deleted = 0;\n+\t\t} else {\n+\t\t\tdata->added = diff_filespec_size(one);\n+\t\t\tdata->deleted = diff_filespec_size(two);\n+\t\t}\n \t}\n \n \telse if (complete_rewrite) {\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex 392dfef..693bfc4 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -46,18 +46,12 @@ test_expect_success '--shortstat output after text chmod' '\n \n test_expect_success '--stat output after binary chmod' '\n \ttest_chmod +x binbin &&\n-\tcat >expect <<-EOF &&\n-\t binbin |  Bin 1024 -> 1024 bytes\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\tEOF\n+\techo \" 0 files changed\" >expect &&\n \tgit diff HEAD --stat >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '--shortstat output after binary chmod' '\n-\tcat >expect <<-EOF &&\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\tEOF\n \tgit diff HEAD --shortstat >actual &&\n \ttest_cmp expect actual\n '\n"},{"id":"190467","messageId":"4FA03BE2.2030107@in.waw.pl","threadId":"30381","inReplyTo":"7vvckf92pp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T19:39:14Z","receivedAt":"2012-05-01T19:39:14Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 05/01/2012 08:27 PM, Junio C Hamano wrote:\n> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:\n> \n>> Binary files chmoded without content change were reported as if they\n>> were rewritten. At the same time, text files in the same situation\n>> were reported as \"unchanged\". Let's treat binary files like text files\n>> here, and simply say that they are unchanged.\n>>\n>> For text files, we knew that they were unchanged if the numbers of\n>> lines added and deleted were both 0. For binary files this metric does\n>> not make sense and is not calculated, so a new way of conveying this\n>> information is needed. A new flag is_unchanged is added in struct\n>> diffstat_t that is set if the contents of both files are identical.\n>> For consistency, this new flag is used both for text files and binary\n>> files.\n>>\n>> Output of --shortstat is modified in the same way.\n>>\n>> Reported-by: Martin Mareš <mj@ucw.cz>\n>> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n>> ---\n>>  diff.c               |   28 +++++++++++++++++-----------\n>>  t/t4006-diff-mode.sh |    8 +-------\n>>  2 files changed, 18 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 7da16c9..6eb2946 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -1299,6 +1299,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_unchanged:1;\n> \n> The name is somewhat misleading, as a filepair that consists of two blobs\n> with the same contents with different mode bits is still \"changed\", and\n> you are trying to say that they have the same contents.\n> \n>> @@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\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\t\t (change == 0)) {\n>> +\t\t    data->files[i]->is_unchanged) {\n> \n> I am not sure if all these hunks are needed.  If you are going to show\n> only \"  Bin\\n\" for a filepair with the same binary contents, perhaps it is\n> simpler to set added/deleted fields of such a filepair to 0?  Then most of\n> the hunks in this patch can disappear, no?\n> \n>> @@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n>>  \t\treturn;\n>>  \t}\n>>  \n>> +\tdata->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;\n> \n> Please write it as \"!hashcmp(a, b)\", not \"hashcmp(a, b) == 0\".\n> \n> In any case, how about doing it like this instead?\nYeah, this is much nicer.\n\nOn top of this, 4/4 becomes:\n-       else {\n+       else if (hashcmp(one->sha1, two->sha1)) {\nand the time improvement is the same (0.8 vs 2.0 s).\n\nDo you want me to resend with your replacement patch?\n\nZbyszek\n"},{"id":"190471","messageId":"4FA03F9B.1020402@in.waw.pl","threadId":"30381","inReplyTo":"7vzk9r93ym.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] test: modernize style of t4006","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-05-01T19:55:07Z","receivedAt":"2012-05-01T19:55:07Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 05/01/2012 08:00 PM, Junio C Hamano wrote:\n> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:\n> \n>> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n>> ---\n>>  t/t4006-diff-mode.sh |   32 +++++++++++++++-----------------\n>>  1 file changed, 15 insertions(+), 17 deletions(-)\n> \n> Style update is welcome, but shouldn't the assignment to sed_script\n> be done in the second test if it is the only user?  If you are going to\n> add more tests at the end, then it should be away from the second test to\n> make it clear that it is not part of it.\n\nHi,\n$sed_script is indeed only used in that one test. But moving the\nassignment inside would complicate the quoting rules (the script is now\nquoted with ', but this would have to change inside the test case which\nis quoted with ' too). I actually think it's simpler this way.\n\nThanks,\nZbyszek\n\n>> -sed -e 's/\\(:100644 100755\\) \\('\"$_x40\"'\\) \\2 /\\1 X X /' <current >check\n>> -echo \":100644 100755 X X M\trezrov\" >expected\n>> +# $_x40 is defined in test-lib.sh\n>> +sed_script='s/\\(:100644 100755\\) \\('\"$_x40\"'\\) \\2 /\\1 X X /'\n"},{"id":"190526","messageId":"4FA0E40C.6030809@viscovery.net","threadId":"30381","inReplyTo":"1335892215-21331-3-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 2/4] tests: check --[short]stat output after chmod","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-05-02T07:36:44Z","receivedAt":"2012-05-02T07:36:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/1/2012 19:10, schrieb Zbigniew Jędrzejewski-Szmek:\n> +test_expect_success 'prepare binary file' '\n> +\tgit commit -m rezrov &&\n> +\tdd if=/dev/zero of=binbin bs=1024 count=1 &&\n> +\tgit add binbin &&\n> +\tgit commit -m binbin\n> +'\n\nPlease squash in this fixup; we do not have /dev/zero on Windows.\n\n\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex 693bfc4..7a3e1f9 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -27,7 +27,7 @@ test_expect_success 'chmod' '\n \n test_expect_success 'prepare binary file' '\n \tgit commit -m rezrov &&\n-\tdd if=/dev/zero of=binbin bs=1024 count=1 &&\n+\tprintf \"\\00\\01\\02\\03\\04\\05\\06\" >binbin &&\n \tgit add binbin &&\n \tgit commit -m binbin\n '\n-- \n1.7.10.1.1568.gede6096\n"},{"id":"190634","messageId":"mj+md-20120503.114519.14372.nikam@ucw.cz","threadId":"30381","inReplyTo":"1335892215-21331-1-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 0/4] report chmod'ed binary files the same as text files","fromName":"Martin Mares","fromEmail":"mj@ucw.cz","sentAt":"2012-05-03T11:45:30Z","receivedAt":"2012-05-03T11:45:30Z","isPatch":true,"sender":{"key":"mj@ucw.cz","avatar":null},"body":"Hi!\n\n> This patch series fixes a small discrepancy between the way that\n> text files and binary files are treated. Reported by Martin Mareš in [1].\n> Firt patch is cleanup, second describes current behaviour, third does\n> the change, and fourth is a bonus micro-opt.\n\nThanks for the fix!\n\n\t\t\t\tHave a nice fortnight\n-- \nMartin `MJ' Mares                          <mj@ucw.cz>   http://mj.ucw.cz/\nFaculty of Math and Physics, Charles University, Prague, Czech Rep., Earth\nThis mail doesn't contain viruses, because it wasn't sent from MS Windows. Checked by eyes.\n"}]}