Capabilities with better moved files support

Review Request #2918 — Created Feb. 26, 2012 and submitted

Information

RBTools
master

Reviewers

Added post-review client capability checking and git moved-files support.
Tested the following cases:
1. Posted a review with up-to-date RB and rbtools. Moved files were tracked.
2. Posted a review where the RB server has moved_files=False. Moved files were not tracked.
3. Posted a review where the RB server has no "compatabilities". Moved files were not tracked.
4. Posted a review where the RB server has no "moved_files" capability (and again with no "diffs" category). Moved files were not tracked.

Unit tests pass, don't really know how to write tests for this since it requires a server (mock objects?).
Description From Last Updated

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

chipx86chipx86

Should specifically catch a KeyError.

chipx86chipx86

Too much repeat code. I'd suggest something like: cmdline = [self.git, "diff", .....] if has_capability(...): cmdline.append('-M') return execute(cmdline)

chipx86chipx86

I'd prefer we set the server instead. There may be other useful objects later. Given that, we'd probably want to …

chipx86chipx86

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

chipx86chipx86

I'd imagine this could fit on one line. if not, perhaps rename capability_list to capabilities or caps.

chipx86chipx86

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)

ME medanat

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

chipx86chipx86

Blank line above this.

chipx86chipx86

See below on this, but I don't think capabilities should be a default parameter. I think it should always take …

chipx86chipx86
chipx86
  1. 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.
  2. 
      
DD
chipx86
  1. 
      
  2. rbtools/api/capabilities.py (Diff revision 2)
     
     
     
     
    Show all issues
    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"
  3. rbtools/api/capabilities.py (Diff revision 2)
     
     
    Show all issues
    Should specifically catch a KeyError.
  4. rbtools/clients/git.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues
    Too much repeat code. I'd suggest something like:
    
    cmdline = [self.git, "diff", .....]
    
    if has_capability(...):
        cmdline.append('-M')
    
    return execute(cmdline)
  5. rbtools/postreview.py (Diff revision 2)
     
     
     
    Show all issues
    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.
  6. 
      
DD
DD
  1. 
      
  2. 
      
chipx86
  1. 
      
  2. rbtools/postreview.py (Diff revision 3)
     
     
    Show all issues
    Should just call this capabilities, or caps, or something.
  3. rbtools/postreview.py (Diff revision 3)
     
     
     
    Show all issues
    I'd imagine this could fit on one line.
    
    if not, perhaps rename capability_list to capabilities or caps.
  4. 
      
mike_conley
  1. Beyond what Christian has pointed out, this looks good to me.
  2. 
      
DD
ME
  1. 
      
  2. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
    Show all issues
    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)
  3. 
      
DD
chipx86
  1. 
      
  2. rbtools/api/capabilities.py (Diff revision 5)
     
     
    Show all issues
    We always pass a value to this, right? Probably no need for a default.
  3. rbtools/clients/git.py (Diff revision 5)
     
     
    Show all issues
    Blank line above this.
  4. rbtools/postreview.py (Diff revision 5)
     
     
    Show all issues
    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 :)
  5. 
      
DD
DD
MW
  1. 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.
  2. 
      
chipx86
  1. Ship It!
  2. 
      
DD
Review request changed
Status:
Completed
Change Summary:
Pushed to master (bacdab01f33c75911d03a49ff0115d9d99287a82)