Don't run git add/rm after rebaseline commands for rebaseline-cl.

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
index 6dc139f0fb73204374e29c399068211fb9127799..35b488063df29792fd2b5a3af4223492bb46afc0 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
@@ -434,7 +434,7 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
         return (SKIP in full_expectations.get_expectations(test) and
                 SKIP not in generic_expectations.get_expectations(test))
 
-    def _run_in_parallel_and_update_scm(self, commands):
+    def _run_in_parallel(self, commands, update_scm=True):
         if not commands:
             return {}
 
@@ -445,13 +445,16 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
                 _log.error(line)
 
         files_to_add, files_to_delete, lines_to_remove = self._serial_commands(command_results)
-        if files_to_delete:
-            self._tool.scm().delete_list(files_to_delete)
-        if files_to_add:
-            self._tool.scm().add_list(files_to_add)
+        # TODO(qyearsley): Consider removing this if possible,
+        # or making it work and produce reasonable results for rebaseline-cl.

[qyearsley, 2016/08/19 22:45:46]

Some thoughts: maybe if all file additions and deletions can be kept in a list
for the whole operation, including all commands, and then scm.delete_list /
scm.add_list called once at the end of _rebaseline, then that would be best?

It seems like in general it may be unnecessary to do explicitly stage file
additions/deletions, but I think that auto-rebaseline might rely on it, since it
commits after calling _rebaseline().

[wkorman, 2016/08/19 22:54:07]

Yeah, auto-rebaseline has to commit so that it can upload the CL, right? So we
need to at least preserve this behavior until we turn that down.

It does seem like if we're planning to add a file, we shouldn't bother deleting
it. So I'd be supportive of that change unless we think of something it would
break.

+        if update_scm:

[wkorman, 2016/08/19 22:54:07]

How much of a pain to add unit test to make sure this new param does what we
intend?

[qyearsley, 2016/08/19 23:32:27]

Hmm, maybe not too much. Possible test methods could:
 - Run rebaseline-cl to rebaseline a test (with mock data all set up) and verify
that tool.scm().added_paths is empty.
 - Run another rebaseline command and verify that tool.scm().added_paths has the
right files. Probably will want to modify MockSCM so that deleted_paths can be
verified too.

Although, on the other hand, I did try this out once manually by running
webkit-patch rebaseline-cl, so at least in that instance I verified that it
appears to do what I want it to.

Maybe not worth it to add tests now if (1) it can be tested manually for each
new CL and (2) this behavior is likely to change soon.

[wkorman, 2016/08/20 00:58:29]

Am ok with waiting on test as we hope to turn auto-rebaseline down soon.

+            if files_to_delete:
+                self._tool.scm().delete_list(files_to_delete)
+            if files_to_add:
+                self._tool.scm().add_list(files_to_add)
         return lines_to_remove
 
-    def _rebaseline(self, options, test_prefix_list):
+    def _rebaseline(self, options, test_prefix_list, update_scm=True):
         """Downloads new baselines in parallel, then updates expectations files
         and optimizes baselines.
 
@@ -469,6 +472,7 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
                 "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)
@@ -479,8 +483,8 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
             test_prefix_list, options)
         lines_to_remove = {}
 
-        self._run_in_parallel_and_update_scm(copy_baseline_commands)
-        lines_to_remove = self._run_in_parallel_and_update_scm(rebaseline_commands)
+        self._run_in_parallel(copy_baseline_commands, update_scm=update_scm)
+        lines_to_remove = self._run_in_parallel(rebaseline_commands, update_scm=update_scm)
 
         for test in extra_lines_to_remove:
             if test in lines_to_remove:
@@ -495,7 +499,7 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
             # 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_and_update_scm(self._optimize_baselines(test_prefix_list, options.verbose))
+            self._run_in_parallel(self._optimize_baselines(test_prefix_list, options.verbose), update_scm=update_scm)
 
     def _suffixes_for_actual_failures(self, test, build, existing_suffixes):
         """Gets the baseline suffixes for actual mismatch failures in some results.