Switch ChangeDescription usage to be stricter.

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 16a6e0c402523988ed52ffdcde792fed340158c0..8bfc1061e376bb5825723cc68430cf37f8560444 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -54,7 +54,8 @@ def DieWithError(message):
 def RunCommand(args, error_ok=False, error_message=None, **kwargs):
   try:
     return subprocess2.check_output(args, shell=False, **kwargs)
-  except subprocess2.CalledProcessError, e:
+  except subprocess2.CalledProcessError as e:
+    logging.debug('Failed running %s', args)
     if not error_ok:
       DieWithError(
           'Command "%s" failed.\n%s' % (
@@ -797,44 +798,81 @@ def GetCodereviewSettingsInteractively():
 
 class ChangeDescription(object):
   """Contains a parsed form of the change description."""
-  def __init__(self, log_desc, reviewers):
-    self.log_desc = log_desc
-    self.reviewers = reviewers
-    self.description = self.log_desc
-
-  def Prompt(self):
-    content = """# 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.
-"""
-    content += self.description
-    if ('\nR=' not in self.description and
-        '\nTBR=' not in self.description and
-        self.reviewers):
-      content += '\nR=' + self.reviewers
+  R_LINE = r'^\s*(TBR|R)\s*=\s*(.+)\s*$'
+
+  def __init__(self, description):
+    self._description = (description or '').strip()
+
+  @property
+  def description(self):
+    return self._description
+
+  def update_reviewers(self, reviewers):

[iannucci, 2013/04/08 22:49:40]

Can we make reviewers a list of reviewers instead of expecting the
comma-delimited string? That will make it easier to manipulate and makes this
interface more logical.

[Dirk Pranke, 2013/04/09 00:07:52]

I don't think we can easily do this without pushing logic into the client that
we don't want to be there.

An alternative might be to provide static methods that did the converstion
(splitting/joining).

If we did make reviewers a list internally, we'd also want to split it out as a
separate member variable distinct from the free-text part of the description.

There is a certain self-enforcing consistency with using self._description for
all of the fields, at least.

[M-A Ruel, 2013/04/11 00:01:38]

So reviewers is now a list all the time.

+    """Rewrites the R=/TBR= line(s) as a single line."""
+    if not reviewers:
+      return
+    regexp = re.compile(self.R_LINE, re.MULTILINE)
+    matches = list(regexp.finditer(self.description))
+    is_tbr = any(m.group(1) == 'TBR' for m in matches)
+    if len(matches) > 1:
+      # Erase all except the first one.
+      for i in xrange(len(matches) - 1, 0, -1):
+        self._description = (
+            self.description[:matches[i].start()] +
+            self.description[matches[i].end()+1:])

[Dirk Pranke, 2013/04/09 00:07:52]

switching back and forth between self._description and self.description is
confusing. You should use one or the other consistently.

[M-A Ruel, 2013/04/11 00:01:38]

Done.

+
+    if is_tbr:
+      new_r_line = 'TBR=' + reviewers
+    else:
+      new_r_line = 'R=' + reviewers
+
+    if matches:
+      self._description = (
+        self.description[:matches[0].start()] + new_r_line +
+        self.description[matches[0].end()+1:])
+    else:
+      self.append_line(new_r_line)
+
+  def prompt(self):
+    """Asks the user to update the description."""
+    self._description = (
+      '# 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'
+    ) + self.description
+
     if '\nBUG=' not in self.description:
-      content += '\nBUG='
-    content = content.rstrip('\n') + '\n'
-    content = gclient_utils.RunEditor(content, True)
+      self.append_line('BUG=')
+    content = gclient_utils.RunEditor(self.description, True)
     if not content:
       DieWithError('Running editor failed')
+
+    # Strip off comments.
     content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip()
-    if not content.strip():
+    if not content:
       DieWithError('No CL description, aborting')
-    self.description = content
-
-  def ParseDescription(self):
-    """Updates the list of reviewers and subject from the description."""
-    self.description = self.description.strip('\n') + '\n'
-    # Retrieves all reviewer lines
-    regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE)
-    reviewers = ','.join(
-        i.group(2).strip() for i in regexp.finditer(self.description))
-    if reviewers:
-      self.reviewers = reviewers
+    self._description = content
 
-  def IsEmpty(self):
-    return not self.description
+  def append_line(self, line):

[Dirk Pranke, 2013/04/09 00:07:52]

I would probably call this something like add_keyword, since (I think?) we don't
want this to be used to generically append lines, right?

+    # Adds a LF if the last line did not have 'FOO=BAR' or if the new one isn't.
+    if self.description:
+      if '\n' not in self.description:
+        self._description += '\n'
+      else:
+        last_line = self.description.rsplit('\n', 1)[1]
+        if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or
+            not presubmit_support.Change.TAG_LINE_RE.match(line)):
+          self._description += '\n'
+    self._description += '\n' + line
+
+  def get_reviewers(self):
+    """Retrieves the list of reviewers."""
+    regexp = re.compile(self.R_LINE, re.MULTILINE)
+    reviewers = sum(
+        (i.group(2).split(',') for i in regexp.finditer(self.description)),
+        [])
+    reviewers = (i.strip() for i in reviewers)
+    return sorted(i for i in reviewers if i)
 
 
 def FindCodereviewSettingsFile(filename='codereview.settings'):
