From: Dan McGee <dpmcgee@gmail.com>
Date: Sat, 02 Apr 2011 18:38:21 GMT
Subject: Fwd: [PATCH 1/5] diff_tree_sha1: skip diff_tree if old == new
Message-ID: <BANLkTi=hJm4ax__5DDCvK9VdLcNxVO2bVA@mail.gmail.com>
In-Reply-To: <AANLkTinPSqDPdGi5nA3sH1D2wMSW1SQc+5gRqdLy++y0@mail.gmail.com>

Forgot to forward this to the list as well, I apologize.

On Fri, Apr 1, 2011 at 5:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> This was seen to happen in some invocations of git-log with a filtered
>> path. Only do it if we are not recursively descending, as otherwise we
>> mess with copy and rename detection in full tree moves.
>
> There is no code that corresponds to your "Only do it..." description in
> your patch, though.  The existing code already takes care of that part
> with or without your patch, no?

Damn, I forgot to update the message- see below.

>> diff --git a/tree-diff.c b/tree-diff.c
>> index 76f83fc..ab90f1a 100644
>> --- a/tree-diff.c
>> +++ b/tree-diff.c
>> @@ -286,6 +286,9 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
>>       unsigned long size1, size2;
>>       int retval;
>>
>> +     if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(old, new))
>> +             return 0;
>> +
>
> I am very curious why this patch makes a difference; doesn't an existing
> test in compare_tree_entry() oalready cull extra recursion?  There is:

This was originally testing RECURSIVE; however I discovered that was
not the culprit to my failed tests.

t9300-fastimport.sh was failing on "copy then modify subdirectory" due
to the full info not being loaded for the before sha1 in that test-
instead of showing the fcf778cda ... C100 part (this is just the first
line of expected, all were the same), it was 000000 ... A. once I
added the above fallthrough to not shortcut if this option was
enabled, things worked fine and all tests passed.

>        if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) &&
>                mode1 == mode2)
>                return 0;
>
> before a recursive call to diff_tree_sha1() to dig deeper.
>

I'm not totally sure why this check wasn't working, but without the
above exception my patch definitely broke tests.

-Dan