Refactoring: Extract low-level git from push and merge scripts.

- This adds a python layer on top of the low-level git calls to control parameter passing and return values.
- This also fixes a bug in FindLastTrunkPush that only shows up in manual mode when the last push versions are iterated.
- The order of some parameters changed in some git calls in the tests to be uniform.

BUG=
R=ulan@chromium.org

Review URL: https://codereview.chromium.org/171423013

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19515 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

tools/push-to-trunk/auto_roll.py

@@ -93,8 +93,7 @@ class FetchLatestRevision(Step):
   MESSAGE = "Fetching latest V8 revision."
 
   def RunStep(self):
-    log = self.Git("svn log -1 --oneline").strip()
-    match = re.match(r"^r(\d+) ", log)
+    match = re.match(r"^r(\d+) ", self.GitSVNLog())
     if not match:
       self.Die("Could not extract current svn revision from log.")
     self["latest"] = match.group(1)
@@ -105,7 +104,7 @@ class CheckLastPush(Step):
 
   def RunStep(self):
     last_push_hash = self.FindLastTrunkPush()
-    last_push = int(self.Git("svn find-rev %s" % last_push_hash).strip())
+    last_push = int(self.GitSVNFindSVNRev(last_push_hash))
 
     # TODO(machenbach): This metric counts all revisions. It could be
     # improved by counting only the revisions on bleeding_edge.

tools/push-to-trunk/common_includes.py

@@ -36,6 +36,8 @@ import textwrap
 import time
 import urllib2
 
+from git_recipes import GitRecipesMixin
+
 PERSISTFILE_BASENAME = "PERSISTFILE_BASENAME"
 TEMP_BRANCH = "TEMP_BRANCH"
 BRANCHNAME = "BRANCHNAME"
@@ -232,11 +234,11 @@ class CommonOptions(object):
     self.force_readline_defaults = not manual
     self.force_upload = not manual
     self.manual = manual
-    self.reviewer = getattr(options, 'reviewer', None)
-    self.author = getattr(options, 'a', None)
+    self.reviewer = getattr(options, 'reviewer', "")
+    self.author = getattr(options, 'a', "")
 
 
-class Step(object):
+class Step(GitRecipesMixin):
   def __init__(self, text, requires, number, config, state, options, handler):
     self._text = text
     self._requires = requires
@@ -363,12 +365,11 @@ class Step(object):
     return answer == "" or answer == "Y" or answer == "y"
 
   def DeleteBranch(self, name):
-    git_result = self.Git("branch").strip()
-    for line in git_result.splitlines():
+    for line in self.GitBranch().splitlines():
       if re.match(r".*\s+%s$" % name, line):
         msg = "Branch %s exists, do you want to delete it?" % name
         if self.Confirm(msg):
-          self.Git("branch -D %s" % name)
+          self.GitDeleteBranch(name)
           print "Branch %s deleted." % name
         else:
           msg = "Can't continue. Please delete branch %s and try again." % name
@@ -386,36 +387,30 @@ class Step(object):
 
   def CommonPrepare(self):
     # Check for a clean workdir.
-    if self.Git("status -s -uno").strip() != "":
+    if not self.GitIsWorkdirClean():
       self.Die("Workspace is not clean. Please commit or undo your changes.")
 
     # Persist current branch.
-    self["current_branch"] = ""
-    git_result = self.Git("status -s -b -uno").strip()
-    for line in git_result.splitlines():
-      match = re.match(r"^## (.+)", line)
-      if match:
-        self["current_branch"] = match.group(1)
-        break
+    self["current_branch"] = self.GitCurrentBranch()
 
     # Fetch unfetched revisions.
-    self.Git("svn fetch")
+    self.GitSVNFetch()
 
   def PrepareBranch(self):
     # Get ahold of a safe temporary branch and check it out.
     if self["current_branch"] != self._config[TEMP_BRANCH]:
       self.DeleteBranch(self._config[TEMP_BRANCH])
-      self.Git("checkout -b %s" % self._config[TEMP_BRANCH])
+      self.GitCreateBranch(self._config[TEMP_BRANCH])
 
     # Delete the branch that will be created later if it exists already.
     self.DeleteBranch(self._config[BRANCHNAME])
 
   def CommonCleanup(self):
-    self.Git("checkout -f %s" % self["current_branch"])
+    self.GitCheckout(self["current_branch"])
     if self._config[TEMP_BRANCH] != self["current_branch"]:
-      self.Git("branch -D %s" % self._config[TEMP_BRANCH])
+      self.GitDeleteBranch(self._config[TEMP_BRANCH])
     if self._config[BRANCHNAME] != self["current_branch"]:
-      self.Git("branch -D %s" % self._config[BRANCHNAME])
+      self.GitDeleteBranch(self._config[BRANCHNAME])
 
     # Clean up all temporary files.
     Command("rm", "-f %s*" % self._config[PERSISTFILE_BASENAME])
@@ -460,17 +455,17 @@ class Step(object):
       answer = self.ReadLine()
 
   # Takes a file containing the patch to apply as first argument.
