refactor parsing of change descriptions, fix TBR=

git_cl/git_cl.py

 #!/usr/bin/python
 # git-cl -- a git-command for integrating reviews on Rietveld
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 import errno
 import logging
 import optparse
 import os
 import re
 import subprocess
 import sys
-import tempfile
 import textwrap
 import urlparse
 import urllib2
 
 try:
   import readline  # pylint: disable=W0611
 except ImportError:
   pass
 
 # TODO(dpranke): don't use relative import.
 import upload  # pylint: disable=W0403
-try:
-  # TODO(dpranke): We wrap this in a try block for a limited form of
-  # backwards-compatibility with older versions of git-cl that weren't
-  # dependent on depot_tools. This version should still work outside of
-  # depot_tools as long as --bypass-hooks is used. We should remove this
-  # once this has baked for a while and things seem safe.
-  depot_tools_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-  sys.path.append(depot_tools_path)
-  import breakpad  # pylint: disable=W0611
-except ImportError:
-  pass
+
+# TODO(dpranke): move this file up a directory so we don't need this.
+depot_tools_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+sys.path.append(depot_tools_path)
+
+import breakpad  # pylint: disable=W0611
+
+import presubmit_support
+import scm
+import watchlists
+
 
 DEFAULT_SERVER = 'http://codereview.appspot.com'
 POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
 DESCRIPTION_BACKUP_FILE = '~/.git_cl_description_backup'
 
 def DieWithError(message):
   print >> sys.stderr, message
   sys.exit(1)
 
 
@@ skipping 281 matching lines @@
     else:
       self.branch = None
     self.rietveld_server = None
     self.upstream_branch = None
     self.has_issue = False
     self.issue = None
     self.has_description = False
     self.description = None
     self.has_patchset = False
     self.patchset = None
+    self.tbr = False
 
   def GetBranch(self):
     """Returns the short branch name, e.g. 'master'."""
     if not self.branch:
       self.branchref = RunGit(['symbolic-ref', 'HEAD']).strip()
       self.branch = ShortBranchName(self.branchref)
     return self.branch
 
   def GetBranchRef(self):
     """Returns the full branch name, e.g. 'refs/heads/master'."""
@@ skipping 182 matching lines @@
 
   SetProperty(settings.GetCCList(), 'CC list', 'cc')
   SetProperty(settings.GetTreeStatusUrl(error_ok=True), 'Tree status URL',
               'tree-status-url')
   SetProperty(settings.GetViewVCUrl(), 'ViewVC URL', 'viewvc-url')
 
   # TODO: configure a default branch to diff against, rather than this
   # svn-based hackery.
 
 
-class ChangeDescription(object):
-  """Contains a parsed form of the change description."""
-  def __init__(self, subject, log_desc, reviewers):
-    self.subject = subject
-    self.log_desc = log_desc
-    self.reviewers = reviewers
-    self.description = self.log_desc
-
-  def Update(self):
-    initial_text = """# Enter a description of the change.
-# This will displayed on the codereview site.
-# The first line will also be used as the subject of the review.
-"""
-    initial_text += self.description
-    if 'R=' not in self.description and self.reviewers:
-      initial_text += '\nR=' + self.reviewers
-    if 'BUG=' not in self.description:
-      initial_text += '\nBUG='
-    if 'TEST=' not in self.description:
-      initial_text += '\nTEST='
-    self._ParseDescription(UserEditedLog(initial_text))
-
-  def _ParseDescription(self, description):
-    if not description:
-      self.description = description
-      return
-
-    parsed_lines = []
-    reviewers_regexp = re.compile('\s*R=(.+)')
-    reviewers = ''
-    subject = ''
-    for l in description.splitlines():
-      if not subject:
-        subject = l
-      matched_reviewers = reviewers_regexp.match(l)
-      if matched_reviewers:
-        reviewers = matched_reviewers.group(1)
-      parsed_lines.append(l)
-
-    self.description = '\n'.join(parsed_lines) + '\n'
-    self.subject = subject
-    self.reviewers = reviewers
-
-  def IsEmpty(self):
-    return not self.description
-
-
 def FindCodereviewSettingsFile(filename='codereview.settings'):
   """Finds the given file starting in the cwd and going up.
 
   Only looks up to the top of the repository unless an
   'inherit-review-settings-ok' file exists in the root of the repository.
   """
   inherit_ok_file = 'inherit-review-settings-ok'
   cwd = os.getcwd()
   root = os.path.abspath(RunGit(['rev-parse', '--show-cdup']).strip())
   if os.path.isfile(os.path.join(root, inherit_ok_file)):
