Fix case where TBR=foo would remove the -r flag in git-cl

Add unit test!

R=dpranke@chromium.org
BUG=
TEST=


Review URL: http://codereview.chromium.org/8715007

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

git_cl.py

@@ -48,6 +48,10 @@ POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
 DESCRIPTION_BACKUP_FILE = '~/.git_cl_description_backup'
 
 
+# Initialized in main()
+settings = None
+
+
 def DieWithError(message):
   print >> sys.stderr, message
   sys.exit(1)
@@ -151,6 +155,7 @@ class Settings(object):
     self.tree_status_url = None
     self.viewvc_url = None
     self.updated = False
+    self.did_migrate_check = False
 
   def LazyUpdateIfNeeded(self):
     """Updates the settings from a codereview.settings file, if available."""
@@ -272,10 +277,6 @@ class Settings(object):
     return RunGit(['config', param], **kwargs).strip()
 
 
-settings = Settings()
-
-
-did_migrate_check = False
 def CheckForMigration():
   """Migrate from the old issue format, if found.
 
@@ -285,8 +286,7 @@ def CheckForMigration():
   """
 
   # Don't run more than once.
-  global did_migrate_check
-  if did_migrate_check:
+  if settings.did_migrate_check:
     return
 
   gitdir = RunGit(['rev-parse', '--git-dir']).strip()
@@ -300,7 +300,7 @@ def CheckForMigration():
               issue])
     store.close()
     os.remove(storepath)
-  did_migrate_check = True
+  settings.did_migrate_check = True
 
 
 def ShortBranchName(branch):
@@ -642,12 +642,15 @@ class ChangeDescription(object):
 # 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:
+    if ('\nR=' not in self.description and
+        '\nTBR=' not in self.description and
+        self.reviewers):
       initial_text += '\nR=' + self.reviewers
-    if 'BUG=' not in self.description:
+    if '\nBUG=' not in self.description:
       initial_text += '\nBUG='
-    if 'TEST=' not in self.description:
+    if '\nTEST=' not in self.description:
       initial_text += '\nTEST='
+    initial_text = initial_text.rstrip('\n') + '\n'
     content = gclient_utils.RunEditor(initial_text, True)
     if not content:
       DieWithError('Running editor failed')
@@ -657,25 +660,17 @@ class ChangeDescription(object):
     self._ParseDescription(content)
 
   def _ParseDescription(self, description):
