Make merge_to_branch more git-friendly.

tools/push-to-trunk/merge_to_branch.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
@@ skipping 14 matching lines @@
 # 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 argparse
 from collections import OrderedDict
 import sys
 
 from common_includes import *
 
+def IsSvnNumber(rev):
+  return rev.isdigit() and len(rev) < 8
+
 class Preparation(Step):
   MESSAGE = "Preparation."
 
   def RunStep(self):
     if os.path.exists(self.Config("ALREADY_MERGING_SENTINEL_FILE")):
       if self._options.force:
         os.remove(self.Config("ALREADY_MERGING_SENTINEL_FILE"))
       elif self._options.step == 0:  # pragma: no cover
         self.Die("A merge is already in progress")
     open(self.Config("ALREADY_MERGING_SENTINEL_FILE"), "a").close()
@@ skipping 20 matching lines @@
 
 
 class SearchArchitecturePorts(Step):
   MESSAGE = "Search for corresponding architecture ports."
 
   def RunStep(self):
     self["full_revision_list"] = list(OrderedDict.fromkeys(
         self._options.revisions))
     port_revision_list = []
     for revision in self["full_revision_list"]:
-      # Search for commits which matches the "Port rXXX" pattern.
+      # Search for commits which matches the "Port XXX" pattern.
       git_hashes = self.GitLog(reverse=True, format="%H",
-                               grep="Port r%d" % int(revision),
+                               grep="Port %s" % revision,
                                branch=self.vc.RemoteMasterBranch())
       for git_hash in git_hashes.splitlines():
-        svn_revision = self.vc.GitSvn(git_hash, self.vc.RemoteMasterBranch())
-        if not svn_revision:  # pragma: no cover
-          self.Die("Cannot determine svn revision for %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"]:
-          print("Found port of r%s -> r%s (already included): %s"
-                % (revision, svn_revision, revision_title))
+        if git_hash in self["full_revision_list"]:
+          print("Found port of %s -> %s (already included): %s"
+                % (revision, git_hash, revision_title))
         else:
-          print("Found port of r%s -> r%s: %s"
-                % (revision, svn_revision, revision_title))
-          port_revision_list.append(svn_revision)
+          print("Found port of %s -> %s: %s"
+                % (revision, git_hash, revision_title))
+          port_revision_list.append(git_hash)
 
     # Do we find any port?
     if len(port_revision_list) > 0:
       if self.Confirm("Automatically add corresponding ports (%s)?"
                       % ", ".join(port_revision_list)):
         #: 'y': Add ports to revision list.
         self["full_revision_list"].extend(port_revision_list)
 
 
-class FindGitRevisions(Step):
-  MESSAGE = "Find the git revisions associated with the patches."
+class CreateCommitMessage(Step):
+  MESSAGE = "Create commit message."
 
   def RunStep(self):
-    self["patch_commit_hashes"] = []
-    for revision in self["full_revision_list"]:
-      next_hash = self.vc.SvnGit(revision, self.vc.RemoteMasterBranch())
-      if not next_hash:  # pragma: no cover
-        self.Die("Cannot determine git hash for r%s" % revision)
-      self["patch_commit_hashes"].append(next_hash)
 
     # Stringify: [123, 234] -> "r123, r234"
     self["revision_list"] = ", ".join(map(lambda s: "r%s" % s,
                                       self["full_revision_list"]))
 
     if not self["revision_list"]:  # pragma: no cover
       self.Die("Revision list is empty.")
 
+    if self._options.revert and not self._options.revert_bleeding_edge:
+      action_text = "Rollback of %s"
+    else:
+      action_text = "Merged %s"
     # The commit message title is added below after the version is specified.
-    self["new_commit_msg"] = ""
+    self["new_commit_msg"] = "\n".join(map(lambda s: action_text % s,
+                                           self["full_revision_list"]))

[tandrii(chromium), 2014/11/05 12:59:15]

I think this is shorter and more readable:
join(action_text % s for s in self["full_revision_list"])

Also, I don't like += to grow strings, though here it's arguably justified.
Still, consider building self["new_commit_msg"] using a list of strings, joined
at the end.

[Michael Achenbach, 2014/11/05 13:30:37]

Done.

+    self["new_commit_msg"] += "\n\n"
 
-    for commit_hash in self["patch_commit_hashes"]:
+    for commit_hash in self["full_revision_list"]:
       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"]:
+    for commit_hash in self["full_revision_list"]:
       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(",")))

[tandrii(chromium), 2014/11/05 12:59:15]

