Patch: Enable support for paths with spaces in Mercurial (hg) diff parser

Review Request #451 — Created July 16, 2008 and submitted

Information

Review Board SVN (deprecated)
541

Reviewers

Enables the Mercurial diff parser to handle file paths with spaces.

I don't know Python, and was therefore "coding by Google", so please expect to find some issues.

Update: diff #2 comes with proposed changes generously applied by bboissin (via http://code.google.com/p/reviewboard/issues/detail?id=541)
Tested locally on Debian with various changesets that do and do not include paths with spaces.
chipx86
  1. 
      
  2. trunk/reviewboard/scmtools/hg.py (Diff revision 1)
     
     
    Are we guaranteed that diffLine[3] will be "-r" when it's uncommitted?
    1. If it's uncommitted, you will get one of:
      
      1. diff -r abcdefghi1234 path/file.name
      2. diff -r abcdefghi1234 path/file name with some spaces.name
      
      If it's committed, you will get:
      
      3. diff -r abcdefghi1234 -r ihgfedcba4321 path/file.name
      4. diff -r abcdefghi1234 -r ihgfedcba4321 path/file name with some spaces.name
      
      If you read carefully you will find that it is broken when the file name is "-r". "hg add" won't let me add a file with that name, though, so I think it works. There might be some other way to sneak one in, but I don't expect that it's a major concern.
    2. Okay. Can you add some comments with those examples to show that above the code?
  3. trunk/reviewboard/scmtools/hg.py (Diff revision 1)
     
     
    We usually prefer these to be more verbose.
    
    What you could so is:
    
    isCommitted = ...
    
    if isCommitted:
       nameStartIndex = 5
       info[...]
    else:
       nameStartIndex = 3
       info[...]
    1. Gotcha ... just out of curiosity, does nameStartIndex get scoped to the "try" body or the function?
    2. The function.
  4. trunk/reviewboard/scmtools/hg.py (Diff revision 1)
     
     
    You should probably use urllib.quote() for escaping contents.
    1. Nice ... urlencode is too heavy in this case, didn't realize there was something in between. It even defaults to allowing '/' ... it's almost like this has come up before :)
  5. 
      
david
  1. Committed as r1442.  Thanks!!
  2. 
      
Loading...