[RBTools] Brand new Clear Case implementation

Review Request #1843 — Created Oct. 18, 2010 and submitted

jan.koprowski
RBTools
1375, 1516, 1553, 1588, 1599, 1644, 1727, 1926, 1930, 1931
rbtools
This is new, wrote from scratch Clear Case implementation which provide new functionality and fix old issues:
* recognize view type
* remove necessity of using xargs trick to send review
* pre-review all CHECKEDOUT files in view - just run "post-review" command
* post-review all changes made on branch passed as --tracking-branch
* post-review custom revision range passed as --revision-range
* remove useless --label option
* simple command "post-review" at least generate diff of CHECKEDOUT files
* fixed "\No new line at end of file" workaround
* diffs generate properly under windows
* no cygwin required
* Make use of diff instead broken unified_diff Python implementation where it is important
* Handle binary files
* Updated documentation
Linux:
* Tested

Windows:
* Need tests after changes
jan.koprowski
  1. Removing extraordinary whitespace at end of lines is obvious for me so :) don't waste time to this :)
  2. 
      
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
  1. 
      
  2. rbtools/postreview.py (Diff revision 2)
     
     
    I see this :) next time will be fixed :D
  3. 
      
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
  1. 
      
  2. rbtools/postreview.py (Diff revision 3)
     
     
     
     
    I'm confused. I can't understand role of path and base_path parameter. They cause that on ReviewBoard side i get relative or absolute path but I don't know how use it to get correct result.
  3. 
      
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
chipx86
  1. Hey Jan,
    
    Thanks for the change! There's a bunch of stylistic things to fix, and a few small logic things.
    
    Most of this review really boils down to both style and English grammar, which I'm happy to help with as I understand more about what the functions do.
  2. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    Doc strings should have the form of:
    
    """One line summary.
    
    Any remaining documentation.
    """
  3. rbtools/postreview.py (Diff revision 6)
     
     
     
    Should be:
    
    """Returns information on the Clear Case repository.
    
    This will first check if the cleartool command is installed and in the path, and post-review was run from inside of the view.
    """
    
    (Word-wrap that so nothing goes over 80 columns. Same with any other examples I give.)
  4. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    No blank line between the docstrings and the code.
  5. rbtools/postreview.py (Diff revision 6)
     
     
    "your", not "Your".
  6. rbtools/postreview.py (Diff revision 6)
     
     
    What happens if you're in the source tree for a non-Clear Case repository? Will this die still? It doesn't look safe.
    1. Very good point. I did not think about this.
      The same issue is probably with "cleartool help" command. I will remove it.
  7. rbtools/postreview.py (Diff revision 6)
     
     
    "Get the properties of the view"
  8. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    This looks kind of odd to me. Is this meant to be one big statement? I'd prefer the result of execute() be computed first, and then passed to the filter.
    
    Actually, filter shouldn't really be used anymore. Instead, do:
    
    property_lines = execute(...)
    properties_line = [
        line for line in property_lines
        if line.startswith("Properties: ")
    ]
  9. rbtools/postreview.py (Diff revision 6)
     
     
    Oh, if we only ever use the first line, why not just use a for loop for the above, and then when we find that line, we do the calculations and break?
  10. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    # Determine the view type and check if it's supported.
    #
    # Specifically check if webview was listed in properties
    # because webview types also list the 'snapshot'
    # entry in properties.
  11. rbtools/postreview.py (Diff revision 6)
     
     
     
    "Webviews are not supported." instead of "We don't support webviews."
  12. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line before the next comment.
  13. rbtools/postreview.py (Diff revision 6)
     
     
    Not sure this comment adds any value. The code is self-explanatory.
  14. rbtools/postreview.py (Diff revision 6)
     
     
     
    "To generate a diff using a parent branch or by passing revision ranges, you must use a dynamic view."
  15. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    Should follow the docstring pattern mentioned above.
  16. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    I don't understand this at all. Why are we using infinity?
    1. In "_sanitize_*_changeset" functions we search highest version number comparing numbers. ClearCase versions is: 0,1,2,...1000 ... and the highest is CHECKEDOUT. But 1000 > "CHECKEDOUT" means nothing to python. So we convert CHECKDOUT to infinity because this is the highest possible version number in view.
  17. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  18. rbtools/postreview.py (Diff revision 6)
     
     
    No comma after version.
  19. rbtools/postreview.py (Diff revision 6)
     
     
     
    """Returns a sanitized version the diff.
    
    This will ensure that the diff either ends with a newline or has the appropriate "No newline at end of file" marker.
    """
  20. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
     
     
     
    This will actually be more efficient if you do:
    
    fixed_diff = list(diff)
    
    if not fixed_diff[-1].endswith('\n'):
        fixed_diff.append('\n\ No newline at end of file\n'
    
    return ''.join(fixed_diff)
    1. But doesn't work for many cases e.g. like this:
      
      
      --- a   2010-12-31 14:29:21.866601998 +0100
      +++ b   2010-12-31 14:27:49.976608625 +0100
      @@ -1 +1,3 @@
      -a
      \ No newline at end of file
      +a
      +b
      +c
    2. But doesn't work for many cases e.g. like this:
      
      
      --- a   2010-12-31 14:29:21.866601998 +0100
      +++ b   2010-12-31 14:27:49.976608625 +0100
      @@ -1 +1,3 @@
      -a
      \ No newline at end of file
      +a
      +b
      +c
      
      If i remember well I also met cases where \ No new line at end of file shows more then one time. I believe like this:
      
      >>> gen = unified_diff("one\ntwo\nthree".splitlines(1),
      ...                    "one\ntwo\ntrois".splitlines(1))
      >>> print ''.join(gen)
      ---
      +++
      @@ -1,3 +1,3 @@
       one
       two
      -three
      \ No newline at end of file
      +trois
      \ No newline at end of file
      
      
      I *believe* that "\ No newline at end of file" are the appropriate
      markers -- that tools like "patch" will know how to use. At least this
      is what "svn diff" generates.
      
      Because many details are described under http://bugs.python.org/issue2142
  21. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    Combine to one line.
  22. rbtools/postreview.py (Diff revision 6)
     
     
    I don't really understand what this means?
    1. This is hard to explain. I will change description to something more understandable.
  23. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  24. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    No blank line here.
    
    Also, 'type' is a reserved word. Should use a different variable here.
    1. I didn't know. I will change it to "kind". This is word from ClearCase documentation describing this variable.
  25. rbtools/postreview.py (Diff revision 6)
     
     
    debug("Skipping compressed file %s." % path)
  26. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  27. rbtools/postreview.py (Diff revision 6)
     
     
    if path not in changelist:
  28. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    Should combine this:
    
    changelist[path] = {
        'highest': ...
        ...
    }
  29. rbtools/postreview.py (Diff revision 6)
     
     
    No parens.
  30. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    No trailing comas.
    
    The indentation is messed up. Params should always align. Maybe:
    
    changeranges.append(self._construct_extended_path(
        path, version['previous']))
  31. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    One line.
  32. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
     
     
    Same about the blank lines, 'type' variable, and debug output.
  33. rbtools/postreview.py (Diff revision 6)
     
     
     
    No trailing comma.
    
    Make sure these fit in 80 columns.
    1. 78 :) and 77 :] But it was close :D
  34. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    One line.
  35. rbtools/postreview.py (Diff revision 6)
     
     
    "Return the content of the object in the specified path."
  36. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    Use 'f' instead of 'file'. 'file' is a reserved keyword.
  37. rbtools/postreview.py (Diff revision 6)
     
     
    You can check for this in order to have a more specific error message by doing:
    
    elif os.access(extended_path, os.R_OK):
    
    Also, %s instead of %r, and no need to wrap extended_path in parens.
    1. I think about changing this to "debug" message because one file can break whole review.
  38. rbtools/postreview.py (Diff revision 6)
     
     
     
    """Return information about the checked out changeset."""
    
    And then maybe something about what type of data is returned.
  39. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    No blank line.
  40. rbtools/postreview.py (Diff revision 6)
     
     
     
    "We ignore return code 1 in order to omit files that Clear Case can't read."
  41. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    All parameters should be aligned, and only one parameter per line.
    1. I didn't know how to split this correctly so just gave me some advice If I made this wrong.
  42. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  43. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  44. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
     
    The wrapping is sort of strange. Make sure doc strings are wrapped to just under 80 columns.
    
    Please change to:
    
    """Returns information about the versions changed on a branch.
    
    This takes into account the changes on the branch owned by the current user in all vobs of the current view.
    """
  45. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    No blank line after the docstring, but there should be one between 'changeset = []' and the comments.
  46. rbtools/postreview.py (Diff revision 6)
     
     
     
    Should be changed to what I mentioned above.
  47. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    Is this so the environment variable will be calculated in the command line? If so, can we just get it ourselves using os.getenv and then pass that into the command line?
  48. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    Align all parameters, and only one parameter per line.
    
    Also, for readability, each parameter to cleartool should also be on its own line.
  49. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  50. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
     
    This is duplicated in two places. Probably should be pulled out into a common function.
  51. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    One line.
    1. I will change separator to "," because ":" is part of oid and uuid so in future this will cause problems if we want "oid" and "uuid" in revision range.
    2. I will change separator to "," because ":" is part of oid and uuid so in future this will cause problems if we want "oid" and "uuid" in revision range.
      But maybe better choice is ';'.
    3. I will change separator to "," because ":" is part of oid and uuid so in future this will cause problems if we want "oid" and "uuid" in revision range.
      But maybe better choice is ';'?
  52. rbtools/postreview.py (Diff revision 6)
     
     
    No need to define this, since it'll be defined to something regardless.
  53. rbtools/postreview.py (Diff revision 6)
     
     
     
    Blank line between these.
  54. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    Add a period after the sentence.
    
    Also, should be one line.
  55. rbtools/postreview.py (Diff revision 6)
     
     
    "Performs a diff between the passed revisions from the Clear Case repository."
  56. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    No blank line here.
    1. I'm thinking about changing separator because ':' is part of oid/uuid. I'm not sure which is the best: ',', ';' or ' '?
    2. 
       
    3. I will change ":" to ";" because ":" is part of Windows path (i.e. drive C:, D: etc...)
  57. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    Add a period. And should be one line.
  58. rbtools/postreview.py (Diff revision 6)
     
     
    "Generates a unified diff for all files in the changeset."
  59. rbtools/postreview.py (Diff revision 6)
     
     
     
     
    No blank line here.
  60. rbtools/postreview.py (Diff revision 6)
     
     
     
     
     
    This really should be:
    
    for previous, current in changeset:
    
    And then when building this, put previous and current together in a tuple, instead of their own entries.
  61. rbtools/postreview.py (Diff revision 6)
     
     
     
    These can be combined into one statement.
  62. 
      
