Submitter:	Christian Hammond	Reviewers
Branch:	trunk	Groups:	reviewboard
Bugs:	423, 543, 582, 594	People:
Change Number:	None	Repository:	Review Board SVN

Description:

We were relying on some really bad assumptions in post-review. First, as of r1415, we were assuming all parameters to diff should be quoted, which broke post-review with Perforce when it expected a value that could be cast to an integer. We now represent all commands as lists so that we can use subprocess's support for auto-escaping/quoting of a command and parameters. This should remove the hacks we use and prevent future problems.

Second, we were assuming that we could call "env" to set an environment variable for Subversion. This of course doesn't work on Windows. We should have instead used subprocess.Popen's native support for setting the environment.

Testing Done:

Couldn't test other SCMTools due to not having a setup here, but I used this code to post this review request.

Diff revision 1 (Latest)

Files Changed:

/trunk/reviewboard/contrib/tools/post-review: 40 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 ]

Expand changes

         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)), None)
+        return (self.do_diff(files), None)
     def diff_between_revisions(self, revision_range):
         """
         Performs a diff between 2 revisions of a CVS repository.
         """
-        rev_str = ''
+        revs = []
         for rev in revision_range.split(":"):
-            rev_str += "-r %s" % rev
+            revs += ["-r", rev]
-        return self.do_diff(rev_str)
+        return self.do_diff(revs)
     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 -uN %s' % params,
+        return execute(["cvs", "diff", "-uN"] + params,
                         extra_ignore_errors=(1,))
 class SVNClient(SCMClient):
     """
     A wrapper around the svn Subversion tool that fetches repository
     information and generates compatible diffs.
     """
     def get_repository_info(self):
         if not check_install('svn help'):
             return None
-        data = execute('env LANG=en_US.UTF-8 svn info', ignore_errors=True)
+        data = execute(["svn", "info"],
                        env={'LANG': 'en_US.UTF-8'},
                        ignore_errors=True)
         m = re.search(r'^Repository Root: (.+)$', data, re.M)
         if not m:
             return None
         path = m.group(1)
         return self.scan_for_server_property(repository_info)
     def scan_for_server_property(self, repository_info):
         def get_url_prop(path):
-            url = execute("env LANG=en_US.UTF-8 svn propget reviewboard:url %s" % path).strip()
+            url = execute(["svn", "propget", "reviewboard:url", path],
                           env={'LANG': 'en_US.UTF-8'}).strip()
             return url or None
         for path in walk_parents(os.getcwd()):
             if not os.path.exists(os.path.join(path, ".svn")):
                 break
         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('env LANG=en_US.UTF-8 svn diff --diff-cmd=diff %s' % ' '.join(files)),
+        return (self.do_diff(["svn", "diff", "--diff-cmd=diff"] + files),
                 None)
     def diff_between_revisions(self, revision_range):
         """
         Performs a diff between 2 revisions of a Subversion repository.
         """
