Refactor and improve bug line generation for push-to-trunk.

tools/push-to-trunk/common_includes.py

 #!/usr/bin/env python
 # Copyright 2013 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 12 matching lines @@
 # 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 os
 import re
 import subprocess
 import sys
+import textwrap
 
 PERSISTFILE_BASENAME = "PERSISTFILE_BASENAME"
 TEMP_BRANCH = "TEMP_BRANCH"
 BRANCHNAME = "BRANCHNAME"
 DOT_GIT_LOCATION = "DOT_GIT_LOCATION"
 VERSION_FILE = "VERSION_FILE"
 CHANGELOG_FILE = "CHANGELOG_FILE"
 CHANGELOG_ENTRY_FILE = "CHANGELOG_ENTRY_FILE"
 COMMITMSG_FILE = "COMMITMSG_FILE"
 PATCH_FILE = "PATCH_FILE"
@@ skipping 17 matching lines @@
 
 def FileToText(file_name):
   with open(file_name) as f:
     return f.read()
 
 
 def MSub(rexp, replacement, text):
   return re.sub(rexp, replacement, text, flags=re.MULTILINE)
 
 
+def Fill80(line):
+  return textwrap.fill(line, width=80, initial_indent="        ",
+                       subsequent_indent="        ")
+
+
 def GetLastChangeLogEntries(change_log_file):
   result = []
   for line in LinesInFile(change_log_file):
     if re.search(r"^\d{4}-\d{2}-\d{2}:", line) and result: break
     result.append(line)
   return "".join(result)
 
 
 def MakeChangeLogBody(commit_generator):
   result = ""
   for (title, body, author) in commit_generator():
     # Add the commit's title line.
     result += "%s\n" % title.rstrip()
 
-    # Grep for "BUG=xxxx" lines in the commit message and convert them to
-    # "(issue xxxx)".
-    out = body.splitlines()
-    out = filter(lambda x: re.search(r"^BUG=", x), out)
-    out = filter(lambda x: not re.search(r"BUG=$", x), out)
-    out = filter(lambda x: not re.search(r"BUG=none$", x), out)
-
-    # TODO(machenbach): Handle multiple entries (e.g. BUG=123, 234).
-    def FormatIssue(text):
-      text = re.sub(r"BUG=v8:(.*)$", r"(issue \1)", text)
-      text = re.sub(r"BUG=chromium:(.*)$", r"(Chromium issue \1)", text)
-      text = re.sub(r"BUG=(.*)$", r"(Chromium issue \1)", text)
-      return "        %s\n" % text
-
-    for line in map(FormatIssue, out):
-      result += line
+    # Add bug references.
+    result += MakeChangeLogBugReference(body)
 
     # Append the commit's author for reference.
     result += "%s\n\n" % author.rstrip()
   return result
 
 
+def MakeChangeLogBugReference(body):
+  # Grep for "BUG=xxxx" lines in the commit message and convert them to
+  # "(issue xxxx)".
+  out = body.splitlines()
+  out = filter(lambda x: re.search(r"^[ \t]*BUG[ \t]*=", x), out)

[Jakob Kummerow, 2013/11/19 13:39:29]

I don't think we need any of these three "filter" lines any more. AddIssues()
below should handle all of that anyway.

[Michael Achenbach, 2013/11/19 15:02:57]

Done.

+  out = filter(lambda x: not re.search(r"BUG[ \t]*=[ \t]*$", x), out)
+  out = filter(lambda x: not re.search(r"BUG[ \t]*=[ \t]*none[ \t]*$", x), out)
+
+  crbugs = []
+  v8bugs = []
+
+  def AddSafe(bugs, bug):

[Jakob Kummerow, 2013/11/19 13:39:29]

I think you don't need this function at all, if below in the regexes you match
against (\d+) instead of (.*).

[Michael Achenbach, 2013/11/19 15:02:57]

Done.

+    try:
+      bugs.append(int(bug))
+    except ValueError:
+      pass
+
+  def AddIssues(text):
+    ref = re.match(r"^[ \t]*BUG[ \t]*=[ \t]*(.*?)[ \t]*$", text)

[Jakob Kummerow, 2013/11/19 13:39:29]

I think the regex can be simplified to:

r"^[ \t]*BUG=(.*?)$"

Reasons:
- whitespace around '=' doesn't happen in practice
- whitespace at the end would be strip()'ed away below anyway.
- the simpler a regex, the better :-)

[Michael Achenbach, 2013/11/19 15:02:57]

Regarding the inner whitespace I had
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/git_...
as reference. That's what is allowed when depot tools use it.

Agreed on the outer whitespace. Added a strip.

+    if not ref:
+      return
+    for bug in ref.group(1).split(","):
+      bug = bug.strip()
+      match = re.match(r"^v8[ \t]*:[ \t]*(.*)$", bug)

[Jakob Kummerow, 2013/11/19 13:39:29]

similarly, we don't need to support whitespace around ':'

[Michael Achenbach, 2013/11/19 15:02:57]

Done.

+      if match: AddSafe(v8bugs, match.group(1))
+      else:
+        match = re.match(r"^(?:chromium[ \t]*:)?[ \t]*(.*)$", bug)
+        if match: AddSafe(crbugs, match.group(1))
+
+  # Add issues to crbugs and v8bugs.
+  map(AddIssues, out)
+
+  # Filter duplicates, sort, stringify.
+  crbugs = map(str, sorted(set(crbugs)))
+  v8bugs = map(str, sorted(set(v8bugs)))
+
+  bug_groups = []
+  def FormatIssues(prefix, bugs):
+    if len(bugs) > 0:
+      plural = "s" if len(bugs) > 1 else ""
+      bug_groups.append("%sissue%s %s" % (prefix, plural, ", ".join(bugs)))
+
+  FormatIssues("Chromium ", crbugs)
+  FormatIssues("", v8bugs)

[Jakob Kummerow, 2013/11/19 13:39:29]

V8 bugs first, please :-)

[Michael Achenbach, 2013/11/19 15:02:57]

Done. That required some test changes...

+
+  if len(bug_groups) > 0:
+    # Format with 8 characters indentation and max 80 character lines.
+    return "%s\n" % Fill80("(%s)" % ", ".join(bug_groups))
+  else:
+    return ""
+
+
 # Some commands don't like the pipe, e.g. calling vi from within the script or
 # from subscripts like git cl upload.
 def Command(cmd, args="", prefix="", pipe=True):
   cmd_line = "%s %s %s" % (prefix, cmd, args)
   print "Command: %s" % cmd_line
   try:
     if pipe:
       return subprocess.check_output(cmd_line, shell=True)
     else:
       return subprocess.check_call(cmd_line, shell=True)
@@ skipping 225 matching lines @@
     step.SetNumber(number)
     step.SetConfig(config)
     step.SetOptions(options)
     step.SetState(state)
     step.SetSideEffectHandler(side_effect_handler)
     steps.append(step)
     number += 1
 
   for step in steps[options.s:]:
     step.Run()