Summary

[ReviewBoard] Brand new Clear Case implementation

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

Information

Owner

jan.koprowski

Repository

Review Board

Branch

Bugs

1375, 1516, 1553, 1588, 1599, 1644, 1727, 1926, 1930, 1931

Depends On

Reviewers

Groups

reviewboard

People

Description

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

Testing Done

Linux:
* Tested

Windows:
* Need tests after changes

You seem to have two of these reviews. Is this one valid? Can you discard it if not?

jan.koprowski Oct. 27, 2010, 12:35 a.m.

Both is valid. One concer Review Board second RBTools. I can add prefix to both to be clear.

Change Summary:

Add prefix to title to be clear.

Summary:

-	Brand new Clear Case implementation
+	[ReviewBoard] Brand new Clear Case implementation

Change Summary:

Following sanitize of ClearCase ReviewBoard side implementation.

Description:

		Fixed:
		* Removed necessary adjust_path function - paths should be well formed from outside
~		* Speed up reading files using Python internal open file instead "cat" command
	~	* 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

		TODO:
~		* Try to understand whole implementation and make simple as possible
	~	* Understand purpose of get_filenames_in_revision() function
	+	* Think about check_repository() function to monit appropriate configspec

Diff:

Revision 2 (+53 -94)

Show changes

reviewboard/scmtools/clearcase.py

Change Summary:

Remove regular expression typo.

Diff:

Revision 3 (+53 -94)

Show changes

reviewboard/scmtools/clearcase.py

reviewboard/scmtools/clearcase.py (Diff revisions 1 - 3)

Very good line. Very good line indeed!

pawel_i Oct. 27, 2010, 1:18 a.m.

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!

Change Summary:

Display normalized path instead ClearCase extended_path

Description:

		Fixed:
		* Removed necessary 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
	~	* Name of tool changed to ClearCase
	+	* DiffViewer print normalized_path insted ClearCase extended_path

		TODO:
		* Understand purpose of get_filenames_in_revision() function
		* Think about check_repository() function to monit appropriate configspec

Diff:

Revision 4 (+57 -95)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

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".

Change Summary:

Fix typo in description

Description:

		Fixed:
~		* Removed necessary adjust_path function - paths should be well formed from outside
	~	* 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

		TODO:
		* Understand purpose of get_filenames_in_revision() function
		* Think about check_repository() function to monit appropriate configspec

Change Summary:

Fix regular expression to handle many crazy paths.

Diff:

Revision 5 (+71 -95)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

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.

Change Summary:

* Sanitize syntax of list comprehension
* Remove doubled realpath call and join it with os.path.join

Diff:

Revision 6 (+74 -95)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

Change Summary:

Remove debug print function

Diff:

Revision 7 (+73 -95)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

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.

reviewboard/scmtools/clearcase.py (Diff revision 7)

Imports should be in alphabetical order.

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.

reviewboard/scmtools/clearcase.py (Diff revision 7)

Two lines between imports and class.

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 """

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.

reviewboard/scmtools/clearcase.py (Diff revision 7)
```
period after "list"
```

reviewboard/scmtools/clearcase.py (Diff revision 7)

Same comment about a one-line summary.

reviewboard/scmtools/clearcase.py (Diff revision 7)

Condense this to one statement.

return os.path.relpath(
    ...
)

reviewboard/scmtools/clearcase.py (Diff revision 7)

Collapse to one line, with """ on both ends.

reviewboard/scmtools/clearcase.py (Diff revision 7)

Is there never an equivalent to PRE_CREATION? How are new files marked up?

jan.koprowski Oct. 28, 2010, 4:55 a.m.

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.

reviewboard/scmtools/clearcase.py (Diff revision 7)

Use 'f' instead of 'file', since file is meant to be reserved in Python.

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?

jan.koprowski Oct. 28, 2010, 4:44 a.m.

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.

I will be glad to review also this part: http://reviews.reviewboard.org/r/1843/

