Search for missing CVS files in the Attic

Review Request #555 — Created Sept. 18, 2008 and discarded

tflatter
Review Board SVN (deprecated)
394
reviewboard
Whenever a CVS file is not found on the server an error occurs. 
Now it is possible that the file was moved out of or into the Attic. 

In this case, the patched code looks for the file also in the Attic (if the file was not found outside the Attic) of outside the Attic (if the file could not be found within the Attic)

As I do not have write access to the reviewboard repository, the contents of the review cannot be commited by me. 
A member of the project has to merge the changeset into the file. Thanks.
I tested both ways within our development project
chipx86
  1. 
      
  2. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
     
     
    Why two functions? It seems you could just do the attic checks in cat_file and then just not return if it fails, and instead fall through to the code we already had.
  3. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    No need for parens.
  4. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    No need for parens, and space after the if.
    
    This would be better written as:
    
        if "/Attic/" in filename:
  5. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    This should probably use the os.path functions.
  6. 
      
TF
  1. I did some changes but I will wait until these changes are testet in our installation before I will publish the next diff version. 
    1. OK, I published the diff 6 days ago, just for documentation as I've seen one could think this is still missing...
  2. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    I did not want to duplicate code. 
    My code extension is only necessary if no file was found. 
    Then, the cvs access has to be done again, this time with the Attic-path-element added or removed, depending on the previous way it failed. 
    
    I added a second method as I had to add a parameter to prevent endless recursion. The Attic-swap may only be done once as it could cycle endless (try with Attic, then without, then with, then without, ...)
    If there is a possibility in python to set a parameter to a default value, then this would be better (e.g. cat_file(self, filename, revision, tryAttic = 1). I don't know this as I am "only" a java and .net developer with only little experience in python.
  3. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    OK
  4. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    sounds fine. 
  5. trunk/reviewboard/scmtools/cvs.py (Diff revision 2)
     
     
    not so good as this would imply that the CVS repository resides on the same os as Review Board does, what normally must not be true.
    
    I made the implementation better as I checked before, what kind of path separator is used within the current cvs filename and adept the Attic-part to that. 
  6. 
      
TF
  1. 
      
  2. trunk/reviewboard/scmtools/cvs.py (Diff revision 3)
     
     
    The type of the path delimeters depends on the TARGET operating system, where the CVS resides. This is why os.path is not possible here.
  3. 
      
chipx86
  1. Sorry for taking so long to get to this.
    
    I'm unsure about your changes here. I have more details in the comments below, but it looks like the core problem is that we strip out the "/Attic/" early. I would imagine what we really want to do is save the non-stripped version and check both.
    
    What tests did you run?
    
    By the way, to reply to this, comment on the review request page, not in the diff viewer. You can't reply to existing comments there, and it really messes with the e-mails and flow of the discussion.
  2. trunk/reviewboard/scmtools/cvs.py (Diff revision 3)
     
     
     
     
     
    We already do this above (see the line above that does a filename.rsplit('/Attic/', 1)). So this probably isn't necessary. I imagine we'll just need to try putting the "/Attic/" on.
  3. trunk/reviewboard/scmtools/cvs.py (Diff revision 3)
     
     
     
     
    The rest of this file makes the assumption that paths are always "/". I believe CVS on both Windows and Linux represents paths at this point as "/", since it's dealing in server paths.
    
    Given the assumption, we probably don't need the Windows-style checks due to this, as they just won't work anyway. If the assumption is wrong on Windows, then we'll have to do a lot more work in this function and should just have some path normalization function at the end that goes from "/" to "\".
    
    So we should really only need the "/" checks for now.
  4. 
      
Loading...