{"thread":{"id":"16648","subject":"[PATCH 1/2] diff: fix handling of binary rewrite diffs","startedAt":"2008-12-09T08:12:28Z","lastAt":"2008-12-10T20:27:18Z","messageCount":7,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"97406","messageId":"20081209081227.GA19626@coredump.intra.peff.net","threadId":"16648","inReplyTo":null,"subject":"[PATCH 1/2] diff: fix handling of binary rewrite diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-09T08:12:28Z","receivedAt":"2008-12-09T08:12:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current emit_rewrite_diff code always writes a text\npatch without checking whether the content is binary. This\nmeans that if you end up with a rewrite diff for a binary\nfile, you get lots of raw binary goo in your patch.\n\nInstead, if we have binary files, then let's just skip\nemit_rewrite_diff altogether. We will already have shown the\n\"dissimilarity index\" line, so it is really about the diff\ncontents. If binary diffs are turned off, the \"Binary files\na/file and b/file differ\" message should be the same in\neither case. If we do have binary patches turned on, there\nisn't much point in making a less-efficient binary patch\nthat does a total rewrite; no human is going to read it, and\nsince binary patches don't apply with any fuzz anyway, the\nresult of application should be the same.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI couldn't think of a good reason to want a different format of binary\npatch for a rewrite versus a regular diff, but others may be able to.\nIn that case, you have to mimic the binary codepath somewhat in\nemit_rewrite_diff; I started on it, but realized I was just copying the\ncode.\n\nHowever, we should at least do something before 1.6.1; as it is, it\nspews binary garbage.\n\nI discovered this because one of my textconv'd files had a rewrite\n(which apparently happens by changing a few lines in a word document :))\nand gave me a bogus diff. However, the problem is not unique to\ntextconv; patch 1/2 handles binary files and 2/2 handles the textconv\ncase.\n\nI didn't put the tests into the existing t4022-diff-rewrite because I\nwanted to keep the textconv tests together with these tests, and I think\nit makes sense to test features in isolation before testing them\ntogether. IOW:\n\n  t4022 - rewrite works\n  t4030 - textconv works\n  t4031 - rewrite AND textconv work together\n\n diff.c                         |    4 ++-\n t/t4031-diff-rewrite-binary.sh |   42 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+), 1 deletions(-)\n create mode 100755 t/t4031-diff-rewrite-binary.sh\n\ndiff --git a/diff.c b/diff.c\nindex f644947..ea958a2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1376,7 +1376,9 @@ static void builtin_diff(const char *name_a,\n \t\t */\n \t\tif ((one->mode ^ two->mode) & S_IFMT)\n \t\t\tgoto free_ab_and_return;\n-\t\tif (complete_rewrite) {\n+\t\tif (complete_rewrite &&\n+\t\t    !diff_filespec_is_binary(one) &&\n+\t\t    !diff_filespec_is_binary(two)) {\n \t\t\temit_rewrite_diff(name_a, name_b, one, two, o);\n \t\t\to->found_changes = 1;\n \t\t\tgoto free_ab_and_return;\ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nnew file mode 100755\nindex 0000000..4b522f7\n--- /dev/null\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='rewrite diff on binary file'\n+\n+. ./test-lib.sh\n+\n+# We must be large enough to meet the MINIMUM_BREAK_SIZE\n+# requirement.\n+make_file() {\n+\tfor i in 1 2 3 4 5 6 7 8 9 10; do\n+\t\tfor j in 1 2 3 4 5 6 7 9 10; do\n+\t\t\tfor k in 1 2 3 4 5; do\n+\t\t\t\tprintf \"$1\\n\"\n+\t\t\tdone\n+\t\tdone\n+\tdone >file\n+}\n+\n+test_expect_success 'create binary file with changes' '\n+\tmake_file \"\\\\0\" &&\n+\tgit add file &&\n+\tmake_file \"\\\\01\"\n+'\n+\n+test_expect_success 'vanilla diff is binary' '\n+\tgit diff >diff &&\n+\tgrep \"Binary files a/file and b/file differ\" diff\n+'\n+\n+test_expect_success 'rewrite diff is binary' '\n+\tgit diff -B >diff &&\n+\tgrep \"dissimilarity index\" diff &&\n+\tgrep \"Binary files a/file and b/file differ\" diff\n+'\n+\n+test_expect_success 'rewrite diff can show binary patch' '\n+\tgit diff -B --binary >diff &&\n+\tgrep \"dissimilarity index\" diff &&\n+\tgrep \"GIT binary patch\" diff\n+'\n+\n+test_done\n-- \n1.6.1.rc2.1.g8f945.dirty\n"},{"id":"97407","messageId":"20081209081321.GA19707@coredump.intra.peff.net","threadId":"16648","inReplyTo":"20081209081227.GA19626@coredump.intra.peff.net","subject":"[PATCH 2/2] diff: respect textconv in rewrite diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-09T08:13:21Z","receivedAt":"2008-12-09T08:13:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently we just skip rewrite diffs for binary files; this\npatch makes an exception for files which will be textconv'd,\nand actually performs the textconv before generating the\ndiff.\n\nConceptually, rewrite diffs should be in the exact same\nformat as the a non-rewrite diff, except that we refuse to\nshare any context. Thus it makes very little sense for \"git\ndiff\" to show a textconv'd diff, but for \"git diff -B\" to\nshow \"Binary files differ\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                         |   48 ++++++++++++++++++++++++++++++----------\n t/t4031-diff-rewrite-binary.sh |   21 ++++++++++++++++-\n 2 files changed, 56 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ea958a2..6dc19bc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -229,6 +229,8 @@ static void emit_rewrite_diff(const char *name_a,\n \t\t\t      const char *name_b,\n \t\t\t      struct diff_filespec *one,\n \t\t\t      struct diff_filespec *two,\n+\t\t\t      const char *textconv_one,\n+\t\t\t      const char *textconv_two,\n \t\t\t      struct diff_options *o)\n {\n \tint lc_a, lc_b;\n@@ -241,6 +243,8 @@ static void emit_rewrite_diff(const char *name_a,\n \tconst char *reset = diff_get_color(color_diff, DIFF_RESET);\n \tstatic struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;\n \tconst char *a_prefix, *b_prefix;\n+\tconst char *data_one, *data_two;\n+\tsize_t size_one, size_two;\n \n \tif (diff_mnemonic_prefix && DIFF_OPT_TST(o, REVERSE_DIFF)) {\n \t\ta_prefix = o->b_prefix;\n@@ -262,8 +266,27 @@ static void emit_rewrite_diff(const char *name_a,\n \n \tdiff_populate_filespec(one, 0);\n \tdiff_populate_filespec(two, 0);\n-\tlc_a = count_lines(one->data, one->size);\n-\tlc_b = count_lines(two->data, two->size);\n+\tif (textconv_one) {\n+\t\tdata_one = run_textconv(textconv_one, one, &size_one);\n+\t\tif (!data_one)\n+\t\t\tdie(\"unable to read files to diff\");\n+\t}\n+\telse {\n+\t\tdata_one = one->data;\n+\t\tsize_one = one->size;\n+\t}\n+\tif (textconv_two) {\n+\t\tdata_two = run_textconv(textconv_two, two, &size_two);\n+\t\tif (!data_two)\n+\t\t\tdie(\"unable to read files to diff\");\n+\t}\n+\telse {\n+\t\tdata_two = two->data;\n+\t\tsize_two = two->size;\n+\t}\n+\n+\tlc_a = count_lines(data_one, size_one);\n+\tlc_b = count_lines(data_two, size_two);\n \tfprintf(o->file,\n \t\t\"%s--- %s%s%s\\n%s+++ %s%s%s\\n%s@@ -\",\n \t\tmetainfo, a_name.buf, name_a_tab, reset,\n@@ -273,9 +296,9 @@ static void emit_rewrite_diff(const char *name_a,\n \tprint_line_count(o->file, lc_b);\n \tfprintf(o->file, \" @@%s\\n\", reset);\n \tif (lc_a)\n-\t\tcopy_file_with_prefix(o->file, '-', one->data, one->size, old, reset);\n+\t\tcopy_file_with_prefix(o->file, '-', data_one, size_one, old, reset);\n \tif (lc_b)\n-\t\tcopy_file_with_prefix(o->file, '+', two->data, two->size, new, reset);\n+\t\tcopy_file_with_prefix(o->file, '+', data_two, size_two, new, reset);\n }\n \n static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n@@ -1334,6 +1357,11 @@ static void builtin_diff(const char *name_a,\n \tconst char *a_prefix, *b_prefix;\n \tconst char *textconv_one = NULL, *textconv_two = NULL;\n \n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(one);\n+\t\ttextconv_two = get_textconv(two);\n+\t}\n+\n \tdiff_set_mnemonic_prefix(o, \"a/\", \"b/\");\n \tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n \t\ta_prefix = o->b_prefix;\n@@ -1377,9 +1405,10 @@ static void builtin_diff(const char *name_a,\n \t\tif ((one->mode ^ two->mode) & S_IFMT)\n \t\t\tgoto free_ab_and_return;\n \t\tif (complete_rewrite &&\n-\t\t    !diff_filespec_is_binary(one) &&\n-\t\t    !diff_filespec_is_binary(two)) {\n-\t\t\temit_rewrite_diff(name_a, name_b, one, two, o);\n+\t\t    (textconv_one || !diff_filespec_is_binary(one)) &&\n+\t\t    (textconv_two || !diff_filespec_is_binary(two))) {\n+\t\t\temit_rewrite_diff(name_a, name_b, one, two,\n+\t\t\t\t\t\ttextconv_one, textconv_two, o);\n \t\t\to->found_changes = 1;\n \t\t\tgoto free_ab_and_return;\n \t\t}\n@@ -1388,11 +1417,6 @@ static void builtin_diff(const char *name_a,\n \tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n-\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n-\t\ttextconv_one = get_textconv(one);\n-\t\ttextconv_two = get_textconv(two);\n-\t}\n-\n \tif (!DIFF_OPT_TST(o, TEXT) &&\n \t    ( (diff_filespec_is_binary(one) && !textconv_one) ||\n \t      (diff_filespec_is_binary(two) && !textconv_two) )) {\ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex 4b522f7..72e5bfd 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -7,13 +7,15 @@ test_description='rewrite diff on binary file'\n # We must be large enough to meet the MINIMUM_BREAK_SIZE\n # requirement.\n make_file() {\n+\t# common first line to help identify rewrite versus regular diff\n+\tprintf \"=\\n\" >file\n \tfor i in 1 2 3 4 5 6 7 8 9 10; do\n \t\tfor j in 1 2 3 4 5 6 7 9 10; do\n \t\t\tfor k in 1 2 3 4 5; do\n \t\t\t\tprintf \"$1\\n\"\n \t\t\tdone\n \t\tdone\n-\tdone >file\n+\tdone >>file\n }\n \n test_expect_success 'create binary file with changes' '\n@@ -39,4 +41,21 @@ test_expect_success 'rewrite diff can show binary patch' '\n \tgrep \"GIT binary patch\" diff\n '\n \n+cat >dump <<'EOF'\n+#!/bin/sh\n+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < \"$1\"\n+EOF\n+chmod +x dump\n+test_expect_success 'setup textconv' '\n+\techo file diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"$PWD\"/dump\n+'\n+\n+test_expect_success 'rewrite diff respects textconv' '\n+\tgit diff -B >diff &&\n+\tgrep \"dissimilarity index\" diff &&\n+\tgrep \"^-61\" diff &&\n+\tgrep \"^-0\" diff\n+'\n+\n test_done\n-- \n1.6.1.rc2.1.g8f945.dirty\n"},{"id":"97472","messageId":"7v3agw2zu5.fsf@gitster.siamese.dyndns.org","threadId":"16648","inReplyTo":"20081209081321.GA19707@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] diff: respect textconv in rewrite diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T08:34:26Z","receivedAt":"2008-12-10T08:34:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Currently we just skip rewrite diffs for binary files; this patch makes\n> an exception for files which will be textconv'd, and actually performs\n> the textconv before generating the diff.\n>\n> Conceptually, rewrite diffs should be in the exact same format as the a\n> non-rewrite diff, except that we refuse to share any context. Thus it\n> makes very little sense for \"git diff\" to show a textconv'd diff, but\n> for \"git diff -B\" to show \"Binary files differ\".\n\nMakes sense.\n\n> +cat >dump <<'EOF'\n> +#!/bin/sh\n> +perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < \"$1\"\n> +EOF\n\nI'll squash in a change to make this part use $SHELL_PATH for\nconsistency.  Thanks.\n"},{"id":"97473","messageId":"7vvdts1l92.fsf@gitster.siamese.dyndns.org","threadId":"16648","inReplyTo":"20081209081227.GA19626@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] diff: fix handling of binary rewrite diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T08:34:49Z","receivedAt":"2008-12-10T08:34:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Instead, if we have binary files, then let's just skip emit_rewrite_diff\n> altogether. We will already have shown the \"dissimilarity index\" line,\n> so it is really about the diff contents. If binary diffs are turned off,\n> the \"Binary files a/file and b/file differ\" message should be the same\n> in either case. If we do have binary patches turned on, there isn't much\n> point in making a less-efficient binary patch that does a total rewrite;\n> no human is going to read it, and since binary patches don't apply with\n> any fuzz anyway, the result of application should be the same.\n\nMakes sense.\n\n>  diff.c                         |    4 ++-\n>  t/t4031-diff-rewrite-binary.sh |   42 ++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 45 insertions(+), 1 deletions(-)\n>  create mode 100755 t/t4031-diff-rewrite-binary.sh\n>\n> diff --git a/diff.c b/diff.c\n> index f644947..ea958a2 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1376,7 +1376,9 @@ static void builtin_diff(const char *name_a,\n>  \t\t */\n>  \t\tif ((one->mode ^ two->mode) & S_IFMT)\n>  \t\t\tgoto free_ab_and_return;\n> -\t\tif (complete_rewrite) {\n> +\t\tif (complete_rewrite &&\n> +\t\t    !diff_filespec_is_binary(one) &&\n> +\t\t    !diff_filespec_is_binary(two)) {\n>  \t\t\temit_rewrite_diff(name_a, name_b, one, two, o);\n>  \t\t\to->found_changes = 1;\n>  \t\t\tgoto free_ab_and_return;\n\nAnd looks correct.\n\n> diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\n> new file mode 100755\n> index 0000000..4b522f7\n> --- /dev/null\n> +++ b/t/t4031-diff-rewrite-binary.sh\n> @@ -0,0 +1,42 @@\n> +#!/bin/sh\n> +\n> +test_description='rewrite diff on binary file'\n> +\n> +. ./test-lib.sh\n> +\n> +# We must be large enough to meet the MINIMUM_BREAK_SIZE\n> +# requirement.\n> +make_file() {\n> +\tfor i in 1 2 3 4 5 6 7 8 9 10; do\n> +\t\tfor j in 1 2 3 4 5 6 7 9 10; do\n> +\t\t\tfor k in 1 2 3 4 5; do\n> +\t\t\t\tprintf \"$1\\n\"\n> +\t\t\tdone\n> +\t\tdone\n> +\tdone >file\n> +}\n> +\n> +test_expect_success 'create binary file with changes' '\n> +\tmake_file \"\\\\0\" &&\n> +\tgit add file &&\n> +\tmake_file \"\\\\01\"\n> +'\n\nHmm... \"1 2 3 4 5 6 7 9 10\"?\n"},{"id":"97476","messageId":"20081210090221.GA11367@coredump.intra.peff.net","threadId":"16648","inReplyTo":"7v3agw2zu5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] diff: respect textconv in rewrite diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-10T09:02:22Z","receivedAt":"2008-12-10T09:02:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2008 at 12:34:26AM -0800, Junio C Hamano wrote:\n\n> > +cat >dump <<'EOF'\n> > +#!/bin/sh\n> > +perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < \"$1\"\n> > +EOF\n> \n> I'll squash in a change to make this part use $SHELL_PATH for\n> consistency.  Thanks.\n\nIt was cut-and-paste from t4030, so if we care, it might be worth\nchanging there, too (and naming it \"dump\" instead of \"hexdump\", because\nit actually dumps in decimal :) ).\n\nBut more importantly, the fixup you just pushed seems to have an extra\n\">dump\":\n\n> +{\n> +       echo \"#!$SHELL_PATH\"\n> +       cat >dump <<'EOF'\n> +perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < \"$1\"\n> +EOF\n> +} >dump\n> +chmod +x dump\n\n-Peff\n"},{"id":"97477","messageId":"20081210090407.GB11367@coredump.intra.peff.net","threadId":"16648","inReplyTo":"7vvdts1l92.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] diff: fix handling of binary rewrite diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-10T09:04:07Z","receivedAt":"2008-12-10T09:04:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2008 at 12:34:49AM -0800, Junio C Hamano wrote:\n\n> > +\t\tfor j in 1 2 3 4 5 6 7 9 10; do\n> Hmm... \"1 2 3 4 5 6 7 9 10\"?\n\nHeh. Oops.\n\nI see you fixed this up as 1 through 9. My intent was 1 through 10, but\nit really doesn't matter. The point is just to hit the minimum break\nsize, and I left a lot of leeway in case it ever gets increased later.\n\n-Peff\n"},{"id":"97516","messageId":"7vd4fzzsgp.fsf@gitster.siamese.dyndns.org","threadId":"16648","inReplyTo":"20081210090221.GA11367@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] diff: respect textconv in rewrite diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T20:27:18Z","receivedAt":"2008-12-10T20:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But more importantly, the fixup you just pushed seems to have an extra\n> \">dump\":\n\nOops, sorry.\n"}]}