4194: Include real line numbers for diff comments in API, webhook payloads

pawolf

What version are you running?

Review Board 2.5.3

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

Review page (http://reviewboard.mycompany.com/r/17/diff/1/

What steps will reproduce the problem?

  1. Create a Review Request and add a diff/patch file. I've attached a sample file created by 'svn diff' that demonstrates the issue. The file has one change where two lines were replaced with two different lines, followed by another change where some lines were added.
  2. Add a comment on a line on or after the first change. In this case, I add it on line 613 (on the left), 620 (on the right). I've attached a screenshot with the code obscured.

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

Using FireBug, if I look in the data posted for the comment, I see the 'first_line' field being sent with a value 622 and not 620 as I would expect. The comment is displayed on the correct line (613/620) in Review Board, but the 622 value is what eventually gets sent to our Web Hook as well as what is stored in the database. I'm trying to understand how this line gets calculated, as it is causing confusion for people that use the data received by our Web Hook when they go to the Review Request in Review Board.

What operating system are you using? What browser?

Windows 7, FireFox 44.0.2

Please provide any additional information below.

Index: my_dir/my_file.php
===================================================================
--- my_dir/my_file.php	(revision 12345)
+++ my_dir/my_file.php	(working copy)
@@ -605,11 +605,18 @@
             // Some comment
 
             return $a;
-        }
-        // Some other comment
 
+        } // Some other comment
+
+
         $s = '';
 
+        $s = '';
+        $s = '';
+        $s = '';
+        $s = '';
+        $s = '';
+
         foreach ($a as $o) {
 
             $s = $o->f();
david
#1 david

That refers to the "row number" within the side-by-side diff table. We should include the real line numbers in the webhook payload and API, though

  • -Type:Defect
    +Component:API
    +Component:Webhooks
    +Type:Enhancement
  • -First line for diff comment not as expected
    +Include real line numbers for diff comments in API, webhook payloads