Summary

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

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

Information

Owner

derek

Repository

Review Board SVN (deprecated)

Branch

Bugs

541

Depends On

Reviewers

Groups

reviewboard

People

Description

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)

Testing Done

Tested locally on Debian with various changesets that do and do not include paths with spaces.

trunk/reviewboard/scmtools/hg.py (Diff revision 1)

Are we guaranteed that diffLine[3] will be "-r" when it's uncommitted?

derek July 23, 2008, 11:50 a.m.

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.

chipx86 July 23, 2008, 5:18 p.m.

Okay. Can you add some comments with those examples to show that above the code?

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[...]

derek July 23, 2008, 11:47 a.m.

Gotcha ... just out of curiosity, does nameStartIndex get scoped to the "try" body or the function?

chipx86 July 23, 2008, 5:18 p.m.
```
The function.
```

trunk/reviewboard/scmtools/hg.py (Diff revision 1)

You should probably use urllib.quote() for escaping contents.

derek July 23, 2008, 11:46 a.m.

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 :)

Ship it!

```
Committed as r1442.  Thanks!!
```