Diff in Pull Request View is Incorrect

Issue #694 resolved
Na'Tosha Bard created an issue

The diff generated as part of the pull request turns out to not actually be useful/correct. It seems to be doing a blind diff between the oldest and newest changes in the pull request. This means the aggregate diff contains the entire diff for all merge commits that are in the fork, even though these "code changes" are actually not unique to the fork (they exist in the parent).

Example: Go to http://demo.rhodecode.org/diffing-test-fork/pull-request/new and look at the "detailed compare view". It shows a diff for 3 files -- two were changed in the fork itself and one was changed by a merge commit from the fork's parent. This is, therefore, an inaccurate representation of the outstanding code changes from the fork to the parent.

Appropriate behavior would be to calculate the most recent common ancestor between the fork and parent, and do a diff between the fork's tip and that changeset. In this case, the most recent common ancestor would be revision r5:d395e22e8e16 -- "modify baz". Doing a diff of the fork tip to this revision would show a useful diff (which excludes the file changed simply because of the merge commit, but would include any files changed uniquely as a result of conflict resolution).

It is correct, however, to show all outgoing changesets as part of the Pull Request -- the bug here is only in how the diff is being calculated.

I think it's reasonable that the saved pull request would cache what this most recent common ancestor is between the fork and parent (it could be expensive to calculate), and when you load the Pull Request view, it could lazily re-check if there is a more recent common ancestor, and if so, make a note at the top that this pull request is outdated.

Comments (3)

  1. Log in to comment