-        return self.do_diff('env LANG=en_US.UTF-8 svn diff --diff-cmd=diff -r %s' % revision_range)
+        return self.do_diff(["svn", "diff", "--diff-cmd=diff", "-r",
                              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 = execute(cmd, env={'LANG': 'en_US.UTF-8'}, split_lines=True)
         diff = self.handle_renames(diff)
         diff = self.convert_to_absolute_paths(diff)
         return ''.join(diff)
         return result
     def svn_info(self, path):
         """Return a dict which is the result of 'svn info' at a given path."""
         svninfo = {}
-        for info in execute("env LANG=en_US.UTF-8 svn info '%s'" % path).splitlines():
+        for info in execute(["svn", "info", path],
                             env={'LANG': 'en_US.UTF-8'},
                             split_lines=True):
             parts = info.split(": ", 1)
             if len(parts) == 2:
                 key, value = parts
                 svninfo[key] = value
     """
     def get_repository_info(self):
         if not check_install('p4 help'):
             return None
-        data = execute('p4 info', ignore_errors=True)
+        data = execute(["p4", "info"], ignore_errors=True)
         m = re.search(r'^Server address: (.+)$', data, re.M)
         if not m:
             return None
         except ValueError:
             die("You must enter a valid change number")
         debug("Generating diff for changenum %s" % changenum)
-        description = execute('p4 describe -s %d' % changenum).splitlines()
+        description = execute(["p4", "describe", "-s", str(changenum)],
                               split_lines=True)
         if '*pending*' in description[0]:
             cl_is_pending = True
         # Get the file list
         for line_num, line in enumerate(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_cmd = ["diff", "-urNp", old_file, new_file]
             # Diff returns "1" if differences were found.
             dl = execute(diff_cmd, extra_ignore_errors=(1,2)).splitlines(True)
             if local_name.startswith(cwd):
                 local_path = local_name[len(cwd) + 1:]
         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
         a permissions bug on Windows.
         """
         debug('Writing "%s" to "%s"' % (depot_path, tmpfile))
-        data = execute('p4 print -q "%s"' % depot_path)
+        data = execute(["p4", "print", "-q", depot_path])
         f = open(tmpfile, "w")
         f.write(data)
         f.close()
     def _depot_to_local(self, depot_path):
         """
         Given a path in the depot return the path on the local filesystem to
         the same file.
         """
         # $ p4 where //user/bvanzant/main/testing
         # //user/bvanzant/main/testing //bvanzant:test05:home/testing /home/bvanzant/home-versioned/testing
-        cmd = 'p4 where "%s"' % (depot_path,)
+        where_output = execute(["p4", "where", depot_path], split_lines=True)
-        where_output = execute(cmd).splitlines()
         # Take only the last line from the where command.  If you have a
         # multi-line view mapping with exclusions, Perforce will display
         # the exclusions in order, with the last line showing the actual
         # location.
         last_line = where_output[-1]
     """
     def get_repository_info(self):
         if not check_install('hg --help'):
             return None
-        data = execute('hg root', ignore_errors=True)
+        data = execute(["hg", "root"], ignore_errors=True)
         if data.startswith('abort:'):
             # hg aborted => no mercurial repository here.
             return None
         # Elsewhere, hg root output give us the repository path.
         """
         Performs a diff across all modified files in a Mercurial repository.
         """
         # We don't support parent diffs with Mercurial yet, so return None
         # for the parent diff.
-        return (execute('hg diff %s' % ' '.join(files)), None)
+        return (execute(["hg", "diff"] + files), None)
     def diff_between_revisions(self, revision_range):
         """
         Performs a diff between 2 revisions of a Mercurial repository.
         """
         r1, r2 = revision_range.split(':')
-        return execute('hg diff -r %s -r %s' % (r1, r2))
+        return execute(["hg", "diff", "-r", r1, "-r", r2])
 class GitClient(SCMClient):
     """
     A wrapper around git that fetches repository information and generates
     compatible diffs. This will attempt to generate a diff suitable for the
     remote repository, whether git, SVN or Perforce.
     """
     def get_repository_info(self):
         if not check_install('git --help'):
             return None
-        git_dir = execute('git rev-parse --git-dir', ignore_errors=True).strip()
+        git_dir = execute(["git", "rev-parse", "--git-dir"],
                           ignore_errors=True).strip()
         if git_dir.startswith("fatal:") or not os.path.isdir(git_dir):
             return None
         # We know we have something we can work with. Let's find out
         # what it is. We'll try SVN first.
-        data = execute("git svn info", ignore_errors=True)
+        data = execute(["git", "svn", "info"], ignore_errors=True)
         m = re.search(r'^Repository Root: (.+)$', data, re.M)
         if m:
             path = m.group(1)
             m = re.search(r'^URL: (.+)$', data, re.M)
         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.
-            version = execute("git svn --version", ignore_errors=True)
+            version = execute(["git", "svn", "--version"], ignore_errors=True)
             version_parts = re.search('version (\d+)\.(\d+)\.(\d+)',
                                       version)
             if (version_parts and
                 not self.is_valid_version((int(version_parts.group(1)),
                                            int(version_parts.group(2)),
     def scan_for_server(self, repository_info):
         # Scan first for dot files, since it's faster and will cover the
         # user's $HOME/.reviewboardrc
         # TODO: Maybe support a server per remote later? Is that useful?
-        url = execute("git config --get reviewboard.url").strip()
+        url = execute(["git", "config", "--get", "reviewboard.url"]).strip()
         if url:
             return url
         if self.type == "svn":
             # Try using the reviewboard:url property on the SVN repo, if it
     def make_diff(self, parent_branch, source_branch=""):
         """
         Performs a diff on a particular branch range.
         """
-        diff_lines = execute("git diff --no-color --no-prefix -r -u %s..%s" %
+        diff_lines = execute(["git", "diff", "--no-color", "--no-prefix",
-                             (parent_branch, source_branch),
+                              "-r", "-u", "%s..%s" % (parent_branch,
                                                       source_branch)],
                              split_lines=True)
         if self.type == "svn":
             return self.make_svn_diff(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 master").strip()
+        rev = execute(["git-svn", "find-rev", "master"]).strip()
         if not rev:
             return None
         diff_data = ""
         return True
     except OSError:
         return False
-def execute(command, split_lines=False, ignore_errors=False,
+def execute(command, env=None, split_lines=False, ignore_errors=False,
             extra_ignore_errors=()):
     """
     Utility function to execute a command and return the output.
     """
     # Internally, subprocess.Popen calls this anyway, but since we're
     # showing it on the command line, we may as well do it ourselves.
     if isinstance(command, list):
         command = subprocess.list2cmdline(command)
     debug(command)
     if sys.platform.startswith('win'):
         p = subprocess.Popen(command,
                              stdin=subprocess.PIPE,
                              stdout=subprocess.PIPE,
                              stderr=subprocess.STDOUT,
                              shell=True,
-                             universal_newlines=True)
+                             universal_newlines=True,
                              env=env)
     else:
         p = subprocess.Popen(command,
                              stdin=subprocess.PIPE,
                              stdout=subprocess.PIPE,
                              stderr=subprocess.STDOUT,
                              shell=True,
                              close_fds=True,
-                             universal_newlines=True)
+                             universal_newlines=True,
                              env=env)
     if split_lines:
         data = p.stdout.readlines()
     else:
         data = p.stdout.read()
     rc = p.wait()
     if options.revision_range:
         diff = tool.diff_between_revisions(options.revision_range)
         parent_diff = None
     else:
-        args = ["'%s'" % arg for arg in args] # quote filenames
         diff, parent_diff = tool.diff(args)
     if options.output_diff_only:
         print diff
         sys.exit(0)

/trunk/reviewboard/contrib/tools/post-review: 40 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 ]

Review Board

beta

This change has been marked as submitted.

Properly escape execute commands and handle environment variables cross-platform

Diff revision 1 (Latest)

Other reviews

Your comment