HomePhabricator

Improve the construction of synthetic "comparison/intradiff" changesets

Description

Improve the construction of synthetic "comparison/intradiff" changesets

Summary:
Ref T13523. Currently, when building a "comparison" changeset, metadata is taken from the left changeset. This is somewhat arbitrary.

This means that intradiffs of images don't work properly because the rendered changeset has only the left (usually "old") information.

Later, some of the code attempts to ignore the file data stored on the changeset and reconstruct the correct file data, which is how the result ends up not-completely-wrong.

Be more careful about building sensible-ish metadata, and then just use it directly later on. This fixes the "spooky" code referencing D955 + D6851.

There are some related issues, where "change type" and "file type" are selected arbitrarily and then used to determine whether the change has an "old/new" state or not (i.e., is the left side of the diff empty, since the change creates the file)?

In many cases, neither of the original changesets have a "change type" which will answer this question correctly. Separate this concept from "has state" from "change type", and make more of the code ask narrower questions about the specific conditions or states it cares about, rather than "change type".

Test Plan:

  • Created a revision with Diff 1, Diff 2, and Diff 3. Diff 1 takes an image from "null -> A". Diff 2 takes the same image from "null -> B". Diff 3 takes the same image from "A -> B'.
  • Intradiffed 1v2 and 1v3.
  • Before patch:
    • Left side usually missing, which is incorrect (should always be "A").
    • Change properties are a mess ("null -> image/png" for MIME type, e.g.)
    • Uninteresting/incorrect "unix:filemode" stuff.
  • After patch;
    • Left side shows state "A".
    • Change properties only show size changes (which is correct).

Screen Shot 2020-04-28 at 4.59.38 AM.png (734×1 px, 91 KB)

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21180