refactor parsing of change descriptions, fix TBR=

gcl.py

 #!/usr/bin/python
 # Copyright (c) 2010 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """\
 Wrapper script around Rietveld's upload.py that simplifies working with groups
 of files.
 """
 
@@ skipping 272 matching lines @@
     rietveld: rietveld server for this change
   """
   # Kept for unit test support. This is for the old format, it's deprecated.
   _SEPARATOR = "\n-----\n"
 
   def __init__(self, name, issue, patchset, description, files, local_root,
                rietveld, needs_upload=False):
     self.name = name
     self.issue = int(issue)
     self.patchset = int(patchset)
-    self._description = None
-    self._subject = None
-    self._reviewers = None
+    self._change_desc = None
     self._set_description(description)
     if files is None:
       files = []
     self._files = files
     self.patch = None
     self._local_root = local_root
     self.needs_upload = needs_upload
     self.rietveld = rietveld
     if not self.rietveld:
       # Set the default value.
       self.rietveld = GetCodeReviewSetting('CODE_REVIEW_SERVER')
 
   def _get_description(self):
-    return self._description
+    return self._change_desc.description
 
   def _set_description(self, description):
-    # TODO(dpranke): Cloned from git_cl.py. These should be shared.
-    if not description:
-      self._description = description
-      return
-
-    parsed_lines = []
-    reviewers_re = re.compile(REVIEWERS_REGEX)
-    reviewers = ''
-    subject = ''
-    for l in description.splitlines():
-      if not subject:
-        subject = l
-      matched_reviewers = reviewers_re.match(l)
-      if matched_reviewers:
-        reviewers = matched_reviewers.group(1).split(',')
-      parsed_lines.append(l)
-
-    if len(subject) > 100:
-      subject = subject[:97] + '...'
-
-    self._subject = subject
-    self._reviewers = reviewers
-    self._description = '\n'.join(parsed_lines)
+    self._change_desc = presubmit_support.ChangeDescription(
+        description=description)
 
   description = property(_get_description, _set_description)
 
   @property
   def reviewers(self):
-    return self._reviewers
+    return self._change_desc.reviewers
 
   @property
   def subject(self):
-    return self._subject
+    return self._change_desc.subject
 
   def NeedsUpload(self):
     return self.needs_upload
 
   def GetFileNames(self):
     """Returns the list of file names included in this change."""
     return [f[1] for f in self._files]
 
   def GetFiles(self):
     """Returns the list of files included in this change with their status."""
@@ skipping 16 matching lines @@
     """Returns a list of files added in this change."""
     return [f[1] for f in self.GetFiles() if f[0].startswith("A")]
 
   def Save(self):
     """Writes the changelist information to disk."""
     data = json.dumps({
           'issue': self.issue,
           'patchset': self.patchset,
           'needs_upload': self.NeedsUpload(),
           'files': self.GetFiles(),
-          'description': self.description,
+          'description': self._change_desc.description,
           'rietveld': self.rietveld,
         }, sort_keys=True, indent=2)
     gclient_utils.FileWrite(GetChangelistInfoFile(self.name), data)
 
   def Delete(self):
     """Removes the changelist information from disk."""
     os.remove(GetChangelistInfoFile(self.name))
 
   def CloseIssue(self):
     """Closes the Rietveld issue for this changelist."""
@@ skipping 340 matching lines @@
   if (files.get('') or (show_unknown_files and len(unknown_files))):
     print "\n--- Not in any changelist:"
     for item in files.get('', []):
       print "".join(item)
     if show_unknown_files:
       for filename in unknown_files:
         print "?      %s" % filename
   return 0
 
 
-def GetEditor():
-  editor = os.environ.get("SVN_EDITOR")
-  if not editor:
-    editor = os.environ.get("EDITOR")
-
-  if not editor:
-    if sys.platform.startswith("win"):
-      editor = "notepad"
-    else:
-      editor = "vi"
-
-  return editor
-
-
 def GenerateDiff(files, root=None):
   return SVN.GenerateDiff(files, root=root)
 
 
 def OptionallyDoPresubmitChecks(change_info, committing, args):
   if FilterFlag(args, "--no_presubmit") or FilterFlag(args, "--force"):
     return presubmit_support.PresubmitOutput()
   return DoPresubmitChecks(change_info, committing, True)
 
 
@@ skipping 325 matching lines @@
       description = change_info.description
 
   other_files = GetFilesNotInCL()
 
   # Edited files (as opposed to files with only changed properties) will have
   # a letter for the first character in the status string.
   file_re = re.compile(r"^[a-z].+\Z", re.IGNORECASE)
   affected_files = [x for x in other_files if file_re.match(x[0])]
   unaffected_files = [x for x in other_files if not file_re.match(x[0])]
 
