Re: [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 5, 2018, 10:39 UTC
Message-ID: <CAPig+cS-b973NzRvvcKyx6t5ChO_VDf=Bs8zoE-B_iPa3HWv5g@mail.gmail.com>
In-Reply-To: <pull.15.git.gitgitgadget@gmail.com>

On Sat, Aug 4, 2018 at 6:18 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 9 quoted lines

> Now, I am fairly certain that the changes are correct, but given my track
> record with off-by-one bugs (and once even an off-by-two bug), I would
> really appreciate some thorough review of this code, in particular the
> second one that is the actual bug fix. I am specifically interested in
> reviews from people who know line-log.c pretty well and can tell me whether
> the src[i].end > target[j].end is correct, or whether it should actually
> have been a >= (I tried to wrap my head around this, but I would feel more
> comfortable if a domain expert would analyze this, whistling, and looking
> Eric's way).

Although I fixed a number of bugs in basic range-set manipulation code (5 years ago), I have no experience with or knowledge of the code actually dealing with range-set manipulation in relation to diffs, so I'm not a domain expert by any means, and I'm _still_ trying to wrap my head around that code.

That said, I left some comments on the individual patches suggesting a simpler and more correct fix for the crash. (Despite that suggestion for fixing the crash, I still can't say whether the actual computations performed by the existing code are correct, since I still don't understand that aspect of it.)