Re: [RFC PATCH, WAS: "weird diff output?"] Implement better chunk heuristics.

From: Stefan Beller <sbeller@google.com>
Date: Apr 15, 2016, 03:33 UTC
Message-ID: <CAGZ79kbgYkjbpnk8LTOyHPRPAKi2s0p+iRk6FkWCN2KpHELgVA@mail.gmail.com>
In-Reply-To: <xmqqbn5bei7x.fsf@gitster.mtv.corp.google.com>

On Thu, Apr 14, 2016 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>>
>> +static int starts_with_emptyline(const char *recs)
>> +{
>> +     return recs[0] == '\n'; /* CRLF not covered here */
>> +}
>> +
>> +
>
> That's "is-empty-line", not "starts-with" ;-)

heh, ok. To understand the code, I was debugging it and looking at the pointers `recs[ix - 1]->ptr` which is pointing into the text file, i.e. when printing it in the debugger it would read

    '\n\tbla\n\nfoo\n ...'

so I found that a proper description at the time.

Show 23 quoted lines

>
>> +
>> +             /*
>> +              * If a group can be moved back and forth, see if there is an
>> +              * empty line in the moving space. If there is an empty line,
>> +              * make sure the last empty line is the end of the group.
>> +              *
>> +              * As we shifted the group forward as far as possible, we only
>> +              * need to shift it back if at all.
>> +              */
>
> Sounds sensible.
>
>> +             if (has_emptyline) {
>> +                     while (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&
>> +                            xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags) &&
>> +                            !starts_with_emptyline(recs[ix - 1]->ptr)) {
>
> You probably want to wrap the "hash compares equal and recmatch does
> say they are the same" into a helper function (to be automatically
> inlined by the compiler) to make it more readable here.  I think
> is-empty is a lot cheaper than the recmatch so that should probably
> be done earlier in the && chain.

ok, will do. Given that xdiff upstream and our code diverged over the years, I could apply this helper function at other places in the code as well.

Show 9 quoted lines

>
>> +                             rchg[--ixs] = 1;
>> +                             rchg[--ix] = 0;
>> +
>> +                             /*
>> +                              * This change did not join two change groups,
>> +                              * as we did that before already, so there is no
>
> Sorry, cannot quite parse the part before "already".

I think to drop this comment in the final version of this patch. As I `wrote` this loop by copying it from above, I tried justifying each change to it. (More to prove to myself I understood the code)

will drop this comment.

Show 11 quoted lines

>
>> +                              * need to adapt the other-file, i.e.
>> +                              * running
>> +                              *     for (; rchg[ixs - 1]; ixs--);
>> +                              *     while (rchgo[--ixo]);
>> +                              */
>> +                     }
>> +             }
>>       }
>>
>>       return 0;