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
Unknown Object (File)
Mon, Apr 29, 4:35 PM
Unknown Object (File)
Sat, Apr 27, 4:49 PM
Unknown Object (File)
Wed, Apr 24, 10:58 PM
Unknown Object (File)
Sun, Apr 21, 2:39 PM
Unknown Object (File)
Sat, Apr 20, 10:52 PM
Unknown Object (File)
Wed, Apr 17, 3:09 PM
Unknown Object (File)
Wed, Apr 10, 4:56 AM
Unknown Object (File)
Wed, Apr 10, 4:56 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}