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.

No need for parens.

No need for parens, and space after the if.

This would be better written as:

    if "/Attic/" in filename:

This should probably use the os.path functions.

I did some changes but I will wait until these changes are testet in our installation before I will publish the next diff version. 

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.

OK

sounds fine. 

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. 

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.

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.

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.

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.