Clearcase module with post-review support

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

Information

Review Board SVN (deprecated)

Reviewers

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.
chipx86
  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.
      2 - Comments.
      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.
  3. This probably doesn't belong in this change.
  4. Two blank lines before this.
    
    Also, This should be spelled out as ClearCaseClient.
    1. CC changed to ClearCase
  5. 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. Remove this line.
  7. Comments should be in sentence form (start with a capital letter, end with a period).
  8. 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.
  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()
  11. Remove this blank line.
  12. Looks like we're just stripping leading "\"s, so we can instead do:
    
    s = s.lstrip("\\")
  13. /trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    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("/", "\\")
  14. 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. No space after execute.
  16. Same here.
  17. Make sure this wraps to about ~75 characters.
  18. Seems like overkill for a regex.
  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. No space after execute.
  21. Only one blank line here.
    1. Blank line removed.
  22. Do we really need a regex for this? We can't just do:
    
    if "@@" in whatever: ?
    1. Removed all the regex's. 
  23. 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.
  25. 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. No space before execute.
  28. You can just do:
    
    if this_version:
  29. No blank line here.
  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. No blank line.
  33. No blank line.
  34. 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.
  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.
  41. This should be named ClearCaseTool.
  42. Shouldn't be here.
  43. 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.
  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).
  46. 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.
  48. ClearCaseDiffParser.
  49. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)
     
     
     
    Can collapse these:
    
    if 'index' in info and self.lines[linenum] == self.BINARY_STRING:
  50. ClearCaseClient.
  51. You can pass in filename directly. No need for "%s" % filename.
  52. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Simpler as:
    
    if not failure:
        return contents
    
    if errmsg.startswith(...)
        ...
    else:
        ...
  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. 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. Yes, we will. But this is necessary because we use this string later on, the single \ just disappear.
  3. 
      
D.
  1. 
      
  2. 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. Any idea? I just need to remove all the extended path data to display something readable.
  3. 
      
D.
  1. 
      
  2. Removed
  3. 
      
D.
  1. 
      
  2. Sorted
  3. 
      
D.
  1. 
      
  2. Modified
  3. 
      
D.
D.
D.
david
  1. These comments are mostly stylistic issues:
  2. Can you align the env= with the ["clear... ?
  3. Can you name this "file" ?
  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')
  5. Similarly here,
    
    pre_version = pre_version.split(':')[1].strip()
  6. You don't need the parentheses here.
  7. If you're going to comment this as 'fname', please call the variable 'fname' as well.
  8. 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:]
  9. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
     
     
     
     
    Alignment looks odd here.
  10. Please indent the vkey line only 4 spaces
  11. Please call this "name"
  12. Alignment looks odd here.
  13. /trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
     
     
     
     
    Alignment here too.
  14. No space between "except" and ":"
  15. No parentheses here:
    
    return set(filelist)
  16. No parentheses around the return value
  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. Add spaces around operators.
  19. Add spaces around operators.
  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'))):
        ...
  21. Is this line longer than 80 characters now?
  22. Remove this blank line.
  23. /trunk/reviewboard/scmtools/clearcase.py (Diff revision 3)
     
     
     
    Instead of these, you can just do:
    fpath = ['vobs']
  24. What about linux filenames that contain :?
  25. 
      
D.
david
  1. Just a few trivial comments now.
  2. You've got some trailing whitespace here.
  3. Whitespace here too. Please go through the diff viewer on RB and check for red.
  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.
  6. /trunk/reviewboard/contrib/tools/post-review (Diff revision 4)
     
     
     
     
     
    Indent these 4 spaces past the last block indent.
  7. Is this supposed to be /-joined or system separator joined?
    1. /-joined, because is only the filename string to be displayed.
  8. Trailing whitespace
  9. 
      
D.
Review request changed
david
  1. This looks good. Committed as r1725. Thanks!
  2. 
      
Loading...