Review Board

beta

Create a mechanism to fetch SVN repository info via JSON

Updated 9 months ago

Michael Plump Reviewers
trunk reviewboard
None Review Board SVN
I'm writing a plugin for IntelliJ IDEA (a Java IDE) that will publish code reviews to Review Board (http://code.google.com/p/idea-reviewboard/).  Currently our company has a similar plugin for Code Striker, so getting this working is the last thing we need before everyone will migrate to Review Board.

What I'd like to be able to do is have the plugin inspect the local checkout and then automatically determine the correct repository and base diff path.  This is complicated by the fact that Subversion checkouts might not use the same repository URL as the server.  For example, one person might be using "http://svn/repos/" while another uses "http://svn.scm/repos" while another has an NFS mount and uses "file:///mnt/svn/repos".  The only sure way to identify same repositories is by the repository UUID.  Similarly, to determine the correct base diff path, I need to know the repository root (in case Review Board is configured with "http://svn/repos/trunk" or something).

So this is some code that lets me get that information from Review Board via JSON.  I'm not sure if this is the best way to go about it, but it serves my purposes.  It's perhaps a little weird to throw a method in there that only works for Subversion repositories.  Maybe you can come up with something better?  But I'd love it if this could get checked in.  Otherwise I'll have to tell people to apply a patch to their server if they want to use my plugin.

Let me know what you think.  Thanks!
I've used this to submit reviews.  It works for me.
Posted 10 months, 4 weeks ago (February 12th, 2008, 1:58 p.m.)
I'm curious how you make use of this in your IDE. Got any sample code (pseudo-code would be fine).

Also, would you mind updating this for the new API changes that went in? It's basically s/JSON/WebAPI/g and s/json/webapi/g/
  1. Michael Plump 10 months, 2 weeks ago (February 21st, 2008, 12:27 p.m.)
    Okay, I updated the diff so that it works with trunk svn.
    
    I took the (ugly, static-typed) Java and kinda converted it into prettier pseudo-Python (my Python is sadly kinda rusty, so it might not be perfect):
    
    # The second argument is a list of all the repositories on the ReviewBoard 
    # server.
    def findRepository( project, repositories ):
        vcs = project.vcs
        localInfo = vcs.getInfo( project.baseDir )
        localUuid = localInfo.repositoryUUID
        # This gives you the directory of the repository you've checked out.  So if
        # the repository is "http://svn/repos" and you checked out
        # "http://svn/repos/trunk/reviewboard", this will return "trunk/reviewboard"
        localRelative = getRelativePath( localInfo.URL, 
                                         localInfo.repositoryRootURL )
    
        client = ReviewBoardPlugin.getClient( project )
    
        svnRepositories = filterSvnRepositories( repositories )
    
        for repository in svnRepositories:
            try:
                info = client.getRepositoryInfo( repository.id )
                remoteUuid = info.repositoryUUID
                if localUuid != remoteUuid:
                    continue;
    
                remoteRelative = getRelativePath( info.URL, info.repositoryRootURL )
    
                # Now, if the remote relative path is "trunk" and we have
                # "trunk/reviewboard", this will return "reviewboard".  This is the
                # path we need to hand off to reviewboard as the base diff path.
                relative = getRelativePath( localRelative, remoteRelative )
    
                # This is the same repository, but they're different paths.  The
                # local checkout might be "trunk/reviewboard" and the one on the
                # server might be "branches/2.0".
                if not relative:
                    continue;
    
                return ( repository, relative );
            except Exception:
                pass
    
        return None;
    
Posted 10 months ago (March 9th, 2008, 1:33 p.m.)

   

  
Loading diff fragment...
This constant name doesn't really match up with its meaning.  Perhaps REPO_NOT_IMPLEMENTED ?
  1. Michael Plump 9 months, 4 weeks ago (March 12th, 2008, 9:58 a.m.)
    Yeah, good point.  I'll fix that in a new patch once I understand what you mean on the below comment.
Loading diff fragment...
This should probably return a generic error instead of just dropping the request.
  1. Michael Plump 9 months, 4 weeks ago (March 12th, 2008, 9:57 a.m.)
    I'm sorry; I don't understand what you mean here.  Do you think there should be more info in the JSON error besides just the error type?
  2. David Trowbridge 9 months, 3 weeks ago (March 14th, 2008, 10:31 p.m.)
    I meant in cases of other exceptions.
  3. Christian Hammond 9 months, 1 week ago (April 1st, 2008, 2:33 a.m.)
    I want to see this get in, so to check with what David meant, did you want something like:
    
    try:
        return WebAPIResponse(...)
    except NotImplementedError:
        return WebAPIResponseError(request, INVALID_REPO_TYPE)
    except Exception, e:
        return WebAPIResponseError(request, something else?)
  4. David Trowbridge 9 months, 1 week ago (April 1st, 2008, 9:35 a.m.)
    Yes.
Posted 9 months, 1 week ago (April 4th, 2008, 1:33 a.m.)

   

  
Loading diff fragment...
Maybe this should just catch SCMError?  I dunno.  Is this more along the lines of what you were thinking?
  1. David Trowbridge 9 months ago (April 4th, 2008, 11:10 a.m.)
    Yes, this is what I meant.  In future, you can just do "except:".  I'll give this one more look-through, and then probably commit it.  Thanks!
Ship it!
Posted 9 months ago (April 4th, 2008, 12:22 p.m.)
Committed as r1263.  Thanks!