Add support for working interdiffs

 






 

 


  Review Request #174
  —
  Created Nov. 11, 2007 and submitted

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board SVN (deprecated)
 









 
 

  trunk
 









 
 

  103
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This change adds support for interdiffs, both in the backend and in the web UI.

While we had some basic hidden support for interdiffs before, it was very buggy. It only really worked when the set of files were the same. Now we handle the case where the set of files between the two revisions change.

We also allow commenting on interdiffs. The comments will show up on the review and it will indicate the revision range. This prompted another change that's been a long time coming, which is to prevent the problem where a person is commenting on a diff and the author then uploads a new diff, causing the comments to "disappear."

We now have a new diff header above the file index that shows the current revision, a list of other revisions to jump to, and a little interdiff selector. If the user has comments on a diff other than the displayed one, we display an info box telling them and showing which revisions (and interdiffs) have comments. The user can then delete them if they choose.

Reviews are no longer tied to a specific revision of a diff. A user can have comments on older diffs in the review. It's now their responsibility to make sure they're still valid. Still better than us just "losing" them.

 








  


 
 

  
Uploaded several iterations of a diff and left comments on various revisions, as well as on some interdiffs. Saw the info box appear. It showed me each revision with comments. I tested the cases where the comments are only on normal diffs you're not seeing, on diffs you are seeing, on interdiffs you're not seeing, and on interdiffs you are seeing.  It worked fine in each case.

Left comments on the various revisions and verified I was seeing every one of them in the review. Published the review and saw each one appear on the page.

Created a diff that modified three files and uploaded it. Then I created another that had a new file not found in the first diff, and reverted a file that was in the first diff. The resulting interdiff correctly showed new changes for the new file and deletes for the changes in the old file (as well as saying "Reverted" in the revision area).

 







 









 





  







 

  
Screenshots

  
 



 









 






  

 








 














 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This is a really big change, so I'm going to review it in chunks.  First, screenshots:
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 











  2. 
 
    
  
  
    
  
 
    
  
   
    Interdiff
   
  
  
    Shouldn't "Changes between r0 and: " have "5" as the selected item, here?

It's a little unclear how to get an "orig" diff with this UI.
    
 
    

   
    
    
    





    
    Shouldn't "Changes between r0 and: " have "5" as the selected item, here?

It's a little unclear how to get an "orig" diff with this UI.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Oops, forgot to update the screenshot after I fixed that.

Yes, in the code up for review, 5 would be highlighted.

I was trying to think of how best to make a quick, easy to use selector that let's you specify both endpoints of the diff. I could do a dropdown for both and a button for viewing it, but I think it looks ugly.

I believe the most common usecase will be that people will want to see what changed in the most recent version of the diff. This will be easy as it currently is. If people want to jump around looking at various interdiffs, they'll have to do so by jumping to one revision in that span (either end works) and then click "Changes between ..."
      
 
      

      2. 


 
    


    

 
    

    3. 










  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I looked through this, but kind of superficially.  Given how long and complicated this change is, there might be things I missed.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       /trunk/reviewboard/diffviewer/diffutils.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    We should probably change these to be an object identity test instead of an strcmp at some point.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4.