# Clearcase module with post-review support

Review Request #653 — Created Dec. 1, 2008 and submitted

d.belz
Review Board SVN (deprecated)
reviewboard
Another clearcase integration module with post-review support. It supports windows, linux and cygwin environments, and both snapshot and dynamic views.
The module depends on a mounted view on the reviewboard server. It is capable of submit reviews for checked out and checked in elements.
Submitted reviews on linux, windows and cygwin using snapshot and dynamic views.
1. 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.
1. Christian,
thank you for the inputs. I made some improvements on the second version. I addressed most of the initial issues, but as you said it is a complex change so more work has to be done.
2. Some more comments here, based on our earlier discussions. Also, it's hard to figure out where your comments as separate reviews tie in with my earlier comments, so I left those out, but it would be nice to move those up into this discussion if it's not too much work. I'll take another look at the diff in a bit, probably after I understand some of the aspects of this change better from this discussion.
3. Most of the changes I made were related to:
1 - Blank lines.
3 - Line wraps.
4 - Platform dependent choices.

The only functional change I made from the first version to the current is related to the label submission function. 
2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)




Should be sorted alphabetically in the include list.
1. Sorted.
3. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



This probably doesn't belong in this change.
1. Removed.
4. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Two blank lines before this.

Also, This should be spelled out as ClearCaseClient.
1. CC changed to ClearCase
5. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


How do clearcase views work? Is this an okay assumption to make? What if the Review Board server is talking to multiple clearcase servers?
1. For clearcase installs, there are generally not more than one repository, but agreed, there may be. AFAIK, clearcase is generally site installed. I don’t see any reason not to generalize this to allow for multiple clearcase repositories, each with their own view. The standard usage, however, would be to have one repository that all clearcase users point to.
I'll try to address this on the next version.
6. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Remove this line.
1. Removed.
7. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Comments should be in sentence form (start with a capital letter, end with a period).
1. Ok
8. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



This shouldn't be part of this change.
1. This was shown to be helpful for us. I'll submit as a different change.
9. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)




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.
1. Removed.
10. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)




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

md5.md5(fname).hexdigest()
1. Ok.
11. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Remove this blank line.
1. Removed.
12. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



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

s = s.lstrip("\\")
1. Ok
13. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)











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

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("/", "\\")
1. Removed.
14. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Does this really make sense? Wouldn't we end up with something like C:\\foo\\bar\\abc?
1. This was somehow needed when running execute. Not anymore.
15. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


No space after execute.
1. Removed.
16. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Same here.
1. Removed.
17. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Make sure this wraps to about ~75 characters.
1. Wrapped.
18. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



Seems like overkill for a regex.
1. Regex removed.
19. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)




For things like this, os.path.join is the correct method.
1. Found the problem with path. Linux uses posixpath and windows uses ntpath. Cygwin should use ntpath as well, but it uses posix, and this makes the things a mess. The import on the beginning of the new version will fix this issue, and will be the only platform-dependent choice in the code.
20. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


No space after execute.
1. Space removed.
21. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



Only one blank line here.
1. Blank line removed.
22. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


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

if "@@" in whatever: ?
1. Removed all the regex's.
23. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



Only one blank line here.
1. Extra blank line removed.
24. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)




Too many spaces after the "=". Also, there's no reason to special-case these on platforms. The code's the same.
1. Fixed.
25. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



Wrap to ~75 characters.
1. Wrapped code and comments.
26. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)





Use os.path.join.
1. Fixed here and used os.path.join wherever possible.
27. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


No space before execute.
1. Space removed.
28. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


You can just do:

if this_version:
1. Fixed.
29. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


No blank line here.
1. Removed.
30. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)





These are exactly the same. No need to special case on platform.
1. This is not fully implemented yet. I need to find a better way to check which files were affected by the label, but if I rely just on the label description the info may be wrong or incomplete. If I do check file by file it takes too long.
2. This was fixed in the latest version.
31. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)










This is better done as:

return [line.strip() for line in ldesc
if line.startswith("/vbos")]
32. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


No blank line.
1. Removed.
33. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


No blank line.
1. Removed.
34. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)



Only one blank line. Same with other things below. I recommend going through and making sure you're not adding condensed lines.
1. Removed one line.
35. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)








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.
1. Removed the line endings handling functions.
36. /trunk/reviewboard/diffviewer/diffutils.py (Diff revision 1)



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?
1. We had lots of problems when generating reviews from files changed in different OS's. The problem was that sometimes the entire file was shown in the reviewboard page as changed. Sometimes the reviewboard server just failed to apply the patch to display the changes. This was the only way I could find to make it work right.
2. This certainly shouldn't be a problem. We do a lot of work to make sure line endings are always correct. If there's ever any point where the diff's line endings get changed before being submitted, then it's possible to have this problem.

I'd like to remove this and then find out what combination of server/client operating systems and line endings are triggering this problem so we can fix it correctly.
3. It was the difflib problem with "no new line at the end of the file".  I fixed it on the post-review tool. I'll upload this fix soon.
37. /trunk/reviewboard/diffviewer/diffutils.py (Diff revision 1)







