diff --git a/reviewboard/diffviewer/opcode_generator.py b/reviewboard/diffviewer/opcode_generator.py index a13b550370f42fdff0e3f70870bf5e39bb15aeb1..79d1339e7c0bc39c35494e3b8b34c129c39979a6 100644 --- a/reviewboard/diffviewer/opcode_generator.py +++ b/reviewboard/diffviewer/opcode_generator.py @@ -10,6 +10,9 @@ class DiffOpcodeGenerator(object): ALPHANUM_RE = re.compile(r'\w') WHITESPACE_RE = re.compile(r'\s') + MOVE_PREFERRED_MIN_LINES = 2 + MOVE_MIN_LINE_LENGTH = 20 + def __init__(self, differ, filediff=None, interfilediff=None): self.differ = differ self.filediff = filediff @@ -252,16 +255,16 @@ class DiffOpcodeGenerator(object): # only increment i_move_cur by one, because i_move_cur # already factored in the + 1 by being at the end of # the while loop. - i_move_range = range(i_move_range[0] + 1, - i_move_cur + 1) - r_move_range = range(r_move_range[0] + 1, - r_move_range[1] + 2) + i_range = range(i_move_range[0] + 1, + i_move_cur + 1) + r_range = range(r_move_range[0] + 1, + r_move_range[1] + 2) - rmeta = rgroup[-1] + rmeta = r_move_range[2][-1] rmeta.setdefault('moved-to', {}).update( - dict(zip(r_move_range, i_move_range))) + dict(zip(r_range, i_range))) imeta.setdefault('moved-from', {}).update( - dict(zip(i_move_range, r_move_range))) + dict(zip(i_range, r_range))) # Reset the state for the next range. prev_key = None @@ -326,13 +329,23 @@ class DiffOpcodeGenerator(object): line = line.strip() if line: - if new_end_i is None: - new_end_i = end_i - i - if len(line) >= 4 and self.ALPHANUM_RE.search(line): valid = True + + if new_end_i is None or valid: + new_end_i = end_i - i + + if valid: break + # Accept this if there's more than one line or if the first + # line is long enough, in order to filter out small bits of garbage. + valid = ( + valid and + (new_end_i - r_move_range[0] + 1 >= + self.MOVE_PREFERRED_MIN_LINES or + len(self.differ.a[r_move_range[0]]) >= self.MOVE_MIN_LINE_LENGTH)) + if not valid: return None diff --git a/reviewboard/diffviewer/tests.py b/reviewboard/diffviewer/tests.py index f1f7924c6e5a1d3a9708b8a7b176b251c735410a..6d815eb173d8a245f926ef701979ff21e93e3015 100644 --- a/reviewboard/diffviewer/tests.py +++ b/reviewboard/diffviewer/tests.py @@ -176,7 +176,7 @@ class InterestingLinesTest(TestCase): return result -class DiffParserTest(unittest.TestCase): +class DiffParserTest(TestCase): PREFIX = os.path.join(os.path.dirname(__file__), 'testdata') def diff(self, options=''): @@ -293,7 +293,6 @@ class DiffParserTest(unittest.TestCase): 29: 16, 30: 17, 31: 18, - 32: 19, } ], [ @@ -302,24 +301,23 @@ class DiffParserTest(unittest.TestCase): 16: 29, 17: 30, 18: 31, - 19: 32, } ]) def test_move_detection_with_replace_lines(self): - """Testing dfif viewer move detection with replace lines""" + """Testing diff viewer move detection with replace lines""" self._test_move_detection( [ - 'this is line 1', - '----------', - '----------', - 'this is line 2', + 'this is line 1, and it is sufficiently long', + '-------------------------------------------', + '-------------------------------------------', + 'this is line 2, and it is sufficiently long', ], [ - 'this is line 2', - '----------', - '----------', - 'this is line 1', + 'this is line 2, and it is sufficiently long', + '-------------------------------------------', + '-------------------------------------------', + 'this is line 1, and it is sufficiently long', ], [ {1: 4}, @@ -332,7 +330,7 @@ class DiffParserTest(unittest.TestCase): ) def test_move_detection_with_adjacent_regions(self): - """Testing dfif viewer move detection with adjacent regions""" + """Testing diff viewer move detection with adjacent regions""" self._test_move_detection( [ '1. Lorem ipsum dolor sit amet, consectetur adipiscing elit.', @@ -370,6 +368,68 @@ class DiffParserTest(unittest.TestCase): ], ) + def test_move_detection_single_line_thresholds(self): + """Testing diff viewer move detection with a single line and + line length threshold + """ + self._test_move_detection( + [ + '0123456789012345678', + '----', + '----', + 'abcdefghijklmnopqrst', + ], + [ + 'abcdefghijklmnopqrst', + '----', + '----', + '0123456789012345678', + ], + [ + {1: 4}, + ], + [ + {4: 1}, + ] + ) + + def test_move_detection_multi_line_thresholds(self): + """Testing diff viewer move detection with a multiple lines and + line count threshold + """ + self._test_move_detection( + [ + '123', + '456', + '789', + 'ten', + 'abcdefghijk', + 'lmno', + 'pqr', + ], + [ + 'abcdefghijk', + 'lmno', + 'pqr', + '123', + '456', + '789', + 'ten', + ], + [ + { + 1: 5, + 2: 6, + }, + ], + [ + { + 5: 1, + 6: 2, + }, + ] + ) + def test_line_counts(self): """Testing DiffParser with insert/delete line counts""" diff = ( diff --git a/reviewboard/static/rb/css/diffviewer.less b/reviewboard/static/rb/css/diffviewer.less index 7a9a8263c8a9f5276e4d3fd6e3f8d167911ea786..262c94df1214b3ed5580e4e999a0aff4124aac24 100644 --- a/reviewboard/static/rb/css/diffviewer.less +++ b/reviewboard/static/rb/css/diffviewer.less @@ -177,7 +177,7 @@ } .moved-to { - border-right: @moved-border-width @diff-moved-to-bg-color solid !important; + border-left: @moved-border-width @diff-moved-to-bg-color solid !important; .moved-flag { background-color: @diff-moved-to-bg-color !important; @@ -207,30 +207,11 @@ .moved-to-start, .moved-from-start { .moved-flag { - padding: 3px 1em; - } - } - - .moved-from-start { - .moved-flag { - padding-left: 0.5em; + padding: 3px 1em 3px 0.5em; .border-radius(0, 8px, 8px, 0); } } - .moved-to-start { - text-align: right; - - pre { - text-align: left; - } - - .moved-flag { - padding-right: 0.5em; - .border-radius(8px, 0, 0, 8px); - } - } - tbody { tr { &.selected {