4533: Interdiffs are not working on some files

besperon
chipx86
chipx86

What version are you running?

2.5.7
Tried 2.5.9 too

The problem didn't exist in the 1.7.6 version (appeared after update)

What's the URL of the page containing the problem?

/r/[review_id]/diff/[interdiff_id_1]-[interdiff_id_2]

What steps will reproduce the problem?

  1. Comparison between two revisions
  2. Taking a look at the files often gives me "only whitespace changes" diffs when there are other changes

What is the expected output? What do you see instead?

An interdiff showing me the changes, instead I only see whitespace changes.

What operating system are you using? What browser?

Server : Suse 11.4. Browser : Firefox / IE11

Please provide any additional information below.

You'll find attached a small git repo with files and diffs that reproduce this precise behaviour, a few screen, and the diffutils.py file that """fixes""" this problem.

I successfully """fixed""" this bug by using the 1.7.6 version of the get_chunks function (and the relevant called functions) in diffviewer/diffutils. It is interesting to note I had the exact same behaviour with the 1.7.6 code, and with get_original_file called repository.get_file() without cache_memoize.

Sorry for the messy patchwork that diffutils.py is ! I hope that can help you.

from __future__ import unicode_literals
import fnmatch
import hashlib
import logging
import os
import re
import subprocess
import tempfile
from difflib import SequenceMatcher
from reviewboard.diffviewer.differ import Differ,get_differ
from django.utils.http import urlquote
from django.utils.safestring import mark_safe
from django.utils.html import escape
try:
    import pygments
    from pygments.lexers import get_lexer_for_filename
    # from pygments.lexers import guess_lexer_for_filename
    from pygments.formatters import HtmlFormatter
except ImportError:
    pass
from django.core.exceptions import ObjectDoesNotExist
from django.utils import six
from django.utils.translation import ugettext as _
from djblets.log import log_timed
from djblets.siteconfig.models import SiteConfiguration
from djblets.util.contextmanagers import controlled_subprocess
from djblets.cache.backend import cache_memoize
from reviewboard.scmtools.core import PRE_CREATION, H
chipx86
#1 chipx86

Thanks! This is a bug I've been working on here (the actual fix is complicated and has been undergoing careful testing and iterations). Having additional testing data to verify against will help a lot.

I think this might be a dup of another bug, but I'll leave it up for now to make sure the data is readily available.

  • -New
    +Confirmed
  • +Release-2.0.x
    +Release-2.5.x
  • -Priority:Medium
    +Component:DiffViewer
    +Interdiffs
    +Priority:High
  • +chipx86
#2 yaqian

Any updates on this bug? In the release announcement for 2.5.10 (https://www.reviewboard.org/news/2017/04/02/new-review-board-2-0-28-and-2-5-10-security-bug-fix-releases/), it mentioned that fixes for interdiffs will be coming in the next 2.5.x release. Now 2.5.12 is out, does it include fixes for this bug?

#3 dkuecker

Any updates on this? My users are very frustrated...

chipx86
#4 chipx86

It's being worked on, but it's not a trivial issue and don't want to risk regressions. We're testing fixes.

#5 viki89

Thank you for working on this. We have run into this as well and it seems to be a major bottleneck. RB 2.5.13.1.

manojm321
#6 manojm321

We are running into this with Perforce repo. Any updates?

jcannon
#7 jcannon

I'm still seeing this when upgrading from 2.5 to 3.0 on a Perforce repo. Any updates on this? Also there seems to be Ticket #4625 and #4682 about the same thing.

#8 tsaintvil

Hello, my org is experiencing a similar issues with interdiffs. Are there any updates on the status of this ticket?

seb
#9 seb

Honestly, it's been three years and no further response or fix for a critical bug in the most fundamental feature of RB? And there isn't even another response despite numerous people complaining about it?

My experience with RBs support in other areas was better, but this is kind of fundamental. At the moment I have to tell my users not to use incremental diffs which of course is kind of nuts.

david
#10 david

I appreciate that you're frustrated but please consider that there are humans on the other side of the screen. If your goal is to get help, insulting us is not a good approach.

As we've mentioned in pretty much every response to this, this is an incredibly difficult problem to solve. It has taken years of analysis and work. We have an experimental fix which will ship in the upcoming 4.0 beta release. If it proves to be sufficiently stable without causing other problems, we'll backport it to a 3.0.x release.

seb
#11 seb

I'm sorry if I have "insulted" anybody. Rereading my post, I cannot see what was insulting about it though. I raised that I found it frustrating that there were no responses to this issue for three years. You say "in pretty much every response", but there only ever was one, 3 years ago. After that only silence.

So, again, my appologies if I insulted anybody and thank for the update.

david
#12 david
  • -Confirmed
    +Fixed