jan.koprowski
jan.koprowski
  1. Hi Christina :)
    
    Thank You for Your points and ideas :] Some of them just change my way of thinking about Python. I fix the code as You said. I hope this will be the last time before You accept this patch.
    I made fixes for Windows :) so there is some changes (which I should also include in documentation). I checked is this patch works also under Windows and it looks like on both systems patches are generating fine.
    
    I thought I will write implementation using oid/uuid ids but I don't have time for that now. This is something for next year :) but I don't know I will do it. There is a little hope there is someone new to this.
    I have removed TODOs from code and raise new tickets on issue list :) I believe this is a good way to remember what will be nice to have. IMHO this implementation is fair enough to be useful :) I hope I get right :)
    I'm looking forward Your review :)
    
    Happy New Year! and See You in 2011 :)
  2. 
      
jan.koprowski
jan.koprowski
jan.koprowski
jan.koprowski
david
  1. Most of this looks pretty good. Is this at a state where you want it committed?
  2. rbtools/postreview.py (Diff revision 9)
     
     
    Can you align this with the beginning of the list?
  3. rbtools/postreview.py (Diff revision 9)
     
     
    Trailing whitespace.
  4. rbtools/postreview.py (Diff revision 9)
     
     
     
    Indent the second line 1 more space.
  5. rbtools/postreview.py (Diff revision 9)
     
     
    Trailing whitespace.
  6. rbtools/postreview.py (Diff revision 9)
     
     
    Trailing whitespace.
  7. rbtools/postreview.py (Diff revision 9)
     
     
     
     
     
    Why the move?
  8. 
      
