394: if a diff includes a file that has been cvs remove'd, reviewboard won't accept it

tim****@gmai***** (Google Code) (Is this you? Claim this profile.)
chipx86
 chipx86
June 30, 2010 
If you cvs remove a file and then do a cvs diff -N to create the patch,
reviewboard will say "The file '...' could not be found in the repository"
and refuse to open the review request.
chipx86
#1 chipx86
  • +Component-SCMTools
#2 c.gosch%*********@gtempacc******** (Google Code) (Is this you? Claim this profile.) 
This happens to all reviews containing files actually not being visible any more at 
the location where they were when the review was created.

Causing situations:

- A review is created containing new files created on a branch not being HEAD, and 
the new files are merged into HEAD afterwords: The new files are placed in "Attic" 
first by CVS and then moved away from "Attic" on making them present in HEAD.

- A review is created containing a file which is deleted from HEAD afterwords (like 
originally written here): The file is moved to "Attic" on CVS and not visible 
anymore on the original location.

Reviews having this problem are not editable any more: They cannot be discarded, nor 
can they be set to Submitted to clean My Dashboard.

IMHO that is an annoying bug having higher priority than "medium".

ReviewBoard *should* ignore files not visible any more *or* try to locate them in 
the corresponding Attic (or just outside if they were inside originally). Anyway the 
review should at least open for editing it as a whole.
chipx86
#3 chipx86 
If we fail to find it, we should just check in Attic next before giving up. Probably
an easy patch if someone wants to give it a shot.
chipx86
#4 chipx86 
I thought though that if you accessed a file that has since moved to Attic, but you
accessed it with a revision number from when it wasn't in Attic, that it would find
and fetch the file?
#5 c.gosch%*********@gtempacc******** (Google Code) (Is this you? Claim this profile.) 
However both causing situations should just be checked by the developer(s) willing 
to do something about it.

