Index: contrib/tools/post-review =================================================================== --- contrib/tools/post-review (revision 1137) +++ contrib/tools/post-review (working copy) @@ -15,7 +15,7 @@ from tempfile import mkstemp from urlparse import urljoin -VERSION = "0.6" +VERSION = "0.7" # Who stole the cookies from the cookie jar? # Was it you? @@ -29,15 +29,42 @@ cj = cookielib.MozillaCookieJar() cookiefile = os.path.join(homepath, ".post-review-cookies.txt") -opener = urllib2.build_opener(urllib2.HTTPCookieProcessor(cj)) +opener = urllib2.OpenerDirector() +# default handlers, minus redirecthandler (hack to not hang when +# a django view returns a redirect). DON'T use build_opener or it +# will add redirecthandler back to the list. +for handler in [urllib2.ProxyHandler(), urllib2.UnknownHandler(), + urllib2.HTTPHandler(), urllib2.HTTPDefaultErrorHandler(), + urllib2.HTTPErrorProcessor(), + urllib2.HTTPCookieProcessor(cj)]: + opener.add_handler(handler) opener.addheaders = [('User-agent', 'post-review/' + VERSION)] urllib2.install_opener(opener) -user_config = None -tempfiles = [] -options = None +class attrdict(dict): + def __getattr__(self, name): + return self[name] + def __setattr__(self, name, value): + self[name] = value +tempfiles = [] +options = attrdict() +options.TREES = {} +# defaults -- don't give these as defaults to optparse or +# 1. we can't tell if they were really present on the cmdline or not +# 2. it's more difficult to import post_update from outside this script +options.files = [] +options.publish = False +options.open_browser = False +options.output_diff_only = False +options.diff_only = False +options.change_only = False +options.debug = False +mutable_request_fields = ['target_groups', 'target_people', 'summary', 'description', 'branch', 'bugs_closed', 'testing_done'] +for option in ['submit_as', 'server', 'changenum', 'rid', 'repository'] + mutable_request_fields: + options[option] = None + class APIError(Exception): pass @@ -95,6 +122,8 @@ try: debug("Attempting to create review request for %s" % changenum) data = { 'repository_path': self.info.path } + if options.submit_as: + data['submit_as'] = options.submit_as if changenum: data['changenum'] = changenum @@ -150,7 +179,7 @@ debug("Uploading diff") fields = {} - if self.info.base_path: + if self.info.base_path is not None: fields['basedir'] = self.info.base_path rsp = self.api_post('/api/json/reviewrequests/%s/diff/new/' % @@ -172,6 +201,7 @@ APIError is raised. """ rsp = simplejson.loads(data) + debug('response is %s' % rsp) if rsp['stat'] == 'fail': raise APIError, rsp @@ -290,86 +320,57 @@ A base representation of an SCM tool for fetching repository information and generating diffs. """ - def get_repository_info(self): - return None + def __init__(self): + self.repository_info = None - def scan_for_server(self, repository_info): + def scan_for_server(self): """ Scans the current directory on up to find a .reviewboard file containing the server path. """ - server_url = self._get_server_from_config(user_config, repository_info) - if server_url: - return server_url - - for path in walk_parents(os.getcwd()): - filename = os.path.join(path, ".reviewboardrc") - if os.path.exists(filename): - config = load_config_file(filename) - server_url = self._get_server_from_config(config, - repository_info) - if server_url: - return server_url - - return None - - def diff(self, changenum, files): - return None - - def diff_between_revisions(self, revision_range): - return None - - def add_options(self, parser): - """ - Adds options to an OptionParser. - """ - pass - - def _get_server_from_config(self, config, repository_info): - if 'REVIEWBOARD_URL' in config: - return config['REVIEWBOARD_URL'] - elif 'TREES' in config: - trees = config['TREES'] + if 'TREES' in options: + trees = options['TREES'] if not isinstance(trees, dict): die("Warning: 'TREES' in %s is not a dict!" % filename) - if repository_info.path in trees and \ - 'REVIEWBOARD_URL' in trees[repository_info.path]: - return trees[repository_info.path]['REVIEWBOARD_URL'] + if self.repository_info.path in trees and \ + 'REVIEWBOARD_URL' in trees[self.repository_info.path]: + return trees[self.repository_info.path]['REVIEWBOARD_URL'] return None + def diff(self, changenum, files): + raise NotImplementedError() class SVNClient(SCMClient): """ A wrapper around the svn Subversion tool that fetches repository information and generates compatible diffs. """ - def get_repository_info(self): + def __init__(self): + SCMClient.__init__(self) data = execute('svn info') m = re.search(r'^Repository Root: (.+)$', data, re.M) - if not m: - return None + self.cwd_is_wc = m is not None - path = m.group(1) - - m = re.search(r'^URL: (.+)$', data, re.M) - if not m: - return None - - base_path = m.group(1)[len(path):] - - return RepositoryInfo(path=path, base_path=base_path) - - def scan_for_server(self, repository_info): + if options.repository: + self.repository_info = RepositoryInfo(options.repository, '') + else: + path = m.group(1) + m = re.search(r'^URL: (.+)$', data, re.M) + if m: + base_path = m.group(1)[len(path):] + self.repository_info = RepositoryInfo(path=path, base_path=base_path) + + def scan_for_server(self): def get_url_prop(path): url = execute("svn propget reviewboard:url %s" % path).strip() return url or None # Scan first for dot files, since it's faster and will cover the # user's $HOME/.reviewboardrc - server_url = super(SVNClient, self).scan_for_server(repository_info) + server_url = SCMClient.scan_for_server(self) if server_url: return server_url @@ -381,35 +382,36 @@ if prop: return prop - return get_url_prop(repository_info.path) + return get_url_prop(self.repository_info.path) def diff(self, changenum, files): - """ - Performs a diff across all modified files in a Subversion repository. - """ - return execute('svn diff %s' % ' '.join(files)) + if files: + if not self.cwd_is_wc: + die('Cannot diff files; cwd is not a working copy') + return execute('svn diff %s' % ' '.join(files)) + # changenum + if options.repository: + return execute('svn diff -c %s %s' % (changenum, self.repository_info.path)) + else: + if not self.cwd_is_wc: + die('cwd is not a working copy and repository not specified') + return execute('svn diff -c %s' % changenum) - def diff_between_revisions(self, revision_range): - """ - Performs a diff between 2 revisions of a Subversion repository. - """ - return execute('svn diff -r %s' % revision_range) - class PerforceClient(SCMClient): """ A wrapper around the p4 Perforce tool that fetches repository information and generates compatible diffs. """ - def get_repository_info(self): - data = execute('p4 info') + def __init__(self): + SCMClient.__init__(self) + data = execute('p4 info') m = re.search(r'^Server address: (.+)$', data, re.M) if not m: return None repository_path = m.group(1) - try: hostname, port = repository_path.split(":") info = socket.gethostbyaddr(hostname) @@ -419,13 +421,14 @@ return RepositoryInfo(path=repository_path, supports_changesets=True) - def diff(self, changenum, files): """ Goes through the hard work of generating a diff on Perforce in order to take into account adds/deletes and to provide the necessary revision information. """ + if files: + die("p4 post-upload does not support diffing by local filename (use local changenum instead)") debug("Generating diff for changenum %s" % changenum) data = execute('p4 change -o %s' % changenum) client = None @@ -583,6 +586,7 @@ """ Utility function to execute a command and return the output. """ + debug(command) f = os.popen(command, 'r') if split_lines: @@ -609,7 +613,7 @@ if msg: print msg - sys.exit(1) + sys.exit(0) def walk_parents(path): @@ -625,19 +629,11 @@ """ Loads data from a config file. """ - config = { - 'TREES': {}, - } - if os.path.exists(filename): - try: - execfile(filename, config) - except: - pass + # TODO execfile pollutes options with crap we don't want like __builtins__ + execfile(filename, options) - return config - def tempt_fate(server, tool, changenum, files=None, diff_content=None): """ Attempts to create a review request on a Review Board server and upload @@ -652,21 +648,12 @@ else: review_request = server.new_review_request(changenum) - if options.target_groups: - server.set_review_request_field(review_request, 'target_groups', - options.target_groups) - save_draft = True + for field in mutable_request_fields: + if options[field]: + server.set_review_request_field(review_request, field, + getattr(options, field)) + save_draft = True - if options.target_people: - server.set_review_request_field(review_request, 'target_people', - options.target_people) - save_draft = True - - if options.summary: - server.set_review_request_field(review_request, 'summary', - options.summary) - save_draft = True - if save_draft: server.save_draft(review_request) except APIError, e: @@ -685,14 +672,15 @@ if not server.info.supports_changesets or not options.change_only: - try: - server.upload_diff(review_request, diff_content) - except APIError, e: - rsp, = e.args - print "Error uploading diff: %s (%s)" % (rsp['err']['msg'], - rsp['err']['code']) - die("Your review request still exists, but the diff is not " + - "attached.") + if diff_content.strip(): + try: + server.upload_diff(review_request, diff_content) + except APIError, e: + rsp, = e.args + print "Error uploading diff: %s (%s)" % (rsp['err']['msg'], + rsp['err']['code']) + die("Your review request still exists, but the diff is not " + + "attached.") if options.publish: server.publish(review_request) @@ -702,27 +690,24 @@ if not review_url.startswith('http'): review_url = 'http://%s' % review_url - print 'Review request posted.' - print - print review_url + print 'Review request posted.\n%s' % review_url return review_url -def parse_options(tool, repository_info, args): - parser = OptionParser(usage="%prog [-p] [-o] changenum", +def parse_options(args): + parser = OptionParser(usage="%prog [locally modified files to diff]", version="%prog " + VERSION) parser.add_option("-p", "--publish", - action="store_true", dest="publish", default=False, + action="store_true", dest="publish", help="publish the review request immediately after " + "submitting") parser.add_option("-o", "--open", - action="store_true", dest="open_browser", default=False, + action="store_true", dest="open_browser", help="open a web browser to the review request page") parser.add_option("--output-diff", action="store_true", dest="output_diff_only", - default=False, help="outputs a diff to the console and exits. " + "Does not post") parser.add_option("--server", dest="server", @@ -730,97 +715,66 @@ help="specify a different Review Board server " + "to use") parser.add_option("--diff-only", dest="diff_only", - action="store_true", default=False, + action="store_true", help="uploads a new diff, but does not update " + "info from changelist") - parser.add_option("--target-groups", dest="target_groups", - default=None, - help="names of the groups who will perform " + - "the review") - parser.add_option("--target-people", dest="target_people", - default=None, - help="names of the people who will perform " + - "the review") - parser.add_option("--summary", dest="summary", - default=None, - help="summary of the review ") - parser.add_option("--revision-range", dest="revision_range", - default=None, - help="generate the diff for review based on given " + - "revision range") + parser.add_option("-c", "--revision", dest="changenum", + help="generate review for given revision") parser.add_option("-r", "--review-request-id", dest="rid", metavar="ID", - default=None, help="existing review request ID to update") + parser.add_option("--submit-as", dest="submit_as", + help="submit request for someone other than the user you login as") + parser.add_option("--repository", dest="repository", + help="repository root (SVN only)") - if repository_info: - if repository_info.supports_changesets: - parser.add_option("--change-only", dest="change_only", - action="store_true", default=False, - help="updates info from changelist, but does " + - "not upload a new diff") + for field in mutable_request_fields: + parser.add_option('--' + field.replace('_', '-'), dest=field, + help="names of the groups who will perform " + + "the review") - tool.add_options(parser) + parser.add_option("--change-only", dest="change_only", + action="store_true", + help="updates info from changelist, but does " + + "not upload a new diff (Perforce only)") - parser.add_option("-d", "--debug", action="store_true", - dest="debug", default=False, help="display debug output") + dest="debug", help="display debug output") - (globals()["options"], args) = parser.parse_args(args) - + values, options['files'] = parser.parse_args(args) + options.update(values.__dict__) + return args -def main(args): - # Load the config file - globals()['user_config'] = \ - load_config_file(os.path.join(homepath, ".reviewboardrc")) - - # Try to find the SCM Client we're going to be working with - repository_info = None - tool = None - +def post_review(newoptions={}): + options.update(newoptions) + for tool in (SVNClient(), PerforceClient()): - repository_info = tool.get_repository_info() - - if repository_info: + if tool.repository_info: break - args = parse_options(tool, repository_info, args) - - if not repository_info: + if not tool.repository_info.path: print "The current directory does not contain a checkout from a" print "supported source code repository." sys.exit(1) - debug("Repository info '%s'" % repository_info) + debug("Repository info '%s'" % tool.repository_info) # Try to find a valid Review Board server to use. if options.server: server_url = options.server else: - server_url = tool.scan_for_server(repository_info) + server_url = tool.scan_for_server() if not server_url: print "Unable to find a Review Board server for this source code tree." sys.exit(1) - server = ReviewBoardServer(server_url, repository_info) + server = ReviewBoardServer(server_url, tool.repository_info) + if options.files and options.changenum: + die("cannot diff based on both filenames and change number") + diff = tool.diff(options.changenum, options.files) - if repository_info.supports_changesets: - if len(args) != 1: - parser.error("specify the change number of a pending changeset") - - changenum = args[0] - files = None - else: - changenum = None - files = args - - if options.revision_range: - diff = tool.diff_between_revisions(options.revision_range) - else: - diff = tool.diff(changenum, files) - if options.output_diff_only: print diff sys.exit(0) @@ -831,7 +785,7 @@ except IOError: server.login() - review_url = tempt_fate(server, tool, changenum, files, diff_content=diff) + review_url = tempt_fate(server, tool, options.changenum, options.files, diff_content=diff) # Load the review up in the browser if requested to: if options.open_browser == True: @@ -849,4 +803,15 @@ if __name__ == "__main__": - main(sys.argv[1:]) + # Load the main config file + load_config_file(os.path.join(homepath, ".reviewboardrc")) + # Load any other config files in the path, highest first + # (this lets sub-config files override parent ones. does anyone + # actually use that feature?) + for path in reversed(list(walk_parents(os.getcwd()))): + filename = os.path.join(path, ".reviewboardrc") + load_config_file(filename) + + # Try to find the SCM Client we're going to be working with + parse_options(sys.argv[1:]) + post_review()