Fix the duplicate CC issue, hopefully once and for all.

Change-Id: Idd08d28ddeab8b66df1ae9a51d581ceaf62ec984
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4522489
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>

presubmit_support.py

@@ -520,8 +520,7 @@ class OutputApi(object):
 
   def AppendCC(self, cc):
     """Appends a user to cc for this change."""
-    if cc not in self.more_cc:
-      self.more_cc.append(cc)
+    self.more_cc.append(cc)
 
   def PresubmitPromptOrNotify(self, *args, **kwargs):
     """Warn the user when uploading, but only notify if committing."""
@@ -1602,6 +1601,9 @@ class PresubmitExecuter(object):
                                          presubmit_path))
             logging.debug('Running %s done.', function_name)
             self.more_cc.extend(output_api.more_cc)
+            # Clear the CC list between running each presubmit check to prevent
+            # CCs from being repeatedly appended.
+            output_api.more_cc = []
 
         else:  # Old format
           if self.committing:
@@ -1615,11 +1617,16 @@ class PresubmitExecuter(object):
                                          presubmit_path))
             logging.debug('Running %s done.', function_name)
             self.more_cc.extend(output_api.more_cc)
+            # Clear the CC list between running each presubmit check to prevent
+            # CCs from being repeatedly appended.
+            output_api.more_cc = []
 
     finally:
       for f in input_api._named_temporary_files:
         os.remove(f)
 
+    self.more_cc = sorted(set(self.more_cc))
+
     return results
 
   def _run_check_function(self, function_name, context, sink, presubmit_path):

tests/presubmit_unittest.py

@@ -1596,6 +1596,65 @@ class OutputApiUnittest(PresubmitTestsBase):
     self.assertEqual(['chromium-reviews@chromium.org'], output_api.more_cc)
 
 
+  def testAppendCCAndMultipleChecks(self):
+    description_lines = ('Hello there', 'this is a change', 'BUG=123')
+    files = [
+        ['A', 'foo\\blat.cc'],
+    ]
+    fake_presubmit = os.path.join(self.fake_root_dir, 'PRESUBMIT.py')
+
+    change = presubmit.Change('mychange', '\n'.join(description_lines),
+                              self.fake_root_dir, files, 0, 0, None)
+
+    # Base case: AppendCC from multiple different checks should be reflected in
+    # the final result.
+    executer = presubmit.PresubmitExecuter(change, True, None,
+                                           presubmit.GerritAccessor())
+    self.assertFalse(
+        executer.ExecPresubmitScript(
+            "PRESUBMIT_VERSION = '2.0.0'\n"
+            'def CheckChangeAddCC1(input_api, output_api):\n'
+            "  output_api.AppendCC('chromium-reviews@chromium.org')\n"
+            '  return []\n'
+            '\n'
+            'def CheckChangeAppendCC2(input_api, output_api):\n'
+            "  output_api.AppendCC('ipc-security-reviews@chromium.org')\n"
+            '  return []\n', fake_presubmit))
+    self.assertEqual(
+        ['chromium-reviews@chromium.org', 'ipc-security-reviews@chromium.org'],
+        executer.more_cc)
+
+    # Check that if one presubmit check appends a CC, it does not get duplicated
+    # into the more CC list by subsequent checks.
+    executer = presubmit.PresubmitExecuter(change, True, None,
+                                           presubmit.GerritAccessor())
+    self.assertFalse(
+        executer.ExecPresubmitScript(
+            "PRESUBMIT_VERSION = '2.0.0'\n"
+            'def CheckChangeAddCC(input_api, output_api):\n'
+            "  output_api.AppendCC('chromium-reviews@chromium.org')\n"
+            '  return []\n'
+            '\n'
+            'def CheckChangeDoNothing(input_api, output_api):\n'
+            '  return []\n', fake_presubmit))
+    self.assertEqual(['chromium-reviews@chromium.org'], executer.more_cc)
+
+    # Check that if multiple presubmit checks append the same CC, it gets
+    # deduplicated.
+    executer = presubmit.PresubmitExecuter(change, True, None,
+                                           presubmit.GerritAccessor())
+    self.assertFalse(
+        executer.ExecPresubmitScript(
+            "PRESUBMIT_VERSION = '2.0.0'\n"
+            'def CheckChangeAddCC1(input_api, output_api):\n'
+            "  output_api.AppendCC('chromium-reviews@chromium.org')\n"
+            '  return []\n'
+            '\n'
+            'def CheckChangeAppendCC2(input_api, output_api):\n'
+            "  output_api.AppendCC('chromium-reviews@chromium.org')\n"
+            '  return []\n', fake_presubmit))
+    self.assertEqual(['chromium-reviews@chromium.org'], executer.more_cc)
+
   def testOutputApiHandling(self):
     presubmit.OutputApi.PresubmitError('!!!').handle()
     self.assertIsNotNone(sys.stdout.getvalue().count('!!!'))