• 
      

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

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

    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.

    Commits

    Files