Page MenuHomePhabricator

Improve the construction of synthetic "comparison/intradiff" changesets
ClosedPublic

Authored by epriestley on Apr 28 2020, 12:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 1, 2:11 AM
Unknown Object (File)
Mar 13 2024, 8:46 PM
Unknown Object (File)
Feb 22 2024, 1:10 AM
Unknown Object (File)
Feb 19 2024, 5:11 AM
Unknown Object (File)
Feb 18 2024, 4:37 PM
Unknown Object (File)
Jan 19 2024, 7:43 PM
Unknown Object (File)
Jan 7 2024, 5:33 PM
Unknown Object (File)
Jan 3 2024, 6:36 PM
Subscribers
None

Details

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)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2020, 12:09 PM
This revision was automatically updated to reflect the committed changes.