Summary

Support for directory changes in reviewboard diffviewer

Review Request #1519 — Created April 20, 2010 and submitted

Information

Owner

jan.koprowski

Repository

Review Board

Branch

Bugs

1626

Depends On

Reviewers

Groups

reviewboard

People

Description

This patch add support for ClearCase directory revisions. This is diff between "ls -1" command for each of them.

Testing Done

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

Where is filename variable defined? It is not a function arg, and it is not a class member?

jan.koprowski April 25, 2010, 2:33 p.m.
```
I should fix this today. Thanks.
```

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

This isn't necessary and will break if the user doesn't have an 'ls' command. Use os.walk instead.

jan.koprowski April 25, 2010, 2:36 p.m.

Dan. In the first version there was os.listdir() but it fail and generate malformed diffs. ls -1 work. If showing file use "cat" command why I shouldn't use ls? Even in Windows Cygwin is needed. I can't imagine working system without that command. In my opinion this is not good way. I don't have time to reimplement this one more time to using python functions from os module because last time this implementation failed. If os.walk fail I only just lose my time.

jan.koprowski April 26, 2010, 12:06 a.m.

Today I saw something strange. I get informations about malformed diff. Please wait I check the reason.

djs April 26, 2010, 3:16 a.m.

I don't have enough context to know what you're actually trying to fix, but I highly doubt using ls vs using python API is the problem here. 'ls' is not cross-platform and as an example, I often keep my system with cygwin explicitly not in my PATH by default because it causes other problems to do so. This means my windows box doesn't have 'ls'.

If you can give some more context on the problem you're trying to solve, maybe we can root-cause it.

jan.koprowski April 26, 2010, 4:57 a.m.

You convince me to use Python API. Appropriate version solved using Python API should be tomorrow. I already write the code but I want to do some more tests.
I don't know what was happen but today diff, worked in last week, stop working :/ The only change was that I change Django development server to Apache. I check that "ls -1" in mod_python give different results then "ls -1" in command line. Order was different. I guess this was cause to different LC_COLLATION or something from locale staff :/ But I'am not sure. However, when I was see that the problem is order I read information about os.listdir() carefully and I red that "order is arbitrary" :/ I put os.listdir inside sorted() and all start working like a charm. I want check is it work also on Apache server then I will publish a patch.

Change Summary:

Fix copy, paste mistake. Change filename to path :)

Diff:

Revision 2 (+25 -1)

Show changes

reviewboard/scmtools/clearcase.py

Change Summary:

Change method to version using listdir from Python os module.

Diff:

Revision 3 (+17 -1)

Show changes

reviewboard/scmtools/clearcase.py

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

We may as well simplify this and call adjust_path once, and pass the result to the proper function.

jan.koprowski May 15, 2010, 10:03 p.m.
```
ok :)
```
jan.koprowski June 2, 2010, 12:36 a.m.
```
There is only one adjust_path call now.
```

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

This confuses me. We're already getting the '\n's from the join. Why are we appending a '\n' as well? That'll lead to a double '\n'.

jan.koprowski May 15, 2010, 10:04 p.m.

This was my fault until I understand what means "new line at end of file" :) I will fix this soon.

jan.koprowski June 2, 2010, 12:37 a.m.

Now this end of line fixed. I need fix this on the post-review side too.

Diff:

Revision 4 (+17 -1)

Show changes

reviewboard/scmtools/clearcase.py

reviewboard/scmtools/clearcase.py (Diff revision 4)
```
Can do:

if '@@' not in path:
```
reviewboard/scmtools/clearcase.py (Diff revision 4)
```
Blank line between these.
```

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

Should remove the print statement. It can actually cause problems on some web server setups.

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

Is that what you really want? The last line won't have a trailing newline.

Change Summary:

Changed according tips.

Diff:

Revision 5 (+17 -1)

Show changes

reviewboard/scmtools/clearcase.py