Diff:

Revision 8 (+69 -100)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

Description:

		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

		TODO:
~		* Understand purpose of get_filenames_in_revision() function
	~	* Make it works also under Windows
	+	* OID/UUID support
		* Think about check_repository() function to monit appropriate configspec

Change Summary:

Using cpath instead os.path

Description:

		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
~
	~	* DiffViewer print normalized_path insted ClearCase extended_path
	~	* Work also under Windows
-		TODO:
-		* Make it works also under Windows
-		* OID/UUID support
-		* Think about check_repository() function to monit appropriate configspec

Diff:

Revision 9 (+80 -100)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

Change Summary:

Fix regular expression to clear ClearCase paths more aggressive.
This method can loose some parts of path.

Diff:

Revision 10 (+80 -100)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

Change Summary:

Get rid of @@ from unextended_path

Diff:

Revision 11 (+81 -100)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

```
????
```

```
????
```

This looks pretty great. Is this change at the point where you want it committed?

reviewboard/scmtools/clearcase.py (Diff revision 11)
```
Trailing whitespace.
```

Change Summary:

Please review this implementation. After final fixes I will update documentation and it will be ready to release.

Description:

		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
	~	* Work also under Windows
	+	* Recognize binary files
	+	* Make use of HEAD
	+	* Return appropriate files as PRE-CREATION
	+	* Read OIDs and translate to working paths

Diff:

Revision 12 (+157 -120)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

Change Summary:

Update Windows testing status

Testing Done:

	+	Linux:
	+	* Tested
	+
	+	Windows:
	+	* Need tests after changes

Change Summary:

Remove trailing whitespaces

Diff:

Revision 13 (+170 -145)

Show changes

	docs/manual/admin/management/repositories.txt
	docs/manual/users/tools/post-review.txt
	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py

```
Just a few (mostly cosmetic) changes.
```
docs/manual/users/tools/post-review.txt (Diff revision 13)
```
You introduced a typo in this line.
```

reviewboard/scmtools/clearcase.py (Diff revision 13)

Switch these two (we try to keep these alphabetized).

reviewboard/scmtools/clearcase.py (Diff revision 13)

This looks like it should fit on a single line.

reviewboard/scmtools/clearcase.py (Diff revision 13)
```
Get rid of this line.
```

reviewboard/scmtools/clearcase.py (Diff revision 13)

This is wrapped too much. Wrap at 80 columns.

reviewboard/scmtools/clearcase.py (Diff revision 13)

Can you make the docstring summary fit on a single line?

reviewboard/scmtools/clearcase.py (Diff revision 13)

Can you put "cmdline" on the next line, so that all the argument line up?

Change Summary:

Fix multiplatform issues for ClearCase implementation.

Diff:

Revision 14 (+241 -145)

Show changes

	docs/manual/admin/management/repositories.txt
	docs/manual/users/tools/post-review.txt
	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/models.py
	reviewboard/templates/diffviewer/changeindex_entry.html

Can you figure out which of our open clearcase bugs are fixed by this change?

docs/manual/users/tools/post-review.txt (Diff revision 14)
```
whitespace.
```
docs/manual/users/tools/post-review.txt (Diff revision 14)
```
Still says "experimbental"
```
reviewboard/scmtools/clearcase.py (Diff revision 14)
```
whitespace.
```

Change Summary:

Update Bugs to close after release.

Bugs:

+	1644, 1553, 1727, 1588, 1931, 1930, 1375, 1926, 1516, 1599

Change Summary:

Fix some documentation changes and reviewers ideas.

Diff:

Revision 15 (+240 -143)

Show changes

	docs/manual/admin/management/repositories.txt
	docs/manual/users/tools/post-review.txt
	reviewboard/diffviewer/diffutils.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/models.py
	reviewboard/templates/diffviewer/changeindex_entry.html

Ship it!

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.

Ship it!

Pushed to master (1.6-dev) as 7553ec1. Thanks!