diff --git a/reviewboard/diffviewer/chunk_generator.py b/reviewboard/diffviewer/chunk_generator.py index 4233b54d9c79e159056dc491a57158e865525a57..b643774f95b19d081230665be98e7fee995959b8 100644 --- a/reviewboard/diffviewer/chunk_generator.py +++ b/reviewboard/diffviewer/chunk_generator.py @@ -876,7 +876,9 @@ class DiffChunkGenerator(RawDiffChunkGenerator): log_timer.done() - if not self.interfilediff and not self.force_interdiff: + if (not self.interfilediff and + not self.base_filediff and + not self.force_interdiff): insert_count = self.counts['insert'] delete_count = self.counts['delete'] replace_count = self.counts['replace'] diff --git a/reviewboard/diffviewer/diffutils.py b/reviewboard/diffviewer/diffutils.py index 54d1269299c01b65988f159cc36c910116d11bfe..d087a0660aa68e8e11f2a65c2cd2fcecb7fe5ff7 100644 --- a/reviewboard/diffviewer/diffutils.py +++ b/reviewboard/diffviewer/diffutils.py @@ -616,8 +616,8 @@ def get_matched_interdiff_files(tool, filediffs, interfilediffs): def get_diff_files(diffset, filediff=None, interdiffset=None, - interfilediff=None, request=None, filename_patterns=None, - base_commit=None, tip_commit=None): + interfilediff=None, base_filediff=None, request=None, + filename_patterns=None, base_commit=None, tip_commit=None): """Return a list of files that will be displayed in a diff. This will go through the given diffset/interdiffset, or a given filediff @@ -646,6 +646,15 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, ``interdiffset`` are both provided. If it's ``None`` in this case, then the diff will be shown as reverted for this file. + This may not be provided if ``base_filediff`` is provided. + + base_filediff (reviewbaord.diffviewer.models.filediff.FileDiff, + optional): + The base FileDiff to use. + + This may only be provided if ``filediff`` is provided and + ``interfilediff`` is not. + filename_patterns (list of unicode, optional): A list of filenames or :py:mod:`patterns ` used to limit the results. Each of these will be matched against the @@ -681,6 +690,7 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, # It is presently not supported to do an interdiff with commit spans. It # would require base/tip commits for the interdiffset as well. assert not interdiffset or (base_commit is None and tip_commit is None) + assert base_filediff is None or interfilediff is None if (diffset.commit_count > 0 and base_commit and @@ -691,6 +701,7 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, return [] per_commit_filediffs = None + requested_base_filediff = base_filediff if filediff: filediffs = [filediff] @@ -872,13 +883,21 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, filediffs=per_commit_filediffs) if ancestors: - if base_commit: - for ancestor in reversed(ancestors): - if ancestor.commit_id <= base_commit.pk: + if requested_base_filediff: + assert len(filediffs) == 1 + + if requested_base_filediff in ancestors: + base_filediff = requested_base_filediff + else: + raise ValueError( + 'Invalid base_filediff (ID %d) for filediff (ID ' + '%d)' + % (requested_base_filediff.pk, filediff.pk)) + elif base_commit: + for ancestor in ancestors: + if ancestor.commit_id >= base_commit.pk: base_filediff = ancestor break - else: - base_filediff = ancestors[0] f = { 'depot_filename': depot_filename, diff --git a/reviewboard/diffviewer/tests/test_diffutils.py b/reviewboard/diffviewer/tests/test_diffutils.py index c9147405c98473d449666f95d51fc5b95d1e03c1..2500cb279810248b01c5538d80d9917451c076ad 100644 --- a/reviewboard/diffviewer/tests/test_diffutils.py +++ b/reviewboard/diffviewer/tests/test_diffutils.py @@ -1142,9 +1142,7 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): f = files[0] self.assertEqual(f['filediff'], filediff) - self.assertEqual( - f['base_filediff'], - by_details[(1, 'foo', 'PRE-CREATION', 'foo', 'e69de29')]) + self.assertIsNone(f['base_filediff']) def test_get_diff_files_query_count_filediff_ancestors_precomupted(self): """Testing get_diff_files query count for a single FileDiff with @@ -1173,9 +1171,7 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): f = files[0] self.assertEqual(f['filediff'], filediff) - self.assertEqual( - f['base_filediff'], - by_details[(1, 'foo', 'PRE-CREATION', 'foo', 'e69de29')]) + self.assertIsNone(f['base_filediff']) def test_get_diff_files_with_history_base_commit(self): """Testing get_diff_files for a whole diffset with history with a @@ -1249,7 +1245,7 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): [ ( (3, 'foo', '257cc56', 'qux', '03b37a0'), - (1, 'foo', 'PRE-CREATION', 'foo', 'e69de29'), + None, ), ( (2, 'baz', 'PRE-CREATION', 'baz', '280beb2'), @@ -1261,7 +1257,7 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): ), ( (3, 'bar', 'PRE-CREATION', 'bar', '5716ca5'), - (1, 'bar', 'e69de29', 'bar', '8e739cc'), + None, ), ]) @@ -1294,7 +1290,7 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): [ ( (3, 'foo', '257cc56', 'qux', '03b37a0'), - (1, 'foo', 'PRE-CREATION', 'foo', 'e69de29'), + None, ), ( (2, 'baz', 'PRE-CREATION', 'baz', '280beb2'), @@ -1306,7 +1302,7 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): ), ( (3, 'bar', 'PRE-CREATION', 'bar', '5716ca5'), - (1, 'bar', 'e69de29', 'bar', '8e739cc'), + None, ), ]) diff --git a/reviewboard/diffviewer/views.py b/reviewboard/diffviewer/views.py index 6d66debac863fd7a7809ec13fd4199a4ab2a9df6..54ac7987f085e4036617215f47acd9bd209eb57c 100644 --- a/reviewboard/diffviewer/views.py +++ b/reviewboard/diffviewer/views.py @@ -10,6 +10,7 @@ from django.conf import settings from django.core.paginator import InvalidPage, Paginator from django.core.urlresolvers import NoReverseMatch from django.http import (HttpResponse, + HttpResponseBadRequest, HttpResponseNotFound, HttpResponseNotModified, HttpResponseServerError, @@ -358,11 +359,13 @@ class DiffFragmentView(View): ``?lines-of-context=`` A number of lines of context to include above and below the chunk. - ``?base-commit-id=`` - The primary key of the base commit to use to generate the diff. + ``?base-filediff-id<=id>`` + The primary key of the base FileDiff. This parameter is ignored if the review request was created without commit history support. + + This conflicts with the ``interfilediff_id``. """ template_name = 'diffviewer/diff_file_fragment.html' @@ -396,6 +399,8 @@ class DiffFragmentView(View): interfilediff_id = kwargs.get('interfilediff_id') chunk_index = kwargs.get('chunk_index') + base_filediff_id = request.GET.get('base-filediff-id') + try: renderer_settings = self._get_renderer_settings(**kwargs) etag = self.make_etag(renderer_settings, **kwargs) @@ -403,7 +408,9 @@ class DiffFragmentView(View): if etag_if_none_match(request, etag): return HttpResponseNotModified() - diff_info_or_response = self.process_diffset_info(**kwargs) + diff_info_or_response = self.process_diffset_info( + base_filediff_id=base_filediff_id, + **kwargs) if isinstance(diff_info_or_response, HttpResponse): return diff_info_or_response @@ -554,7 +561,7 @@ class DiffFragmentView(View): def process_diffset_info(self, diffset_or_id, filediff_id, interfilediff_id=None, interdiffset_or_id=None, - **kwargs): + base_filediff_id=None, **kwargs): """Process and return information on the desired diff. The diff IDs and other data passed to the view can be processed and @@ -601,18 +608,36 @@ class DiffFragmentView(View): filediff = get_object_or_404(FileDiff, pk=filediff_id, diffset=diffset) - if interfilediff_id: + base_filediff = None + interfilediff = None + + if interfilediff_id and base_filediff_id: + raise UserVisibleError(_( + 'Cannot generate an interdiff when base FileDiff ID is ' + 'specified.' + )) + elif interfilediff_id: interfilediff = get_object_or_404(FileDiff, pk=interfilediff_id, diffset=interdiffset) - else: - interfilediff = None + elif base_filediff_id: + base_filediff = get_object_or_404(FileDiff, pk=base_filediff_id, + diffset=diffset) + + ancestors = filediff.get_ancestors(minimal=False) + + if base_filediff not in ancestors: + raise UserVisibleError(_( + 'The requested FileDiff (ID %s) is not a valid base ' + 'FileDiff for FileDiff %s.' + % (base_filediff_id, filediff_id) + )) # Store this so we don't end up causing an SQL query later when looking # this up. filediff.diffset = diffset - diff_file = self._get_requested_diff_file(diffset, filediff, - interdiffset, interfilediff) + diff_file = self._get_requested_diff_file( + diffset, filediff, interdiffset, interfilediff, base_filediff) if not diff_file: raise UserVisibleError( @@ -697,7 +722,7 @@ class DiffFragmentView(View): } def _get_requested_diff_file(self, diffset, filediff, interdiffset, - interfilediff): + interfilediff, base_filediff): """Fetches information on the requested diff. This will look up information on the diff that's to be rendered @@ -707,31 +732,12 @@ class DiffFragmentView(View): The file will not contain chunk information. That must be specifically populated later. """ - base_commit_id = None - base_commit = None - - if diffset.commit_count > 0 and not interdiffset: - raw_base_commit_id = self.request.GET.get('base-commit-id') - - if raw_base_commit_id is not None: - try: - base_commit_id = int(raw_base_commit_id) - except ValueError: - pass - - base_commit, tip_commit = get_base_and_tip_commits( - base_commit_id=base_commit_id, - tip_commit_id=None, - diffset=diffset) - - # The tip commit is not required since we are requesting get_diff_files - # with a FileDiff. files = get_diff_files(diffset=diffset, interdiffset=interdiffset, filediff=filediff, interfilediff=interfilediff, - request=self.request, - base_commit=base_commit) + base_filediff=base_filediff, + request=self.request) if files: diff_file = files[0] diff --git a/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py b/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py new file mode 100644 index 0000000000000000000000000000000000000000..06299a41e775ca587e7a20439b5fd9724f409223 --- /dev/null +++ b/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py @@ -0,0 +1,147 @@ +"""Tests for reviewboard.diffviewer.views.DiffFragmentView.""" + +from __future__ import unicode_literals + +from reviewboard.site.urlresolvers import local_site_reverse +from reviewboard.testing import TestCase + + +class ReviewsDiffFragmentViewTests(TestCase): + """Tests for reviewboard.diffviewer.views.DiffFragmentView.""" + + fixtures = ['test_scmtools', 'test_users'] + + def test_base_filediff_not_in_diffset(self): + """Testing ReviewsDiffFragmentView.get with ?base-filediff-id= as a + FileDiff outside the current diffset + """ + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request(repository=repository, + create_with_history=True) + review_request.target_people = [review_request.submitter] + + diffset = self.create_diffset(review_request, draft=True) + commit = self.create_diffcommit(diffset=diffset) + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + review_request.publish(user=review_request.submitter) + + filediff = commit.files.get() + + other_diffset = self.create_diffset(repository=repository) + other_filediff = self.create_filediff(diffset=other_diffset) + + rsp = self.client.get( + local_site_reverse( + 'view-diff-fragment', + kwargs={ + 'review_request_id': review_request.display_id, + 'revision': diffset.revision, + 'filediff_id': filediff.pk, + }), + data={'base-filediff-id': other_filediff.pk}) + + self.assertEqual(rsp.status_code, 404) + + def test_base_filediff_and_interfilediff(self): + """Testing ReviewsDiffFragmentView.get with interfilediff and + ?base-filediff-id + """ + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request(repository=repository, + create_with_history=True) + review_request.target_people = [review_request.submitter] + + diffset = self.create_diffset(review_request, draft=True) + diffset_commits = [ + self.create_diffcommit(diffset=diffset, commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffset, commit_id='r2', + parent_id='r1'), + ] + + filediff = diffset_commits[1].files.get() + base_filediff = diffset_commits[0].files.get() + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) + + interdiffset = self.create_diffset(review_request, draft=True) + interdiffset_commit = self.create_diffcommit( + diffset=interdiffset, commit_id='r1', parent_id='r0') + + interdiffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) + + interfilediff = interdiffset_commit.files.get() + + rsp = self.client.get( + local_site_reverse( + 'view-diff-fragment', + kwargs={ + 'review_request_id': review_request.display_id, + 'revision': diffset.revision, + 'interdiff_revision': interdiffset.revision, + 'filediff_id': filediff.pk, + 'interfilediff_id': interfilediff.pk, + }), + data={'base-filediff-id': base_filediff.pk}) + + self.assertEqual(rsp.status_code, 500) + self.assertIn( + 'Cannot generate an interdiff when base FileDiff ID is specified.', + rsp.content) + + def test_base_filediff_not_ancestor(self): + """Testing ReviewsDiffFragmentView.get with ?base-filediff-id set to a + FileDiff that is not an ancestor FileDiff + """ + review_request = self.create_review_request(create_repository=True, + create_with_history=True) + review_request.target_people = [review_request.submitter] + diffset = self.create_diffset(review_request, draft=True) + commits = [ + self.create_diffcommit(diffset=diffset, commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffset, commit_id='r2', + parent_id='r1'), + ] + + base_filediff = commits[0].files.get() + filediff = commits[1].files.get() + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) + + rsp = self.client.get( + local_site_reverse( + 'view-diff-fragment', + kwargs={ + 'revision': diffset.revision, + 'review_request_id': review_request.display_id, + 'filediff_id': filediff.pk, + }), + data={'base-filediff-id': base_filediff.pk}) + + self.assertEqual(rsp.status_code, 500) + self.assertIn( + 'The requested FileDiff (ID %d) is not a valid base FileDiff ' + 'for FileDiff %d.' + % (base_filediff.pk, filediff.pk), + rsp.content) diff --git a/reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js b/reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js index 30bd7718fdb8cdb18abb94b25e0d64aae863b700..5260f3c1d33b123a73a1faf5c6b78b93e2eb3107 100644 --- a/reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js +++ b/reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js @@ -79,8 +79,16 @@ RB.DiffReviewable = RB.AbstractReviewable.extend({ * diff in order to show its deleted content. */ getRenderedDiff(callbacks, context, options={}) { - let url = this._buildRenderedDiffURL() + '?index=' + - this.get('file').get('index'); + let url = this._buildRenderedDiffURL(); + + if (url.includes('?')) { + url += '&'; + } else { + url += '?'; + } + + url += 'index='; + url += this.get('file').get('index'); if (options.showDeleted) { url += '&show-deleted=1'; @@ -175,6 +183,6 @@ RB.DiffReviewable = RB.AbstractReviewable.extend({ revisionStr + '/fragment/' + this.get('fileDiffID') + (interFileDiffID ? '-' + interFileDiffID : '') + '/' + - (baseFileDiffID ? '?base=' + baseFileDiffID : ''); + (baseFileDiffID ? '?base-filediff-id=' + baseFileDiffID : ''); }, }); diff --git a/reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js b/reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js index 760ee08fdd48a8f3ff95406561612fbd3ffeb661..1c958fb2debc518dd6b0dac4116f97dac1989604 100644 --- a/reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js +++ b/reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js @@ -73,6 +73,34 @@ suite('rb/diffviewer/models/DiffReviewable', function() { expect(callbacks.complete).toHaveBeenCalledWith('abc', 'success'); expect(callbacks.error).not.toHaveBeenCalled(); }); + + it('With base FileDiff', function() { + var diffReviewable = new RB.DiffReviewable({ + reviewRequest: reviewRequest, + fileDiffID: 3, + revision: 2, + baseFileDiffID: 1, + file: new RB.DiffFile({ + index: 4 + }) + }); + + spyOn($, 'ajax').and.callFake(function(request) { + expect(request.type).toBe('GET'); + expect(request.url).toBe( + '/r/1/diff/2/fragment/3/?base-filediff-id=1&index=4&' + + TEMPLATE_SERIAL); + + request.success('abc'); + request.complete('abc', 'success'); + }); + + diffReviewable.getRenderedDiff(callbacks); + + expect(callbacks.success).toHaveBeenCalledWith('abc'); + expect(callbacks.complete).toHaveBeenCalledWith('abc', 'success'); + expect(callbacks.error).not.toHaveBeenCalled(); + }); }); describe('getRenderedDiffFragment', function() {