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

[Mads Kiilerich]

On 01/02/2017 07:17 PM, Andrew Shadura wrote:
>   kallithea/lib/diffs.py |  7 +------
>   1 files changed, 1 insertions(+), 6 deletions(-)
>
>
> # HG changeset patch
> # User Andrew Shadura <andrew at shadura.me>
> # Date 1483380766 -3600
> #      Mon Jan 02 19:12:46 2017 +0100
> # Branch stable
> # Node ID ee8c9993a3aacb371fc5e59addc9434008290679
> # Parent  b4dd4c16c12d8932d99a2182c7e8ed0f456fb85f
> 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.

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?

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

Considering these comments, how do you suggest moving forward on the 2 
branches?

/Mads

>
> diff --git a/kallithea/lib/diffs.py b/kallithea/lib/diffs.py
> --- a/kallithea/lib/diffs.py
> +++ b/kallithea/lib/diffs.py
> @@ -62,12 +62,7 @@ def wrapped_diff(filenode_old, filenode_
>       if filenode_old is None:
>           filenode_old = FileNode(filenode_new.path, '', EmptyChangeset())
>   
> -    if filenode_old.is_binary or filenode_new.is_binary:
> -        diff = wrap_to_table(_('Binary file'))
> -        stats = (0, 0)
> -        size = 0
> -
> -    elif cut_off_limit != -1 and (cut_off_limit is None or
> +    if cut_off_limit != -1 and (cut_off_limit is None or
>       (filenode_old.size < cut_off_limit and filenode_new.size < cut_off_limit)):
>   
>           f_gitdiff = get_gitdiff(filenode_old, filenode_new,
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> https://lists.sfconservancy.org/mailman/listinfo/kallithea-general