Fix move ranges spanning different remove groups.

Review Request #5550 — Created Feb. 26, 2014 and submitted

Information

Review Board
release-2.0.x
9d5240e...

Reviewers

When a move range for inserts spanned remove groups on the left-hand
side, the resulting move ranges would appear to be offset in weird ways.

What was happening was that, while determining the range, we'd keep
seeing new remove groups on the left-hand side, which would cause us to
reset our ranges. The final remove group on the left-hand side would
account for the range length, and the range would appear to start on
that side at the top of that group. However, on the right-hand side,
it'd start at the correct place, but have the truncated range.

To fix this, we need to do a few things:

1. We need to glue the remove groups together better. When we see a
new group (such as when going from a delete to a replace), we now
check if it immediately follows the last move range we were working
with. If so, we use that instead.

2. We then needed to terminate those ranges by resetting prev_key when
we know we're not going to be continuing a range.

3. We need to store all groups within the range, and set the move
opcodes on all of them. We were previously only dealing with the
first (or sometimes last -- depending on the code path) group,
which would prevent us from being able to show the range in the
diff viewer.

4. Upon encountering a blank line, we need to be sure to add that
line's opcode group to the list of groups. Even if it's an "equal"
(which will now show up in a move range, but only where it's
appropriately bridging two consecutive ranges).

5. When drawing the move ranges, we can't rely on computations from
the previous line, because we only have the previous line within
that chunk. So now we store info on the start of a range along
with the to/from line number. That lets us simplify the move
range drawing quite nicely, and guarantees it reflects what's
computed.

To make all this a lot nicer, I've added a container for move ranges,
called MoveRange. This keeps track of the start and end, and the
affected groups. It's cleaner than rebuilding and indexing tuples
everywhere.

All existing unit tests passed.

Built a new unit test based on the diff we had in the wild that broke.
Visually confirmed its diff contents reproduced the same behavior by
overriding the buffers in the diff viewer. Hand-inputted the opcodes
based on what I saw in the diff viewer and what I expected, and the unit
test passed. The diff viewer also showed the correct behavior after the
change.

Tested with my other move detection review requests, and didn't see any
regressions.


david
  1. Ship It!

  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Loading...