Switch ChangeDescription usage to be stricter.

Include all the preparatory work to eventually update the R= line to match the
actual reviewers.

The goal here is that ChangeDescription becomes the official implementation for
handling and modifying commit messages accross git-cl, gcl and the commit queue.

This change does slightly tweak the spacing between the hot lines.
It is done on purpose to make them consistent.

R=dpranke@chromium.org
BUG=

Review URL: https://chromiumcodereview.appspot.com/13741015

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@193514 0039d316-1c4b-4281-b951-d872f2087c98

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,79 @@ 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
-    if '\nBUG=' not in self.description:
-      content += '\nBUG='
-    content = content.rstrip('\n') + '\n'
-    content = gclient_utils.RunEditor(content, True)
+  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):
+    """Rewrites the R=/TBR= line(s) as a single line."""
+    assert isinstance(reviewers, list), reviewers
+    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:])
+
+    if is_tbr:
+      new_r_line = 'TBR=' + ', '.join(reviewers)
+    else:
+      new_r_line = 'R=' + ', '.join(reviewers)
+
+    if matches:
+      self._description = (
+        self._description[:matches[0].start()] + new_r_line +
+        self._description[matches[0].end()+1:])
+    else:
+      self.append_footer(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:
+      self.append_footer('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
+    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
+  def append_footer(self, line):
+    # 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 IsEmpty(self):
-    return not self.description
+  def get_reviewers(self):
+    """Retrieves the list of reviewers."""
+    regexp = re.compile(self.R_LINE, re.MULTILINE)
+    reviewers = [i.group(2) for i in regexp.finditer(self._description)]
+    return cleanup_list(reviewers)
 
 
 def FindCodereviewSettingsFile(filename='codereview.settings'):
@@ -1102,16 +1138,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=' + ', '.join(options.reviewers)
-  change_desc = ChangeDescription(log_desc, ', '.join(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:
+    AddChangeIdToCommitMessage(options, args)
+  if options.reviewers:
+    change_desc.update_reviewers(options.reviewers)
 
   receive_options = []
   cc = cl.GetCCList().split(',')
@@ -1120,11 +1155,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, (r.strip() for r in 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:
@@ -1160,24 +1193,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, ','.join(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',
-            ','.join(r.strip() for r in change_desc.reviewers.split(',')),
-          ])
+    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(), ','.join(options.cc))))
@@ -1440,24 +1470,28 @@ 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, because the commit message
+  # contains the link to the Rietveld issue, while the Rietveld message contains
+  # the commit viewvc url.
+  commit_desc = ChangeDescription(change_desc.description)
   if cl.GetIssue():
-    description += "\n\nReview URL: %s" % cl.GetIssueURL()
-
+    commit_desc.append_footer('Review URL: %s' % cl.GetIssueURL())
   if options.contributor:
-    description += "\nPatch from %s." % options.contributor
-  print 'Description:', repr(description)
+    commit_desc.append_footer('Patch from %s.' % options.contributor)
+
+  print 'Description:', repr(commit_desc.description)
 
   branches = [base_branch, cl.GetBranchRef()]
   if not options.force:
@@ -1493,9 +1527,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])
@@ -1534,12 +1572,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_footer('Committed: ' + viewvc_url + revision)
     elif revision:
-      cl.description += ('\n\nCommitted: ' + revision)
+      change_desc.append_footer('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'])

tests/git_cl_test.py

@@ -30,10 +30,23 @@ class PresubmitMock(object):
 class RietveldMock(object):
   def __init__(self, *args, **kwargs):
     pass
+
   @staticmethod
   def get_description(issue):
     return 'Issue: %d' % issue
 
+  @staticmethod
+  def get_issue_properties(_issue, _messages):
+    return {
+      'reviewers': ['joe@chromium.org', 'john@chromium.org'],
+      'messages': [
+        {
+          'approval': True,
+          'sender': 'john@chromium.org',
+        },
+      ],
+    }
+
 
 class WatchlistsMock(object):
   def __init__(self, _):
@@ -271,7 +284,8 @@ class TestGitCl(TestCase):
       ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''),
       ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''),
       ((['git', 'commit', '-m',
-         'Issue: 12345\n\nReview URL: https://codereview.example.com/12345'],),
+         'Issue: 12345\n\n'
+         'Review URL: https://codereview.example.com/12345'],),
        ''),
       ((['git', 'svn', 'dcommit', '-C50', '--no-rebase', '--rmdir'],),
        (('', None), 0)),
