Re: [RFC PATCH, WAS: "weird diff output?" 0/2] implement better chunk heuristics

[Jacob Keller <jacob.keller@gmail.com>]

On Fri, Apr 15, 2016 at 10:10 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Apr 15, 2016 at 10:02 AM, Stefan Beller <sbeller@google.com> wrote:
>>> @@ -458,11 +458,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>>>                          * the group.
>>>                          */
>>>                         while (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {
>>> +                               emptylines += is_emptyline(recs[ix - 1]->ptr);
>>> +
>>>                                 rchg[--ixs] = 1;
>>>                                 rchg[--ix] = 0;
>>>
>>> -                               has_emptyline |=
>>> -                                       starts_with_emptyline(recs[ix]->ptr);
>>
>> How is this fixing the segfault bug?
>> From my understanding ix should have the same value here as ix-1 above.
>>
>>>                                 /*
>>>                                  * This change might have joined two change groups,
>>>                                  * so we try to take this scenario in account by moving
>>> @@ -492,9 +492,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>>>                                 rchg[ixs++] = 0;
>>>                                 rchg[ix++] = 1;
>>>
>>> -                               has_emptyline |=
>>> -                                       starts_with_emptyline(recs[ix]->ptr);
>>> -
>>
>> Or would this fix the segfault bug?
>> I think we would need to have the check for empty lines
>> in both loops to be sure to cover the whole movable range.
>
> Actually we would only need to have the empty line count in the second loop as
> the first loop shifted it as much up(backwards) as possible, such that
> the hunk sits on one
> end of the movable range. The second loop would iterate over the whole
> range, so that
> would be the only place needed to count.

I agree that we can drop the first part and I am testing it now.

Thanks,
Jake