diff --git a/reviewboard/datagrids/columns.py b/reviewboard/datagrids/columns.py index d2ddb6fa7c30b1a9fc981c32e6f7d5269791d124..67dbe577b60e9b11f051eda817c42e780cdec8ea 100644 --- a/reviewboard/datagrids/columns.py +++ b/reviewboard/datagrids/columns.py @@ -530,12 +530,9 @@ class DiffSizeColumn(Column): except ObjectDoesNotExist: return '' - insert_count = 0 - delete_count = 0 - for filediff in diffset.files.all(): - insert_count += filediff.insert_count - delete_count += filediff.delete_count - + counts = diffset.get_total_line_counts() + insert_count = counts['raw_insert_count'] + delete_count = counts['raw_delete_count'] result = [] if insert_count: diff --git a/reviewboard/diffviewer/admin.py b/reviewboard/diffviewer/admin.py index f4277bb23e1fb083951f5a0669f7a6747cd71547..bc71056966432d1e48c5110631ad5e8671d89f45 100644 --- a/reviewboard/diffviewer/admin.py +++ b/reviewboard/diffviewer/admin.py @@ -15,15 +15,19 @@ class FileDiffAdmin(admin.ModelAdmin): 'fields': ('diffset', 'status', 'binary', ('source_file', 'source_revision'), ('dest_file', 'dest_detail'), - 'insert_count', - 'delete_count', 'diff', 'parent_diff') }), + (_('Internal State'), { + 'description': _('

This is advanced state that should not be ' + 'modified unless something is wrong.

'), + 'fields': ('extra_data',), + 'classes': ['collapse'], + }), ) list_display = ('source_file', 'source_revision', 'dest_file', 'dest_detail') raw_id_fields = ('diffset', 'diff_hash', 'parent_diff_hash') - readonly_fields = ('diff', 'parent_diff', 'insert_count', 'delete_count') + readonly_fields = ('diff', 'parent_diff') def diff(self, filediff): return self._style_diff(filediff.diff) diff --git a/reviewboard/diffviewer/chunk_generator.py b/reviewboard/diffviewer/chunk_generator.py index 8dd2df137c843d3f74e77519f30bf210f6598dfd..24f416ccad42e66c52ef56a125c2f05a42f20727 100644 --- a/reviewboard/diffviewer/chunk_generator.py +++ b/reviewboard/diffviewer/chunk_generator.py @@ -243,6 +243,13 @@ class DiffChunkGenerator(object): self.filediff, self.interfilediff) + counts = { + 'equal': 0, + 'replace': 0, + 'insert': 0, + 'delete': 0, + } + for tag, i1, i2, j1, j2, meta in opcodes_generator: old_lines = markup_a[i1:i2] new_lines = markup_b[j1:j2] @@ -255,6 +262,8 @@ class DiffChunkGenerator(object): a[i1:i2], b[j1:j2], old_lines, new_lines) self._cur_meta = None + counts[tag] += num_lines + if tag == 'equal' and num_lines > collapse_threshold: last_range_start = num_lines - context_num_lines @@ -279,6 +288,20 @@ class DiffChunkGenerator(object): log_timer.done() + if not self.interfilediff: + insert_count = counts['insert'] + delete_count = counts['delete'] + replace_count = counts['replace'] + equal_count = counts['equal'] + + self.filediff.set_line_counts( + insert_count=insert_count, + delete_count=delete_count, + replace_count=replace_count, + equal_count=equal_count, + total_line_count=insert_count + delete_count + + replace_count + equal_count) + def _get_enable_syntax_highlighting(self, old, new, a, b): """Returns whether or not we'll be enabling syntax highlighting. diff --git a/reviewboard/diffviewer/managers.py b/reviewboard/diffviewer/managers.py index 462e765b8afbfe02f384bf1544d4dff5ca8de6ff..1f9ff55d966a2ef35ba85226b61bef01a0f29be8 100644 --- a/reviewboard/diffviewer/managers.py +++ b/reviewboard/diffviewer/managers.py @@ -238,7 +238,8 @@ class DiffSetManager(models.Manager): parent_diff=parent_content, binary=f.binary, status=status) - filediff.set_line_counts(f.insert_count, f.delete_count) + filediff.set_line_counts(raw_insert_count=f.insert_count, + raw_delete_count=f.delete_count) if save: filediff.save() diff --git a/reviewboard/diffviewer/models.py b/reviewboard/diffviewer/models.py index 02e8826e0243ece42c0b11e2645ae8fd1c0dbc99..1dde79708d4113232742804303259c4f9695071c 100644 --- a/reviewboard/diffviewer/models.py +++ b/reviewboard/diffviewer/models.py @@ -8,6 +8,7 @@ from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ from djblets.db.fields import Base64Field, JSONField +from djblets.util.compat import six from reviewboard.diffviewer.errors import DiffParserError from reviewboard.diffviewer.managers import (FileDiffDataManager, @@ -56,6 +57,7 @@ class FileDiffData(models.Model): try: files = tool.get_parser(self.binary).parse() + if len(files) != 1: raise DiffParserError( 'Got wrong number of files (%d)' % len(files)) @@ -68,7 +70,9 @@ class FileDiffData(models.Model): file_info = files[0] self.insert_count = file_info.insert_count self.delete_count = file_info.delete_count - self.save() + + if self.pk: + self.save(update_fields=['extra_data']) @python_2_unicode_compatible @@ -176,37 +180,87 @@ class FileDiff(models.Model): parent_diff = property(_get_parent_diff, _set_parent_diff) - @property - def insert_count(self): - if not self.diff_hash: - self._migrate_diff_data() + def get_line_counts(self): + """Returns the stored line counts for the diff. - if self.diff_hash.insert_count is None: - self._recalculate_line_counts(self.diff_hash) + This will return all the types of line counts that can be set: - return self.diff_hash.insert_count + * ``raw_insert_count`` + * ``raw_delete_count`` + * ``insert_count`` + * ``delete_count`` + * ``replace_count`` + * ``equal_count`` + * ``total_line_count`` - @property - def delete_count(self): - if not self.diff_hash: - self._migrate_diff_data() + These are not all guaranteed to have values set, and may instead be + None. Only ``raw_insert_count``, ``raw_delete_count`` + ``insert_count``, and ``delete_count`` are guaranteed to have values + set. - if self.diff_hash.delete_count is None: - self._recalculate_line_counts(self.diff_hash) + If there isn't a processed number of inserts or deletes stored, + then ``insert_count`` and ``delete_count`` will be equal to the + raw versions. + """ + if ('raw_insert_count' not in self.extra_data or + 'raw_delete_count' not in self.extra_data): + if not self.diff_hash: + self._migrate_diff_data() - return self.diff_hash.delete_count + if self.diff_hash.insert_count is None: + self._recalculate_line_counts(self.diff_hash) - def set_line_counts(self, insert_count, delete_count): - """Sets the insert/delete line count on the FileDiff.""" - if not self.diff_hash: + self.extra_data.update({ + 'raw_insert_count': self.diff_hash.insert_count, + 'raw_delete_count': self.diff_hash.delete_count, + }) + + if self.pk: + self.save(update_fields=['extra_data']) + + raw_insert_count = self.extra_data['raw_insert_count'] + raw_delete_count = self.extra_data['raw_delete_count'] + + return { + 'raw_insert_count': raw_insert_count, + 'raw_delete_count': raw_delete_count, + 'insert_count': self.extra_data.get('insert_count', + raw_insert_count), + 'delete_count': self.extra_data.get('delete_count', + raw_delete_count), + 'replace_count': self.extra_data.get('replace_count'), + 'equal_count': self.extra_data.get('equal_count'), + 'total_line_count': self.extra_data.get('total_line_count'), + } + + def set_line_counts(self, raw_insert_count=None, raw_delete_count=None, + insert_count=None, delete_count=None, + replace_count=None, equal_count=None, + total_line_count=None): + """Sets the line counts on the FileDiff. + + There are many types of useful line counts that can be set. + + ``raw_insert_count`` and ``raw_delete_count`` correspond to the + raw inserts and deletes in the actual patch, which will be set both + in this FileDiff and in the associated FileDiffData. + + The other counts are stored exclusively in FileDiff, as they are + more render-specific. + """ + updated = False + + if not self.diff_hash_id: # This really shouldn't happen, but if it does, we should handle # it gracefully. logging.warning('Attempting to call set_line_counts on ' 'un-migrated FileDiff %s' % self.pk) self._migrate_diff_data(False) - if (self.diff_hash.insert_count is not None and - self.diff_hash.insert_count != insert_count): + if (raw_insert_count is not None and + self.diff_hash.insert_count is not None and + self.diff_hash.insert_count != insert_count): + # Allow overriding, but warn. This really shouldn't be called. logging.warning('Attempting to override insert count on ' 'FileDiffData %s from %s to %s (FileDiff %s)' % (self.diff_hash.pk, @@ -214,8 +268,10 @@ class FileDiff(models.Model): insert_count, self.pk)) - if (self.diff_hash.delete_count is not None and - self.diff_hash.delete_count != delete_count): + if (raw_delete_count is not None and + self.diff_hash.delete_count is not None and + self.diff_hash.delete_count != delete_count): + # Allow overriding, but warn. This really shouldn't be called. logging.warning('Attempting to override delete count on ' 'FileDiffData %s from %s to %s (FileDiff %s)' % (self.diff_hash.pk, @@ -223,9 +279,33 @@ class FileDiff(models.Model): delete_count, self.pk)) - self.diff_hash.insert_count = insert_count - self.diff_hash.delete_count = delete_count - self.diff_hash.save() + if raw_insert_count is not None or raw_delete_count is not None: + # New raw counts have been provided. These apply to the actual + # diff file itself, and will be common across all diffs sharing + # the diff_hash instance. Set it there. + if raw_insert_count is not None: + self.diff_hash.insert_count = raw_insert_count + self.extra_data['raw_insert_count'] = raw_insert_count + updated = True + + if raw_delete_count is not None: + self.diff_hash.delete_count = raw_delete_count + self.extra_data['raw_delete_count'] = raw_delete_count + updated = True + + self.diff_hash.save() + + for key, cur_value in (('insert_count', insert_count), + ('delete_count', delete_count), + ('replace_count', replace_count), + ('equal_count', equal_count), + ('total_line_count', total_line_count)): + if cur_value is not None and cur_value != self.extra_data.get(key): + self.extra_data[key] = cur_value + updated = True + + if updated and self.pk: + self.save(update_fields=['extra_data']) def _hash_hexdigest(self, diff): hasher = hashlib.sha1() @@ -303,6 +383,19 @@ class DiffSet(models.Model): objects = DiffSetManager() + def get_total_line_counts(self): + """Returns the total line counts from all files in this diffset.""" + counts = {} + + for filediff in self.files.all(): + for key, value in six.iteritems(filediff.get_line_counts()): + if counts.get(key) is None: + counts[key] = value + elif value is not None: + counts[key] += value + + return counts + def save(self, **kwargs): """ Saves this diffset. diff --git a/reviewboard/diffviewer/tests.py b/reviewboard/diffviewer/tests.py index 66d5b68334740e54c0e793c69b79a91c95e7054f..e8d4da130cd7558ecca7fd2eb8e833de48a9fb0d 100644 --- a/reviewboard/diffviewer/tests.py +++ b/reviewboard/diffviewer/tests.py @@ -479,6 +479,81 @@ class DiffParserTest(TestCase): self.assertEqual(r_moves, expected_r_moves) +class FileDiffTests(TestCase): + """Unit tests for FileDiff.""" + fixtures = ['test_scmtools'] + + def setUp(self): + super(FileDiffTests, self).setUp() + + diff = ( + b'diff --git a/README b/README\n' + b'index d6613f5..5b50866 100644\n' + b'--- README\n' + b'+++ README\n' + b'@ -1,1 +1,2 @@\n' + b'-blah blah\n' + b'+blah!\n' + b'+blah!!\n') + + repository = self.create_repository(tool_name='Test') + diffset = DiffSet.objects.create(name='test', + revision=1, + repository=repository) + self.filediff = FileDiff(source_file='README', + dest_file='README', + diffset=diffset, + diff64=diff, + parent_diff64='') + + def test_get_line_counts_with_defaults(self): + """Testing FileDiff.get_line_counts with default values""" + counts = self.filediff.get_line_counts() + + self.assertIn('raw_insert_count', counts) + self.assertIn('raw_delete_count', counts) + self.assertIn('insert_count', counts) + self.assertIn('delete_count', counts) + self.assertIn('replace_count', counts) + self.assertIn('equal_count', counts) + self.assertIn('total_line_count', counts) + self.assertEqual(counts['raw_insert_count'], 2) + self.assertEqual(counts['raw_delete_count'], 1) + self.assertEqual(counts['insert_count'], 2) + self.assertEqual(counts['delete_count'], 1) + self.assertIsNone(counts['replace_count']) + self.assertIsNone(counts['equal_count']) + self.assertIsNone(counts['total_line_count']) + + diff_hash = self.filediff.diff_hash + self.assertEqual(diff_hash.insert_count, 2) + self.assertEqual(diff_hash.delete_count, 1) + + def test_set_line_counts(self): + """Testing FileDiff.set_line_counts""" + self.filediff.set_line_counts( + raw_insert_count=1, + raw_delete_count=2, + insert_count=3, + delete_count=4, + replace_count=5, + equal_count=6, + total_line_count=7) + + counts = self.filediff.get_line_counts() + self.assertEqual(counts['raw_insert_count'], 1) + self.assertEqual(counts['raw_delete_count'], 2) + self.assertEqual(counts['insert_count'], 3) + self.assertEqual(counts['delete_count'], 4) + self.assertEqual(counts['replace_count'], 5) + self.assertEqual(counts['equal_count'], 6) + self.assertEqual(counts['total_line_count'], 7) + + diff_hash = self.filediff.diff_hash + self.assertEqual(diff_hash.insert_count, 1) + self.assertEqual(diff_hash.delete_count, 2) + + class FileDiffMigrationTests(TestCase): fixtures = ['test_scmtools'] @@ -557,10 +632,10 @@ class FileDiffMigrationTests(TestCase): self.assertEqual(self.filediff.diff_hash, None) # This should prompt the migration - delete_count = self.filediff.delete_count + counts = self.filediff.get_line_counts() self.assertNotEqual(self.filediff.diff_hash, None) - self.assertEqual(delete_count, 1) + self.assertEqual(counts['raw_delete_count'], 1) self.assertEqual(self.filediff.diff_hash.delete_count, 1) def test_migration_by_insert_count(self): @@ -570,10 +645,10 @@ class FileDiffMigrationTests(TestCase): self.assertEqual(self.filediff.diff_hash, None) # This should prompt the migration - insert_count = self.filediff.insert_count + counts = self.filediff.get_line_counts() self.assertNotEqual(self.filediff.diff_hash, None) - self.assertEqual(insert_count, 1) + self.assertEqual(counts['raw_insert_count'], 1) self.assertEqual(self.filediff.diff_hash.insert_count, 1) def test_migration_by_set_line_counts(self): @@ -583,11 +658,14 @@ class FileDiffMigrationTests(TestCase): self.assertEqual(self.filediff.diff_hash, None) # This should prompt the migration, but with our line counts. - self.filediff.set_line_counts(10, 20) + self.filediff.set_line_counts(raw_insert_count=10, + raw_delete_count=20) self.assertNotEqual(self.filediff.diff_hash, None) - self.assertEqual(self.filediff.insert_count, 10) - self.assertEqual(self.filediff.delete_count, 20) + + counts = self.filediff.get_line_counts() + self.assertEqual(counts['raw_insert_count'], 10) + self.assertEqual(counts['raw_delete_count'], 20) self.assertEqual(self.filediff.diff_hash.insert_count, 10) self.assertEqual(self.filediff.diff_hash.delete_count, 20)