19:03:08 ~/Dropbox/code/phalanx/src (T6237)
~> grep -r DifferentialDiffEditor *
__phutil_library_map__.php:    'DifferentialDiffEditor' => 'applications/differential/editor/DifferentialDiffEditor.php',
__phutil_library_map__.php:    'DifferentialDiffEditor' => 'PhabricatorEditor',
applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php:    id(new DifferentialDiffEditor())
applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php:    id(new DifferentialDiffEditor())
applications/differential/editor/DifferentialDiffEditor.php:final class DifferentialDiffEditor extends PhabricatorEditor {

this is very good news... the problem is basically change those conduit endpoints to use the DifferentialTransactionEditor and then delete the DifferentialDiffEditor.

I was basically really far off with my optimistic assessment.

These two end points create diffs that are later added to revisions, or in other words there is no associated revision. The existing DifferentialTransactionEditor editor is revision based, though it does the PhabricatorLiskDAO thing typically so that's not a hard blocker.

I've been playing around with adding a new DifferentialTransaction::TYPE_DIFF and DifferentialTransaction::TYPE_RAW_DIFF that flows through the existing DifferentialTransactionEditor, mapping to each of these two end points. My architectural concerns here are

• these transactions should never render, mail, or show up in feed, so transaction code is shouldHide = true for these types, and then the shouldSendMail, etc logic is a bit more complicated. just seems like these are dumb transactions then.
• these transactions potentially have a lot of data - its the whole raw set of changes - so diffs that were happily made before may break now that all this data is being stored in a single column inside one of these transactions.
• the herald code for DifferentialTransactionEditor basically would look like "if one of these new types, then new code for DifferentialDiff herald, else run the old code for DifferentialRevision herald"

Thus, does this two new transaction approach sound right or am I approaching this wrong?

I haven't looked at the code recently, but I would expect the cleanest approach to be to turn DifferentialDiffEditor into a proper transaction editor, which operates on a DifferentialDiff instead of a DifferentialRevision -- not to merge it into DifferentialTransactionEditor.

The idea being that this page gets the Diff transactions at the bottom:

https://secure.phabricator.com/differential/diff/123/

...and this form gets some stuff like "Repository" and "Can View":

https://secure.phabricator.com/differential/diff/create/

...and when you submit just that form (but not the next one) you end up with a diff with some transactions on it, but no revision yet.

I wouldn't store the actual (source code) changes in transactions, because diffs themselves are immutable and I don't anticipate changing that.

Does that make sense?

Yeah, that helps, thanks.