@@ -1102,16 +1140,15 @@ def GerritUpload(options, args, cl):
   if options.target_branch:
     branch = options.target_branch
 
-  log_desc = options.message or CreateDescriptionFromLog(args)
-  if CHANGE_ID not in log_desc:
-    AddChangeIdToCommitMessage(options, args)
-  if options.reviewers:
-    log_desc += '\nR=' + options.reviewers
-  change_desc = ChangeDescription(log_desc, options.reviewers)
-  change_desc.ParseDescription()
-  if change_desc.IsEmpty():
+  change_desc = ChangeDescription(
+      options.message or CreateDescriptionFromLog(args))
+  if not change_desc.description:
     print "Description is empty; aborting."
     return 1
+  if CHANGE_ID not in change_desc.description:

[Dirk Pranke, 2013/04/09 00:07:52]

I wonder if we should restructure this so that we add the change id during the
constructor (optionally?)

[M-A Ruel, 2013/04/11 00:01:38]

I prefer to keep this out of this CL since it invokes testing with repo style
hooks and there is exactly 0 unit test covering this use case.

+    AddChangeIdToCommitMessage(options, args)
+  if options.reviewers:
+    change_desc.update_reviewers(options.reviewers)
 
   receive_options = []
   cc = cl.GetCCList().split(',')
@@ -1120,10 +1157,9 @@ def GerritUpload(options, args, cl):
   cc = filter(None, cc)
   if cc:
     receive_options += ['--cc=' + email for email in cc]
-  if change_desc.reviewers:
-    reviewers = filter(None, change_desc.reviewers.split(','))
-    if reviewers:
-      receive_options += ['--reviewer=' + email for email in reviewers]
+  if change_desc.get_reviewers():
+    receive_options.extend(
+        '--reviewer=' + email for email in change_desc.get_reviewers())
 
   git_command = ['push']
   if receive_options:
@@ -1159,20 +1195,21 @@ def RietveldUpload(options, args, cl):
     if options.title:
       upload_args.extend(['--title', options.title])
     message = options.title or options.message or CreateDescriptionFromLog(args)
-    change_desc = ChangeDescription(message, options.reviewers)
+    change_desc = ChangeDescription(message)
+    if options.reviewers:
+      change_desc.update_reviewers(options.reviewers)
     if not options.force:
-      change_desc.Prompt()
-    change_desc.ParseDescription()
+      change_desc.prompt()
 
