Support for directory changes in reviewboard diffviewer

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

Information

Review Board

Reviewers

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

 
CK
  1. 
      
  2. 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? 
    1. I should fix this today. Thanks.
  3. 
      
DJ
  1. 
      
  2. 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.
    1. 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.
    2. Today I saw something strange. I get informations about malformed diff. Please wait I check the reason.
    3. 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.
    4. 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.
  3. 
      
jan.koprowski
jan.koprowski
chipx86
  1. 
      
  2. 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.
  3. 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'.
    1. This was my fault until I understand what means "new line at end of file" :) I will fix this soon.
    2. Now this end of line fixed. I need fix this on the post-review side too.
  4. 
      
jan.koprowski
chipx86
  1. 
      
  2. reviewboard/scmtools/clearcase.py (Diff revision 4)
     
     
    Can do:
    
    if '@@' not in path:
  3. reviewboard/scmtools/clearcase.py (Diff revision 4)
     
     
     
    Blank line between these.
  4. reviewboard/scmtools/clearcase.py (Diff revision 4)
     
     
    Should remove the print statement. It can actually cause problems on some web server setups.
  5. reviewboard/scmtools/clearcase.py (Diff revision 4)
     
     
    Is that what you really want? The last line won't have a trailing newline.
  6. 
      
jan.koprowski
Review request changed
Change Summary:
Changed according tips.