[ReviewBoard] Brand new Clear Case implementation

Review Request #1845 — Created Oct. 19, 2010 and submitted

Information

Review Board

Reviewers

Fixed:
* Removed unnecessary adjust_path function - paths should be well formed from outside
* Speed up reading files using Python internal open file instead "cat" command
* Function unextend_path implemented by compiled regular expression instead unreadable loop
* Removed unnecessary functions copied from subversion implementation
* Function normalize_path_for_display return path relative to repository
* Name of tool changed to ClearCase
* DiffViewer print normalized_path insted ClearCase extended_path
* Work also under Windows
* Recognize binary files
* Make use of HEAD
* Return appropriate files as PRE-CREATION
* Read OIDs and translate to working paths
Linux:
* Tested

Windows:
* Need tests after changes
david
  1. You seem to have two of these reviews. Is this one valid? Can you discard it if not?
    1. Both is valid. One concer Review Board second RBTools. I can add prefix to both to be clear.
  2. 
      
jan.koprowski
jan.koprowski
jan.koprowski
PA
  1. 
      
  2. reviewboard/scmtools/clearcase.py (Diff revisions 1 - 3)
     
     
    Very good line. Very good line indeed!
    1. Sorry, for that. Came here from "Demo" link on RB site. First I thought it's just a random RB playground, but it seems that is a living site and your doing some good job here. Cheers!
  3. 
      
jan.koprowski
jan.koprowski
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
    I'm not happy for getting scmtool each time in loop. I will be glad show me the way to optimize this part. I also noticed only my changes need to recreate scmtool object (I made one more change half year ago in pygments part). So maybe instead this model should be hacked to return normalized_paths in some cases. I don't know... I'm just thinking loud.
    
    I'm also wondering to rename "tool" to "scmtool".
  3. 
      
jan.koprowski
jan.koprowski
jan.koprowski
  1. 
      
  2. reviewboard/scmtools/clearcase.py (Diff revision 5)
     
     
     
    I'm not 100% convinced that implementation with so complicated regular expression is more readable and easiest to understand then version with loop. I will be appreciated for discussion in this manner.
  3. 
      
jan.koprowski
jan.koprowski
chipx86
  1. Awesome, Jan! Glad to see some of this cleaned up. Got some changes to make (mostly stylistic and documentation), and a couple questions.
    
    Regarding your TODO entry on get_filenames_in_revision... You can ignore this function. It's from a long, long time ago and isn't even used anymore. I really should get rid of it.
  2. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
     
    Imports should be in alphabetical order.
  3. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
    No need to import os.path, since you're already importing os and it brings path with it.
  4. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
     
    Two lines between imports and class.
  5. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
    The first line of a doc block should be a very short summary about what it does. Ideally one line.
    
    Then, a blank line, and then the rest of the description.
    
    The first line of the summary should be on the line containing the """
  6. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
     
     
    For better documentation generation, each indented area should be separated by a blank line, and the preceding text with a ":" should actually do "::". This will tell Sphinx/ReST that the following text should be marked up as monospaced text.
  7. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
    period after "list"
  8. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
     
     
    Same comment about a one-line summary.
  9. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
     
    Condense this to one statement.
    
    return os.path.relpath(
        ...
    )
  10. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
    Collapse to one line, with """ on both ends.
  11. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
    Is there never an equivalent to PRE_CREATION? How are new files marked up?
    1. I'm not sure what PRE_CREATION is in this situation... Hmm... You mean. File which was send to review but never checked in (commit)? In other words file which exists only in developer view (repository) and no one else. Frankly - I didn't check something like this because we base on post review model. I will check this and see how Review Board and RBTools behave in such situation.
  12. reviewboard/scmtools/clearcase.py (Diff revision 7)
     
     
     
     
    Use 'f' instead of 'file', since file is meant to be reserved in Python.
  13. 
      
DJ
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 7)
     
     
     
     
     
     
    I guess you figured out how to change the displayed filename! On the other hand, what's the point in storing the extended path if we don't display it? Why not remove it in post-review?
    1. 1) Answering Your question - because only file indicated by extended_path exists in file system.
      cat /vobs/ABC/dir@@/main/7/file.c@@/main/22
      #include <stdio.h>
      
      void main() {
      ...
      
      cat /vobs/ABC/dir/file.c
      cat: /vobs/ABC/dir/file.c: No such file or directory
      
      The point this path doesn't exists in file system so You never even read content of file. You loose information about revision/branch - everything. Removing this parts make our path completely useless.
      
      2) I don't call get_scmtool() in loop. This is not good idea. But I need a clue how to fix this or guarantee that Python can optimize loop as this to take tool only once.
  3. 
      
jan.koprowski
  1. I will be glad to review also this part: http://reviews.reviewboard.org/r/1843/
  2. 
      
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
LI
  1. ????
  2. 
      
LI
  1. ????
  2. 
      
david
  1. This looks pretty great. Is this change at the point where you want it committed?
  2. reviewboard/scmtools/clearcase.py (Diff revision 11)
     
     
    Trailing whitespace.
  3. 
      
jan.koprowski
jan.koprowski
jan.koprowski
david
  1. Just a few (mostly cosmetic) changes.
  2. docs/manual/users/tools/post-review.txt (Diff revision 13)
     
     
     
    You introduced a typo in this line.
  3. reviewboard/scmtools/clearcase.py (Diff revision 13)
     
     
     
    Switch these two (we try to keep these alphabetized).
  4. reviewboard/scmtools/clearcase.py (Diff revision 13)
     
     
     
    This looks like it should fit on a single line.
  5. reviewboard/scmtools/clearcase.py (Diff revision 13)
     
     
    Get rid of this line.
  6. reviewboard/scmtools/clearcase.py (Diff revision 13)
     
     
     
     
    This is wrapped too much. Wrap at 80 columns.
  7. reviewboard/scmtools/clearcase.py (Diff revision 13)
     
     
     
    Can you make the docstring summary fit on a single line?
  8. reviewboard/scmtools/clearcase.py (Diff revision 13)
     
     
     
     
     
    Can you put "cmdline" on the next line, so that all the argument line up?
  9. 
      
jan.koprowski
david
  1. Can you figure out which of our open clearcase bugs are fixed by this change?
  2. docs/manual/users/tools/post-review.txt (Diff revision 14)
     
     
    whitespace.
  3. docs/manual/users/tools/post-review.txt (Diff revision 14)
     
     
    Still says "experimbental"
  4. reviewboard/scmtools/clearcase.py (Diff revision 14)
     
     
    whitespace.
  5. 
      
jan.koprowski
jan.koprowski
Review request changed
jan.koprowski
  1. If I didn't done something stupid it is ready to release for me. I developed it on 1.6beta1 branch but I hope it is possible to backport this stuff into ReviewBoard 1.5.x. This should be backward compatible - but I don't give my head for this. I hope fix will be fast :) I don't like keep my own version of ReviewBoard on branch.
  2. 
      
david
  1. Pushed to master (1.6-dev) as 7553ec1. Thanks!
  2.