Re: [PATCH 2/4] line-log: adjust start/end of ranges individually

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 5, 2018, 10:14 UTC
Message-ID: <CAPig+cRWcFVbA76_HT2iVD16bsUmbWdCgk_07rmiGneM5czdOQ@mail.gmail.com>
In-Reply-To: <7f92d92154143127734a638e41e064adce46a2e2.1533421100.git.gitgitgadget@gmail.com>

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

Show 24 quoted lines

> When traversing commits and adjusting the ranges, things can get really
> tricky. For example, when the line range of interest encloses several
> hunks of a commit, the line range can actually shrink.
>
> Currently, range_set_shift_diff() does not anticipate that scenario and
> blindly adjusts start and end by the same offset ("shift" the range).
> [...]
> Let's fix this by adjusting the start and the end offsets individually.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/line-log.c b/line-log.c
> @@ -438,7 +438,13 @@ static void range_set_shift_diff(struct range_set *out,
>                                 - (target[j].end-target[j].start);
>                         j++;
>                 }
> -               range_set_append(out, src[i].start+offset, src[i].end+offset);
> +               start_offset = offset;
> +               while (j < diff->target.nr && src[i].end > target[j].end) {
> +                       offset += (parent[j].end-parent[j].start)
> +                               - (target[j].end-target[j].start);
> +                       j++;
> +               }
> +               range_set_append(out, src[i].start+start_offset, src[i].end+offset);

I'm still trying to wrap my head around the original code, so I'm not even at the point of being able to say if this fix is correct. What happens if the "start_offset" loop consumes all of 'j' before it even gets to the new loop? Why does the new loop use '>' whereas the existing uses '>='?

Having said that, a much easier fix is to use range_set_append_unsafe() here, and then at the bottom of the loop, invoke 'sort_and_merge_range_set(out)' to restore range-set invariants and ensure that neighboring ranges are coalesced. Not only does that resolve the crash and other weird behavior, but it means you don't have to add a special-case to range_set_append(), thus the fix becomes simpler overall.

Aside from simplicity, I think the suggested use of range_set_append_unsafe() and sort_and_merge_range_set() _is_ the correct fix anyhow because this code isn't taking care to ensure that the range, after applying 'offset', doesn't abut or overlap with an earlier range, and sort_and_merge_range_set() is meant to be used exactly in cases like this when invariants may be broken.

So, while the suggested fix is simpler and "better" and fixes the crash, that doesn't necessarily mean that the values computed here are actually correct. As noted, I'm still trying to grok the computation of these values, but that's a separate issue from the crash itself.