Reland of "update SCM and expectations all at once after all commands".

third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py

 # Copyright (c) 2010 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 69 matching lines @@
     The reason for keeping track of these changes together is that after rebaselining
     many tests in parallel by subprocesses, we can update the git staged files list
     and the TestExpectations all at once at the end.
 
     Each subprocess can communicate some set of changes by outputting JSON lines,
     and another process can aggregate the changes.
 
     TODO(qyearsley): Refactor this so that lines to remove are only tracked in one format.
     """
     def __init__(self, files_to_add=None, files_to_delete=None, lines_to_remove=None):
-        self.files_to_add = files_to_add or []
-        self.files_to_delete = files_to_delete or []
-        self.lines_to_remove = lines_to_remove or {}
+        self._files_to_add = set(files_to_add or [])
+        self._files_to_delete = set(files_to_delete or [])
+        lines_to_remove = lines_to_remove or {}
+        self._lines_to_remove = {t: set(builders) for t, builders in lines_to_remove.iteritems()}
 
     def add_file(self, path):
-        self.files_to_add.append(path)
+        self._files_to_add.add(path)
 
     def delete_file(self, path):
-        self.files_to_delete.append(path)
+        self._files_to_delete.add(path)
 
     def remove_line(self, test, builder):
-        if test not in self.lines_to_remove:
-            self.lines_to_remove[test] = []
-        self.lines_to_remove[test].append(builder)
+        if test not in self._lines_to_remove:
+            self._lines_to_remove[test] = set()
+        self._lines_to_remove[test].add(builder)
+
+    @property
+    def files_to_add(self):
+        return sorted(self._files_to_add - self._files_to_delete)
+
+    @property
+    def files_to_delete(self):
+        return sorted(self._files_to_delete - self._files_to_add)
+
+    @property
+    def lines_to_remove(self):
+        return {t: sorted(set(builders)) for t, builders in sorted(self._lines_to_remove.iteritems())}
 
     def to_dict(self):
         remove_lines = []
-        for test in self.lines_to_remove:
-            for builder in self.lines_to_remove[test]:
+        for test in sorted(self.lines_to_remove):
+            for builder in sorted(set(self.lines_to_remove[test])):
                 remove_lines.append({'test': test, 'builder': builder})
         return {
-            'add': list(self.files_to_add),
-            'delete': list(self.files_to_delete),
+            'add': self.files_to_add,
+            'delete': self.files_to_delete,
             'remove-lines': remove_lines,
         }
 
     @staticmethod
     def from_dict(change_dict):
         files_to_add = set()
         files_to_delete = set()
         lines_to_remove = {}
         if 'add' in change_dict:
             files_to_add.update(change_dict['add'])
         if 'delete' in change_dict:
             files_to_delete.update(change_dict['delete'])
         if 'remove-lines' in change_dict:
             for line_to_remove in change_dict['remove-lines']:
                 test = line_to_remove['test']
                 builder = line_to_remove['builder']
                 if test not in lines_to_remove:
                     lines_to_remove[test] = []
                 lines_to_remove[test].append(builder)
         return ChangeSet(
-            files_to_add=list(files_to_add),
-            files_to_delete=list(files_to_delete),
+            files_to_add=files_to_add,
+            files_to_delete=files_to_delete,
             lines_to_remove=lines_to_remove)
 
     def update(self, other):
         assert isinstance(other, ChangeSet)
-        assert type(other.lines_to_remove) is dict
-        self.files_to_add.extend(other.files_to_add)
-        self.files_to_delete.extend(other.files_to_delete)
+        self._files_to_add.update(other.files_to_add)
+        self._files_to_delete.update(other.files_to_delete)
         for test in other.lines_to_remove:
-            if test not in self.lines_to_remove:
-                self.lines_to_remove[test] = []
-            self.lines_to_remove[test].extend(other.lines_to_remove[test])
+            if test not in self._lines_to_remove:
+                self._lines_to_remove[test] = set()
+            self._lines_to_remove[test].update(other.lines_to_remove[test])
 
 
 class BaseInternalRebaselineCommand(AbstractRebaseliningCommand):
     """Base class for rebaseline-related commands that are intended to be used by other commands."""
     # Not overriding execute() - pylint: disable=abstract-method
 
     def __init__(self):
         super(BaseInternalRebaselineCommand, self).__init__(options=[
             self.results_directory_option,
             self.suffixes_option,
@@ skipping 324 matching lines @@
     def _port_skips_test(port, test, generic_expectations, full_expectations):
         fs = port.host.filesystem
         if port.default_smoke_test_only():
             smoke_test_filename = fs.join(port.layout_tests_dir(), 'SmokeTests')
             if fs.exists(smoke_test_filename) and test not in fs.read_text_file(smoke_test_filename):
                 return True
 
         return (SKIP in full_expectations.get_expectations(test) and
                 SKIP not in generic_expectations.get_expectations(test))
 
-    def _run_in_parallel(self, commands, update_scm=True):
+    def _run_in_parallel(self, commands):
         if not commands:
-            return {}
+            return ChangeSet()
 
         command_results = self._tool.executive.run_in_parallel(commands)
         for _, _, stderr in command_results:
             if stderr:
                 _log.error(stderr)
 
-        change_set = self._extract_scm_changes(command_results)
+        return self._extract_scm_changes(command_results)
 
-        # TODO(qyearsley): Instead of updating the SCM state here, aggregate changes
-        # and update once in _rebaseline. See http://crbug.com/639410.
-        if update_scm:
-            if change_set.files_to_delete:
-                self._tool.scm().delete_list(change_set.files_to_delete)
-            if change_set.files_to_add:
-                self._tool.scm().add_list(change_set.files_to_add)
-
-        return change_set.lines_to_remove
-
-    def _rebaseline(self, options, test_prefix_list, update_scm=True):
+    def _rebaseline(self, options, test_prefix_list):
         """Downloads new baselines in parallel, then updates expectations files
         and optimizes baselines.
 
         Args:
             options: An object with the options passed to the current command.
             test_prefix_list: A map of test names to Build objects to file suffixes
                 for new baselines. For example:
                 {
                     "some/test.html": {Build("builder-1", 412): ["txt"], Build("builder-2", 100): ["txt"]},
                     "some/other.html": {Build("builder-1", 412): ["txt"]}
                 }
                 This would mean that new text baselines should be downloaded for
                 "some/test.html" on both builder-1 (build 412) and builder-2
                 (build 100), and new text baselines should be downloaded for
                 "some/other.html" but only from builder-1.
                 TODO(qyearsley): Replace test_prefix_list everywhere with some
                 sort of class that contains the same data.
