trunk/reviewboard/scmtools/hg.py (Diff revision 1)
48	info['newFile'] = info['origFile'] = diffLine[-1]	48	isCommitted = len(diffLine) > 4 and diffLine[3] == '-r'

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

1. Derek Slager 3 months, 4 weeks ago (July 23rd, 2008, 6:50 p.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.

2. Christian Hammond 3 months, 4 weeks ago (July 24th, 2008, 12:18 a.m.)

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

trunk/reviewboard/scmtools/hg.py (Diff revision 1)
		49	nameStartIndex = 5 if isCommitted else 3

We usually prefer these to be more verbose.

What you could so is:

isCommitted = ...

if isCommitted:
   nameStartIndex = 5
   info[...]
else:
   nameStartIndex = 3
   info[...]

1. Derek Slager 3 months, 4 weeks ago (July 23rd, 2008, 6:47 p.m.)

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

2. Christian Hammond 3 months, 4 weeks ago (July 24th, 2008, 12:18 a.m.)

   The function.

trunk/reviewboard/scmtools/hg.py (Diff revision 1)
93	f = opener.open('%s/raw-file/%s/%s' % (self.url, rev, path))	96	f = opener.open('%s/raw-file/%s/%s' % (self.url, rev, path.replace(' ', '%20')))

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

1. Derek Slager 3 months, 4 weeks ago (July 23rd, 2008, 6:46 p.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 :)

Committed as r1442.  Thanks!!