Support post-review in Clearcase snapshot view

Review Request #3822 — Created Feb. 5, 2013 and submitted

Information

RBTools

Reviewers

Currrently run RBTools in snapshot view will get error message saying "To generate diff using parent branch or by passing revision ranges, you must use a dynamic view.

So I made some change to support snapshot view post-review, as it will be useful for clearcase users working on snapshot views.

As new to Python, I think the ugly code I changed could be improved. Anyway, it works.

The idea is to use 'cleartool get -to dest-pname pname' command to copy the revision to temp dir and then run diff on temp old new files.
Tested on Clearcase 7.1.2 snapshot view, the diff could be uploaded just like dynamic view.
Description From Last Updated

too verbose, need improve here ...

MO moonese

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

if diff is dir, which is the proper way to handle them?

MO moonese

Col: 80 E501 line too long (99 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 57 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 57 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (93 > 79 characters)

reviewbotreviewbot

This is kind of crummy (and suffers from the same security problems as mktemp). I'd suggest using tempfile.mkdtemp() to create …

daviddavid

I think it would be better to wrap these in try/except: try: os.remove(temp_old_file) except OSError: pass try: os.remove(temp_new_file) except OSError: …

daviddavid

Col: 18 E111 indentation is not a multiple of four

reviewbotreviewbot

it should be "tmp_old_file", not "temp_old_file"

PE penguinol

it should be "tmp_new_file", not "temp_new_file"

PE penguinol

File "clearcase.py", line 282, in diff_files AttributeError: 'str' object has no attribute 'name' use "tmp_old_file" instead of "tmp_old_file.name" the same …

PE penguinol
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  4. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (99 > 79 characters)
    
  5. 
      
MO
MO
  1. 
      
  2. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues
    too verbose, need improve here ...
  3. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues
    if diff is dir, which is the proper way to handle them?
  4. 
      
MO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  3. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  4. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
  5. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Col: 57
     E702 multiple statements on one line (semicolon)
    
  6. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Col: 57
     E702 multiple statements on one line (semicolon)
    
  7. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. 
      
MO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. rbtools/clients/clearcase.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
  3. 
      
MO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. 
      
david
  1. What testing did you do?
    1. During my test, if I change some files on snapshot view, diff could be uploaded and view diff works fine just like dynamic view, no problem found.
  2. 
      
MO
david
  1. 
      
  2. rbtools/clients/clearcase.py (Diff revision 4)
     
     
     
     
     
    Show all issues
    This is kind of crummy (and suffers from the same security problems as mktemp). I'd suggest using tempfile.mkdtemp() to create a temporary directory, and then put your temporary files in there. Make sure to remove that directory at the end.
    1. We actually have rbtools.utils.filesystem.make_tempfile, which you should be using. Those will get tracked by RBTools, and on exit, all temp files will be cleaned up.
    2. OK, I just changed the way to generate temp files, plz take a review, Thanks.
  3. 
      
MO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. 
      
david
  1. Just one last comment from me:
  2. rbtools/clients/clearcase.py (Diff revision 5)
     
     
     
     
     
    Show all issues
    I think it would be better to wrap these in try/except:
    
    try:
        os.remove(temp_old_file)
    except OSError:
        pass
    
    try:
        os.remove(temp_new_file)
    except OSError:
        pass
    
    That way it's a little more robust against races.
  3. 
      
MO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. rbtools/clients/clearcase.py (Diff revision 6)
     
     
    Show all issues
    Col: 18
     E111 indentation is not a multiple of four
    
  3. 
      
MO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/clearcase.py
      Ignored Files:
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
MO
Review request changed
Status:
Completed
Change Summary:
Pushed to rbtools master (16cf475). Thanks!
PE
  1. 
      
    1. Thanks for these review comments!
  2. rbtools/clients/clearcase.py (Diff revision 7)
     
     
    Show all issues
    it should be "tmp_old_file", not "temp_old_file"
  3. rbtools/clients/clearcase.py (Diff revision 7)
     
     
    Show all issues
    it should be "tmp_new_file", not "temp_new_file"
  4. rbtools/clients/clearcase.py (Diff revision 7)
     
     
    Show all issues
    File "clearcase.py", line 282, in diff_files
    AttributeError: 'str' object has no attribute 'name'
    
    use "tmp_old_file" instead of "tmp_old_file.name"
    
    the same problem @ line 282,283,284,293,294 
  5.