-  if not change_info.reviewers:
+  reviewers = change_info.reviewers
+  if not reviewers:
     files_for_review = affected_files[:]
     files_for_review.extend(change_info.GetFiles())
-    suggested_reviewers = suggest_reviewers(change_info, files_for_review)
-    if suggested_reviewers:
-      reviewers_re = re.compile(REVIEWERS_REGEX)
-      if not any(reviewers_re.match(l) for l in description.splitlines()):
-        description += '\n\nR=' + ','.join(suggested_reviewers)
-
-  description = description.rstrip() + '\n'
+    reviewers = suggest_reviewers(change_info, files_for_review)
 
   separator1 = ("\n---All lines above this line become the description.\n"
                 "---Repository Root: " + change_info.GetLocalRoot() + "\n"
                 "---Paths in this changelist (" + change_info.name + "):\n")
   separator2 = "\n\n---Paths modified but not in any changelist:\n\n"
-  text = (description + separator1 + '\n' +
-          '\n'.join([f[0] + f[1] for f in change_info.GetFiles()]))
+
+  footer = (separator1 + '\n' +
+            '\n'.join([f[0] + f[1] for f in change_info.GetFiles()]))
 
   if change_info.Exists():
-    text += (separator2 +
-            '\n'.join([f[0] + f[1] for f in affected_files]) + '\n')
+    footer += (separator2 +
+              '\n'.join([f[0] + f[1] for f in affected_files]) + '\n')
   else:
-    text += ('\n'.join([f[0] + f[1] for f in affected_files]) + '\n' +
-            separator2)
-  text += '\n'.join([f[0] + f[1] for f in unaffected_files]) + '\n'
+    footer += ('\n'.join([f[0] + f[1] for f in affected_files]) + '\n' +
+              separator2)
+  footer += '\n'.join([f[0] + f[1] for f in unaffected_files]) + '\n'
 
-  handle, filename = tempfile.mkstemp(text=True)
-  os.write(handle, text)
-  os.close(handle)
+  change_desc = presubmit_support.ChangeDescription(description=description,
+      reviewers=reviewers)
 
-  # Open up the default editor in the system to get the CL description.
-  try:
-    if not silent:
-      cmd = '%s %s' % (GetEditor(), filename)
-      if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
-        # Msysgit requires the usage of 'env' to be present.
-        cmd = 'env ' + cmd
-      # shell=True to allow the shell to handle all forms of quotes in $EDITOR.
-      subprocess.check_call(cmd, shell=True)
-    result = gclient_utils.FileRead(filename, 'r')
-  finally:
-    os.remove(filename)
+  # These next few lines are equivalent to change_desc.UserUpdate(). We
+  # call them individually to avoid passing a lot of state back and forth.
+  original_description = change_desc.description
+
+  text = change_desc.EditableDescription() + footer

[M-A Ruel, 2011/03/22 17:24:16]

Why not?
result = change_desc...
if not silent:
  result = change_desc.editor(result)

+
+  if not silent:
+    result = change_desc.editor(text)
+  else:
+    result = text
 
   if not result:
     return 0
 
   split_result = result.split(separator1, 1)
   if len(split_result) != 2:
     ErrorExit("Don't modify the text starting with ---!\n\n" + result)
 
   # Update the CL description if it has changed.
   new_description = split_result[0]
   cl_files_text = split_result[1]
-  if new_description != description or override_description:
-    change_info.description = new_description
+  change_desc.Parse(new_description)
+  if change_desc.description != original_description or override_description:
     change_info.needs_upload = True
 
   new_cl_files = []
   for line in cl_files_text.splitlines():
     if not len(line):
       continue
     if line.startswith("---"):
       break
     status = line[:7]
     filename = line[7:]
     new_cl_files.append((status, filename))
 
   if (not len(change_info.GetFiles()) and not change_info.issue and
-      not len(new_description) and not new_cl_files):
+      not len(change_desc.description) and not new_cl_files):
     ErrorExit("Empty changelist not saved")
 
   change_info._files = new_cl_files
   change_info.Save()
   if svn_info.get('URL', '').startswith('http:'):
     Warn("WARNING: Creating CL in a read-only checkout.  You will not be "
          "able to commit it!")
 
   print change_info.name + " changelist saved."
   if change_info.MissingTests():
@@ skipping 287 matching lines @@
     if e.code != 500:
       raise
     print >> sys.stderr, (
         'AppEngine is misbehaving and returned HTTP %d, again. Keep faith '
         'and retry or visit go/isgaeup.\n%s') % (e.code, str(e))
     return 1
 
 
 if __name__ == "__main__":
   sys.exit(main(sys.argv[1:]))