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

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 416 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_and_update_scm(self, commands):
+    def _run_in_parallel(self, commands, update_scm=True):
         if not commands:
             return {}
 
         command_results = self._tool.executive.run_in_parallel(commands)
         log_output = '\n'.join(result[2] for result in command_results).replace('\n\n', '\n')
         for line in log_output.split('\n'):
             if line:
                 _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.
 
         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_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:
                 lines_to_remove[test] = lines_to_remove[test] + extra_lines_to_remove[test]
             else:
                 lines_to_remove[test] = extra_lines_to_remove[test]
 
         if lines_to_remove:
             self._update_expectations_files(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_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.
 
         Args:
             test: A full test path string.
             build: A Build object.
             existing_suffixes: A collection of all suffixes to consider.
 
         Returns:
@@ skipping 113 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)