Implement viewing versions and downloading patches in Phragment
ClosedPublic
Actions

Authored by hach-que on Dec 7 2013, 6:07 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4205: Implement Phragment

Commits

Restricted Diffusion Commit
rPc48cfb31bc76: Implement viewing versions and downloading patches in Phragment

Summary

This adds support for viewing individual versions on a fragment as well as comparing versions and downloading diff_match_patch-based patches.

It does not use the side-by-side diff format as while it works for small changes, it quickly becomes impossible to distingush what changes have been made due to the diff_match_patch format.

Test Plan

Clicked on versions and downloaded patches.

Diff Detail

Branch

phragment-view-versions

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

epriestley accepted this revision.Dec 7 2013, 7:29 PM

epriestley added inline comments.

src/applications/phragment/controller/PhragmentPatchController.php
17–23	This seems weird. I'd expect the URIs to look like: /patch/a/b/ (patch from a -> b) /patch/b/ (patch from null -> b) Is there a compelling reason not to just make the second component optional, and drop this `x` magic?

The main reason for that format is that you're not omitting the last version; you're omitting the first version. So having "null -> b" in the format of just /version/b/ is a bit weird in my opinion.

Closed by commit rPc48cfb31bc76 (authored by @hach-que).

I think that's reasonable, but there's no way anyone could ever guess /x/ is the magical incantation to omit a source. They could reasonably guess /b/, or a less-surprsing string like /null/, maybe. We could also flip it to be /b/a/ instead of /a/b/, and then /b/ is just a shortening, rather than a skip. This leaves the two-arg case with an unintuitive order, though.