Submitter:	Christian Hammond	Reviewers
Branch:	trunk	Groups:	reviewboard
Bugs:		People:
Change Number:	None	Repository:	Review Board SVN

Description:

This adds support for uploading parent diffs in the JSON API, and also introduces a --parent flag to post-review for doing parent diffs on supporting repository types (currently only git-svn). It should be pretty easy to add the support to the other types.

Testing Done:

Posted some standard diffs to make sure they didn't break. Then posted a diff with a parent diff. Saw that it uploaded as I expected.

Diff revision 2 (Latest)

Jump to revision:	1 2
Changes between r2 and:	1 2

Files Changed:

/trunk/reviewboard/contrib/tools/post-review: 69 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 ]
/trunk/reviewboard/webapi/json.py: 2 changes [ 1 2 ]

Expand changes

 ###
 # End user-settable variables.
 ###
-VERSION = "0.7"
+VERSION = "0.8"
 user_config = None
 tempfiles = []
 options = None
 class RepositoryInfo:
     """
     A representation of a source code repository.
     """
-    def __init__(self, path=None, base_path=None, supports_changesets=False):
+    def __init__(self, path=None, base_path=None, supports_changesets=False,
                  supports_parent_diffs=False):
         self.path = path
         self.base_path = base_path
         self.supports_changesets = supports_changesets
         self.supports_parent_diffs = supports_parent_diffs
     def __str__(self):
         return "Path: %s, Base path: %s, Supports changesets: %s" % \
             (self.path, self.base_path, self.supports_changesets)
         """
         self.api_post("api/json/reviewrequests/%s/draft/save/" %
                       review_request['id'])
         debug("Review request draft saved")
-    def upload_diff(self, review_request, diff_content):
+    def upload_diff(self, review_request, diff_content, parent_diff_content):
         """
         Uploads a diff to a Review Board server.
         """
-        debug("Uploading diff, size: %d" % (len(diff_content),))
+        debug("Uploading diff, size: %d" % len(diff_content))
         if parent_diff_content:
             debug("Uploading parent diff, size: %d" % len(parent_diff_content))
         fields = {}
         files = {}
         if self.info.base_path:
             fields['basedir'] = self.info.base_path
         files['path'] = {
             'filename': 'diff',
             'content': diff_content
+        }
         if parent_diff_content:
             files['parent_diff_path'] = {
                 'filename': 'parent_diff',
                 'content': parent_diff_content
+            }
         self.api_post('api/json/reviewrequests/%s/diff/new/' %
-                      review_request['id'], fields,
+                      review_request['id'], fields, files)
-                      {'path': {'filename': 'diff',
-                                'content': diff_content}})
     def publish(self, review_request):
         """
         Publishes a review request.
         """
                     return server_url
         return None
     def diff(self, args):
-        return None
+        """
         Returns the generated diff and optional parent diff for this
         repository.
         The returned tuple is (diff_string, parent_diff_string)
         """
         return (None, None)
     def diff_between_revisions(self, revision_range):
         """
         Returns the generated diff between revisions in the repository.
         """
         return None
     def add_options(self, parser):
         """
         Adds options to an OptionParser.
         return RepositoryInfo(path=repository_path)
     def diff(self, files):
         """
         Performs a diff across all modified files in a CVS repository.
         CVS repositories do not support branches of branches in a way that
         makes parent diffs possible, so we never return a parent diff
         (the second value in the tuple).
         """
-        return self.do_diff(' '.join(files))
+        return (self.do_diff(' '.join(files)), None)
     def diff_between_revisions(self, revision_range):
         """
         Performs a diff between 2 revisions of a CVS repository.
         """
         rev_str = ''
         for rev in revision_range.split(":"):
             rev_str += "-r %s" % rev
         return self.do_diff(rev_str)
     def do_diff(self, params):
         """
         Performs the actual diff operation through cvs diff, handling
         fake errors generated by CVS.
         """
         # Diff returns "1" if differences were found.
         return execute('cvs diff -u %s' % params,
-                       extra_ignore_errors=(1,))
+                        extra_ignore_errors=(1,))
         return get_url_prop(repository_info.path)
     def diff(self, files):
         """
         Performs a diff across all modified files in a Subversion repository.
         SVN repositories do not support branches of branches in a way that
         makes parent diffs possible, so we never return a parent diff
         (the second value in the tuple).
         """
