970: diff viewer is incorrect

tph***@gmai***** (Google Code) (Is this you? Claim this profile.)
March 25, 2009 
What's the URL of the page containing the problem?
http://url/review/r/487/diff/

What steps will reproduce the problem?
1. checkout file and create temporary change
2. post-review <change>
3. visit the url or "View Diff" to see the diff

What is the expected output? What do you see instead?
- The diff shown is wrong. It's showing lines 511-512 deleted in right
file, lines 515-515 added in left file.

What operating system are you using? What browser?
- Fedora8. Firefox 2.

Please provide any additional information below.
- "p4 diff" and post-review generated diff matches
- post-review generated diff (Download diff) is correct
- In "View diff", the patched file is correct. Only the diff shown is wrong.

I can only provide partial diffs from downloading raw diff file bugxxx.patch

--- file
+++ file
@@ -508,11 +508,11 @@ function...
...line
...line
...line
+    echo -e "\nEnclosure Show Topology"
+    echo      "-----------------------"
+    cat $TMP_LOG_ENCL
...line
...line
-      echo -e "\nEnclosure Show Topology"
-      echo      "-----------------------"
-      cat $TMP_LOG_ENCL
...line
...line
...line
#1 tph***@gmai***** (Google Code) (Is this you? Claim this profile.) 
This is the 2nd incident an user reported the diff is wrong. The previous incident I
had no data.
chipx86
#2 chipx86 
I don't fully understand the issue. Is there any way you could provide a screenshot?
(using a dummy file or with stuff blacked out).
  • +NeedInfo
  • +Component-DiffViewer
#3 tph***@gmai***** (Google Code) (Is this you? Claim this profile.) 
files attached.
screen1.jpg --> from "View Diff"
patch.patch --> from "Download Diff"
  • +
    screen1.jpg
    None
    +
    patch.patch 
    --- //file	//file#99
    +++ //file	2009-03-13 11:21:35
    @@ -508,11 +508,11 @@ function show_encl_topology_status() {
    <line>
    <line>
    <line>
    +    echo -e "\nEnclosure Show Topology"
    +    echo      "-----------------------"
    +    cat $TMP_LOG_ENCL
         grep -i "hardware access error\|no paths found within the system\|operation is not supported on this platform" $TMP_LOG_ENCL > /dev/null
         if [ $? -eq 1 ]; then
    -      echo -e "\nEnclosure Show Topology"
    -      echo      "-----------------------"
    -      cat $TMP_LOG_ENCL
    <line>
    <line>
    <line>
chipx86
#4 chipx86 
Ah, this is actually correct. It just looks funky due to how the diff is computed
(and we use the standard algorithm for this, so it's not Review Board-specific really).

As a human, you know that what happened was you moved the "Enclosure Show Topology"
up a bit. However, when the diff algorithm computed the diff, what it saw as the
optimal strategy for representing the diff was that the grep line moved. It saw the
"Enclosure Show Topology" part as remaining constant. This is due to us ignoring
leading whitespace (so that indentation changes don't create a bunch of extra lines
changed when you don't really care too much about them).

I hope that made sense?
  • -NeedInfo
    +NotABug