@@ skipping 129 matching lines @@
     log_args = [args[0] + '..']
   elif len(args) == 1 and args[0].endswith('...'):
     log_args = [args[0][:-1]]
   elif len(args) == 2:
     log_args = [args[0] + '..' + args[1]]
   else:
     log_args = args[:]  # Hope for the best!
   return RunGit(['log', '--pretty=format:%s\n\n%b'] + log_args)
 
 
-def UserEditedLog(starting_text):
-  """Given some starting text, let the user edit it and return the result."""
-  editor = os.getenv('EDITOR', 'vi')
-
-  (file_handle, filename) = tempfile.mkstemp()
-  fileobj = os.fdopen(file_handle, 'w')
-  fileobj.write(starting_text)
-  fileobj.close()
-
-  # Open up the default editor in the system to get the CL description.
-  try:
-    cmd = '%s %s' % (editor, 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)
-    fileobj = open(filename)
-    text = fileobj.read()
-    fileobj.close()
-  finally:
-    os.remove(filename)
-
-  if not text:
-    return
-
-  stripcomment_re = re.compile(r'^#.*$', re.MULTILINE)
-  return stripcomment_re.sub('', text).strip()
-
-
 def ConvertToInteger(inputval):
   """Convert a string to integer, but returns either an int or None."""
   try:
     return int(inputval)
   except (TypeError, ValueError):
     return None
 
 
