This came out of here, right?

http://llvm-reviews.chandlerc.com/D24

It looks like we aren't handling multiple-hunk diffs correctly with respect to diff headers.

Correct.

I think this might be because the diff (http://llvm-reviews.chandlerc.com/D24) wasn't created with full context? If so, not sure what the workaround do make the patch properly would be and we'd probably be best off just disabling the UI in these cases?

We can build a multipart patch -- essentially, reconstruct the original patch.

There's a slightly-related issue here:

https://groups.google.com/forum/?hl=en_US&fromgroups=#!topic/phabricator-dev/N1-_QgMk6hg

I believe the issue is that we don't store which diff format the source was in for raw diffs, so for raw mercurial input we generate unified output. Storing this and/or popping a dialog to select a format would improve this case.

◀ Merged tasks: T2096.

T2096 has some specific examples as well.

One interesting thing to note is that the patch that gets send with the mail is correct - can we not use that code path to fix the issue in the interim?

The code in the "Download" link is building a synthetic patch representing exactly what's visible (e.g., if you've viewed a diff-of-diffs). We should possibly just remove that and download the right-side patch instead, but I want to spend more time on this before giving up on the feature -- particularly with T2096, introducing a menu like:

Download: [Visible Changes (Diff 89323 vs 79827)  V]
          [Visible Diff (Diff 89323)               ]
          [Most recent Diff (Diff 99234)           ]

Format:   [Git Patch (Git, Hg)  V]
          [Unified Patch (SVN)   ]

...might make sense. Then we could selectively disable the "visible changes" option for diffs which can't be computed.

But more likely, closer examination of the diff-of-diff implementation in D2855 will conclude there are way too many cases we get wrong and that we should never provide it. Its utility seems low compared to its complexity.

Ooooh. This wasn't clear to me from the UI :) I would have expected to always get the latest full diff...

Yeah -- we could safely special case the (likely overwhelmingly common) case where the left side is the same diff as the right side, but I'd guess it will take about an equal amount of effort to do that as to come up with enough evidence to throw away the feature entirely, and that's a cleaner patch.

We hit this problem as well. Some of our developer teams tend to use IDE's such as eclipse etc instead of command line. So they usually create differential revisions through patch diffs instead of using archanist. A speedy fix would be appreciated.

The actual issue (bad diffs) should now be fixed at HEAD. I'm going to repurpose this task since I think the dialog with diff format options might be nice to add some day (from https://secure.phabricator.com/T1715#18).