DifferentialChangesetParser is the probably the worst piece of code in the codebase. It has far far too many responsibilities and is generally twisted and evil. Unfortunately, it's also difficult to understand/separate, and has little-to-no test coverage.
It should be separated into a number of smaller pieces which each have less responsibility. In particular:
Rendering
- All rendering should be separated out of the main class, into rendering classes with different targets.
- Today we should support these targets:
- 2-up target for normal diffs.
- Plain-text target for unit testing.
- In the future we should support these targets:
- 1-up target for mobile/alternate stuff.
- And maybe these targets:
- Email+Text target for the stuff currently controlled by metamta.differential.unified-comment-context.
- Email+HTML target to possibly restore the "!showdiff" email reply action.
Parsing
Currently we do manual hunk parsing in several places, notably DifferentialChangeset::makeContextDiff() (which support only metamta.differential.unified-comment-context), DifferentialChangesetParser itself (in multiple places), DifferentialDiff::detectCopiedCode(), and probably some others. Most of these can probably be served by creating a more pure changeset parser (which is originally what DifferentialChangesetParser was) which accepts DifferentialHunk objects, parses them, and then exposes a general useful interface for accessing the data they contain that all of the manual parsing implementations can use.
Glue
Ideally this should leave DifferentialChangesetParser as a small amount of glue code covering:
- Whitespace options.
- Cache.
- Inline comments.
- "Shields" ("This file was not changed, click to view contents.", etc).
- JS hooks?
- Reading inputs from Parsers.
- Sending outputs to Renderers.
That's still kind of a lot of stuff so maybe there are options to further split it apart.
Some discussion at the bottom in T972: Enable differential diff view to span entire width of the screen (and not be fixed width).