@@ -334,68 +348,68 @@ class TestGitCl(TestCase):
   def test_no_reviewer(self):
     self._run_reviewer_test(
         [],
-        'desc\n\nBUG=\n',
-        '# Blah blah comment.\ndesc\n\nBUG=\n',
-        'desc\n\nBUG=\n',
+        'desc\n\nBUG=',
+        '# Blah blah comment.\ndesc\n\nBUG=',
+        'desc\n\nBUG=',
         [])
 
   def test_keep_similarity(self):
     self._run_reviewer_test(
         ['--similarity', '70'],
-        'desc\n\nBUG=\n',
-        '# Blah blah comment.\ndesc\n\nBUG=\n',
-        'desc\n\nBUG=\n',
+        'desc\n\nBUG=',
+        '# Blah blah comment.\ndesc\n\nBUG=',
+        'desc\n\nBUG=',
         [])
 
   def test_keep_find_copies(self):
     self._run_reviewer_test(
         ['--no-find-copies'],
-        'desc\n\nBUG=\n',
+        'desc\n\nBUG=',
         '# Blah blah comment.\ndesc\n\nBUG=\n',
-        'desc\n\nBUG=\n',
+        'desc\n\nBUG=',
         [])
 
   def test_reviewers_cmd_line(self):
     # Reviewer is passed as-is
-    description = 'desc\n\nR=foo@example.com\nBUG=\n'
+    description = 'desc\n\nR=foo@example.com\nBUG='
     self._run_reviewer_test(
         ['-r' 'foo@example.com'],
         description,
         '\n%s\n' % description,
         description,
-        ['--reviewers', 'foo@example.com'])
+        ['--reviewers=foo@example.com'])
 
   def test_reviewer_tbr_overriden(self):
     # Reviewer is overriden with TBR
     # Also verifies the regexp work without a trailing LF
-    description = 'Foo Bar\nTBR=reviewer@example.com\n'
+    description = 'Foo Bar\n\nTBR=reviewer@example.com'
     self._run_reviewer_test(
         ['-r' 'foo@example.com'],
-        'desc\n\nR=foo@example.com\nBUG=\n',
+        'desc\n\nR=foo@example.com\nBUG=',
         description.strip('\n'),
         description,
-        ['--reviewers', 'reviewer@example.com'])
+        ['--reviewers=reviewer@example.com'])
 
   def test_reviewer_multiple(self):
     # Handles multiple R= or TBR= lines.
     description = (
-        'Foo Bar\nTBR=reviewer@example.com\nBUG=\nR=another@example.com\n')
+        'Foo Bar\nTBR=reviewer@example.com\nBUG=\nR=another@example.com')
     self._run_reviewer_test(
         [],
-        'desc\n\nBUG=\n',
+        'desc\n\nBUG=',
         description,
         description,
-        ['--reviewers', 'reviewer@example.com,another@example.com'])
+        ['--reviewers=another@example.com,reviewer@example.com'])
 
   def test_reviewer_send_mail(self):
     # --send-mail can be used without -r if R= is used
-    description = 'Foo Bar\nR=reviewer@example.com\n'
+    description = 'Foo Bar\nR=reviewer@example.com'
     self._run_reviewer_test(
         ['--send-mail'],
-        'desc\n\nBUG=\n',
+        'desc\n\nBUG=',
         description.strip('\n'),
         description,
-        ['--reviewers', 'reviewer@example.com', '--send_mail'])
+        ['--reviewers=reviewer@example.com', '--send_mail'])
 
   def test_reviewer_send_mail_no_rev(self):
     # Fails without a reviewer.
@@ -490,7 +504,8 @@ class TestGitCl(TestCase):
     receive_pack += '--cc=joe@example.com'  # from watch list
     if reviewers:
       receive_pack += ' '
-      receive_pack += ' '.join(['--reviewer=' + email for email in reviewers])
+      receive_pack += ' '.join(
+          '--reviewer=' + email for email in sorted(reviewers))
     receive_pack += ''
     calls += [
         ((['git', 'push', receive_pack, 'origin', 'HEAD:refs/for/master'],),
@@ -590,6 +605,21 @@ class TestGitCl(TestCase):
         ]
     git_cl.main(['config'])
 
+  def test_update_reviewers(self):
+    data = [
+      ('foo', [], 'foo'),
+      ('foo', ['a@c'], 'foo\n\nR=a@c'),
+      ('foo\nBUG=', ['a@c'], 'foo\nBUG=\nR=a@c'),
+      ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], 'foo\nTBR=a@c'),
+      ('foo', ['a@c', 'b@c'], 'foo\n\nR=a@c, b@c'),
+    ]
+    for orig, reviewers, expected in data:
+      obj = git_cl.ChangeDescription(orig)
+      obj.update_reviewers(reviewers)
+      self.assertEqual(expected, obj.description)
+
 
 if __name__ == '__main__':
+  git_cl.logging.basicConfig(
+      level=git_cl.logging.DEBUG if '-v' in sys.argv else git_cl.logging.ERROR)
   unittest.main()