1113: post-review tool cannot handle revisions with deleted files in Subversion

mdel****@gmai***** (Google Code) (Is this you? Claim this profile.)
david
 david
March 5, 2012
1738 
What version are you running?

RBTools 0.2beta1

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

N/A

What steps will reproduce the problem?
1. "svn delete <some_file>"
2. "svn commit -m 'deleting a file'"
3. "post-review --revision-range=<revision-1>:<revision>"

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

Expected: 
login prompt

Actual: 
Failed to execute command: ['svn', 'info', '<path>/<deleted_file>']
["svn: '<path>/<deleted_file>' is not under version control\n"]


What operating system are you using? What browser?

OS X

Please provide any additional information below.

Manually posting a diff to a ReviewBoard website that includes a deleted file works fine in the 
latest version, but the post-review tool does not work.
david
#1 david
  • +Component-RBTools
david
#2 david
#3 bobafe******@gmai***** (Google Code) (Is this you? Claim this profile.) 
I added a workaround for that issue on my local installation. I hope it can help anybody having the same problem until an official fix come out.

Attached are two files:
- postreview.org is the original one upon which the fix is based (RBTools-0.2)
- postreview.py is the modified one that handles the case of deleted files.
  • +
    postreview.py 
    #!/usr/bin/env python
    import cookielib
    import difflib
    import getpass
    import marshal
    import mimetools
    import ntpath
    import os
    import re
    import socket
    import stat
    import subprocess
    import sys
    import tempfile
    import urllib
    import urllib2
    from optparse import OptionParser
    from tempfile import mkstemp
    from urlparse import urljoin, urlparse
    try:
        from hashlib import md5
    except ImportError:
        # Support Python versions before 2.5.
        from md5 import md5
    try:
        import json
    except ImportError:
        import simplejson as json
    # This specific import is necessary to handle the paths for
    # cygwin enabled machines.
    if (sys.platform.startswith('win')
        or sys.platform.startswith('cygwin')):
        import ntpath as cpath
    else:
        import posixpath as cpath
    from rbtools import get_package_version, get_version_string
    ###
    # Default configuration -- user-settable variables follow.
    ###
    # The following settings usually aren't needed, but if your Review
    # Board crew has specific preferences and do
    
 
    

    


    +


    
 
    
  postreview.org
  
    

  
    
 
    

 
    
  
     
    #!/usr/bin/env python
    import cookielib
    import difflib
    import getpass
    import marshal
    import mimetools
    import ntpath
    import os
    import re
    import socket
    import stat
    import subprocess
    import sys
    import tempfile
    import urllib
    import urllib2
    from optparse import OptionParser
    from tempfile import mkstemp
    from urlparse import urljoin, urlparse
    try:
        from hashlib import md5
    except ImportError:
        # Support Python versions before 2.5.
        from md5 import md5
    try:
        import json
    except ImportError:
        import simplejson as json
    # This specific import is necessary to handle the paths for
    # cygwin enabled machines.
    if (sys.platform.startswith('win')
        or sys.platform.startswith('cygwin')):
        import ntpath as cpath
    else:
        import posixpath as cpath
    from rbtools import get_package_version, get_version_string
    ###
    # Default configuration -- user-settable variables follow.
    ###
    # The following settings usually aren't needed, but if your Review
    # Board crew has specific preferences and do
    
 
    

    


    
   
    • 


  


 








 

   

 
 

  

    
     

   
   #4
   rix****@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
Still not fixed in 0.3.2.



 








 

   

 
 

  

    
     

   
   #5
   under*****@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
This is an issue for us as well. Seeing as a fix has been sumbitted and it looks reasonably simple, could we please get a fix for this?




 








 

  

   david

  

 
 

  

    
     

   
   #6
   david


  


  
We need fixes submitted to reviews.reviewboard.org, not posted here (especially in the format that it was). The code in post-review has also diverged a lot since 0.2.



 








 

   

 
 

  

    
     

   
   #7
   jhuf*****@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
I ran into this problem recently.  I understand that you have an established workflow but this is a nasty bug that cost more than a few of us time.  I don't know your code base, but was successfully (easily) able to port the files from above (0.2 to 0.3.4).

I'm going to attach my diff from 0.3.4, if you really need me to apply it to reviews.reviewboard.org I can, but it seems like you should be able to do it more quickly.  I really hope this gets incorporated into future versions as it is quite annoying.




  
    


   
  • 
    
    
    +


    
 
    
  postreview.py.diff
  
    

  
    
 
    

 
    
  
     
    --- C:/Users/endeca/Desktop/temp/postreview.old.py	Mon Sep 26 22:19:18 2011
    +++ C:/Users/endeca/Desktop/temp/postreview.new.py	Tue Feb 21 13:25:42 2012
    @@ -1753,8 +1753,8 @@
                 # This is where we decide how mangle the previous '--- '
                 if self.DIFF_NEW_FILE_LINE_RE.match(line):
                     to_file, _ = self.parse_filename_header(line[4:])
    -                info       = self.svn_info(to_file)
    -                if info.has_key("Copied From URL"):
    +                info       = self.svn_info(to_file, True)
    +                if info is not None and info.has_key("Copied From URL"):
                         url       = info["Copied From URL"]
                         root      = info["Repository Root"]
                         from_file = urllib.unquote(url[len(root):])
    @@ -1780,6 +1780,7 @@
     
             for line in diff_content:
                 front = None
    +            orgLine = line
                 if (self.DIFF_NEW_FILE_LINE_RE.match(line)
                     or self.DIFF_ORIG_FILE_LI
    
 
    

    


    
   
    • 


  


 








 

  

   david

  

 
 

  

    
     

   
   #8
   david


  


  
I've refactored the patch to apply to 0.4.x and it's up for review: http://reviews.reviewboard.org/r/2932/



  
    


   
  • 
    
    
    +PendingReview
    
   
    • 



   
  • 
    
    
    +david
    
   
    • 


  


 








 

  

   david

  

 
 

  

    
     

   
   #9
   david


  


  
Fixed in master (f226c46). Thanks!



  
    


   
  • 
    
    
    -PendingReview
    +Fixed
    
   
    • 


  


 








 

   

 
 

  

    
     

   
   #10
   abedna******@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
Still an issue for version 0.4.2:

E200009: Could not display info for all targets because some targets don't exist\n"