Re: [PATCH 5/8] xdl_change_compact(): fix compaction heuristic to adjust io

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2016, 18:43 UTC
Message-ID: <xmqqshuk2x3p.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <ae7590443737a3996ec4973fd868ce89dc78a576.1470259583.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> The code branch used for the compaction heuristic incorrectly forgot to
> keep io in sync while the group was shifted. I think that could have
> led to reading past the end of the rchgo array.

I had to read the first sentence three times as "incorrectly forgot" was a bit strange thing to say (as if there is a situation where 'forgetting to do' is the correct thing to do, but in that case we would phrase it to stress that not doing is a deliberate choice, e.g. 'refraining from doing'). Perhaps s/incorrectly // is the simplest readability improvement?

Show 17 quoted lines

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> I didn't actually try to verify the presence of a bug, because it
> seems like more work than worthwhile. But here is my reasoning:
>
> If io is not decremented correctly during one iteration of the outer
> `while` loop, then it will loose sync with the `end` counter. In
> particular it will be too large.
>
> Suppose that the next iterations of the outer `while` loop (i.e.,
> processing the next block of add/delete lines) don't have any sliders.
> Then the `io` counter would be incremented by the number of
> non-changed lines in xdf, which is the same as the number of
> non-changed lines in xdfo that *should have* followed the group that
> experienced the malfunction. But since `io` was too large at the end
> of that iteration, it will be incremented past the end of the
> xdfo->rchg array, and will try to read that memory illegally.

I agree with Peff that these should be in the log message.