929: reviewboard should not alter the diff file

Vijai*****@gmai***** (Google Code) (Is this you? Claim this profile.)
chipx86
 chipx86
Sept. 24, 2012
1229, 2172, 2420, 2690, 2953 
What's the URL of the page containing the problem?
http://reviews/r/18/diff/raw/

What steps will reproduce the problem?
1. svn diff > changes.diff
2. Create a review request
3. Download the new diff as rboard.diff

What is the expected output? What do you see instead?
changes.diff and rboard.diff should be same but they are not. rboard.diff
doesn't apply cleanly on a fresh copy of svn base.
david
#1 david
david
#2 david
  • +Component-DiffParser
#3 ash****@ashee****** (Google Code) (Is this you? Claim this profile.) 
I'm thinking about submitting a patch to ReviewBoard that fixes this. It seems like reviewboard could simply save the uploaded diff somewhere, and return that to the user when the user asks to download it.

Do you think it'd likely to be that simple? If so I will submit a patch that does this.
chipx86
#4 chipx86 
It does do this. It just breaks it up per-file (which is needed). The problem is that the parser doesn't know about some of the extra metadata. So really, the parser just needs to be fixed to not lose this.
#5 djs%n-c*******@gtempacc******** (Google Code) (Is this you? Claim this profile.) 
I wonder if this wouldn't still be a good idea. I was trying to review http://reviews.reviewboard.org/r/1737/ the other day because I thought it might address this problem, at least for git. However, besides missing the mail headers from format-patch, it was also missing the new file mode lines, so even once I had an appropriate base to apply on, I had to manually edit the diff just to get it to apply.

I suspect there's always going to be the risk of losing portions of a diff if you break up and process it and then try to recreate it. Saving a pristine 'patch' alongside the reviewboard internal diff segments might actually be a more robust approach.

Whatever we do, we really need to do something. I was hoping Eduardo's patch would do this, but it definitely had some bugs in it and I'm a bit confused over the 'bundle' concept he introduced since a format-patch output is only a single patch...
david
#6 david
david
#7 david
#8 tif***@gmai***** (Google Code) (Is this you? Claim this profile.) 
This really seems like a big issue. It causes git diff files containing diffs concerning binary or utf16 files to become corrupted. This makes reviewboard an unreliable diff sharing location.

Is there anyone working on this issue?
david
#9 david 
Our initial designs never anticipated reviewboard being used as a "diff sharing location", merely as a code review tool.

I agree it would be nice to fix but right now we're busy with other things. Unless someone steps up to do it, we don't have a timeline for fixing this.
#10 sed***@gmai***** (Google Code) (Is this you? Claim this profile.) 
All "git diff format support" tickets were closed as duplicate of this ticket. I don't know why but the need for git diff format support has more ramifications than just "sharing diffs":

- It can describe move/rename operations which saves the reviewer's time to understand the "three pages of removed line block" and "three pages of added line block" were actually a single rename. 

- Mercurial Queues REQUIRE git-diff format to work correctly. It's recommended to be set it in the .hgrc. That prevents developer from being able to generate unified diff format and requires editing .hgrc file over and over for each specific goal.

- When binary files are present, MQ switches to git format which cannot be turned off in newer Mercurial versions (namely 2.2.1).
chipx86
#11 chipx86 
Hi sedatk,

