mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <>
Cc: Git List <>, Thomas Rast <>,
	Junio C Hamano <>,
	Johannes Schindelin <>
Subject: Re: [PATCH 2/4] line-log: adjust start/end of ranges individually
Date: Sun, 5 Aug 2018 06:14:01 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Sat, Aug 4, 2018 at 6:18 PM Johannes Schindelin via GitGitGadget
<> wrote:
> 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 <>
> ---
> 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-> && 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.

  reply	other threads:[~2018-08-05 10:14 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-04 22:18 [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges Johannes Schindelin via GitGitGadget
2018-08-04 22:18 ` [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges Johannes Schindelin via GitGitGadget
2018-08-05  1:59   ` Jonathan Nieder
2018-08-06 10:27     ` Johannes Schindelin
2018-08-06 14:47       ` Jonathan Nieder
2018-08-06 15:33         ` Jonathan Nieder
2018-08-04 22:18 ` [PATCH 2/4] line-log: adjust start/end of ranges individually Johannes Schindelin via GitGitGadget
2018-08-05 10:14   ` Eric Sunshine [this message]
2018-08-05 10:57     ` Eric Sunshine
2018-08-06 12:52     ` Johannes Schindelin
2018-08-04 22:18 ` [PATCH 3/4] line-log: optimize ranges by joining them when possible Johannes Schindelin via GitGitGadget
2018-08-05  6:11   ` Junio C Hamano
2018-08-05  8:45   ` Andrei Rybak
2018-08-05 10:31     ` Eric Sunshine
2018-08-04 22:18 ` [PATCH 4/4] line-log: convert an assertion to a full BUG() call Johannes Schindelin via GitGitGadget
2018-08-05 10:42   ` Eric Sunshine
2018-08-06 13:14     ` Johannes Schindelin
2018-08-07  9:09       ` Eric Sunshine
2018-08-07 22:00         ` Eric Sunshine
2018-08-05 10:39 ` [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).