consider shorter:
bugs.extend(s.strip() for s in bug.split(","))

[Michael Achenbach, 2014/11/05 13:30:37]

Done.

     bug_aggregate = ",".join(sorted(filter(lambda s: s and s != "none", bugs)))
     if bug_aggregate:
       self["new_commit_msg"] += "BUG=%s\nLOG=N\n" % bug_aggregate
 
 
 class ApplyPatches(Step):
   MESSAGE = "Apply patches for selected revisions."
 
   def RunStep(self):
-    for commit_hash in self["patch_commit_hashes"]:
+    for commit_hash in self["full_revision_list"]:
       print("Applying patch for %s to %s..."
             % (commit_hash, self["merge_to_branch"]))
       patch = self.GitGetPatch(commit_hash)
       TextToFile(patch, self.Config("TEMPORARY_PATCH_FILE"))
       self.ApplyPatch(self.Config("TEMPORARY_PATCH_FILE"), self._options.revert)
     if self._options.patch:
       self.ApplyPatch(self._options.patch, self._options.revert)
 
 
 class PrepareVersion(Step):
@@ skipping 29 matching lines @@
                                        self["new_minor"],
                                        self["new_build"],
                                        self["new_patch"])
 
 
 class CommitLocal(Step):
   MESSAGE = "Commit to local branch."
 
   def RunStep(self):
     # Add a commit message title.
-    if self._options.revert:
-      if not self._options.revert_bleeding_edge:
-        title = ("Version %s (rollback of %s)"
-                 % (self["version"], self["revision_list"]))
-      else:
-        title = "Revert %s." % self["revision_list"]
+    if self._options.revert and self._options.revert_bleeding_edge:
+      # TODO(machenbach): Find a better convention if multiple patches are
+      # reverted in one CL.
+      self["commit_title"] = "Revert on master"
     else:
-      title = ("Version %s (merged %s)"
-               % (self["version"], self["revision_list"]))
-    self["commit_title"] = title
-    self["new_commit_msg"] = "%s\n\n%s" % (title, self["new_commit_msg"])
+      self["commit_title"] = "Version %s (cherry-pick)" % self["version"]
+    self["new_commit_msg"] = "%s\n\n%s" % (self["commit_title"],
+                                           self["new_commit_msg"])
     TextToFile(self["new_commit_msg"], self.Config("COMMITMSG_FILE"))
     self.GitCommit(file_name=self.Config("COMMITMSG_FILE"))
 
 
 class CommitRepository(Step):
   MESSAGE = "Commit to the repository."
 
   def RunStep(self):
     self.GitCheckout(self.Config("BRANCHNAME"))
     self.WaitForLGTM()
@@ skipping 55 matching lines @@
     if len(options.revisions) < 1:
       if not options.patch:
         print "Either a patch file or revision numbers must be specified"
         return False
       if not options.message:
         print "You must specify a merge comment if no patches are specified"
         return False
     options.bypass_upload_hooks = True
     # CC ulan to make sure that fixes are merged to Google3.
     options.cc = "ulan@chromium.org"
+
+    # Thd old git-svn workflow is deprecated for this script.
+    assert options.vc_interface != "git_svn"
+
+    # Make sure to use git hashes in the new workflows.
+    for revision in options.revisions:
+      if (IsSvnNumber(revision) or
+          (revision[0:1] == "r" and IsSvnNumber(revision[1:]))):
+        print "Please provide full git hashes of the patches to merge."
+        print "Got: %s" % revision
+        return False
     return True
 
   def _Config(self):
     return {
       "BRANCHNAME": "prepare-merge",
       "PERSISTFILE_BASENAME": "/tmp/v8-merge-to-branch-tempfile",
       "ALREADY_MERGING_SENTINEL_FILE":
           "/tmp/v8-merge-to-branch-tempfile-already-merging",
       "TEMPORARY_PATCH_FILE": "/tmp/v8-prepare-merge-tempfile-temporary-patch",
       "COMMITMSG_FILE": "/tmp/v8-prepare-merge-tempfile-commitmsg",
     }
 
   def _Steps(self):
     return [
       Preparation,
       CreateBranch,
       SearchArchitecturePorts,
-      FindGitRevisions,
+      CreateCommitMessage,
       ApplyPatches,
       PrepareVersion,
       IncrementVersion,
       CommitLocal,
       UploadStep,
       CommitRepository,
       TagRevision,
       CleanUp,
     ]
 
 
 if __name__ == "__main__":  # pragma: no cover
   sys.exit(MergeToBranch().Run())