-  def ApplyPatch(self, patch_file, reverse_patch=""):
-    args = "apply --index --reject %s \"%s\"" % (reverse_patch, patch_file)
+  def ApplyPatch(self, patch_file, revert=False):
     try:
-      self.Git(args)
+      self.GitApplyPatch(patch_file, revert)
     except GitFailedException:
       self.WaitForResolvingConflicts(patch_file)
 
-  def FindLastTrunkPush(self):
+  def FindLastTrunkPush(self, parent_hash=""):
     push_pattern = "^Version [[:digit:]]*\.[[:digit:]]*\.[[:digit:]]* (based"
-    args = "log -1 --format=%%H --grep=\"%s\" svn/trunk" % push_pattern
-    return self.Git(args).strip()
+    branch = "" if parent_hash else "svn/trunk"
+    return self.GitLog(n=1, format="%H", grep=push_pattern,
+                       parent_hash=parent_hash, branch=branch)
 
 
 class UploadStep(Step):
@@ -484,14 +479,7 @@ class UploadStep(Step):
       print "Please enter the email address of a V8 reviewer for your patch: ",
       self.DieNoManualMode("A reviewer must be specified in forced mode.")
       reviewer = self.ReadLine()
-    author_option = self._options.author
-    author = " --email \"%s\"" % author_option if author_option else ""
-    force_flag = " -f" if self._options.force_upload else ""
-    args = ("cl upload%s -r \"%s\" --send-mail%s"
-            % (author, reviewer, force_flag))
-    # TODO(machenbach): Check output in forced mode. Verify that all required
-    # base files were uploaded, if not retry.
-    self.Git(args, pipe=False)
+    self.GitUpload(reviewer, self._options.author, self._options.force_upload)
 
 
 def MakeStep(step_class=Step, number=0, state=None, config=None,

tools/push-to-trunk/git_recipes.py

+#!/usr/bin/env python
+# Copyright 2014 the V8 project authors. All rights reserved.
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+#       copyright notice, this list of conditions and the following
+#       disclaimer in the documentation and/or other materials provided
+#       with the distribution.
+#     * Neither the name of Google Inc. nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+import re
+
+def Strip(f):
+  def new_f(*args, **kwargs):
+    return f(*args, **kwargs).strip()
+  return new_f
+
+
+def MakeArgs(l):
+  """['-a', '', 'abc', ''] -> '-a abc'"""
+  return " ".join(filter(None, l))
+
+
+def Quoted(s):
+  return "\"%s\"" % s
+
+
+class GitRecipesMixin(object):
+  def GitIsWorkdirClean(self):
+    return self.Git("status -s -uno").strip() == ""
+
+  @Strip
+  def GitBranch(self):
+    return self.Git("branch")
+
+  def GitCreateBranch(self, name, branch=""):
+    assert name
+    self.Git(MakeArgs(["checkout -b", name, branch]))
+
+  def GitDeleteBranch(self, name):
+    assert name
+    self.Git(MakeArgs(["branch -D", name]))
+
+  def GitCheckout(self, name):
+    assert name
+    self.Git(MakeArgs(["checkout -f", name]))
+
+  @Strip
+  def GitCurrentBranch(self):
+    for line in self.Git("status -s -b -uno").strip().splitlines():
+      match = re.match(r"^## (.+)", line)
+      if match: return match.group(1)
+    raise Exception("Couldn't find curent branch.")
+
+  @Strip
+  def GitLog(self, n=0, format="", grep="", git_hash="", parent_hash="",
+             branch="", reverse=False, patch=False):
+    assert not (git_hash and parent_hash)
+    args = ["log"]
+    if n > 0:
+      args.append("-%d" % n)
+    if format:
+      args.append("--format=%s" % format)
+    if grep:
+      args.append("--grep=\"%s\"" % grep)
+    if reverse:
+      args.append("--reverse")
+    if patch:
+      args.append("-p")
+    if git_hash:
+      args.append(git_hash)
+    if parent_hash:
+      args.append("%s^" % parent_hash)
+    args.append(branch)
+    return self.Git(MakeArgs(args))
+
+  def GitAdd(self, name):
+    assert name
+    self.Git(MakeArgs(["add", Quoted(name)]))
+
+  def GitApplyPatch(self, patch_file, reverse=False):
+    assert patch_file
+    args = ["apply --index --reject"]
+    if reverse:
+      args.append("--reverse")
+    args.append(Quoted(patch_file))
+    self.Git(MakeArgs(args))
+
+  def GitUpload(self, reviewer="", author="", force=False):
+    args = ["cl upload --send-mail"]
+    if author:
+      args += ["--email", Quoted(author)]
+    if reviewer:
+      args += ["-r", Quoted(reviewer)]
+    if force:
+      args.append("-f")
+    # TODO(machenbach): Check output in forced mode. Verify that all required
+    # base files were uploaded, if not retry.
+    self.Git(MakeArgs(args), pipe=False)
+
+  def GitCommit(self, message="", file_name=""):
+    assert message or file_name
+    args = ["commit"]
+    if file_name:
+      args += ["-aF", Quoted(file_name)]
+    if message:
+      args += ["-am", Quoted(message)]
+    self.Git(MakeArgs(args))
+
+  def GitPresubmit(self):
+    self.Git("cl presubmit", "PRESUBMIT_TREE_CHECK=\"skip\"")
+
+  def GitDCommit(self):
+    self.Git("cl dcommit -f --bypass-hooks", retry_on=lambda x: x is None)
+
+  def GitDiff(self, loc1, loc2):
+    return self.Git(MakeArgs(["diff", loc1, loc2]))
+
+  def GitPull(self):
+    self.Git("pull")
+
+  def GitSVNFetch(self):
+    self.Git("svn fetch")
+
+  @Strip
+  def GitSVNLog(self):
+    return self.Git("svn log -1 --oneline")
+
+  @Strip
+  def GitSVNFindGitHash(self, revision, branch=""):
+    assert revision
+    return self.Git(MakeArgs(["svn find-rev", "r%s" % revision, branch]))
+
+  @Strip
+  def GitSVNFindSVNRev(self, git_hash, branch=""):
+    return self.Git(MakeArgs(["svn find-rev", git_hash, branch]))
+
+  def GitSVNDCommit(self):
+    return self.Git("svn dcommit 2>&1", retry_on=lambda x: x is None)
+
+  def GitSVNTag(self, version):
+    self.Git(("svn tag %s -m \"Tagging version %s\"" % (version, version)),
+             retry_on=lambda x: x is None)

tools/push-to-trunk/merge_to_branch.py

@@ -57,7 +57,7 @@ class MergeToBranchOptions(CommonOptions):
     self.wait_for_lgtm = True
     self.delete_sentinel = options.f
     self.message = getattr(options, "message", "")
-    self.revert = "--reverse" if getattr(options, "r", None) else ""
+    self.revert = getattr(options, "r", False)
     self.revert_bleeding_edge = getattr(options, "revert_bleeding_edge", False)
     self.patch = getattr(options, "p", "")
     self.args = args
@@ -91,8 +91,8 @@ class CreateBranch(Step):
   MESSAGE = "Create a fresh branch for the patch."
 
   def RunStep(self):
-    self.Git("checkout -b %s svn/%s" % (self.Config(BRANCHNAME),
-                                        self["merge_to_branch"]))
+    self.GitCreateBranch(self.Config(BRANCHNAME),
+                         "svn/%s" % self["merge_to_branch"])
 
 
 class SearchArchitecturePorts(Step):
@@ -103,15 +103,14 @@ class SearchArchitecturePorts(Step):
     port_revision_list = []
     for revision in self["full_revision_list"]:
       # Search for commits which matches the "Port rXXX" pattern.
-      args = ("log svn/bleeding_edge --reverse "
-              "--format=%%H --grep=\"Port r%d\"" % int(revision))
-      git_hashes = self.Git(args) or ""
-      for git_hash in git_hashes.strip().splitlines():
-        args = "svn find-rev %s svn/bleeding_edge" % git_hash
-        svn_revision = self.Git(args).strip()
+      git_hashes = self.GitLog(reverse=True, format="%H",
+                               grep="Port r%d" % int(revision),
+                               branch="svn/bleeding_edge")
+      for git_hash in git_hashes.splitlines():
+        svn_revision = self.GitSVNFindSVNRev(git_hash, "svn/bleeding_edge")
         if not svn_revision:
           self.Die("Cannot determine svn revision for %s" % git_hash)
-        revision_title = self.Git("log -1 --format=%%s %s" % git_hash)
+        revision_title = self.GitLog(n=1, format="%s", git_hash=git_hash)
 
         # Is this revision included in the original revision list?
         if svn_revision in self["full_revision_list"]:
@@ -136,7 +135,7 @@ class FindGitRevisions(Step):
   def RunStep(self):
     self["patch_commit_hashes"] = []
     for revision in self["full_revision_list"]:
-      next_hash = self.Git("svn find-rev \"r%s\" svn/bleeding_edge" % revision)
+      next_hash = self.GitSVNFindGitHash(revision, "svn/bleeding_edge")
       if not next_hash:
         self.Die("Cannot determine git hash for r%s" % revision)
       self["patch_commit_hashes"].append(next_hash)
@@ -160,12 +159,12 @@ class FindGitRevisions(Step):
     self["new_commit_msg"] += "\n\n"
 
     for commit_hash in self["patch_commit_hashes"]:
-      patch_merge_desc = self.Git("log -1 --format=%%s %s" % commit_hash)
-      self["new_commit_msg"] += "%s\n\n" % patch_merge_desc.strip()
+      patch_merge_desc = self.GitLog(n=1, format="%s", git_hash=commit_hash)
+      self["new_commit_msg"] += "%s\n\n" % patch_merge_desc
 
     bugs = []
     for commit_hash in self["patch_commit_hashes"]:
-      msg = self.Git("log -1 %s" % commit_hash)
+      msg = self.GitLog(n=1, git_hash=commit_hash)
       for bug in re.findall(r"^[ \t]*BUG[ \t]*=[ \t]*(.*?)[ \t]*$", msg,
                             re.M):
         bugs.extend(map(lambda s: s.strip(), bug.split(",")))
@@ -182,7 +181,7 @@ class ApplyPatches(Step):
     for commit_hash in self["patch_commit_hashes"]:
       print("Applying patch for %s to %s..."
             % (commit_hash, self["merge_to_branch"]))
-      patch = self.Git("log -1 -p %s" % commit_hash)
+      patch = self.GitLog(n=1, patch=True, git_hash=commit_hash)
       TextToFile(patch, self.Config(TEMPORARY_PATCH_FILE))
       self.ApplyPatch(self.Config(TEMPORARY_PATCH_FILE), self._options.revert)
     if self._options.patch:
@@ -224,17 +223,17 @@ class CommitLocal(Step):
   MESSAGE = "Commit to local branch."
 
   def RunStep(self):
-    self.Git("commit -a -F \"%s\"" % self.Config(COMMITMSG_FILE))
+    self.GitCommit(file_name=self.Config(COMMITMSG_FILE))
 
 
 class CommitRepository(Step):
   MESSAGE = "Commit to the repository."
 
   def RunStep(self):
-    self.Git("checkout %s" % self.Config(BRANCHNAME))
+    self.GitCheckout(self.Config(BRANCHNAME))
     self.WaitForLGTM()
-    self.Git("cl presubmit", "PRESUBMIT_TREE_CHECK=\"skip\"")
-    self.Git("cl dcommit -f --bypass-hooks", retry_on=lambda x: x is None)
+    self.GitPresubmit()
+    self.GitDCommit()
 
 
 class PrepareSVN(Step):
@@ -243,14 +242,12 @@ class PrepareSVN(Step):
   def RunStep(self):
     if self._options.revert_bleeding_edge:
       return
-    self.Git("svn fetch")
-    args = ("log -1 --format=%%H --grep=\"%s\" svn/%s"
-            % (self["new_commit_msg"], self["merge_to_branch"]))
-    commit_hash = self.Git(args).strip()
+    self.GitSVNFetch()
+    commit_hash = self.GitLog(n=1, format="%H", grep=self["new_commit_msg"],
+                              branch="svn/%s" % self["merge_to_branch"])
     if not commit_hash:
       self.Die("Unable to map git commit to svn revision.")
-    self["svn_revision"] = self.Git(
-        "svn find-rev %s" % commit_hash).strip()
+    self["svn_revision"] = self.GitSVNFindSVNRev(commit_hash)
     print "subversion revision number is r%s" % self["svn_revision"]
 
 

tools/push-to-trunk/push_to_trunk.py

@@ -98,23 +98,20 @@ class FreshBranch(Step):
   MESSAGE = "Create a fresh branch."
 
   def RunStep(self):
-    args = "checkout -b %s svn/bleeding_edge" % self.Config(BRANCHNAME)
-    self.Git(args)
+    self.GitCreateBranch(self.Config(BRANCHNAME), "svn/bleeding_edge")
 
 
 class DetectLastPush(Step):
   MESSAGE = "Detect commit ID of last push to trunk."
 
   def RunStep(self):
-    last_push_trunk = self._options.l or self.FindLastTrunkPush()
+    last_push = self._options.l or self.FindLastTrunkPush()
     while True:
       # Print assumed commit, circumventing git's pager.
-      print self.Git("log -1 %s" % last_push_trunk)
+      print self.GitLog(n=1, git_hash=last_push)
       if self.Confirm("Is the commit printed above the last push to trunk?"):
         break
-      args = ("log -1 --format=%%H %s^ --grep=\"%s\""
-              % (last_push_trunk, push_pattern))
-      last_push_trunk = self.Git(args).strip()
+      last_push = self.FindLastTrunkPush(parent_hash=last_push)
 
     if self._options.b:
       # Read the bleeding edge revision of the last push from a command-line
@@ -123,21 +120,19 @@ class DetectLastPush(Step):
     else:
       # Retrieve the bleeding edge revision of the last push from the text in
       # the push commit message.
-      args = "log -1 --format=%%s %s" % last_push_trunk
-      last_push_trunk_title = self.Git(args).strip()
-      last_push_be_svn = PUSH_MESSAGE_RE.match(last_push_trunk_title).group(1)
+      last_push_title = self.GitLog(n=1, format="%s", git_hash=last_push)
+      last_push_be_svn = PUSH_MESSAGE_RE.match(last_push_title).group(1)
       if not last_push_be_svn:
         self.Die("Could not retrieve bleeding edge revision for trunk push %s"
-                 % last_push_trunk)
-      args = "svn find-rev r%s" % last_push_be_svn
-      last_push_bleeding_edge = self.Git(args).strip()
+                 % last_push)
+      last_push_bleeding_edge = self.GitSVNFindGitHash(last_push_be_svn)
       if not last_push_bleeding_edge:
         self.Die("Could not retrieve bleeding edge git hash for trunk push %s"
-                 % last_push_trunk)
+                 % last_push)
 
     # TODO(machenbach): last_push_trunk points to the svn revision on trunk.
     # It is not used yet but we'll need it for retrieving the current version.
