Page MenuHomePhabricator

In Differential, diff-of-diffs where both diffs added an identical file is misleading
Open, NormalPublic

Description

See PHI65. To reproduce:

  • Create a revision which adds A.txt and B.txt.
  • Edit B.txt, update the revision.
  • View the interdiff of Diff 1 and Diff 2.

This shows something like this:

Screen Shot 2017-09-12 at 7.01.05 PM.png (682×1 px, 52 KB)

The rendering for A.txt is misleading. It should be rendered more accurately ("This file was not changed." instead of "This is an empty file.") or omitted entirely (probably preferable).

See PHI65 for some details on why this is difficult and additional considerations in fixing it. The pathway is something like:

  • Add a hunksOldHash and hunksNewHash to changeset.
  • Compute the hashes by making the relevant changes only, plus metadata (line numbers?).
  • Migrate existing changesets (somewhat optional).
  • Add a $changeset->isPrettyMuchIdenticalTo($other_changeset) sort of method which:
    • Compares hunk hashes.
    • Checks for property changes (like adding +x in diff 2).
    • Checks for file type changes (see below).
    • Checks for property metadata? See PHI64.
  • In RevisionViewController, drop changesets where both halves are "pretty much" identical.

Note this scenario:

  • Andrew modifies obsolete.c and creates diff A.
  • It doesn't get reviewed for a while.
  • Bethany deletes obsolete.c in a separate cleanup change.
  • Andrew goes to update his revision, and makes a mistake with merging, adding obsolete.c back into the project.
  • Andrew creates diff B.

The change should be shown in this case. Andrew has made a mistake and added obsolete.c back into the project, and this should not be obscured from reviewers.