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 _______________________________________________ kallithea-general mailing list [email protected] https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