-        return self.do_diff('svn diff --diff-cmd=diff %s' % ' '.join(files))
+        return (self.do_diff('svn diff --diff-cmd=diff %s' % ' '.join(files)),
                 None)
     def diff_between_revisions(self, revision_range):
         """
         Performs a diff between 2 revisions of a Subversion repository.
         """
         return self.do_diff('svn diff --diff-cmd=diff -r %s' % revision_range)
     def do_diff(self, cmd):
         """
         Performs the actual diff operation, handling renames and converting
         paths to absolute.
         """
         diff = execute(cmd, split_lines=True)
         diff = self.handle_renames(diff)
         diff = self.convert_to_absolute_paths(diff)
         return ''.join(diff)
             return parts
         # strip off ending newline, and return it as the second component
         return [s.split('\n')[0], '\n']
 class PerforceClient(SCMClient):
     """
     A wrapper around the p4 Perforce tool that fetches repository information
     and generates compatible diffs.
     """
         description = description[line_num+2:]
         cwd = os.getcwd()
         diff_lines = []
         empty_filename = make_tempfile()
         tmp_diff_from_filename = make_tempfile()
         tmp_diff_to_filename = make_tempfile()
         for line in description:
                 old_file = tmp_diff_from_filename
                 changetype_short = "D"
             else:
                 die("Unknown change type '%s' for %s" % (changetype, depot_path))
             diff_cmd = 'diff -urNp "%s" "%s"' % (old_file, new_file)
             # Diff returns "1" if differences were found.
             dl = execute(diff_cmd, extra_ignore_errors=(1,)).splitlines(True)
             if local_name.startswith(cwd):
             #     "Binary files "
             if len(dl) == 1 and \
                dl[0] == ('Files %s and %s differ'% (old_file, new_file)):
                 dl = ['Binary files %s and %s differ'% (old_file, new_file)]
             if dl == [] or dl[0].startswith("Binary files "):
                 if dl == []:
                     print "Warning: %s in your changeset is unmodified" % \
                         local_path
                 dl.insert(0, "==== %s#%s ==%s== %s ====\n" % \
                     (depot_path, base_revision, changetype_short, local_path))
             else:
                 m = re.search(r'(\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d)', dl[1])
                 if m:
                     timestamp = m.group(1)
                 else:
                     # Thu Sep  3 11:24:48 2007
                 dl[0] = "--- %s\t%s#%s\n" % (local_path, depot_path, base_revision)
                 dl[1] = "+++ %s\t%s\n" % (local_path, timestamp)
             diff_lines += dl
         os.unlink(empty_filename)
         os.unlink(tmp_diff_from_filename)
         os.unlink(tmp_diff_to_filename)
-        return ''.join(diff_lines)
+        return (''.join(diff_lines), None)
     def _write_file(self, depot_path, tmpfile):
         """
         Grabs a file from Perforce and writes it to a temp file. We do this
         wrather than telling p4 print to write it out in order to work around
         # We save data here to use it as a fallback. See below
         local_data = data.strip()
         # We are going to search .hg/hgrc for the default path.
         file_name = os.path.join(local_data,'.hg', 'hgrc')
-        if not  os.path.exists(file_name):
+        if not os.path.exists(file_name):
-            return RepositoryInfo(path=local_data, base_path='/')
+            return RepositoryInfo(path=local_data, base_path='/',
                                   supports_parent_diffs=True)
         f = open(file_name)
         data = f.read()
         f.close()
         m = re.search(r'^default\s+=\s+(.+)$', data, re.M)
         if not m:
             # Return the local path, if no default value is found.
-            return RepositoryInfo(path=local_data, base_path='/')
+            return RepositoryInfo(path=local_data, base_path='/',
                                   supports_parent_diffs=True)
         path = m.group(1).strip()
-        return RepositoryInfo(path=path, base_path='')
+        return RepositoryInfo(path=path, base_path='',
                               supports_parent_diffs=True)
     def diff(self, files):
         """
         Performs a diff across all modified files in a Mercurial repository.
         """
-        return execute('hg diff %s' % ' '.join(files))
+        # We don't support parent diffs with Mercurial yet, so return None
         # for the parent diff.
         return (execute('hg diff %s' % ' '.join(files)), None)
     def diff_between_revisions(self, revision_range):
         """
         Performs a diff between 2 revisions of a Mercurial repository.
         """
             m = re.search(r'^URL: (.+)$', data, re.M)
             if m:
                 base_path = m.group(1)[len(path):]
                 self.type = "svn"
-                return RepositoryInfo(path=path, base_path=base_path)
+                return RepositoryInfo(path=path, base_path=base_path,
                                       supports_parent_diffs=True)
         else:
             # Versions of git-svn before 1.5.4 don't (appear to) support
             # 'git svn info'.  If we fail because of an older git install,
             # here, figure out what version of git is installed and give
             # the user a hint about what to do next.
         return None
     def diff(self, args):
         """