-    self["last_push_trunk"] = last_push_trunk
+    self["last_push_trunk"] = last_push
     # TODO(machenbach): This currently points to the prepare push revision that
     # will be deprecated soon. After the deprecation it will point to the last
     # bleeding_edge revision that went into the last push.
@@ -175,16 +170,15 @@ class PrepareChangeLog(Step):
     output = "%s: Version %s\n\n" % (self["date"],
                                      self["version"])
     TextToFile(output, self.Config(CHANGELOG_ENTRY_FILE))
-
-    args = "log %s..HEAD --format=%%H" % self["last_push_bleeding_edge"]
-    commits = self.Git(args).strip()
+    commits = self.GitLog(format="%H",
+        git_hash="%s..HEAD" % self["last_push_bleeding_edge"])
 
     # Cache raw commit messages.
     commit_messages = [
       [
-        self.Git("log -1 %s --format=\"%%s\"" % commit),
-        self.Reload(self.Git("log -1 %s --format=\"%%B\"" % commit)),
-        self.Git("log -1 %s --format=\"%%an\"" % commit),
+        self.GitLog(n=1, format="%s", git_hash=commit),
+        self.Reload(self.GitLog(n=1, format="%B", git_hash=commit)),
+        self.GitLog(n=1, format="%an", git_hash=commit),
       ] for commit in commits.splitlines()
     ]
 
