1251455 - diff viewer is lying about moved chunks

Status: RESOLVED WONTFIX

(MozReview Graveyard :: Review Board: Upstream, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm trying to review https://reviewboard.mozilla.org/r/35163/diff/4 , and the diff viewer in MozReview is just blatantly lying.  (It's a classic example of a patch where the diff viewer in MozReview should be better -- lots of moved code, and I'm happy to let MozReview check that chunks are just moved.)

However, if you look at property_database.js, there's a chunk in the old code under "mask": at line 4115 that says "moved to line 6941".  If you click that and go to line 6941 in the new code, you'll see a chunk that looks nothing like it (4 lines long rather than 71 lines long, although the first 3 lines are the same).  So it's lying about both the destination of the move, and the fact that the new code at 6941 is a "move" from elsewhere in the file.  If you scroll up, you will in fact find the chunk -- it was moved to 6732 (and that chunk is 71 lines long).

So there are two problems here:
 * on the left (old) side, the chunk at 4115 should say it was moved to 6732 rather than to 6941
 * on the right (new) side, the chunk at 6941 should not indicate that it was moved from 4115


This bug can lead to unreviewed code landing in our repositories, since the diff viewer is claiming code is existing when it is, in fact, not existing code.

Comment 1

[Mark Côté [:mcote]]

Wow, yeah, that's all kinds of broken.  smacleod, this is another thing we need to fix asap.

Comment 2

[Steven MacLeod [:smacleod]]

I'll look into this

Comment 3

[:Gijs (he/him)]

Steven suggested I post something here relating to what I'm seeing. I don't know if this is an accident or not, but here goes:

https://reviewboard.mozilla.org/r/56584/diff/4#17

Scroll down to line 76 on the right-hand-side of the diff. It says "moved from line 220".

Clicking that link goes to line 220 on the right-hand-side of the diff (too), which isn't at all like the first chunk you were looking at. It says "moved from line 154" and that link works correctly.

However, if you scroll back up to what was line 220 on the *left-hand-side* of the diff, you will in fact find a line that looks the same. So I think the annotation is correct, but the link points to the wrong point in the diff.

Another fun thing is that if you scroll down further than before, to line 103 on the right-hand-side, that *also* says "moved from line 220", for the same line, with the same problem on the link. Then on line 109 (right-hand-side), we see a pointer to line 219, again pointing to that line on the right-hand-side, for a 2-line hunk thus including line 220. :-\

The line pointers should go to the right line, and ideally there should be some indication that a line that occurred once in the old version of the file has now been duplicated thrice over (with the comment reproduced once).

Comment 4

[Piotr Zalewa [:zalun]]

I've started to work on the bug. I think the wrong "jumps" are created in ReviewBoard's opcode_generator.py file. Either _find_longes_move_range or _determine_move_range are for some reason adding wrong line numbers to MoveRange object. I will discuss it in more detail in the bug mentioned above (https://hellosplat.com/s/beanbag/tickets/4371/).

Comment 5

[Piotr Zalewa [:zalun]]

Issue is now fixed upstream in ReviewBoard. I will close the bug when changes will be pulled to MozReview.
"Pushed to release-2.0.x (151e986)"

Comment 6

[Piotr Zalewa [:zalun]]

I will implement the changes from upstream into our fork. This is a temporary solution until we will upgrade the fork.

Comment 7

[Piotr Zalewa [:zalun]]

Solution provided in the upstream is not solving all of the problems raised above. We might have to split the bug as these might be different issues.

Comment 8

[Piotr Zalewa [:zalun]]

Attachment: mozreview: implement moved chunk upstream fix (bug 1251455)

Moved blocks are badly recognized. Sometimes, if a similar shorter block of code is found interdiff mistakenly choses it.

Moved chunks are recognized, but are overwritten by the latest similar blocks. The fix is to remember the moved chunks and skip further recognition for already processed blocks.
Another issue solved by this code is to differentiate left and right side of the move blocks. If there was a move from line number `A` to `X` and from line number `Y` line to `A`, left and right `A` lines where the same for the "jump" action. This could lead to link wrong blocks.
This fix is implemented in the upstream. It needs to be removed before upgrading reviewboard.

Review commit: https://reviewboard.mozilla.org/r/123930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123930/

Comment 9

[Piotr Zalewa [:zalun]]

Comment on attachment 8851663 [details]
mozreview: implement moved chunk upstream fix (bug 1251455)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123930/diff/1-2/