-        Performs a diff across all modified files in the branch.
+        Performs a diff across all modified files in the branch, taking into
         account a parent branch.
         """
-        if len(args) == 0:
+        parent_branch = options.parent_branch or "master"
             branch_against = "master"
-        elif len(args) == 1:
+        diff_lines = self.make_diff(parent_branch)
             branch_against = args[0]
         if parent_branch != "master":
             parent_diff_lines = self.make_diff("master", parent_branch)
         else:
-            print "No more than one branch can be supplied."
+            parent_diff_lines = None
             sys.exit(1)
         return (diff_lines, parent_diff_lines)
-        diff_lines = execute("git diff --no-color --no-prefix -r -u %s.." %
+    def make_diff(self, parent_branch, source_branch=""):
-                             branch_against,
+        """
         Performs a diff on a particular branch range.
         """
         diff_lines = execute("git diff --no-color --no-prefix -r -u %s..%s" %
                              (parent_branch, source_branch),
                              split_lines=True)
         if self.type == "svn":
-            return self.make_svn_diff(branch_against, diff_lines)
+            return self.make_svn_diff(parent_branch, diff_lines)
         return None
-    def make_svn_diff(self, branch_against, diff_lines):
+    def make_svn_diff(self, parent_branch, diff_lines):
         """
         Formats the output of git diff such that it's in a form that
         svn diff would generate. This is needed so the SVNTool in Review
         Board can properly parse this diff.
         """
-        rev = execute("git-svn find-rev %s" % branch_against).strip()
+        rev = execute("git-svn find-rev master").strip()
         if not rev:
             return None
         diff_data = ""
 def debug(s):
     """
     Prints debugging information if post-review was run with --debug
     """
-    if options.debug:
+    if options and options.debug:
         print ">>> %s" % s
 def make_tempfile():
     """
 def execute(command, split_lines=False, ignore_errors=False,
             extra_ignore_errors=()):
     """
     Utility function to execute a command and return the output.
     """
     debug(command)
     if sys.platform.startswith('win'):
         p = subprocess.Popen(command,
                              stdin=subprocess.PIPE,
                              stdout=subprocess.PIPE,
                              stderr=subprocess.STDOUT,
             pass
     return config
-def tempt_fate(server, tool, changenum, diff_content=None, submit_as=None):
+def tempt_fate(server, tool, changenum, diff_content=None,
                parent_diff_content=None, submit_as=None):
     """
     Attempts to create a review request on a Review Board server and upload
     a diff. On success, the review request path is displayed.
     """
     try:
             server.save_draft(review_request)
     except APIError, e:
         rsp, = e.args
         if rsp['err']['code'] == 103: # Not logged in
             server.login()
-            tempt_fate(server, tool, changenum, diff_content, submit_as)
+            tempt_fate(server, tool, changenum, diff_content,
                        parent_diff_content, submit_as)
             return
         if options.rid:
             die("Error getting review request %s: %s (code %s)" % \
                 (options.rid, rsp['err']['msg'], rsp['err']['code']))
         else:
             die("Error creating review request: %s (code %s)" % \
                 (rsp['err']['msg'], rsp['err']['code']))
     if not server.info.supports_changesets or not options.change_only:
         try:
-            server.upload_diff(review_request, diff_content)
+            server.upload_diff(review_request, diff_content,
                                parent_diff_content)
         except APIError, e:
             rsp, = e.args
             print "Error uploading diff: %s (%s)" % (rsp['err']['msg'],
                                                      rsp['err']['code'])
             debug(rsp)
     parser = OptionParser(usage="%prog [-pond] [-r review_id] [changenum]",
                           version="%prog " + VERSION)
     parser.add_option("-p", "--publish",
                       dest="publish", action="store_true", default=PUBLISH,
-                      help="publish the review request immediately after " +
+                      help="publish the review request immediately after "
                            "submitting")
     parser.add_option("-r", "--review-request-id",
                       dest="rid", metavar="ID", default=None,
                       help="existing review request ID to update")
     parser.add_option("-o", "--open",
                       dest="open_browser", action="store_true",
                       default=OPEN_BROWSER,
                       help="open a web browser to the review request page")
     parser.add_option("-n", "--output-diff",
                       dest="output_diff_only", action="store_true",
                       default=False,
-                      help="outputs a diff to the console and exits. " +
+                      help="outputs a diff to the console and exits. "
                            "Does not post")
     parser.add_option("--server",
                       dest="server", default=REVIEWBOARD_URL,
                       metavar="SERVER",
-                      help="specify a different Review Board server " +
+                      help="specify a different Review Board server "
                            "to use")
     parser.add_option("--diff-only",
                       dest="diff_only", action="store_true", default=False,
-                      help="uploads a new diff, but does not update " +
+                      help="uploads a new diff, but does not update "
                            "info from changelist")
     parser.add_option("--target-groups",
                       dest="target_groups", default=TARGET_GROUPS,
-                      help="names of the groups who will perform " +
+                      help="names of the groups who will perform "
                            "the review")
     parser.add_option("--target-people",
                       dest="target_people", default=TARGET_PEOPLE,
-                      help="names of the people who will perform " +
+                      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("--description",
                       dest="description", default=None,
                       help="description of the review ")
     parser.add_option("--revision-range",
                       dest="revision_range", default=None,
-                      help="generate the diff for review based on given " +
+                      help="generate the diff for review based on given "
                            "revision range")
     parser.add_option("--submit-as",
                       dest="submit_as", default=SUBMIT_AS, metavar="USERNAME",
                       help="user name to be recorded as the author of the "
                            "review request, instead of the logged in user")
     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 " +
+                              help="updates info from changelist, but does "
                                    "not upload a new diff")
         if repository_info.supports_parent_diffs:
             parser.add_option("--parent",
                               dest="parent_branch", default=None,
                               metavar="PARENT_BRANCH",
                               help="the parent branch this diff should be "
                                    "against")
         tool.add_options(parser)
     parser.add_option("-d", "--debug",
                       action="store_true", dest="debug", default=DEBUG,
     else:
         changenum = None
     if options.revision_range:
         diff = tool.diff_between_revisions(options.revision_range)
         parent_diff = None
     else:
-        diff = tool.diff(args)
+        diff, parent_diff = tool.diff(args)
     if options.output_diff_only:
         print diff
         sys.exit(0)
     # Let's begin.
     server.login()
     review_url = tempt_fate(server, tool, changenum, diff_content=diff,
                             parent_diff_content=parent_diff,
                             submit_as=options.submit_as)
     # Load the review up in the browser if requested to:
     if options.open_browser:
         try:

     if not form.is_valid():
         return WebAPIResponseFormError(request, form)
     try:
-        diffset = form.create(request.FILES['path'])
+        diffset = form.create(request.FILES['path'],
                               request.FILES.get('parent_diff_path'))
         # Set the initial revision to be one newer than the most recent
         # public revision, so we can reference it in the diff viewer.
+        #
         # TODO: It would be nice to later consolidate this with the logic in

/trunk/reviewboard/contrib/tools/post-review: 69 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 ]
/trunk/reviewboard/webapi/json.py: 2 changes [ 1 2 ]

Review Board

beta

This change has been marked as submitted.

Add parent diff support to the JSON API and post-review

Diff revision 2 (Latest)

Other reviews

Your comment