@@ -266,10 +260,11 @@ class CommitLocal(Step):
     # Include optional TBR only in the git command. The persisted commit
     # message is used for finding the commit again later.
     if self._options.tbr_commit:
-      review = "\n\nTBR=%s" % self._options.reviewer
+      message = "%s\n\nTBR=%s" % (self["prep_commit_msg"],
+                                  self._options.reviewer)
     else:
-      review = ""
-    self.Git("commit -a -m \"%s%s\"" % (self["prep_commit_msg"], review))
+      message = "%s" % self["prep_commit_msg"]
+    self.GitCommit(message)
 
 
 class CommitRepository(Step):
@@ -282,8 +277,8 @@ class CommitRepository(Step):
     TextToFile(GetLastChangeLogEntries(self.Config(CHANGELOG_FILE)),
                self.Config(CHANGELOG_ENTRY_FILE))
 
-    self.Git("cl presubmit", "PRESUBMIT_TREE_CHECK=\"skip\"")
-    self.Git("cl dcommit -f --bypass-hooks", retry_on=lambda x: x is None)
+    self.GitPresubmit()
+    self.GitDCommit()
 
 
 class StragglerCommits(Step):
@@ -291,10 +286,10 @@ class StragglerCommits(Step):
              "started.")
 
   def RunStep(self):
