pawolf

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. 

  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.
    3. 



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.
temp.patch 
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();
Review Board diff comment.png
None
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
#2 pianoboy1 
Hello -


Has there been any update on this?  Can you tell me if newer versions of ReviewBoard return the real line numbers in the webhook payload and API?  We have version 2.5.7 of ReviewBoard and I see there is a 3.0.18 version available.
chipx86
#3 chipx86 
I'm sorry, not really. There are some reasons it's not provided that are more technical implementation details than anything. It is something we really should be doing, and I'm going to try to get this on the roadmap, but it won't help you for now.


There is a workaround, though. You can access the file diff data API by making a GET request to this with an Accept: application/vnd.reviewboard.org.diff.data+json header and the parameters for the file being commented on.


This will give you underlying data used to build the diff viewer. This contains the real line numbers and those row numbers. From there, you can access the appropriate row and get the original/modified line numbers for the row number.


This is available in Review Board 2.5 as well, but it's worth mentioning that 2.5 is no longer seeing any new development. 3.0 will soon be going into maintenance mode as well, as 4.0 is coming out before long.


Would you be able to talk about what your webhook does? We're planning work on a new major version of the API and reworking some internals, so I want to gather use cases and see what makes sense.