This is a complex change, so there's a number of things we'll have to go back and forth on. As you read through, you'll see a trend in my comments (and I didn't address every relevant line). The main things are:

1) Make sure you're not using too many or too few lines. You want 2 blank lines before classes, 1 blank line between functions, and 1 blank line before/after blocks (if statements, while loops, etc.)

2) Comments should always be in sentence form. Begin with a capital letter, end with a period.

3) Always wrap lines to about 75 characters. They should never go past 80.

4) A lot of this code is making platform-dependent choices. I'm not very comfortable with this. Where possible, use standard os.path functions. Try to minimize the number of platform checks as best as you can.

Should be sorted alphabetically in the include list.

This probably doesn't belong in this change.

Two blank lines before this.

Also, This should be spelled out as ClearCaseClient.

How do clearcase views work? Is this an okay assumption to make? What if the Review Board server is talking to multiple clearcase servers?

Remove this line.

Comments should be in sentence form (start with a capital letter, end with a period).

This shouldn't be part of this change.

This should all be removed as well. In fact, this whole function should go away so the default SCMClient implementation can be used to determine the server based on config files.

If you didn't need the debug output, this could be simplified as:

md5.md5(fname).hexdigest()

Remove this blank line.

Looks like we're just stripping leading "\"s, so we can instead do:

s = s.lstrip("\\")

Shame the unc path code doesn't exist on non-Windows installs of Python...

It would be nice to add comments explaining what's going on here a bit.

I have to wonder, though.. Since we're already converting "/" to "\", why not make sure we're using "/" first and then do:

s = os.path.normpath(s)
s = s.replace("/", "\\")

Does this really make sense? Wouldn't we end up with something like C:\\foo\\bar\\abc?

No space after execute.

Same here.

Make sure this wraps to about ~75 characters.

Seems like overkill for a regex.

For things like this, os.path.join is the correct method.

No space after execute.

Only one blank line here.

Do we really need a regex for this? We can't just do:

if "@@" in whatever: ?

Only one blank line here.

Too many spaces after the "=". Also, there's no reason to special-case these on platforms. The code's the same.

Wrap to ~75 characters.

Use os.path.join.

No space before execute.

You can just do:

if this_version:

No blank line here.

These are exactly the same. No need to special case on platform.

This is better done as:


return [line.strip() for line in ldesc
        if line.startswith("/vbos")]

No blank line.

No blank line.

Only one blank line. Same with other things below. I recommend going through and making sure you're not adding condensed lines.

Better as:

return [line.rstrip("\r\n") + "\n"
        for line in data]

How necessary is this, though? We should handle line endings in Review Board natively. I'd be a little concerned with doing this in this code.

I'm really worried about this causing regressions. Especially when it's perfectly valid for a file to have multiple newlines at the end, and this is assuming we'll only have one. Not to mention that I'm not even sure we'd have any \r\n anyway, given the previous lines. 

What's the rationale for this?

We can't have any SCMTool-specific code in this file. This logic will have to be done in the SCMTool somehow.

Should be alphabetical.

I don't think you really mean to import this.

This shouldn't be in this change. If you absolutely need debugging, use the standard Python logging support, with a log level of DEBUG.

This should be named ClearCaseTool.

Shouldn't be here.

What's the purpose of this? I noticed you're trying to use it in common code, but it should only ever be used internally.

I have a hunch you really want to use os.path stuff for this.

Fix indentation.

What's the purpose of the str assignment? And some documentation for the function would be nice, because the name is ambiguous (and doesn't follow the same naming style as other functions).

Can you provide docs for this function as well?

Should be removed.

ClearCaseDiffParser.

Can collapse these:

if 'index' in info and self.lines[linenum] == self.BINARY_STRING:

ClearCaseClient.

You can pass in filename directly. No need for "%s" % filename.

Simpler as:

if not failure:
    return contents

if errmsg.startswith(...)
    ...
else:
    ...

This part of the function basically removes the "previous dir (..)" and "this dir (.)" redirections. For example:
/root/dir1/dir2/../dir3 becomes /root/dir1/dir3

Yes, we will. But this is necessary because we use this string later on, the single \ just disappear.

I got lots of problems with line endings. I tried to create a common way to create the diff so it won't show the whole file in the post review server or fail to apply the patch. It just doesn't work well if I don't handle this problem. 

Any idea? I just need to remove all the extended path data to display something readable.

Removed

Sorted

Modified

These comments are mostly stylistic issues:

Can you align the env= with the ["clear... ?

Can you name this "file" ?

Instead of defining so many variables here, just do:

curr_version, pre_version = execute(...).split('\n')

Similarly here,

pre_version = pre_version.split(':')[1].strip()

You don't need the parentheses here.

If you're going to comment this as 'fname', please call the variable 'fname' as well.

A few style issues here. I think this would be much clearer written as:

bpath = vkey[:vkey.rfind('vobs') + 4]
fpath = vkey[vkey.rfind('vobs') + 5:]

Alignment looks odd here.

Please indent the vkey line only 4 spaces

Please call this "name"

Alignment looks odd here.

Alignment here too.

No space between "except" and ":"

No parentheses here:

return set(filelist)

No parentheses around the return value

Just write this as one line:

temp.append(line.rstrip('\r\n') + '\n')

Add spaces around operators.

Add spaces around operators.

Formatting is a bit odd here.  Can you write it like this?

if (self.viewtype == 'snapshot'
    and (sys.platform.startswith('win') or
         sys.platform.startswith('cygwin'))):
    ...

Is this line longer than 80 characters now?

Remove this blank line.

Instead of these, you can just do:
fpath = ['vobs']

What about linux filenames that contain :?

Just a few trivial comments now.

You've got some trailing whitespace here.

Whitespace here too. Please go through the diff viewer on RB and check for red.

What are the parentheses here for?

Indent the parameters 4 spaces from the previous block.

Indent these 4 spaces past the last block indent.

Is this supposed to be /-joined or system separator joined?

Trailing whitespace

This looks good. Committed as r1725. Thanks!