Summary:

Support post-review in Clearcase snapshot view

Review Request #3822 — Created Feb. 6, 2013 and submitted Feb. 6, 2013, 6:14 a.m.

Information
Owner:	moonese
Repository:	RBTools
Branch:
Bugs:
Depends On:
Reviewers
Groups:	rbtools
People:

Description

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.

Testing Done

Tested on Clearcase 7.1.2 snapshot view, the diff could be uploaded just like dynamic view.

Issues

Description	From	Last Updated
too verbose, need improve here ...	MO moonese	Feb. 4, 2013, 10:10 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 4, 2013, 10:09 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 4, 2013, 10:10 a.m.
if diff is dir, which is the proper way to handle them?	MO moonese	Feb. 4, 2013, 10:10 a.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 57 E702 multiple statements on one line (semicolon)	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 57 E702 multiple statements on one line (semicolon)	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 4, 2013, 10:10 a.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	Feb. 4, 2013, 8:40 a.m.
This is kind of crummy (and suffers from the same security problems as mktemp). I'd suggest using tempfile.mkdtemp() to create …	david	Feb. 6, 2013, 2:56 a.m.
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: …	david	Feb. 6, 2013, 4:34 a.m.
Col: 18 E111 indentation is not a multiple of four	reviewbot	Feb. 6, 2013, 4:36 a.m.
it should be "tmp_old_file", not "temp_old_file"	PE penguinol	Feb. 26, 2013, 10:23 a.m.
it should be "tmp_new_file", not "temp_new_file"	PE penguinol	Feb. 26, 2013, 10:23 a.m.
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	Feb. 26, 2013, 10:23 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

rbtools/clients/clearcase.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
rbtools/clients/clearcase.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
rbtools/clients/clearcase.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (99 > 79 characters)
```

Description:

		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.

rbtools/clients/clearcase.py (Diff revision 1)
Show all issues
```
too verbose, need improve here ...
```

rbtools/clients/clearcase.py (Diff revision 1)

Show all issues

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

Change Summary:

1. change the way to generate temp file using tempfile.NamedTemporaryFile()
2. some other minor fix

Diff:

Revision 2 (+20 -8)

Show changes

rbtools/clients/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```

rbtools/clients/clearcase.py (Diff revision 2)

Show all issues

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

rbtools/clients/clearcase.py (Diff revision 2)

Show all issues

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

rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Change Summary:

fix error reported by Review Bot

Diff:

Revision 3 (+22 -8)

Show changes

rbtools/clients/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

rbtools/clients/clearcase.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```

Change Summary:

fix previous Review Bot Error:

E501 line too long (93 > 79 characters)

Diff:

Revision 4 (+22 -8)

Show changes

rbtools/clients/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

What testing did you do?

moonese Feb. 5, 2013, 3:34 a.m.

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.

Change Summary:

add description for Testing Done.

Testing Done:

Tested on Clearcase 7.1.2 snapshot view, the diff could be uploaded just like dynamic view.

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.

chipx86 Feb. 6, 2013, 1 a.m.

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.

moonese Feb. 6, 2013, 2:57 a.m.

OK, I just changed the way to generate temp files, plz take a review, Thanks.

Change Summary:

use rbtools.utils.filesystem.make_tempfile() to generate temporary files

Diff:

Revision 5 (+28 -8)

Show changes

rbtools/clients/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

```
Just one last comment from me:
```

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.

Change Summary:

per David's suggestion on more robust code

Diff:

Revision 6 (+32 -8)

Show changes

rbtools/clients/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

rbtools/clients/clearcase.py (Diff revision 6)

Show all issues

Col: 18
 E111 indentation is not a multiple of four

Change Summary:

fix indention problem,

Diff:

Revision 7 (+32 -8)

Show changes

rbtools/clients/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/clearcase.py
  Ignored Files:

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to rbtools master (16cf475). Thanks!

1. MO
  
  moonese Feb. 26, 2013, 10:24 a.m.
  Thanks for these review comments!
rbtools/clients/clearcase.py (Diff revision 7)
Show all issues
```
it should be "tmp_old_file", not "temp_old_file"
```
rbtools/clients/clearcase.py (Diff revision 7)
Show all issues
```
it should be "tmp_new_file", not "temp_new_file"
```

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