This is a good first step, but it'll end up breaking older installs.

What we'll need is to conditionally do this based on server capabilities.

I'd suggest a new singleton API resource, /api/capabilities/, containing a payload that looks like:

{
    'capabilities': {
        'diffs': {
            'moved-files': true
        }
    }
}

Then post-review can check for the capability. I'd suggest a Capabilities class in rbtools/api/capabilities.py, with a has_capability(category, name) function. If /api/capabilities/ is 404, then a cap defaults to false. Otherwise, try to find it in the the payload (which would be fetched only once).

It's a little more work, but really helps us long-term and makes sure your moved files work won't break on older versions of Review Board.

Summary should be on the same line. Since this all fits on one line, just put the text right up against the """.

Probably makes more sense to say "Retrieves and stores"

Should specifically catch a KeyError.

Too much repeat code. I'd suggest something like:

cmdline = [self.git, "diff", .....]

if has_capability(...):
    cmdline.append('-M')

return execute(cmdline)

I'd prefer we set the server instead. There may be other useful objects later.

Given that, we'd probably want to rename get_capabilities() to load_capabilities(), and put that call after check_api_version(). Then things can just access server.capabilities.

Should just call this capabilities, or caps, or something.

I'd imagine this could fit on one line.

if not, perhaps rename capability_list to capabilities or caps.

Beyond what Christian has pointed out, this looks good to me.

This could just be return execute(cmdline).

i.e:
if (self.server and
    self.server.capabilities.has_capability('diffs', 'moved_files')):	
    cmdline.append('-M')

return execute(cmdline)

We always pass a value to this, right? Probably no need for a default.

Blank line above this.

See below on this, but I don't think capabilities should be a default parameter. I think it should always take a value. In that case, just pass caps as a standard argument, not keyword argument. Makes this line read better too :)

Note: this is related to (the more recently filed) http://code.google.com/p/reviewboard/issues/detail?id=2824; someone with permissions (i.e. Dave) may want to add that to the review details.

Ship It!