We can't have any SCMTool-specific code in this file. This logic will have to be done in the SCMTool somehow.
1. Any idea where and how can I move this?
2. Can you explain the reason for this? Is it just to pretty-print the revision?

The source revision should be stored in the filediff. If it's part of the filename, it'll have to be parsed out in the SCMTool when parsing the diff.
3. Please take a look on the screenshots. It is to pretty-print the filename. The revision is OK.
38. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)







Should be alphabetical.
1. Ok
39. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)





I don't think you really mean to import this.
1. Ok. I based (copy-and-paste) this module on the SVN module.
40. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)




This shouldn't be in this change. If you absolutely need debugging, use the standard Python logging support, with a log level of DEBUG.
1. Removed.
41. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


This should be named ClearCaseTool.
1. Renamed.
42. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


Shouldn't be here.
1. Removed
43. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


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.
1. When submitting a review from post-review, the tool uses the extended path to generate the diff. This is necessary to get the right element version from the repository. But this makes the path displayed on the tool almost unreadable. This function display adjust the path string to show the regular path instead the extended path.
2. Okay, so this should happen when parsing the diff, before a FileDiff is even generated.
3. Take a look on the screenshots. The Extended Path is hard to read/understand. The unextended path is a regular path. The problem is that I need to cat the file using the extended path, to make sure I'm getting the right file. Then, I want to display the regular path. If I do this before the filediff, I believe I'll be reading the file from the regular path. Is that true?
44. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)



Fix indentation.
1. Done.
45. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)




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).
1. I'll check this.
2. Removed.
46. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


Can you provide docs for this function as well?
1. Commented the function.
47. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)




Should be removed.
1. Removed.
48. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


ClearCaseDiffParser.
1. Changed.
49. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)



Can collapse these:

if 'index' in info and self.lines[linenum] == self.BINARY_STRING:
1. Collapsed.
50. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


ClearCaseClient.
1. Changed.
51. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)


You can pass in filename directly. No need for "%s" % filename.
1. Changed.
52. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)








Simpler as:

if not failure:
return contents

if errmsg.startswith(...)
...
else:
...
1. Changed.
53.

D.
1.

1. In the future, please reply to comments in the existing review. The diff viewer does not offer a mechanism for replying.
2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


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

D.
1.

2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


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

D.
1.

2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


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.
3.

D.
1.

2. /trunk/reviewboard/diffviewer/diffutils.py (Diff revision 1)


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

D.
1.

2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Removed
3.

D.
1.

2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Sorted
3.

D.
1.

2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)


Modified
3.

D.
D.
D.
1. These comments are mostly stylistic issues:
2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)



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

3. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


Can you name this "file" ?
1. Done.

4. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)






Instead of defining so many variables here, just do:

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

5. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)



Similarly here,

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

6. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


You don't need the parentheses here.
1. Removed.
7. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


If you're going to comment this as 'fname', please call the variable 'fname' as well.
1. Fixed.
8. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)



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:]
1. Ok.
9. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)




Alignment looks odd here.
1. Aligned.
10. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)



Please indent the vkey line only 4 spaces
1. Done.
11. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


Please call this "name"
1. Done.

12. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)



Alignment looks odd here.
1. Aligned.
13. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)




Alignment here too.
1. Aligned.
14. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


No space between "except" and ":"
1. Removed.
15. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


No parentheses here:

return set(filelist)
1. Done.
16. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


No parentheses around the return value
1. Removed.
17. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)




Just write this as one line:

temp.append(line.rstrip('\r\n') + '\n')
1. Removed. Not necessary anymore.
18. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)



Add spaces around operators.
1. Done.
19. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


Add spaces around operators.
1. Done.
20. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)




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'))):
...
1. Fixed.
21. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)


Is this line longer than 80 characters now?
1. Done.

22. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 3)


Remove this blank line.
1. Done.
23. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 3)



Instead of these, you can just do:
fpath = ['vobs']
1. Done.
24. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 3)


What about linux filenames that contain :?
1. Fixed
25.

D.
1. Just a few trivial comments now.
2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 4)



You've got some trailing whitespace here.
1. Removed
3. /trunk/reviewboard/contrib/tools/post-review (Diff revision 4)


Whitespace here too. Please go through the diff viewer on RB and check for red.
1. Removed
4. /trunk/reviewboard/contrib/tools/post-review (Diff revision 4)



What are the parentheses here for?
5. /trunk/reviewboard/contrib/tools/post-review (Diff revision 4)




Indent the parameters 4 spaces from the previous block.
1. Done
6. /trunk/reviewboard/contrib/tools/post-review (Diff revision 4)





Indent these 4 spaces past the last block indent.
1. Done
7. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 4)


Is this supposed to be /-joined or system separator joined?
1. /-joined, because is only the filename string to be displayed.
8. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 4)


Trailing whitespace
1. Removed.
9.

D.
Review request changed

### Change Summary:

Changes suggested by David.

### Diff:

Revision 5 (+547 -3)

Show changes

1. This looks good. Committed as r1725. Thanks!
2.