I put a HTML attachment here with "case 1", which is: File is originally added and 
edited for some steps on a branch not being HEAD, a review is created in this 
situation, and THEN the file is merged onto HEAD. (In a multi-branch development 
environment that's not that unusual.)

btw the file is kind of "anonymous" now, but I really was surprised what is inside 
this error report page but not visible at all...
  • +
    reviewboard_issue394_example.html 
    <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
    <html lang="en">
    <head>
      <meta http-equiv="content-type" content="text/html; charset=utf-8">
      <meta name="robots" content="NONE,NOARCHIVE">
      <title>FileNotFoundError at /r/11/</title>
      <style type="text/css">
        html * { padding:0; margin:0; }
        body * { padding:10px 20px; }
        body * * { padding:0; }
        body { font:small sans-serif; }
        body>div { border-bottom:1px solid #ddd; }
        h1 { font-weight:normal; }
        h2 { margin-bottom:.8em; }
        h2 span { font-size:80%; color:#666; font-weight:normal; }
        h3 { margin:1em 0 .5em 0; }
        h4 { margin:0 0 .5em 0; font-weight: normal; }
        table { border:1px solid #ccc; border-collapse: collapse; width:100%; background:white; }
        tbody td, tbody th { vertical-align:top; padding:2px 3px; }
        thead th { padding:1px 6px 1px 3px; background:#fefefe; text-align:left; font-weight:normal; font-size:11px; bor
#6 torsten********@gmai***** (Google Code) (Is this you? Claim this profile.) 
Hello alltogether,

I just wrote a patch for the file scmtools/cvs.py
This patch will fix that issue. 

Regards,

Torsten Flatter
  • +
    cvs.py 
    import os
    import re
    import subprocess
    import tempfile
    from reviewboard.scmtools.core import SCMError, FileNotFoundError, \
                                          SCMTool, HEAD, PRE_CREATION
    from reviewboard.diffviewer.parser import DiffParser, DiffParserError
    class CVSTool(SCMTool):
        regex_rev = re.compile(r'^.*?(\d+(\.\d+)+)\r?$')
        regex_repopath = re.compile(r'^(?P<hostname>.*):(?P<port>\d+)?(?P<path>.*)')
        def __init__(self, repository):
            SCMTool.__init__(self, repository)
            self.cvsroot, self.repopath = self.build_cvsroot()
            self.client = CVSClient(self.cvsroot, self.repopath)
        def build_cvsroot(self):
            # NOTE: According to cvs, the following formats are valid.
            #
            #  :(gserver|kserver|pserver):[[user][:password]@]host[:[port]]/path
            #  [:(ext|server):][[user]@]host[:]/path
            #  :local:e:\path
            #  :fork:/path
            if not self.repository.path.startswith(":"):
                # The user has a path or someth
    
 
    

    


    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #7
   chipx86


  


  
Can you please provide this as a diff on http://reviews.review-board.org/ ?



  
    


   
  • 
    
    
    +PendingReview
    
   
    • 


  


 








 

   

 
 

  

    
     

   
   #8
   torsten********@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
Done: http://reviews.review-board.org/r/555/ 



 








 

  

   david

  

 
 

  

    
     

   
   #9
   david


  



  
    


   
  • 
    
    
    +Started
    
   
    • 


  


 








 

  

   david

  

 
 

  

    
     

   
   #10
   david


  



  
    


   
  • 
    
    
    -Started
    +Fixed
    
   
    • 


  


 








 

   

 
 

  

    
     

   
   #11
   msb****@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
I'm having this same issue in ReviewBoard 1.5 beta 2. Was this fix ever applied/accepted?



 








 

   

 
 

  

    
     

   
   #12
   msb****@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
Whoops sorry - I'm on 1.0.8, not 1.5.



 








 

  

   chipx86

  

 
 

  

    
     

   
   #13
   chipx86


  


  
It does look like the fix exists in the codebase, both on master and 1.0.x.



 








 

   

 
 

  

    
     

   
   #14
   msb****@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
As mentioned above I seem to be having this same issue with ReviewBoard 1.0.8 and a CVS project.

Please let me know if I should file this as a separate issue.

Attached is the output from running post-review with the --output-diff switch. Note I've edited out the empty output from CVS like "cvs diff: Diffing web/css" for directories in which there were no change. The diff should show that I removed one file (build.properties) and edited two others.

The output from attempting to upload this change to reviewboard with post-review is:

>>> HTTP POSTing to http://localhost/reviewboard/api/json/reviewrequests/new/: {'repository_path': 'cvshost:/cvsroot/gotoaudio'}
>>> Review request created
>>> Attempting to set field 'summary' to 'g2atest 2' for review request '32'
>>> HTTP POSTing to http://localhost/reviewboard/api/json/reviewrequests/32/draft/set/: {'summary': 'g2atest 2'}
>>> Uploading diff, size: 14196
>>> HTTP POSTing to http://localhost/reviewboard/api/json/reviewrequests/32/diff/new/: {}
>>> Got API Error 207 (HTTP code 200): The file was not found in the repository
>>> Error data: {u'stat': u'fail', u'file': u'build.properties', u'err': {u'msg': u'The file was not found in the repository', u'code': 207}, u'revision': u'1.8'}

Error uploading diff

Your review request still exists, but the diff is not attached.


Is there any other data I could supply which would be useful to help troubleshoot this?



  
    


   
  • 
    
    
    +


    
 
    
  diff.txt
  
    

  
    
 
    

 
    
  
     
    cvs diff: Diffing .
    Index: build.properties
    ===================================================================
    RCS file: build.properties
    diff -N build.properties
    --- build.properties	2 Mar 2010 16:52:36 -0000	1.8
    +++ /dev/null	1 Jan 1970 00:00:00 -0000
    @@ -1,17 +0,0 @@
    -# Ant properties for building projectname-mvc
    -cvs.user = user
    -
    -appserver.home=C:/Tools/apache-tomcat-6.0.20
    -# for Tomcat 5 use $appserver.home}/server/lib
    -# for Tomcat 6 use $appserver.home}/lib
    -#appserver.lib=${appserver.home}/server/lib
    -appserver.lib=${appserver.home}/lib
    -
    -#port app server is on
    -appserver.port = 8686
    -
    -deploy.path=${appserver.home}/webapps
    -
    -tomcat.manager.url=http://localhost:${appserver.port}/manager
    -tomcat.manager.username=tomcat
    -tomcat.manager.password=tomcat
    Index: build.xml
    ===================================================================
    RCS file: /cvsroot/projectname/projectname/build.xml,v
    retrieving revision 1.51
    diff -u -r1.51 build.xml
    --- build.xml	20 Jan 2010 17:45:20 -
    
 
    

    


    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #15
   chipx86


  


  
Looks like we need to make sure when we see the "+++ /dev/null" line that we should mark the file as deleted and not attempt to open it.



  
    


   
  • 
    
    
    -Fixed
    +Confirmed
    
   
    • 



   
  • 
    
    
    -PendingReview
    +Milestone-Release1.5
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #16
   chipx86


  


  
Actually, that won't be good enough. We really do need the full file path. cvs really should be showing the full path information. The fact that it's not is bad, and we need to investigate how long this has been happening and figure out if there's a fix for it.



 








 

  

   chipx86

  

 
 

  

    
     

   
   #17
   chipx86


  


  
Been poking at this. Since the problem is that the RCS File line doesn't contain any information on which module the file is in, we may have to post-process the diff in post-review. Another option is to finally give Review Board a concept of "deleted files", but that's potentially a larger change.

This appears to be a conscious choice on behalf of cvs:

    /* This is fullname, not file, possibly despite the POSIX.2
     * specification, because that's the way all the Larry Wall
     * implementations of patch (are there other implementations?) want
     * things and the POSIX.2 spec appears to leave room for this.
     */



  
    


   
  • 
    
    
    -Component-SCMTools
    +Component-DiffViewer
    +Component-DiffParser
    
   
    • 



   
  • 
    
    
    +chipx86
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #18
   chipx86


  


  
That wasn't too bad.. I have a patch up for review at http://reviews.reviewboard.org/r/1685/



  
    


   
  • 
    
    
    -Confirmed
    +PendingReview
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #19
   chipx86


  


  
This should work fine now. We now indicate deletes as actual deletes, instead of bothering with any patch operations.



  
    


   
  • 
    
    
    -PendingReview
    +Fixed
    
   
    •