The reason they were closed is that they're all symptoms of Review Board inadvertently stripping data from the diff when storing in the database. That's a key problem that must be fixed (and I believe we've done that for Git at least, but maybe not in a released version -- I forget when that landed).

Can you describe for me how fixing that isn't sufficient for the cases you described?
  • +NeedInfo
#12 *@vidsol******* (Google Code) (Is this you? Claim this profile.) 
Hi Chris,

can you elaborate on how you changed diff handling for git? We have a user of reviewboard.kde.org who saw that all information above line 10 was stripped. I guess he was expecting to be able to see this information (e.g. commit message) even after he uploaded the patch to review board.

Any news on this issue?

David
  • +
    0001-When-editing-the-Tracks-of-Multiple-Files-show-the-D.patch 
    diff --git a/src/dialogs/TagDialog.cpp b/src/dialogs/TagDialog.cpp
    --- a/src/dialogs/TagDialog.cpp
    +++ b/src/dialogs/TagDialog.cpp
    @@ -1193,9 +1193,10 @@ TagDialog::readMultipleTracks()
         m_currentData = QVariantMap();
     
         QVariantMap first = dataForTrack( it.peekNext() );
    +    QString firstDirectory = it.peekNext()->playableUrl().directory();
     
         bool artist=true, album=true, genre=true, comment=true, year=true,
    -         score=true, rating=true, composer=true, discNumber=true;
    +         score=true, rating=true, composer=true, discNumber=true, directory = true;
         int songCount=0, ratingCount=0, ratingSum=0, scoreCount=0;
         double scoreSum = 0.f;
         while( it.hasNext() )
    @@ -1210,6 +1211,7 @@ TagDialog::readMultipleTracks()
             }
     
             QVariantMap data = dataForTrack( next );
    +        QString currDirectory = next->playableUrl().directory();
             songCount++;
             if ( data.value( Meta::Field::RATING ).toInt() )
             {
    @@ -1221,11 +1223,10 @@ TagDi
#14 *@vidsol******* (Google Code) (Is this you? Claim this profile.) 
Ups sorry, attached the stripped down version of the patch. Here is the original patch. It was created with git-format-patch btw.
  • +
    0001-When-editing-the-Tracks-of-Multiple-Files-show-the-D (1).patch 
    From 41bf5e4105c5dbab1782711b331086662d878f97 Mon Sep 17 00:00:00 2001
    From: Stefan Derkits <stefan@derkits.net>
    Date: Wed, 15 Sep 2010 22:21:50 +0200
    Subject: [PATCH] When editing the Tracks of Multiple Files show the Directory of the Files in the TagDialog where the Files reside when all Files are local Files & are in the same Directory
    ---
     src/dialogs/TagDialog.cpp |   18 +++++++++++++++---
     1 files changed, 15 insertions(+), 3 deletions(-)
    diff --git a/src/dialogs/TagDialog.cpp b/src/dialogs/TagDialog.cpp
    index 68fc5e3..26d4eb8 100644
    --- a/src/dialogs/TagDialog.cpp
    +++ b/src/dialogs/TagDialog.cpp
    @@ -1193,9 +1193,10 @@ TagDialog::readMultipleTracks()
         m_currentData = QVariantMap();
     
         QVariantMap first = dataForTrack( it.peekNext() );
    +    QString firstDirectory = it.peekNext()->playableUrl().directory();
     
         bool artist=true, album=true, genre=true, comment=true, year=true,
    -         score=true, rating=true, composer=true, discNumber=true;
    +         score=true, rati
    
 
    

    


    
   
    • 


  


 








 

  

   david

  

 
 

  

    
     

   
   #15
   david


  



  

 








 

   

 
 

  

    
     

   
   #16
   srivatsav*********@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
It is not clear whether this issue has been closed or not. Comment #11 gives me the impression this has been fixed? Could you please let us know the version of RB that has the fix. We're desperately trying to get this to work at reviews.apache.org. 



 








 

  

   david

  

 
 

  

    
     

   
   #17
   david


  



  
    


   
  • 
    
    
    -NeedInfo
    +New
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #18
   chipx86


  



  
    


   
  • 
    
    
    -New
    +PendingReview
    
   
    • 



   
  • 
    
    
    -Priority-Medium
    +Priority-Critical
    +Milestone-Release1.6.x
    
   
    • 



   
  • 
    
    
    +chipx86
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #19
   chipx86


  


  
This should be fixed properly now, and a release is going up tonight.



  
    


   
  • 
    
    
    -PendingReview
    +Fixed
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #20
   chipx86


  


  
Pushed to release-1.6.x (8281c37c4fae1f5a283c735f1c7e7cf8546767ac)



 








 

  

   david

  

 
 

  

    
     

   
   #21
   david