Re: [PATCH] diff: fix "git show -C -C" output when renaming a binary file

[Christian Couder <chriscool@tuxfamily.org>]

On Thursday 03 June 2010 01:38:55 Junio C Hamano wrote:
> 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.