-    self.Git("svn fetch")
-    self.Git("checkout svn/bleeding_edge")
-    args = "log -1 --format=%%H --grep=\"%s\"" % self["prep_commit_msg"]
-    self["prepare_commit_hash"] = self.Git(args).strip()
+    self.GitSVNFetch()
+    self.GitCheckout("svn/bleeding_edge")
+    self["prepare_commit_hash"] = self.GitLog(n=1, format="%H",
+                                              grep=self["prep_commit_msg"])
 
 
 class SquashCommits(Step):
@@ -303,8 +298,8 @@ class SquashCommits(Step):
   def RunStep(self):
     # Instead of relying on "git rebase -i", we'll just create a diff, because
     # that's easier to automate.
-    args = "diff svn/trunk %s" % self["prepare_commit_hash"]
-    TextToFile(self.Git(args), self.Config(PATCH_FILE))
+    TextToFile(self.GitDiff("svn/trunk", self["prepare_commit_hash"]),
+               self.Config(PATCH_FILE))
 
     # Convert the ChangeLog entry to commit message format.
     text = FileToText(self.Config(CHANGELOG_ENTRY_FILE))
@@ -314,8 +309,7 @@ class SquashCommits(Step):
 
     # Retrieve svn revision for showing the used bleeding edge revision in the
     # commit message.
-    args = "svn find-rev %s" % self["prepare_commit_hash"]
-    self["svn_revision"] = self.Git(args).strip()
+    self["svn_revision"] = self.GitSVNFindSVNRev(self["prepare_commit_hash"])
     suffix = PUSH_MESSAGE_SUFFIX % int(self["svn_revision"])
     text = MSub(r"^(Version \d+\.\d+\.\d+)$", "\\1%s" % suffix, text)
 
@@ -336,7 +330,7 @@ class NewBranch(Step):
   MESSAGE = "Create a new branch from trunk."
 
   def RunStep(self):
-    self.Git("checkout -b %s svn/trunk" % self.Config(TRUNKBRANCH))
+    self.GitCreateBranch(self.Config(TRUNKBRANCH), "svn/trunk")
 
 
 class ApplyChanges(Step):
