[PATCH stable] diff: don't cut binary diff parsing short

[Mads Kiilerich]

On 01/03/2017 09:09 AM, Andrew Shadura wrote:
> Right, I made a mistake of rebasing it to the stable branch as I thought
> OOK ran stable, which it didn't. Sure, there's no op there, the crash
> happens on the default branch only, it's
>
> 'in <string>' requires string as left operand, not NoneType
>
> in the line with "if op in 'DM':".
>
> The context is, for example,
> https://kallithea-scm.org/repos/kallithea/diff/rhodecode/public/images/user14.png?diff1=41a695e604ba65f2943cbc7996038c524d692603&diff2=5351a3a32381d3f118720d3e9e1a48b4bdc8b85a&diff=diff&fulldiff=1
> (it's patched in the public instance though).
>
>> It seems like I might have fixed the exception on the default branch in
>> https://kallithea-scm.org/repos/kallithea/changeset/79676fef1ae0 - can
>> you confirm that?
> Well, that's the commit which in fact created it :)

Thanks for clarifying - it would be nice to have some of these details 
and context in the commit message.

>> Creating and parsing a diff can be slow - especially if it is a big or
>> hard diff, as binary diffs often will be. Just showing "binary file" is
>> thus a nice optimization. But showing the right M/A/D is also nice ...
> Are we really saving enough to be worth the troubles?

I guess it would be justified to drop the special handling of binary 
files as you suggest. But then we also have to make sure we handle the 
case of too-large files too so we *never* return unknown op. Perhaps by 
computing the diff but just not render it as html? And perhaps add an 
assert to make sure _parse_gitdiff never returns None? Or just add a "if 
op" in the template?

>> Considering these comments, how do you suggest moving forward on the 2
>> branches?
> Can we detect addition or removal without actually parsing diffs? I
> haven't found an easy way yet :(


For Mercurial, we could look at the output of 'status' between the two 
revisions. For full revision diffs, that could also allow us to compute 
the diffs file by file and perhaps make it easier to fetch and render on 
demand (even thought it probably is better to do that once and cache it 
...).

I doubt that is the right solution right now.

/Mads