-            update_scm: If True, commands like `git add` and `git rm` will be run.
         """
         for test, builds_to_check in sorted(test_prefix_list.items()):
             _log.info("Rebaselining %s", test)
             for build, suffixes in sorted(builds_to_check.items()):
                 _log.debug("  %s: %s", build, ",".join(suffixes))
 
         copy_baseline_commands, rebaseline_commands, extra_lines_to_remove = self._rebaseline_commands(
             test_prefix_list, options)
-        lines_to_remove = {}
 
-        self._run_in_parallel(copy_baseline_commands, update_scm=update_scm)
-        lines_to_remove = self._run_in_parallel(rebaseline_commands, update_scm=update_scm)
+        change_set = ChangeSet(lines_to_remove=extra_lines_to_remove)
 
-        for test in extra_lines_to_remove:
-            if test in lines_to_remove:
-                lines_to_remove[test] = lines_to_remove[test] + extra_lines_to_remove[test]
-            else:
-                lines_to_remove[test] = extra_lines_to_remove[test]
+        change_set.update(self._run_in_parallel(copy_baseline_commands))
+        change_set.update(self._run_in_parallel(rebaseline_commands))
 
-        if lines_to_remove:
-            self._update_expectations_files(lines_to_remove)
+        if change_set.lines_to_remove:
+            self._update_expectations_files(change_set.lines_to_remove)
 
         if options.optimize:
             # TODO(wkorman): Consider changing temporary branch to base off of HEAD rather than
             # origin/master to ensure we run baseline optimization processes with the same code as
             # auto-rebaseline itself.
-            self._run_in_parallel(self._optimize_baselines(test_prefix_list, options.verbose), update_scm=update_scm)
+            change_set.update(self._run_in_parallel(self._optimize_baselines(test_prefix_list, options.verbose)))
+
+        if change_set.files_to_delete:
+            self._tool.scm().delete_list(change_set.files_to_delete)
+        if change_set.files_to_add:
+            self._tool.scm().add_list(change_set.files_to_add)
 
     def _suffixes_for_actual_failures(self, test, build, existing_suffixes):
         """Gets the baseline suffixes for actual mismatch failures in some results.
 
         Args:
             test: A full test path string.
             build: A Build object.
             existing_suffixes: A collection of all suffixes to consider.
 
         Returns:
@@ skipping 116 matching lines @@
             for test in args:
                 if test not in test_prefix_list:
                     test_prefix_list[test] = {}
                 build = Build(builder)
                 test_prefix_list[test][build] = suffixes_to_update
 
         if options.verbose:
             _log.debug("rebaseline-json: " + str(test_prefix_list))
 
         self._rebaseline(options, test_prefix_list)