{"thread":{"id":"23906","subject":"[PATCH] diff: fix \"git show -C -C\" output when renaming a binary file","startedAt":"2010-05-26T02:50:12Z","lastAt":"2010-06-03T04:12:35Z","messageCount":4,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"142302","messageId":"20100526025013.25696.26816.chriscool@tuxfamily.org","threadId":"23906","inReplyTo":null,"subject":"[PATCH] diff: fix \"git show -C -C\" output when renaming a binary file","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-05-26T02:50:12Z","receivedAt":"2010-05-26T02:50:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"A bug was introduced in 3e97c7c6af2901cec63bf35fcd43ae3472e24af8\n(No diff -b/-w output for all-whitespace changes, Nov 19 2009)\nthat made the lines:\n\ndiff --git a/bar b/sub/bar\nsimilarity index 100%\nrename from bar\nrename to sub/bar\n\ndisappear from \"git show -C -C\" output when file bar is a binary\nfile.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n diff.c                        |    4 +++\n t/t4043-diff-rename-binary.sh |   45 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 0 deletions(-)\n create mode 100755 t/t4043-diff-rename-binary.sh\n\ndiff --git a/diff.c b/diff.c\nindex 494f560..0b00271 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1737,6 +1737,10 @@ static void builtin_diff(const char *name_a,\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} else if (diff_filespec_is_binary(one) &&\n+\t\t\t   diff_filespec_is_binary(two)) {\n+\t\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\t\tstrbuf_reset(&header);\n \t\t}\n \t}\n \ndiff --git a/t/t4043-diff-rename-binary.sh b/t/t4043-diff-rename-binary.sh\nnew file mode 100755\nindex 0000000..0601281\n--- /dev/null\n+++ b/t/t4043-diff-rename-binary.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Jakub Narebski, Christian Couder\n+#\n+\n+test_description='Move a binary file'\n+\n+. ./test-lib.sh\n+\n+\n+test_expect_success 'prepare repository' '\n+\tgit init &&\n+\techo foo > foo &&\n+\techo \"barQ\" | q_to_nul > bar &&\n+\tgit add . &&\n+\tgit commit -m \"Initial commit\"\n+'\n+\n+test_expect_success 'move the files into a \"sub\" directory' '\n+\tmkdir sub &&\n+\tgit mv bar foo sub/ &&\n+\tgit commit -m \"Moved to sub/\"\n+'\n+\n+cat > expected <<\\EOF\n+ bar => sub/bar |  Bin 5 -> 5 bytes\n+ foo => sub/foo |    0\n+ 2 files changed, 0 insertions(+), 0 deletions(-)\n+\n+diff --git a/bar b/sub/bar\n+similarity index 100%\n+rename from bar\n+rename to sub/bar\n+diff --git a/foo b/sub/foo\n+similarity index 100%\n+rename from foo\n+rename to sub/foo\n+EOF\n+\n+test_expect_success 'git show -C -C report renames' '\n+\tgit show -C -C --raw --binary --stat | tail -n 12 > current &&\n+\ttest_cmp expected current\n+'\n+\n+test_done\n-- \n1.7.1.226.g770c5.dirty\n"},{"id":"142515","messageId":"7v1vcv5x79.fsf@alter.siamese.dyndns.org","threadId":"23906","inReplyTo":"20100526025013.25696.26816.chriscool@tuxfamily.org","subject":"Re: [PATCH] diff: fix \"git show -C -C\" output when renaming a binary file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-29T01:11:06Z","receivedAt":"2010-05-29T01:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"142851","messageId":"7v4ohl9f8w.fsf@alter.siamese.dyndns.org","threadId":"23906","inReplyTo":"20100526025013.25696.26816.chriscool@tuxfamily.org","subject":"Re: [PATCH] diff: fix \"git show -C -C\" output when renaming a binary file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-02T23:38:55Z","receivedAt":"2010-06-02T23:38:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> A bug was introduced in 3e97c7c6af2901cec63bf35fcd43ae3472e24af8\n> (No diff -b/-w output for all-whitespace changes, Nov 19 2009)\n> that made the lines:\n>\n> diff --git a/bar b/sub/bar\n> similarity index 100%\n> rename from bar\n> rename to sub/bar\n>\n> disappear from \"git show -C -C\" output when file bar is a binary\n> file.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  diff.c                        |    4 +++\n>  t/t4043-diff-rename-binary.sh |   45 +++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 49 insertions(+), 0 deletions(-)\n>  create mode 100755 t/t4043-diff-rename-binary.sh\n>\n> diff --git a/diff.c b/diff.c\n> index 494f560..0b00271 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1737,6 +1737,10 @@ static void builtin_diff(const char *name_a,\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} else if (diff_filespec_is_binary(one) &&\n> +\t\t\t   diff_filespec_is_binary(two)) {\n> +\t\t\tfprintf(o->file, \"%s\", header.buf);\n> +\t\t\tstrbuf_reset(&header);\n>  \t\t}\n\nActually, the logic to single out \"binary vs binary\" feels wrong, as one\nof the cases the output that was overly squelched is this one:\n\n\t\t/* Quite common confusing case */\n\t\tif (mf1.size == mf2.size &&\n       \t            !memcmp(mf1.ptr, mf2.ptr, mf1.size))\n\t\t\tgoto free_ab_and_return;\n\t\tfprintf(o->file, \"%s\", header.buf);\n\t\tstrbuf_reset(&header);\n\nNotice that we have accumulated the rename/copy/new/old in header but if\nthe blobs are the same, we discard it.\n\nAnother case that 3e97c7c6 handled completely incorrectly is when -w/-b is\nin effect and there is a rename or copy.  Because there won't be textual\ndiff emitted by fn_out_consume() at all, we lose the header information as\nwell.\n\nSo I would say we would need something like this on top of your patch.\n\n--\n\n diff.c                     |   27 +++++++++++++++++----------\n t/t4015-diff-whitespace.sh |   37 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 10 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0b00271..ad31b00 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1656,6 +1656,7 @@ static void builtin_diff(const char *name_a,\n \t\t\t struct diff_filespec *one,\n \t\t\t struct diff_filespec *two,\n \t\t\t const char *xfrm_msg,\n+\t\t\t int must_show_header,\n \t\t\t struct diff_options *o,\n \t\t\t int complete_rewrite)\n {\n@@ -1708,16 +1709,19 @@ static void builtin_diff(const char *name_a,\n \t\tstrbuf_addf(&header, \"%snew file mode %06o%s\\n\", set, two->mode, reset);\n \t\tif (xfrm_msg && xfrm_msg[0])\n \t\t\tstrbuf_addf(&header, \"%s%s%s\\n\", set, xfrm_msg, reset);\n+\t\tmust_show_header = 1;\n \t}\n \telse if (lbl[1][0] == '/') {\n \t\tstrbuf_addf(&header, \"%sdeleted file mode %06o%s\\n\", set, one->mode, reset);\n \t\tif (xfrm_msg && xfrm_msg[0])\n \t\t\tstrbuf_addf(&header, \"%s%s%s\\n\", set, xfrm_msg, reset);\n+\t\tmust_show_header = 1;\n \t}\n \telse {\n \t\tif (one->mode != two->mode) {\n \t\t\tstrbuf_addf(&header, \"%sold mode %06o%s\\n\", set, one->mode, reset);\n \t\t\tstrbuf_addf(&header, \"%snew mode %06o%s\\n\", set, two->mode, reset);\n+\t\t\tmust_show_header = 1;\n \t\t}\n \t\tif (xfrm_msg && xfrm_msg[0])\n \t\t\tstrbuf_addf(&header, \"%s%s%s\\n\", set, xfrm_msg, reset);\n@@ -1737,10 +1741,6 @@ static void builtin_diff(const char *name_a,\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} else if (diff_filespec_is_binary(one) &&\n-\t\t\t   diff_filespec_is_binary(two)) {\n-\t\t\tfprintf(o->file, \"%s\", header.buf);\n-\t\t\tstrbuf_reset(&header);\n \t\t}\n \t}\n \n@@ -1751,8 +1751,11 @@ static void builtin_diff(const char *name_a,\n \t\t\tdie(\"unable to read files to diff\");\n \t\t/* Quite common confusing case */\n \t\tif (mf1.size == mf2.size &&\n-\t\t    !memcmp(mf1.ptr, mf2.ptr, mf1.size))\n+\t\t    !memcmp(mf1.ptr, mf2.ptr, mf1.size)) {\n+\t\t\tif (must_show_header)\n+\t\t\t\tfprintf(o->file, \"%s\", header.buf);\n \t\t\tgoto free_ab_and_return;\n+\t\t}\n \t\tfprintf(o->file, \"%s\", header.buf);\n \t\tstrbuf_reset(&header);\n \t\tif (DIFF_OPT_TST(o, BINARY))\n@@ -1770,7 +1773,7 @@ static void builtin_diff(const char *name_a,\n \t\tstruct emit_callback ecbdata;\n \t\tconst struct userdiff_funcname *pe;\n \n-\t\tif (!DIFF_XDL_TST(o, WHITESPACE_FLAGS)) {\n+\t\tif (!DIFF_XDL_TST(o, WHITESPACE_FLAGS) || must_show_header) {\n \t\t\tfprintf(o->file, \"%s\", header.buf);\n \t\t\tstrbuf_reset(&header);\n \t\t}\n@@ -2383,8 +2386,10 @@ static void fill_metainfo(struct strbuf *msg,\n \t\t\t  struct diff_filespec *one,\n \t\t\t  struct diff_filespec *two,\n \t\t\t  struct diff_options *o,\n-\t\t\t  struct diff_filepair *p)\n+\t\t\t  struct diff_filepair *p,\n+\t\t\t  int *must_show_header)\n {\n+\t*must_show_header = 1;\n \tstrbuf_init(msg, PATH_MAX * 2 + 300);\n \tswitch (p->status) {\n \tcase DIFF_STATUS_COPIED:\n@@ -2412,7 +2417,7 @@ static void fill_metainfo(struct strbuf *msg,\n \t\t/* fallthru */\n \tdefault:\n \t\t/* nothing */\n-\t\t;\n+\t\t*must_show_header = 0;\n \t}\n \tif (one && two && hashcmp(one->sha1, two->sha1)) {\n \t\tint abbrev = DIFF_OPT_TST(o, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n@@ -2446,9 +2451,10 @@ static void run_diff_cmd(const char *pgm,\n {\n \tconst char *xfrm_msg = NULL;\n \tint complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;\n+\tint must_show_header = 0;\n \n \tif (msg) {\n-\t\tfill_metainfo(msg, name, other, one, two, o, p);\n+\t\tfill_metainfo(msg, name, other, one, two, o, p, &must_show_header);\n \t\txfrm_msg = msg->len ? msg->buf : NULL;\n \t}\n \n@@ -2467,7 +2473,8 @@ static void run_diff_cmd(const char *pgm,\n \t}\n \tif (one && two)\n \t\tbuiltin_diff(name, other ? other : name,\n-\t\t\t     one, two, xfrm_msg, o, complete_rewrite);\n+\t\t\t     one, two, xfrm_msg, must_show_header,\n+\t\t\t     o, complete_rewrite);\n \telse\n \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n }\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex e92eab0..935d101 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -438,6 +438,43 @@ test_expect_success 'whitespace-only changes not reported' '\n \ttest_cmp expect actual\n '\n \n+cat <<EOF >expect\n+diff --git a/x b/z\n+similarity index NUM%\n+rename from x\n+rename to z\n+index 380c32a..a97b785 100644\n+EOF\n+test_expect_success 'whitespace-only changes reported across renames' '\n+\tgit reset --hard &&\n+\tfor i in 1 2 3 4 5 6 7 8 9; do echo \"$i$i$i$i$i$i\"; done >x &&\n+\tgit add x &&\n+\tgit commit -m \"base\" &&\n+\tsed -e \"5s/^/ /\" x >z &&\n+\tgit rm x &&\n+\tgit add z &&\n+\tgit diff -w -M --cached |\n+\tsed -e \"/^similarity index /s/[0-9][0-9]*/NUM/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expected <<\\EOF\n+diff --git a/empty b/void\n+similarity index 100%\n+rename from empty\n+rename to void\n+EOF\n+\n+test_expect_success 'rename empty' '\n+\tgit reset --hard &&\n+\t>empty &&\n+\tgit add empty &&\n+\tgit commit -m empty &&\n+\tgit mv empty void &&\n+\tgit diff -w --cached -M >current &&\n+\ttest_cmp expected current\n+'\n+\n test_expect_success 'combined diff with autocrlf conversion' '\n \n \tgit reset --hard &&\n"},{"id":"142854","messageId":"201006030612.35931.chriscool@tuxfamily.org","threadId":"23906","inReplyTo":"7v4ohl9f8w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: fix \"git show -C -C\" output when renaming a binary file","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-06-03T04:12:35Z","receivedAt":"2010-06-03T04:12:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thursday 03 June 2010 01:38:55 Junio C Hamano wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > A bug was introduced in 3e97c7c6af2901cec63bf35fcd43ae3472e24af8\n> > (No diff -b/-w output for all-whitespace changes, Nov 19 2009)\n> > that made the lines:\n> >\n> > diff --git a/bar b/sub/bar\n> > similarity index 100%\n> > rename from bar\n> > rename to sub/bar\n> >\n> > disappear from \"git show -C -C\" output when file bar is a binary\n> > file.\n> >\n> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> > ---\n> >  diff.c                        |    4 +++\n> >  t/t4043-diff-rename-binary.sh |   45\n> > +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49\n> > insertions(+), 0 deletions(-)\n> >  create mode 100755 t/t4043-diff-rename-binary.sh\n> >\n> > diff --git a/diff.c b/diff.c\n> > index 494f560..0b00271 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -1737,6 +1737,10 @@ static void builtin_diff(const char *name_a,\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} else if (diff_filespec_is_binary(one) &&\n> > +\t\t\t   diff_filespec_is_binary(two)) {\n> > +\t\t\tfprintf(o->file, \"%s\", header.buf);\n> > +\t\t\tstrbuf_reset(&header);\n> >  \t\t}\n> \n> Actually, the logic to single out \"binary vs binary\" feels wrong, as one\n> of the cases the output that was overly squelched is this one:\n> \n> \t\t/* Quite common confusing case */\n> \t\tif (mf1.size == mf2.size &&\n>        \t            !memcmp(mf1.ptr, mf2.ptr, mf1.size))\n> \t\t\tgoto free_ab_and_return;\n> \t\tfprintf(o->file, \"%s\", header.buf);\n> \t\tstrbuf_reset(&header);\n> \n> Notice that we have accumulated the rename/copy/new/old in header but if\n> the blobs are the same, we discard it.\n> \n> Another case that 3e97c7c6 handled completely incorrectly is when -w/-b is\n> in effect and there is a rename or copy.  Because there won't be textual\n> diff emitted by fn_out_consume() at all, we lose the header information as\n> well.\n> \n> So I would say we would need something like this on top of your patch.\n\nI agree, your fix looks right.\n\nThanks,\nChristian.\n"}]}