+    """Updates the list of reviewers and subject from the 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
+    self.description = description.strip('\n') + '\n'
+    self.subject = description.split('\n', 1)[0]
+    # Retrieves all reviewer lines
+    regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE)
+    self.reviewers = ','.join(
+        i.group(2).strip() for i in regexp.finditer(self.description))
 
   def IsEmpty(self):
     return not self.description
@@ -1376,6 +1371,10 @@ def GenUsage(parser, command):
 def main(argv):
   """Doesn't parse the arguments here, just find the right subcommand to
   execute."""
+  # Reload settings.
+  global settings
+  settings = Settings()
+
   # Do it late so all commands are listed.
   CMDhelp.usage_more = ('\n\nCommands are:\n' + '\n'.join([
       '  %-10s %s' % (fn[3:], Command(fn[3:]).__doc__.split('\n')[0].strip())

tests/git_cl_test.py

@@ -0,0 +1,182 @@
+#!/usr/bin/env python
+# Copyright (c) 2011 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.
+
+"""Unit tests for git_cl.py."""
+
+import os
+import sys
+import unittest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+from testing_support.auto_stub import TestCase
+
+import git_cl
+import subprocess2
+
+
+class TestGitCl(TestCase):
+  def setUp(self):
+    super(TestGitCl, self).setUp()
+    self.calls = []
+    self._calls_done = 0
+    def mock_call(args, **kwargs):
+      expected_args, result = self.calls.pop(0)
+      self.assertEquals(
+          expected_args,
+          args,
+          '@%d  Expected: %r   Actual: %r' % (
+            self._calls_done, expected_args, args))
+      self._calls_done += 1
+      return result
+    self.mock(subprocess2, 'call', mock_call)
+    self.mock(subprocess2, 'check_call', mock_call)
+    self.mock(subprocess2, 'check_output', mock_call)
+    self.mock(subprocess2, 'communicate', mock_call)
+    self.mock(subprocess2, 'Popen', mock_call)
+
+    self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: '')
+
+    class PresubmitMock(object):
+      def __init__(self, *args, **kwargs):
+        self.reviewers = []
+      @staticmethod
+      def should_continue():
+        return True
+    self.mock(git_cl.presubmit_support, 'DoPresubmitChecks', PresubmitMock)
+
+    class RietveldMock(object):
+      def __init__(self, *args, **kwargs):
+        pass
+    self.mock(git_cl.rietveld, 'Rietveld', RietveldMock)
+
+    self.mock(git_cl.upload, 'RealMain', self.fail)
+
+    class WatchlistsMock(object):
+      def __init__(self, _):
+        pass
+      @staticmethod
+      def GetWatchersForPaths(_):
+        return ['joe@example.com']
+    self.mock(git_cl.watchlists, 'Watchlists', WatchlistsMock)
+    # It's important to reset settings to not have inter-tests interference.
+    git_cl.settings = None
+
+  def tearDown(self):
+    if not self.has_failed():
+      self.assertEquals([], self.calls)
+    super(TestGitCl, self).tearDown()
+
+  @staticmethod
+  def _upload_calls():
+    return [
+        (['git', 'update-index', '--refresh', '-q'], ''),
+        (['git', 'diff-index', 'HEAD'], ''),
+        (['git', 'config', 'rietveld.server'], 'codereview.example.com'),
+        (['git', 'symbolic-ref', 'HEAD'], 'master'),
+        (['git', 'config', 'branch.master.merge'], 'master'),
+        (['git', 'config', 'branch.master.remote'], 'origin'),
+        (['git', 'rev-parse', '--show-cdup'], ''),
+        (['git', 'rev-parse', 'HEAD'], '12345'),
+        (['git', 'diff', '--name-status', '-r', 'master...', '.'],
+          'M\t.gitignore\n'),
+        (['git', 'rev-parse', '--git-dir'], '.git'),
+        (['git', 'config', 'branch.master.rietveldissue'], ''),
+        (['git', 'config', 'branch.master.rietveldpatchset'], ''),
+        (['git', 'log', '--pretty=format:%s%n%n%b', 'master...'], 'foo'),
+        (['git', 'config', 'user.email'], 'me@example.com'),
+        (['git', 'diff', '--no-ext-diff', '--stat', '-M', 'master...'], '+dat'),
+        (['git', 'log', '--pretty=format:%s\n\n%b', 'master..'], 'desc\n'),
+        (['git', 'config', 'rietveld.cc'], ''),
+        (['git', 'config', '--get-regexp', '^svn-remote\\.'], (('', None), 0)),
+        (['git', 'rev-parse', '--show-cdup'], ''),
+        (['git', 'svn', 'info'], ''),
+        (['git', 'config', 'branch.master.rietveldissue', '1'], ''),
+        (['git', 'config', 'branch.master.rietveldserver',
+          'http://codereview.example.com'], ''),
+        (['git', 'config', 'branch.master.rietveldpatchset', '2'], ''),
+    ]
+
+  @staticmethod
+  def _cmd_line(description, args):
+    """Returns the upload command line passed to upload.RealMain()."""
+    msg = description.split('\n', 1)[0]
+    return [
+        'upload', '--assume_yes', '--server',
+        'http://codereview.example.com',
+        '--message', msg,
+        '--description', description
+    ] + args + [
+        '--cc', 'joe@example.com',
+        'master...'
+    ]
+
+  def _run_reviewer_test(
+      self,
+      upload_args,
+      expected_description,
+      returned_description,
+      final_description,
+      reviewers):
+    """Generic reviewer test framework."""
+    self.calls = self._upload_calls()
+    def RunEditor(desc, _):
+      self.assertEquals(
+          '# 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' +
+          expected_description,
+          desc)
+      return returned_description
+    self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
+    def check_upload(args):
+      self.assertEquals(self._cmd_line(final_description, reviewers), args)
+      return 1, 2
+    self.mock(git_cl.upload, 'RealMain', check_upload)
+    git_cl.main(['upload'] + upload_args)
+
+  def test_no_reviewer(self):
+    self._run_reviewer_test(
+        [],
+        'desc\n\nBUG=\nTEST=\n',
+        '# Blah blah comment.\ndesc\n\nBUG=\nTEST=\n',
+        'desc\n\nBUG=\nTEST=\n',
+        [])
+
+  def test_reviewers_cmd_line(self):
+    # Reviewer is passed as-is
+    description = 'desc\n\nR=foo@example.com\nBUG=\nTEST=\n'
+    self._run_reviewer_test(
+        ['-r' 'foo@example.com'],
+        description,
+        '\n%s\n' % description,
+        description,
+        ['--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'
+    self._run_reviewer_test(
+        ['-r' 'foo@example.com'],
+        'desc\n\nR=foo@example.com\nBUG=\nTEST=\n',
+        description.strip('\n'),
+        description,
+        ['--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')
+    self._run_reviewer_test(
+        [],
+        'desc\n\nBUG=\nTEST=\n',
+        description,
+        description,
+        ['--reviewers', 'reviewer@example.com,another@example.com'])
+
+
+if __name__ == '__main__':
+  unittest.main()