Page MenuHomePhabricator

Don't try to download diffs-of-diffs
ClosedPublic

Authored by epriestley on Dec 4 2013, 12:52 AM.
Tags
None
Referenced Files
F13220997: D7694.diff
Sun, May 19, 2:11 AM
F13209146: D7694.id17395.diff
Thu, May 16, 10:06 PM
F13202968: D7694.diff
Tue, May 14, 11:21 PM
F13201430: D7694.id.diff
Tue, May 14, 12:08 PM
F13191933: D7694.diff
Sun, May 12, 2:45 AM
F13187643: D7694.diff
Sat, May 11, 4:39 AM
Unknown Object (File)
Tue, May 7, 2:55 AM
Unknown Object (File)
Mon, May 6, 6:40 AM
Subscribers

Details

Summary

Ref T1715. When the user clicks "Download Raw Diff" in Differential, we try to build a diff of exactly what they're seeing. However:

  • This doesn't work if any of the changes have multiple hunks, and fixing it seems hard.
  • I suspect this diff is never actually useful anyway? And probably kind of confusing in the best case. You can't really apply it to anyhting, since you'd have to apply another diff first.

Instead, just build the right-side diff, which should align well with user expectation and doesn't suffer from the multi-hunk bug.

Some day, we could maybe add some of the fancy options in T1715.

See: https://github.com/facebook/phabricator/issues/461

Test Plan

Downloaded a multi-hunk diff, got the original back and applied it cleanly.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Would this be more clear if it was labelled "download revision" or "download revision as diff" or something? I readily admit revision and diff aren't the cleanest terms in my head, with revision being a set of diffs, and a diff being the differences between two versions of the same file OR equivalent to the definition of revision.

The thing being downloaded is accurately a "Diff", according to internal terminology, it's just not necessarily "the set of changes currently being displayed". But it is "the diff selected in the rightmost column of the diff selector", which is maybe reasonableish? I hesitate to use "Download Revision" because we're still sensitive to the rightmost column of the diff selector, just no longer sensitive to the left column.

Here is an unhelpful illustration:

{F87229}