@@ -371,8 +365,8 @@ class CommitTrunk(Step):
   MESSAGE = "Commit to local trunk branch."
 
   def RunStep(self):
-    self.Git("add \"%s\"" % self.Config(VERSION_FILE))
-    self.Git("commit -F \"%s\"" % self.Config(COMMITMSG_FILE))
+    self.GitAdd(self.Config(VERSION_FILE))
+    self.GitCommit(file_name = self.Config(COMMITMSG_FILE))
     Command("rm", "-f %s*" % self.Config(COMMITMSG_FILE))
 
 
@@ -390,7 +384,7 @@ class CommitSVN(Step):
   MESSAGE = "Commit to SVN."
 
   def RunStep(self):
-    result = self.Git("svn dcommit 2>&1", retry_on=lambda x: x is None)
+    result = self.GitSVNDCommit()
     if not result:
       self.Die("'git svn dcommit' failed.")
     result = filter(lambda x: re.search(r"^Committed r[0-9]+", x),
@@ -414,9 +408,7 @@ class TagRevision(Step):
   MESSAGE = "Tag the new revision."
 
   def RunStep(self):
-    self.Git(("svn tag %s -m \"Tagging version %s\""
-              % (self["version"], self["version"])),
-             retry_on=lambda x: x is None)
+    self.GitSVNTag(self["version"])
 
 
 class CheckChromium(Step):
@@ -443,7 +435,7 @@ class SwitchChromium(Step):
     os.chdir(self["chrome_path"])
     self.InitialEnvironmentChecks()
     # Check for a clean workdir.
-    if self.Git("status -s -uno").strip() != "":
+    if not self.GitIsWorkdirClean():
       self.Die("Workspace is not clean. Please commit or undo your changes.")
     # Assert that the DEPS file is there.
     if not os.path.exists(self.Config(DEPS_FILE)):
@@ -456,9 +448,9 @@ class UpdateChromiumCheckout(Step):
 
   def RunStep(self):
     os.chdir(self["chrome_path"])
-    self.Git("checkout master")
-    self.Git("pull")
-    self.Git("checkout -b v8-roll-%s" % self["trunk_revision"])
+    self.GitCheckout("master")
+    self.GitPull()
+    self.GitCreateBranch("v8-roll-%s" % self["trunk_revision"])
 
 
 class UploadCL(Step):
@@ -482,13 +474,11 @@ class UploadCL(Step):
       print "Please enter the email address of a reviewer for the roll CL: ",
       self.DieNoManualMode("A reviewer must be specified in forced mode.")
       rev = self.ReadLine()
-    self.Git("commit -am \"Update V8 to version %s "
-             "(based on bleeding_edge revision r%s).\n\nTBR=%s\""
-             % (self["version"], self["svn_revision"], rev))
-    author_option = self._options.author
-    author = " --email \"%s\"" % author_option if author_option else ""
-    force_flag = " -f" if self._options.force_upload else ""
-    self.Git("cl upload%s --send-mail%s" % (author, force_flag), pipe=False)
+    suffix = PUSH_MESSAGE_SUFFIX % int(self["svn_revision"])
+    self.GitCommit("Update V8 to version %s%s.\n\nTBR=%s"
+                   % (self["version"], suffix, rev))
+    self.GitUpload(author=self._options.author,
+                   force=self._options.force_upload)
     print "CL uploaded."
 
 
@@ -518,7 +508,7 @@ class CleanUp(Step):
 
     self.CommonCleanup()
     if self.Config(TRUNKBRANCH) != self["current_branch"]:
-      self.Git("branch -D %s" % self.Config(TRUNKBRANCH))
+      self.GitDeleteBranch(self.Config(TRUNKBRANCH))
 
 
 def RunPushToTrunk(config,

tools/push-to-trunk/test_scripts.py

@@ -459,21 +459,21 @@ class ScriptTest(unittest.TestCase):
     TEST_CONFIG[CHANGELOG_ENTRY_FILE] = self.MakeEmptyTempFile()
 
     self.ExpectGit([
-      ["log 1234..HEAD --format=%H", "rev1\nrev2\nrev3\nrev4"],
-      ["log -1 rev1 --format=\"%s\"", "Title text 1"],
-      ["log -1 rev1 --format=\"%B\"", "Title\n\nBUG=\nLOG=y\n"],
-      ["log -1 rev1 --format=\"%an\"", "author1@chromium.org"],
-      ["log -1 rev2 --format=\"%s\"", "Title text 2."],
-      ["log -1 rev2 --format=\"%B\"", "Title\n\nBUG=123\nLOG= \n"],
-      ["log -1 rev2 --format=\"%an\"", "author2@chromium.org"],
-      ["log -1 rev3 --format=\"%s\"", "Title text 3"],
-      ["log -1 rev3 --format=\"%B\"", "Title\n\nBUG=321\nLOG=true\n"],
-      ["log -1 rev3 --format=\"%an\"", "author3@chromium.org"],
-      ["log -1 rev4 --format=\"%s\"", "Title text 4"],
-      ["log -1 rev4 --format=\"%B\"",
+      ["log --format=%H 1234..HEAD", "rev1\nrev2\nrev3\nrev4"],
+      ["log -1 --format=%s rev1", "Title text 1"],
+      ["log -1 --format=%B rev1", "Title\n\nBUG=\nLOG=y\n"],
+      ["log -1 --format=%an rev1", "author1@chromium.org"],
+      ["log -1 --format=%s rev2", "Title text 2."],
+      ["log -1 --format=%B rev2", "Title\n\nBUG=123\nLOG= \n"],
+      ["log -1 --format=%an rev2", "author2@chromium.org"],
+      ["log -1 --format=%s rev3", "Title text 3"],
+      ["log -1 --format=%B rev3", "Title\n\nBUG=321\nLOG=true\n"],
+      ["log -1 --format=%an rev3", "author3@chromium.org"],
+      ["log -1 --format=%s rev4", "Title text 4"],
+      ["log -1 --format=%B rev4",
        ("Title\n\nBUG=456\nLOG=Y\n\n"
         "Review URL: https://codereview.chromium.org/9876543210\n")],
-      ["log -1 rev4 --format=\"%an\"", "author4@chromium.org"],
+      ["log -1 --format=%an rev4", "author4@chromium.org"],
     ])
 
     # The cl for rev4 on rietveld has an updated LOG flag.
@@ -675,41 +675,41 @@ Performance and stability improvements on all platforms.""", commit)
       ["log -1 --format=%s hash2",
        "Version 3.4.5 (based on bleeding_edge revision r1234)\n"],
       ["svn find-rev r1234", "hash3\n"],
-      ["log hash3..HEAD --format=%H", "rev1\n"],
-      ["log -1 rev1 --format=\"%s\"", "Log text 1.\n"],
-      ["log -1 rev1 --format=\"%B\"", "Text\nLOG=YES\nBUG=v8:321\nText\n"],
-      ["log -1 rev1 --format=\"%an\"", "author1@chromium.org\n"],
-      [("commit -a -m \"Prepare push to trunk.  "
+      ["log --format=%H hash3..HEAD", "rev1\n"],
+      ["log -1 --format=%s rev1", "Log text 1.\n"],
+      ["log -1 --format=%B rev1", "Text\nLOG=YES\nBUG=v8:321\nText\n"],
+      ["log -1 --format=%an rev1", "author1@chromium.org\n"],
+      [("commit -am \"Prepare push to trunk.  "
         "Now working on version 3.22.6.%s\"" % review_suffix),
        " 2 files changed\n",
         CheckPreparePush],
-      [("cl upload --email \"author@chromium.org\" "
-        "-r \"reviewer@chromium.org\" --send-mail%s" % force_flag),
+      [("cl upload --send-mail --email \"author@chromium.org\" "
+        "-r \"reviewer@chromium.org\"%s" % force_flag),
        "done\n"],
       ["cl presubmit", "Presubmit successfull\n"],
       ["cl dcommit -f --bypass-hooks", "Closing issue\n"],
       ["svn fetch", "fetch result\n"],
-      ["checkout svn/bleeding_edge", ""],
+      ["checkout -f svn/bleeding_edge", ""],
       [("log -1 --format=%H --grep=\"Prepare push to trunk.  "
         "Now working on version 3.22.6.\""),
        "hash1\n"],
       ["diff svn/trunk hash1", "patch content\n"],
       ["svn find-rev hash1", "123455\n"],
       ["checkout -b %s svn/trunk" % TEST_CONFIG[TRUNKBRANCH], ""],
-      ["apply --index --reject  \"%s\"" % TEST_CONFIG[PATCH_FILE], ""],
+      ["apply --index --reject \"%s\"" % TEST_CONFIG[PATCH_FILE], ""],
       ["add \"%s\"" % TEST_CONFIG[VERSION_FILE], ""],
-      ["commit -F \"%s\"" % TEST_CONFIG[COMMITMSG_FILE], "", CheckSVNCommit],
+      ["commit -aF \"%s\"" % TEST_CONFIG[COMMITMSG_FILE], "", CheckSVNCommit],
       ["svn dcommit 2>&1", "Some output\nCommitted r123456\nSome output\n"],
       ["svn tag 3.22.5 -m \"Tagging version 3.22.5\"", ""],
       ["status -s -uno", ""],
-      ["checkout master", ""],
+      ["checkout -f master", ""],
       ["pull", ""],
       ["checkout -b v8-roll-123456", ""],
       [("commit -am \"Update V8 to version 3.22.5 "
         "(based on bleeding_edge revision r123455).\n\n"
         "TBR=reviewer@chromium.org\""),
        ""],
-      ["cl upload --email \"author@chromium.org\" --send-mail%s" % force_flag,
+      ["cl upload --send-mail --email \"author@chromium.org\"%s" % force_flag,
        ""],
       ["checkout -f some_branch", ""],
       ["branch -D %s" % TEST_CONFIG[TEMP_BRANCH], ""],
@@ -896,27 +896,27 @@ LOG=N
       ["checkout -b %s" % TEST_CONFIG[TEMP_BRANCH], ""],
       ["branch", "  branch1\n* branch2\n"],
       ["checkout -b %s svn/trunk" % TEST_CONFIG[BRANCHNAME], ""],
-      ["log svn/bleeding_edge --reverse --format=%H --grep=\"Port r12345\"",
+      ["log --format=%H --grep=\"Port r12345\" --reverse svn/bleeding_edge",
        "hash1\nhash2"],
       ["svn find-rev hash1 svn/bleeding_edge", "45678"],
       ["log -1 --format=%s hash1", "Title1"],
       ["svn find-rev hash2 svn/bleeding_edge", "23456"],
       ["log -1 --format=%s hash2", "Title2"],
-      ["log svn/bleeding_edge --reverse --format=%H --grep=\"Port r23456\"",
+      ["log --format=%H --grep=\"Port r23456\" --reverse svn/bleeding_edge",
        ""],
-      ["log svn/bleeding_edge --reverse --format=%H --grep=\"Port r34567\"",
+      ["log --format=%H --grep=\"Port r34567\" --reverse svn/bleeding_edge",
        "hash3"],
       ["svn find-rev hash3 svn/bleeding_edge", "56789"],
       ["log -1 --format=%s hash3", "Title3"],
-      ["svn find-rev \"r12345\" svn/bleeding_edge", "hash4"],
+      ["svn find-rev r12345 svn/bleeding_edge", "hash4"],
       # Simulate svn being down which stops the script.
-      ["svn find-rev \"r23456\" svn/bleeding_edge", None],
+      ["svn find-rev r23456 svn/bleeding_edge", None],
       # Restart script in the failing step.
-      ["svn find-rev \"r12345\" svn/bleeding_edge", "hash4"],
-      ["svn find-rev \"r23456\" svn/bleeding_edge", "hash2"],
-      ["svn find-rev \"r34567\" svn/bleeding_edge", "hash3"],
-      ["svn find-rev \"r45678\" svn/bleeding_edge", "hash1"],
-      ["svn find-rev \"r56789\" svn/bleeding_edge", "hash5"],
+      ["svn find-rev r12345 svn/bleeding_edge", "hash4"],
+      ["svn find-rev r23456 svn/bleeding_edge", "hash2"],
+      ["svn find-rev r34567 svn/bleeding_edge", "hash3"],
+      ["svn find-rev r45678 svn/bleeding_edge", "hash1"],
+      ["svn find-rev r56789 svn/bleeding_edge", "hash5"],
       ["log -1 --format=%s hash4", "Title4"],
       ["log -1 --format=%s hash2", "Title2"],
       ["log -1 --format=%s hash3", "Title3"],
@@ -928,24 +928,24 @@ LOG=N
       ["log -1 hash1", "Title1"],
       ["log -1 hash5", "Title5"],
       ["log -1 -p hash4", "patch4"],
-      ["apply --index --reject  \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
+      ["apply --index --reject \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
        "", VerifyPatch("patch4")],
       ["log -1 -p hash2", "patch2"],
-      ["apply --index --reject  \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
+      ["apply --index --reject \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
        "", VerifyPatch("patch2")],
       ["log -1 -p hash3", "patch3"],
-      ["apply --index --reject  \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
+      ["apply --index --reject \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
        "", VerifyPatch("patch3")],
       ["log -1 -p hash1", "patch1"],
-      ["apply --index --reject  \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
+      ["apply --index --reject \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
        "", VerifyPatch("patch1")],
       ["log -1 -p hash5", "patch5"],
-      ["apply --index --reject  \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
+      ["apply --index --reject \"%s\"" % TEST_CONFIG[TEMPORARY_PATCH_FILE],
        "", VerifyPatch("patch5")],
-      ["apply --index --reject  \"%s\"" % extra_patch, ""],
-      ["commit -a -F \"%s\"" % TEST_CONFIG[COMMITMSG_FILE], ""],
-      ["cl upload -r \"reviewer@chromium.org\" --send-mail", ""],
-      ["checkout %s" % TEST_CONFIG[BRANCHNAME], ""],
+      ["apply --index --reject \"%s\"" % extra_patch, ""],
+      ["commit -aF \"%s\"" % TEST_CONFIG[COMMITMSG_FILE], ""],
+      ["cl upload --send-mail -r \"reviewer@chromium.org\"", ""],
+      ["checkout -f %s" % TEST_CONFIG[BRANCHNAME], ""],
       ["cl presubmit", "Presubmit successfull\n"],
       ["cl dcommit -f --bypass-hooks", "Closing issue\n", VerifySVNCommit],
       ["svn fetch", ""],
@@ -973,7 +973,7 @@ LOG=N
     self.assertTrue(merge_to_branch.ProcessOptions(options, args))
 
     # The first run of the script stops because of the svn being down.
-    self.assertRaises(Exception,
+    self.assertRaises(GitFailedException,
         lambda: RunMergeToBranch(TEST_CONFIG,
                                  MergeToBranchOptions(options, args),
                                  self))