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

[Andrew Shadura]

On 02/01/17 23:20, Mads Kiilerich wrote:
> On 01/02/2017 07:17 PM, Andrew Shadura wrote:
>> diff: don't cut binary diff parsing short
>>
>> With binary diffs, user might want to see something more meaningful
>> than just 'Binary file', for example, an image diff. However,
>> interrupting parsing on encountering a binary file leaves us in
>> an inconsistent state: op is None, which causes exception later
>> in processing, force setting it to 'M' isn't correct as the diff
>> may be a deletion or creation of a file as well as a simple
>> modification, so we need to actually parse the diff to find out
>> what are we dealing with.
>>
>> With that in mind, this shortcut doesn't save us anything really,
>> but only creates troubles. Hence, this changeset gets rid of it.
> 
> Thanks!
> 
> It looks like this patch only touch the case of looking at a diff for a
> file, not for a changeset / pull request / compare diff? It would be
> nice to get that clarified in the commit message.
> 
> Since we apparently had a serious crash here, can you add a test for
> this case? We don't have other test coverage of wrapped_diff, but
> test_diff_parsers.py comes close and suggests how it perhaps could be
> tested.
> 
> Exactly what exception did you get? The patch seems to apply to stable,
> but there is no 'op' there? I would like to understand the problem so I
> can review and test the fix.

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 :)

> 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?

> 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 :(

-- 
Cheers,
  Andrew