Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

I started to work on this issue.
I've been debugging the example above. It's definitely not finished, but 
I've been asked to brain dump what I know for now. The interdiff is reproduced 
using steps from above. Lines numbers might be not exact as in code as I've 
been adding logging.

Iterating in loop opcode_generator.py line ~394

i_move_cur 6804
 4186: 6803
i_move_cur 6812
 4321:6938 after 
_find_longest_move_range

The same after _determine_move_range

in #491 rmeta is set so MoveRange has line 4115: 6942

This shouldn’t happen as this move is only two lines long and is 
overwriting a pretty long one

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

The problem is within algorithm of detecting a group move.
From opcode_generator.py#~350

   # The way we do that is to find each removed line that matches
   # this inserted line, and for each of those find out if there's
   # an existing move range that the found removed line
   # immediately follows. If there is, we update the existing
   # range.

If there is a MoveRange which starts with the same (2+) lines of code last 
choice is prefered.
I think that the solution could work along the following lines: 
  * create a temp MoveRange
  * continue adding to it until the following lines are the same
  * if the range is longer than existing one replace them entirely.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

I've written a failing test.
I couldn't make it any shorter I'm afraid, hence JSON files. 
It's just for illustrative purpose.

Changes to the code from above example:
 * Removed lines from inside of the moved block.
 * Removed some of the beginning and ending lines.
 * Removed all blocks except of the first one with the issue.

line 4115 becomes 16, 6732 -> 2633, 6943 -> 2669

I couldn't remove lines between new and old code (after old, before new), 
as the issue is no longer present in test.


Files:
- a.json
  
- b.json
  
- tests-diff.patch
  

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

Thanks for looking into all this, and all the information you've provided. 
I'm going to go through what you have and refresh my memory on the details of 
the algorithm.


Assigned to:
+ chipx86

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

The unit test was very helpful, thank you for that! I've spent some time 
today on this, and while I don't have a solution yet, it's becoming more clear. 
(I've had a lot distracting me today, and I'm a bit groggy from a rough night's 
sleep, but hey, progress is progress.)

I think I have a handle on what's going on, but want to verify some of my 
findings before I go into it too much. I'm hoping to have something helpful, 
maybe a fix, this weekend.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

I have a patch that still needs some additional testing, but if you could 
give it a try and see if it works for any cases you've hit (and standard cases 
that appeared to work fine), that would really help :)

I did test it with the review request in the description above, and it 
seems to produce the right results (I haven't inspected every character of move 
range but I did look for the obvious problems that were previously hit).

The attached patch contains the test data you supplied as well. However, I 
noticed that one of the lines in the test case you supplied was still pointing 
to an incorrect move line (`33 -> 2671` should have been `33 -> 2615`). I've 
fixed that in the attached diff.


Files:
- 0001-Fix-reuse-of-previously-used-move-detection-ranges.patch
  

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

We're definitely closer. Thank you.
Applying the patch made the first issue disappear (I've checked on "live" 
db on my local machine).
However, the second one - 
https://bugzilla.mozilla.org/show_bug.cgi?id=1251455#c3 remaines unchanged.
I think it might be a separate issue...

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

Awesome :) Glad it seems to work. The more testing we can get on this fix, 
the better. I think we'll be able to incorporate that into the next 2.0.x and 
2.5.x releases.

Yeah, this other bug is a separate issue, and a very silly one. This one is 
pure JavaScript. It's not differentiating between the left-hand side and 
right-hand side when finding the correct anchor. It just so happens that 
there's a line 220 on the right-hand side with a comment flag, which is taking 
precedence over the line 220 on the left-hand side for the anchor matching. 
I'll get this fixed.

I really appreciate all your help on this!

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

Latest version of this change (with an off-by-two fix) is up at 
https://reviews.reviewboard.org/r/8795/.


Status:
- Confirmed
+ PendingReview


Milestones:
+ Release-2.0.x
+ Release-2.5.x

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

The move flag anchor bug should be fixed at 
https://reviews.reviewboard.org/r/8796/.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

Pushed to release-2.0.x (151e98622de268c38548859569ce2a7715391f55)


Status:
- PendingReview
+ Fixed

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

This doesn't fix all of the issue. I'll create a new test. Do you like this 
ticket to be updated or should I create a new one?

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

There's a series of fixes up. Try the latest changes from release-2.5.x, 
and if it's still broken, I'll need a new test to work off of.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

Are they in any different file? I checked out the 3.0 and I haven't found 
anything different.
I'm unable to fully pull 3.0.x into the MozReview. I'm adding these changes 
manually.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

These are in 2.5.x. There are changes in a few files across I think 4 
fairly recent commits. They haven't yet shipped in a release.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

I've implemented the changes placed in `chunk_generator.py` [1], but the 
error from bug 1251455#c3 [2] still partially exists. I'm talking about the 
double "moved from 220" on the right hand side without the corresponding "moved 
to 220". I will create a test case in the style created here.
I've created a separate bug for the issue in bugzilla [3] as it seems to be 
a related, but different issue.

[1] https://reviewboard.mozilla.org/r/123930/diff/2
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1251455#c3 
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1352340

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

The diff you linked to in the most recent Bugzilla isn't running the line 
number fix we put in. The `data-line="..."` is used the old format of showing 
just the line number, rather than `moved-to-` or 
`moved-from-`. Same with `target=`. Can you verify the patch was 
applied? If this is a diff uploaded before the patch went in, then you're 
probably looking at a stale version in the cache.


Status:
- Fixed
+ NeedInfo

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

That commit is 2f9f515a198deb8f07070f97be24f60fce9f6842, btw.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

When I created the test in plain reviewboard I don't see anything wrong. I 
might have some change missing as it is applied by copy & paste.

It is not about linking two lines on the same side.
It is about displaying two "moved-from-220" (in line 76 and 103) and none 
of "moved-to-76" or "moved-to-103".

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

I've confirmed the bug locally. I'll aim to get a fix put together next 
week. Thanks!

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Christian Hammond]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

Small thing, but it would help going forward with this bug report and 
others. Rather than linking to Bugzilla, can you ensure that any information 
useful to us goes directly here? It's harder to follow when information is on 
an external bug tracker (particularly for users/admins/contributors), since it 
sort of splinters the discussion and makes it harder to search/triage/look for 
duplicates. We've had problems with people being confused about where 
discussion should go in the past, and important things getting missed.

This actually isn't an uncommon problem for us. We sometimes get people 
even storing details on a private bug tracker we don't have access to (not a 
problem here, obviously) and not updating us with any details we can use. We've 
had to make it policy that all details about a bug involving our components be 
posted here instead, to keep things sane.

I know you've been providing fantastic information here as well, so I 
_super_ appreciate that :) Just want to get it all in one place. I hope that 
makes sense.

Thanks again!


Status:
- NeedInfo
+ Confirmed

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

You're right - just browsed this ticket now and there was no mention of the 
commit 3 content

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.

Re: Review Board Ticket #4371: Diff move detection incorrectly matches source and target

[Piotr Zalewa]

--
To reply, visit https://hellosplat.com/s/beanbag/tickets/4371/
--

New update by smacleod
For Beanbag, Inc. > Review Board > Ticket #4371


Reply:

@chipx86.
Please share if/when you've got a failing test.

-- 
You received this message because you are subscribed to the Google Groups 
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at https://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.