jan.koprowski
jan.koprowski
jan.koprowski
david
  1. 
      
  2. rbtools/postreview.py (Diff revision 11)
     
     
     
     
     
    Imports should be alphabetical, so undo this change.
  3. rbtools/postreview.py (Diff revision 11)
     
     
     
     
    Two blank lines between classes; undo this change.
  4. rbtools/postreview.py (Diff revision 11)
     
     
    Delete this blank line.
  5. rbtools/postreview.py (Diff revision 11)
     
     
     
    Align "-cview" with "cleartool"
  6. rbtools/postreview.py (Diff revision 11)
     
     
     
    I think it'll actually be more efficient to swap these:
    
    properties = line.split(' ')
    if properties[0] == 'Properties:':
  7. 
      
jan.koprowski
david
  1. Can you figure out which of our open clearcase bugs are fixed by this change?
  2. rbtools/postreview.py (Diff revision 12)
     
     
     
     
    Two blank lines between classes.
  3. rbtools/postreview.py (Diff revision 12)
     
     
     
     
    whitespace.
  4. rbtools/postreview.py (Diff revision 12)
     
     
    whitespace.
  5. 
      
jan.koprowski
jan.koprowski
  1. If I didn't done something stupid it is ready to release for me. 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. 
      
jan.koprowski
Review request changed

Change Summary:

Fix some more documentation changes and reviewers idea.

Diff:

Revision 13 (+372 -288)

Show changes

david
  1. Pushed to master as 47fb2ce. Thanks!
  2. 
      
Loading...