Fix some commit ordering and DVCS diff generation problems with ranges.

Review Request #10425 — Created Feb. 20, 2019 and submitted

Information

Review Board
release-4.0.x

Reviewers

The new DVCS support in the diff viewer had some apparent off-by-one
issues when attempting to select ranges of commits. Performing a diff
using two commits in a range would often cause the first commit's
changes to be excluded. This was always the case when involving the
first commit in the series, and also when involving the first commit
touching a given file. Further, viewing a subsequent commit by itself
could cause some changes from the parent commit to be included.

This was due to a few problems.

First, some code cleanup changed the behavior of a loop used to
calculate a base FileDiff, and this meant that the wrong base was being
used. It would prioritize the wrong end of the list of ancestors. The
original, unbroken code has been restored, and in order to help isolate
this logic and keep it working, it's been moved to
FileDiff.get_base_filediff. A unit test was added to ensure it
returned the correct results.

Second, DiffChunkGenerator wasn't properly handling the cases of
FileDiffs belonging to commits. When a base FileDiff was provided, its
original file content was being used as the "old" code in the diff
viewer, with its own content becoming part of the "new" code. Since this
is meant to be the base (the parent of what you're seeing), this meant
that too much content was leaking. The solution was to patch the old
content with the base commit's modifications.

Third, and related, FileDiffs belonging to commits weren't working when
a base FileDiff couldn't be provided. This would be the case if viewing
any range involving commit #1, or any commit that initially touches a
file in the series. The base commit and its ancestors didn't modify this
file, and thus a base FileDiff couldn't be provided, and the "old"
content would therefore stay as the tip FileDiff's original content
instead of the first modified version of the file in the range of
commits. We now check for this case, find the first ancestor (which will
be newer than the base commit), and use its original content as the
"old".

Finally, we were relying on dictionary order when computing a list of
commits for the review request page. This wasn't safe and led to some
JavaScript problems. We now sort these before passing them to the page.

Unit tests passed.

Checked this against a few review requests I put together that
reproduced all these problems. Everything appears to work exactly as
expected now.

Summary ID
Fix some commit ordering and DVCS diff generation problems with ranges.
The new DVCS support in the diff viewer had some apparent off-by-one issues when attempting to select ranges of commits. Performing a diff using two commits in a range would often cause the first commit's changes to be excluded. This was always the case when involving the first commit in the series, and also when involving the first commit touching a given file. Further, viewing a subsequent commit by itself could cause some changes from the parent commit to be included. This was due to a few problems. First, some code cleanup changed the behavior of a loop used to calculate a base FileDiff, and this meant that the wrong base was being used. It would prioritize the wrong end of the list of ancestors. The original, unbroken code has been restored, and in order to help isolate this logic and keep it working, it's been moved to `FileDiff.get_base_filediff`. A unit test was added to ensure it returned the correct results. Second, `DiffChunkGenerator` wasn't properly handling the cases of FileDiffs belonging to commits. When a base FileDiff was provided, its original file content was being used as the "old" code in the diff viewer, with its own content becoming part of the "new" code. Since this is meant to be the base (the parent of what you're seeing), this meant that too much content was leaking. The solution was to patch the old content with the base commit's modifications. Third, and related, FileDiffs belonging to commits weren't working when a base FileDiff couldn't be provided. This would be the case if viewing any range involving commit #1, or any commit that initially touches a file in the series. The base commit and its ancestors didn't modify this file, and thus a base FileDiff couldn't be provided, and the "old" content would therefore stay as the tip FileDiff's original content instead of the first modified version of the file in the range of commits. We now check for this case, find the first ancestor (which will be newer than the base commit), and use its original content as the "old". Finally, we were relying on dictionary order when computing a list of commits for the review request page. This wasn't safe and led to some JavaScript problems. We now sort these before passing them to the page.
417d9756b257b623e8d0aa68b2f9dddbfc2685d6
Description From Last Updated

F841 local variable 'commit1' is assigned to but never used

reviewbotreviewbot

F841 local variable 'commit2' is assigned to but never used

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (98facb4)
Loading...