Adding support for difflib

Review Request #648 — Created Nov. 28, 2008 and discarded

Information

Review Board SVN (deprecated)

Reviewers

Removed the dependency on the "diff" command, and replaced with support for Python's difflib module.
This requires a bit of a hack to work around an issue in difflib with handling files that don't end in newlines: http://bugs.python.org/issue2142 (this was fixed in Python 2.6, but the fix doesn't seem to be there in 2.4 or 2.5).

Also added a fix for a minor issue in PerforceClient._depot_to_local() where the local path sometimes has a trailing newline character.
Used by a few people successfully for a couple days on Windows and Linux systems.
chipx86
  1. The general concept sounds good, but there's a few things I'd like to see and want to make sure of.
    
    1) Since we're now doing diffing ourselves, we can avoid creating those empty files. There's no point anymore, as we're just creating them, reading them in, and then deleting them. So we can just skip all that and pass an empty buffer to unified_diff().
    
    2) This is sort of minor right now, but we're losing the ability to list the C function in the file. Review Board isn't doing anything with this yet, but it's nice when actually pulling down the diff. Probably okay not to have it though, I suppose.
    
    3) We're doing our own diff but expecting data from GNU diff. Does unified_diff actually generate "Files ... and ... differ" or "Binary files ... and ... differ" ? If not, we need to find a solution for this.
    
    4) A lot of that post-processing should probably go away. We're replacing the date stamps with revisions, yet we're supplying those initial date stamps anyway. Seems we should just skip a lot of this and provide our own revision.
    
    5) Ideally this would be expanded to be used for other tools. Right now, we have tools (svn in particular) that calls out to GNU diff for the actual diffing. It would be nice if we could call out to post-review with a special argument that did all the diffing work so we wouldn't need an external diff tool. That may be a later thing though.
  2. trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
     
     
     
    These should be in the list alphabetically.
  3. trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
     
     
     
    These will leak. You need to call close on the file handle.
  4. This needs to wrap to ~75 characters.
  5. Should wrap to ~75 characters.
  6.