Re: [PATCH 4/4] line-log: convert an assertion to a full BUG() call

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 7, 2018, 22:00 UTC
Message-ID: <CAPig+cTBUCytuqMZAZshPNzJ28s_vhWNKawamt1=EtNZVcYJsg@mail.gmail.com>
In-Reply-To: <CAPig+cQU-mcUBJGHeZjKeta0_ve8WMr_KL1ToO9j-d=-hS1a=w@mail.gmail.com>

On Tue, Aug 7, 2018 at 5:09 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 11 quoted lines

> On Mon, Aug 6, 2018 at 9:15 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > I think Andrei's assessment is wrong. The code could not test for that
> > earlier, as it did allow ranges to become "abutting" in the process, by
> > failing to merge them. So the invariant you talked about is more of an
> > invariant for the initial state.
>
> My understanding is that range_set_append() is intended to be strict
> by not allowing addition of a range which abuts an existing range
> (although, of course, the assert() checks only the last range, so it's
> not full-proof).

Ignore my parenthetical comment. It is clearly wrong.

Looking at this again, I see that there is some confusion. The comment in line-log.h says:

    /* New range must begin at or after end of last added range */
   void range_set_append(struct range_set *, long start, long end);

However, that comment was added by me in c0babbe695 (range-set: publish API for re-use by git-blame -L, 2013-08-06) -- five years and one day ago -- and it appears to be based upon a direct interpretation of the condition in the assert(), including its off-by-one error.

*But*, one of the invariants of range-set is that the ranges must _not_ abut one another, so the "at or after" is wrong; it should say instead something like "after, and not touching, the end of the last added range".

That invariant about having a gap between ranges is enforced deliberately by range_set_check_invariants(). It's also signaled implicitly by the fact that no callers of range_set_append() ever invoke sort_and_merge_range_set() after calling range_set_append().