Page MenuHomePhabricator
Create Task
Maniphest T13523

Update logic around intradiff changesets to better handle binary files, header information, and attributes
Open, NormalPublic
Actions

Description

See PHI1708. See a related case in T13522 where an image file is not changed between diffs 1 and 2.

  • When changesets are hidden with "Hide Changeset", headers ("This file was added.") are still shown. They should not be.
  • When image A is added in diff 1 and updated in diff 2, the interdiff is completely wrong.

Current state of image interdiffs is:

Diff 1

Screen Shot 2020-04-27 at 8.40.21 AM.png (498×1 px, 42 KB)

Diff 2

Screen Shot 2020-04-27 at 8.40.28 AM.png (491×1 px, 51 KB)

Diff 1 vs 2

Screen Shot 2020-04-27 at 8.40.34 AM.png (528×1 px, 55 KB)

This interdiff is incorrect:

  • The left side should show the Diff 1 version of the image (the featureless sketch).
  • All parts of the property changes are incorrect. The type and file mode are both unchanged.
  • The "This image was added." header is incorrect in the context of an interdiff. It should either be omitted or made more specific ("Overall, this diff adds this file, but both sides of the interdiff contain the file; it was not added between diffs 1 and 2.")

Revisions and Commits

rP Phabricator
D21220rP84353359de7c (stable) Fix an intradiff error when the newer changeset does not exist
D21220rP6430d6d638eb Fix an intradiff error when the newer changeset does not exist
D21210rPaa3b447bb0c8 (stable) Fix an issue where text intradiff bodies may not render
D21210rP17426a60f00a Fix an issue where text intradiff bodies may not render
D21180rPbefeb17f6ff1 Improve the construction of synthetic "comparison/intradiff" changesets

Related Objects
Search...

Event Timeline

epriestley triaged this task as Normal priority.Apr 27 2020, 3:45 PM
epriestley created this task.
Herald added a subscriber: amckinley. · View Herald TranscriptApr 27 2020, 3:45 PM
epriestley added a comment.Apr 27 2020, 10:42 PM

These issues are somewhat complicated because changesets currently have a single changeType attribute and a single fileType attribute, and a lot of rendering logic depends on these attributes. This model isn't correct in the general case.

  • Nothing stops you from replacing the text content (fileType = text) of my-favorite-data.bin with the binary data of a PNG (fileType = image).
  • In an interdiff, Diff 1 may be deleting a file and Diff 2 may be editing (or even creating) it.

Some of the changeType and fileType logic can simply be removed. For example, DocumentEngine cares about TYPE_ADD and TYPE_DELETE, but can probably be changed to only care about the presence of binary PHIDs.

epriestley added a revision: D21180: Improve the construction of synthetic "comparison/intradiff" changesets.Apr 28 2020, 12:00 PM
epriestley added a commit: rPbefeb17f6ff1: Improve the construction of synthetic "comparison/intradiff" changesets.Apr 28 2020, 12:09 PM
epriestley renamed this task from Update logic around interdiff changesets to better handle binary files, header information, and attributes to Update logic around intradiff changesets to better handle binary files, header information, and attributes.Apr 28 2020, 1:22 PM
epriestley moved this task from Backlog to Intradiffs on the Differential board.
epriestley added a revision: D21210: Fix an issue where text intradiff bodies may not render.May 4 2020, 2:39 PM
epriestley added a commit: rP17426a60f00a: Fix an issue where text intradiff bodies may not render.May 4 2020, 2:40 PM
epriestley added a commit: rPaa3b447bb0c8: (stable) Fix an issue where text intradiff bodies may not render.
epriestley added a revision: D21220: Fix an intradiff error when the newer changeset does not exist.May 4 2020, 10:38 PM
epriestley added a commit: rP6430d6d638eb: Fix an intradiff error when the newer changeset does not exist.May 4 2020, 10:42 PM
epriestley added a commit: rP84353359de7c: (stable) Fix an intradiff error when the newer changeset does not exist.
Phabricator · Built by Phacility