-    if change_desc.IsEmpty():
+    if not change_desc.description:
       print "Description is empty; aborting."
       return 1
 
     upload_args.extend(['--message', change_desc.description])
-    if change_desc.reviewers:
-      upload_args.extend(['--reviewers', change_desc.reviewers])
+    if change_desc.get_reviewers():
+      upload_args.append('--reviewers=' + ','.join(change_desc.get_reviewers()))
     if options.send_mail:
-      if not change_desc.reviewers:
+      if not change_desc.get_reviewers():
         DieWithError("Must specify reviewers to send email.")
       upload_args.append('--send_mail')
     cc = ','.join(filter(None, (cl.GetCCList(), options.cc)))
@@ -1416,24 +1453,26 @@ def SendUpstream(parser, args, cmd):
         (cl.GetRietveldServer(), cl.GetIssue()),
         verbose=False)
 
-  description = options.message
-  if not description and cl.GetIssue():
-    description = cl.GetDescription()
+  change_desc = ChangeDescription(options.message)
+  if not change_desc.description and cl.GetIssue():
+    change_desc = ChangeDescription(cl.GetDescription())
 
-  if not description:
+  if not change_desc.description:
     if not cl.GetIssue() and options.bypass_hooks:
-      description = CreateDescriptionFromLog([base_branch])
+      change_desc = ChangeDescription(CreateDescriptionFromLog([base_branch]))
     else:
       print 'No description set.'
       print 'Visit %s/edit to set it.' % (cl.GetIssueURL())
       return 1
 
+  # Keep a separate copy for the commit message.

[Dirk Pranke, 2013/04/09 00:07:52]

Can you clarify *why* we need a separate copy here?

[M-A Ruel, 2013/04/11 00:01:38]

Done.

+  commit_desc = ChangeDescription(change_desc.description)
   if cl.GetIssue():
-    description += "\n\nReview URL: %s" % cl.GetIssueURL()
-
+    commit_desc.append_line('Review URL: %s' % cl.GetIssueURL())
   if options.contributor:
-    description += "\nPatch from %s." % options.contributor
-  print 'Description:', repr(description)
+    commit_desc.append_line('Patch from %s.' % options.contributor)
+
+  print 'Description:', repr(commit_desc.description)
 
   branches = [base_branch, cl.GetBranchRef()]
   if not options.force:
@@ -1469,9 +1508,13 @@ def SendUpstream(parser, args, cmd):
     RunGit(['checkout', '-q', '-b', MERGE_BRANCH])
     RunGit(['reset', '--soft', base_branch])
     if options.contributor:
-      RunGit(['commit', '--author', options.contributor, '-m', description])
+      RunGit(
+          [
+            'commit', '--author', options.contributor,
+            '-m', commit_desc.description,
+          ])
     else:
-      RunGit(['commit', '-m', description])
+      RunGit(['commit', '-m', commit_desc.description])
     if base_has_submodules:
       cherry_pick_commit = RunGit(['rev-list', 'HEAD^!']).rstrip()
       RunGit(['branch', CHERRY_PICK_BRANCH, svn_head])
@@ -1510,12 +1553,12 @@ def SendUpstream(parser, args, cmd):
       return 1
     viewvc_url = settings.GetViewVCUrl()
     if viewvc_url and revision:
-      cl.description += ('\n\nCommitted: ' + viewvc_url + revision)
+      change_desc.append_line('Committed: ' + viewvc_url + revision)
     elif revision:
-      cl.description += ('\n\nCommitted: ' + revision)
+      change_desc.append_line('Committed: ' + revision)
     print ('Closing issue '
            '(you may be prompted for your codereview password)...')
-    cl.UpdateDescription(cl.description)
+    cl.UpdateDescription(change_desc.description)
     cl.CloseIssue()
     props = cl.RpcServer().get_issue_properties(cl.GetIssue(), False)
     patch_num = len(props['patchsets'])