1511: Diffs containing only indentation changes causes the diff viewer to look empty
- Fixed
- Review Board
| AaronJ*******@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| Jan. 30, 2014 | |
| 1780, 1890 |
What version are you running? 1.0.5.1 What's the URL of the page containing the problem? Not public, sadly What steps will reproduce the problem? We can reproduce this issue with a given Perforce changeset, but cannot reproduce it with a new change, so we're not sure how to go about reproducing this in the wild. The issue is this: User uploaded a change from Perforce via post-review. The change consists of just over 100 lines of diff with 2 distinct edits. There was no error reported by post-review and no errors appeared in the logs. When the user visits the review, there is no diff. In fact, when we look at the HTML, the entire table (class="sidebyside") is missing. The page goes from <div id="diffs"> to the javascript that normally comes after the table. Looking at the logs again, generating this page produced no errors and what's really strange is that the diff WAS uploaded just fine, and clicking "Download Diff" yields a perfectly acceptable diff file. What can I provide that will help to track this down? Unfortunately, I can't just upload our code, but if someone tells me what to look for, I'll happily go probing and report back.
Okay, I've investigated this a little and it can basically be reproduced with a diff such as: --- filename.foo (...) +++ filename.foo (...) @@ ... @@ Some text Some text - <Foo> + <Foo> Blah blah - </Foo> + </Foo> Notice how the only thing that is changing is the indentation. To Review Board, this really means nothing has changed. We don't show leading whitespace changes. So, internally, it's finding that nothing has changed with the code and when we go to render this and see no changed chunks, we consider this to be a file in the changeset without any changes and we remove it from the list. This is important for interdiffs, which is mostly why the logic is there. So, this particular diff in question isn't doing anything but changing indentation and Review Board is therefore not seeing that anything is really up for review. We probably should be a little more graceful about this, but I will point out that this is the first time we've seen this sort of an issue where a diff containing nothing but indentation changes has gone up and confused people. We can look at may be special-casing non-interdiffs and displaying some indication that the file hasn't changed. Given the rarity of the case, though, it's not a huge priority at the moment.
-
+ Confirmed -
- Priority-Medium + Priority-Low + Component-DiffViewer -
+ Diffs containing only indentation changes causes the diff viewer to look empty
I can't begin to thank you enough for catching this! I hadn't even noticed that the original review was whitespace-only, or I would have mentioned it in the report. Thanks again!
Not a problem. It wasn't obvious to me at first either.
I have just run into this on our company review board as well. A fix would be nice, since I often change indention in our legacy code.
I'm using Python. In Python, whitespace makes changes on indentation level - which means the code block itself is totally changed. For example,
a.py
"""
if 1 == 2:
print "hello"
print "world"
"""
We will got nothing out. But
b.py
"""
if 1 == 2:
print "hello"
print "world"
"""
we will got "world" printed out on standard output. "Whitespace" changes everything.
So it is a huge problem for Python users.
Review Board 2.0's diff viewer will show all indentation changes. Committed to master (6dcae76)
-
- Confirmed + Fixed -
+ Milestone-Release2.0