Fix and improve move detection.
Review Request #4921 — Created Nov. 9, 2013 and submitted
Fix and improve move detection.
Move detection had some issues that were a result of a stale variable
being accessed. When setting metadata, a previous variable was used,
instead of the instance we were currently operating on. This led to a
bunch of move information getting tacked onto the wrong line and erasing
things. That's been fixed, so move information can be better trusted.Move detection was also picking up lots of little lines we really didn't
care about. It now has a threshold. It must be at least 2 lines of code,
or 1 line at least 20 characters in length. We may decide to tweak these
later on.There was also an off-by-one with move ranges. If a range ended with a
line that wasn't considered interesting enough (< 4 characters), but the
range otherwise passed validation, that uninteresting line would be
included. This isn't always bad (might catch an ending brace, for
example), but it's not intended, and shouldn't be provided unless we
design for it.I also made the move flags more consistent between the left-hand-side
and right-hand-side of the diff viewer. Both the "moved from" and "moved
to" flags are now anchored on the left of their side, so it's easier to
see what lines they span. Basically, I'm applying the new changes
recently made for the right-hand side to the left.
Unit tests pass.
Tested with the diff on /r/4919, which was full of junk and broken
move information before. It looks reasonable now.
| Description | From | Last Updated |
|---|---|---|
|
Care to add a negative case to this? |
 david
|
- Change Summary:
-
- Added test cases for the line range and length thresholds.
- Fixed an off-by-one with computed line ranges when the first line isn't interesting. It would still be included if other lines were.
- Cleaned up some variable names. A variable name was being reused in a statement, without any reason.
- Fixed some docstrings in tests.
- Description:
-
Fix and improve move detection.
Move detection had some issues that were a result of a stale variable
being accessed. When setting metadata, a previous variable was used, instead of the instance we were currently operating on. This led to a bunch of move information getting tacked onto the wrong line and erasing things. That's been fixed, so move information can be better trusted. Move detection was also picking up lots of little lines we really didn't
care about. It now has a threshold. It must be at least 2 lines of code, or 1 line at least 20 characters in length. We may decide to tweak these later on. + There was also an off-by-one with move ranges. If a range ended with a
+ line that wasn't considered interesting enough (< 4 characters), but the + range otherwise passed validation, that uninteresting line would be + included. This isn't always bad (might catch an ending brace, for + example), but it's not intended, and shouldn't be provided unless we + design for it. + I also made the move flags more consistent between the left-hand-side
and right-hand-side of the diff viewer. Both the "moved from" and "moved to" flags are now anchored on the left of their side, so it's easier to see what lines they span. Basically, I'm applying the new changes recently made for the right-hand side to the left. - Diff:
-
Revision 2 (+98 -44)
- Added Files: