Fix storing and locating parent diff source information.

Review Request #11099 — Created July 27, 2020 and submitted

Information

Review Board
release-3.0.x

Reviewers

This makes some important changes to how we store information on source
files referenced in parent diffs, accompanying information for the
primary diff, and how we look up files.

When parent diffs were added, we had some restrictions on our schema.
There was no extra_data, and we only really had the source/destination
file/revision fields to work with. At that point, we opted to set the
source file/revision fields to point to the source of the parent diff,
so that we would fetch the right source file to apply the patch to.

This wasn't really ideal, as it sort of masked the actual revisions of
the modified file the reviewer was looking at. It was also broken -- we
didn't actually update the source filename, just the revision.

That latter issue is what led to this change in the first place. On Git
(which we first introduced parent diff support for), we didn't need that
filename, just the revision (the blob SHA), so a renamed parent file
didn't break anything.

In Mercurial, though, we need both the file and revision. Not having the
true original filename resulted in breakages when the file was
moved/renamed in the parent diff.

Going forward, we store the parent source filename/revision in
FileDiff.extra_data, and we keep the FileDiff fields specific to the
primary diff posted for review.

get_original_file() will attempt to locate using the parent source, if
available, and fall back on the main FileDiff source. This keeps
everything working as it always has for any existing diffs (and of
course for diffs without a parent diff), but allows us to maintain this
clean separation going forward.

Anyone with diffs in a database that broke due to a moved/renamed parent
source filename, who had other means of working around it, can simply
update the FileDiff.extra_data keys to point to the correct location,
fixing the issue without having to re-upload diffs.

Unit tests pass.

Tested viewing existing diffs containing parent diffs, and didn't hit any
issues.

Posted new Mercurial and Git diffs with parent diffs, both with and
without files being moved. In all cases, the source file were correctly
fetched and the files rendered with the expected information.

Summary ID
Fix storing and locating parent diff source information.
This makes some important changes to how we store information on source files referenced in parent diffs, accompanying information for the primary diff, and how we look up files. When parent diffs were added, we had some restrictions on our schema. There was no `extra_data`, and we only really had the source/destination file/revision fields to work with. At that point, we opted to set the source file/revision fields to point to the source of the parent diff, so that we would fetch the right source file to apply the patch to. This wasn't really ideal, as it sort of masked the actual revisions of the modified file the reviewer was looking at. It was also broken -- we didn't actually update the source filename, just the revision. That latter issue is what led to this change in the first place. On Git (which we first introduced parent diff support for), we didn't need that filename, just the revision (the blob SHA), so a renamed parent file didn't break anything. In Mercurial, though, we need both the file and revision. Not having the true original filename resulted in breakages when the file was moved/renamed in the parent diff. Going forward, we store the parent source filename/revision in `FileDiff.extra_data`, and we keep the `FileDiff` fields specific to the primary diff posted for review. `get_original_file()` will attempt to locate using the parent source, if available, and fall back on the main `FileDiff` source. This keeps everything working as it always has for any existing diffs (and of course for diffs without a parent diff), but allows us to maintain this clean separation going forward. Anyone with diffs in a database that broke due to a moved/renamed parent source filename, who had other means of working around it, can simply update the `FileDiff.extra_data` keys to point to the correct location, fixing the issue without having to re-upload diffs.
da6989d5a0145562049058d8b3b15c96754ee067
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (ef8ecf2)