+class GitChangeDescription(presubmit_support.ChangeDescription):
+  def UserEdit(self):
+    header = (
+        "# Enter a description of the change.\n"
+        "# This will displayed on the codereview site.\n"
+        "# The first line will also be used as the subject of the review.\n"
+        "\n")
+    edited_text = self.editor(header + self.EditableDescription())
+    stripcomment_re = re.compile(r'^#.*$', re.MULTILINE)
+    self.Parse(stripcomment_re.sub('', edited_text).strip())
+
+
 def RunHook(committing, upstream_branch, rietveld_server, tbr, may_prompt):
   """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
-  import presubmit_support
-  import scm
-  import watchlists
-
   root = RunCommand(['git', 'rev-parse', '--show-cdup']).strip()
   if not root:
     root = '.'
   absroot = os.path.abspath(root)
   if not root:
     raise Exception('Could not get root directory.')
 
   # We use the sha1 of HEAD as a name of this change.
   name = RunCommand(['git', 'rev-parse', 'HEAD']).strip()
   files = scm.GIT.CaptureStatus([root], upstream_branch)
@@ skipping 48 matching lines @@
   cl = Changelist()
   if args:
     base_branch = args[0]
   else:
     # Default to diffing against the "upstream" branch.
     base_branch = cl.GetUpstreamBranch()
 
   if options.upload:
     print '*** Presubmit checks for UPLOAD would report: ***'
     RunHook(committing=False, upstream_branch=base_branch,
-            rietveld_server=cl.GetRietveldServer(), tbr=False,
+            rietveld_server=cl.GetRietveldServer(), tbr=cl.tbr,
             may_prompt=False)
     return 0
   else:
     print '*** Presubmit checks for DCOMMIT would report: ***'
     RunHook(committing=True, upstream_branch=base_branch,
-            rietveld_server=cl.GetRietveldServer, tbr=False,
+            rietveld_server=cl.GetRietveldServer, tbr=cl.tbr,
             may_prompt=False)
     return 0
 
 
 @usage('[args to "git diff"]')
 def CMDupload(parser, args):
   """upload the current changelist to codereview"""
   parser.add_option('--bypass-hooks', action='store_true', dest='bypass_hooks',
                     help='bypass upload presubmit hook')
   parser.add_option('-f', action='store_true', dest='force',
@@ skipping 22 matching lines @@
 
   cl = Changelist()
   if args:
     base_branch = args[0]
   else:
     # Default to diffing against the "upstream" branch.
     base_branch = cl.GetUpstreamBranch()
     args = [base_branch + "..."]
 
   if not options.bypass_hooks and not options.force:
-    hook_results = RunHook(committing=False, upstream_branch=base_branch,
-                           rietveld_server=cl.GetRietveldServer(), tbr=False,
-                           may_prompt=True)
-    if not options.reviewers and hook_results.reviewers:
-      options.reviewers = hook_results.reviewers
-
+    RunHook(committing=False, upstream_branch=base_branch,
+            rietveld_server=cl.GetRietveldServer(), tbr=cl.tbr,
+            may_prompt=True)
 
   # --no-ext-diff is broken in some versions of Git, so try to work around
   # this by overriding the environment (but there is still a problem if the
   # git config key "diff.external" is used).
   env = os.environ.copy()
   if 'GIT_EXTERNAL_DIFF' in env:
     del env['GIT_EXTERNAL_DIFF']
   subprocess.call(['git', 'diff', '--no-ext-diff', '--stat', '-M'] + args,
                   env=env)
 
@@ skipping 12 matching lines @@
   change_desc = None
 
   if cl.GetIssue():
     if options.message:
       upload_args.extend(['--message', options.message])
     upload_args.extend(['--issue', cl.GetIssue()])
     print ("This branch is associated with issue %s. "
            "Adding patch to that issue." % cl.GetIssue())
   else:
     log_desc = CreateDescriptionFromLog(args)
-    change_desc = ChangeDescription(options.message, log_desc,
-                                    options.reviewers)
-    if not options.from_logs:
-      change_desc.Update()
+    change_desc = GitChangeDescription(subject=options.message,
+        description=log_desc, reviewers=options.reviewers, tbr=cl.tbr)
+    if not options.from_logs and (not options.force):
+      change_desc.UserEdit()
 
     if change_desc.IsEmpty():
       print "Description is empty; aborting."
       return 1
 
     upload_args.extend(['--message', change_desc.subject])
     upload_args.extend(['--description', change_desc.description])
     if change_desc.reviewers:
       upload_args.extend(['--reviewers', change_desc.reviewers])
     cc = ','.join(filter(None, (settings.GetCCList(), options.cc)))
@@ skipping 90 matching lines @@
     extra_commits = RunGit(['rev-list', '^' + svn_head, base_branch])
     if extra_commits:
       print ('This branch has %d additional commits not upstreamed yet.'
              % len(extra_commits.splitlines()))
       print ('Upstream "%s" or rebase this branch on top of the upstream trunk '
              'before attempting to %s.' % (base_branch, cmd))
       return 1
 
   if not options.bypass_hooks and not options.force:
     RunHook(committing=True, upstream_branch=base_branch,
-            rietveld_server=cl.GetRietveldServer(), tbr=options.tbr,
+            rietveld_server=cl.GetRietveldServer(), tbr=(cl.tbr or options.tbr),
             may_prompt=True)
 
     if cmd == 'dcommit':
       # Check the tree status if the tree status URL is set.
       status = GetTreeStatus()
       if 'closed' == status:
         print ('The tree is closed.  Please wait for it to reopen. Use '
                '"git cl dcommit -f" to commit on a closed tree.')
         return 1
       elif 'unknown' == status:
@@ skipping 18 matching lines @@
       print 'Visit %s/edit to set it.' % (cl.GetIssueURL())
       return 1
 
     description += "\n\nReview URL: %s" % cl.GetIssueURL()
   else:
     if not description:
       # Submitting TBR.  See if there's already a description in Rietveld, else
       # create a template description. Eitherway, give the user a chance to edit
       # it to fill in the TBR= field.
       if cl.GetIssue():
-        description = cl.GetDescription()
+        change_desc = GitChangeDescription(description=cl.GetDescription())
 
-      # TODO(dpranke): Update to use ChangeDescription object.
       if not description:
-        description = """# Enter a description of the change.
-# This will be used as the change log for the commit.
+        log_desc = CreateDescriptionFromLog(args)
+        change_desc = GitChangeDescription(description=log_desc, tbr=True)
 
-"""
-        description += CreateDescriptionFromLog(args)
-
-      description = UserEditedLog(description + '\nTBR=')
+      if not options.force:
+        change_desc.UserEdit()
+      description = change_desc.description
 
     if not description:
       print "Description empty; aborting."
       return 1
 
   if options.contributor:
     if not re.match('^.*\s<\S+@\S+>$', options.contributor):
       print "Please provide contibutor as 'First Last <email@example.com>'"
       return 1
     description += "\nPatch from %s." % options.contributor
@@ skipping 316 matching lines @@
             ('AppEngine is misbehaving and returned HTTP %d, again. Keep faith '
              'and retry or visit go/isgaeup.\n%s') % (e.code, str(e)))
 
   # Not a known command. Default to help.
   GenUsage(parser, 'help')
   return CMDhelp(parser, argv)
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv[1:]))