Re: [PATCH] diff: fix "git show -C -C" output when renaming a binary file
- From
Christian Couder <chriscool@tuxfamily.org>
- Date
- Jun 3, 2010, 04:12 UTC
- Message-ID
- <201006030612.35931.chriscool@tuxfamily.org>
- In-Reply-To
- <7v4ohl9f8w.fsf@alter.siamese.dyndns.org>
On Thursday 03 June 2010 01:38:55 Junio C Hamano wrote:
Show 54 quoted lines
> Christian Couder <chriscool@tuxfamily.org> writes:
> > A bug was introduced in 3e97c7c6af2901cec63bf35fcd43ae3472e24af8
> > (No diff -b/-w output for all-whitespace changes, Nov 19 2009)
> > that made the lines:
> >
> > diff --git a/bar b/sub/bar
> > similarity index 100%
> > rename from bar
> > rename to sub/bar
> >
> > disappear from "git show -C -C" output when file bar is a binary
> > file.
> >
> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> > ---
> > diff.c | 4 +++
> > t/t4043-diff-rename-binary.sh | 45
> > +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49
> > insertions(+), 0 deletions(-)
> > create mode 100755 t/t4043-diff-rename-binary.sh
> >
> > diff --git a/diff.c b/diff.c
> > index 494f560..0b00271 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -1737,6 +1737,10 @@ static void builtin_diff(const char *name_a,
> > textconv_one, textconv_two, o);
> > o->found_changes = 1;
> > goto free_ab_and_return;
> > + } else if (diff_filespec_is_binary(one) &&
> > + diff_filespec_is_binary(two)) {
> > + fprintf(o->file, "%s", header.buf);
> > + strbuf_reset(&header);
> > }
>
> Actually, the logic to single out "binary vs binary" feels wrong, as one
> of the cases the output that was overly squelched is this one:
>
> /* Quite common confusing case */
> if (mf1.size == mf2.size &&
> !memcmp(mf1.ptr, mf2.ptr, mf1.size))
> goto free_ab_and_return;
> fprintf(o->file, "%s", header.buf);
> strbuf_reset(&header);
>
> Notice that we have accumulated the rename/copy/new/old in header but if
> the blobs are the same, we discard it.
>
> Another case that 3e97c7c6 handled completely incorrectly is when -w/-b is
> in effect and there is a rename or copy. Because there won't be textual
> diff emitted by fn_out_consume() at all, we lose the header information as
> well.
>
> So I would say we would need something like this on top of your patch.I agree, your fix looks right.
Thanks, Christian.