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.

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

[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

[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

[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:

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:

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:

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:

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:

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:

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:

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

[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:

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:

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

[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:

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:

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

[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 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