Refactoring: Split up "commit_changes_if_needed" method.

third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py

 # Copyright 2014 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.
 
 """Fetches a copy of the latest state of a W3C test repository and commits.
 
 If this script is given the argument --auto-update, it will also attempt to
 upload a CL, triggery try jobs, and make any changes that are required for
 new failing tests before committing.
 """
@@ skipping 34 matching lines @@
         log_level = logging.DEBUG if self.verbose else logging.INFO
         logging.basicConfig(level=log_level, format='%(message)s')
 
         if not self.checkout_is_okay(options.allow_local_commits):
             return 1
 
         self.git_cl = GitCL(self.host, auth_refresh_token_json=options.auth_refresh_token_json)
 
         _log.info('Noting the current Chromium commit.')
         _, show_ref_output = self.run(['git', 'show-ref', 'HEAD'])
-        chromium_commitish = show_ref_output.split()[0]
+        chromium_commit = show_ref_output.split()[0]
 
         if options.target == 'wpt':
-            import_commitish = self.update(WPT_DEST_NAME, WPT_REPO_URL, options.keep_w3c_repos_around, options.revision)
+            import_commit = self.update(WPT_DEST_NAME, WPT_REPO_URL, options.keep_w3c_repos_around, options.revision)
             self._copy_resources()
         elif options.target == 'css':
-            import_commitish = self.update(CSS_DEST_NAME, CSS_REPO_URL, options.keep_w3c_repos_around, options.revision)
+            import_commit = self.update(CSS_DEST_NAME, CSS_REPO_URL, options.keep_w3c_repos_around, options.revision)
         else:
             raise AssertionError("Unsupported target %s" % options.target)
 
-        has_changes = self.commit_changes_if_needed(chromium_commitish, import_commitish)
-        if options.auto_update and has_changes:
+        has_changes = self._has_changes()
+        if not has_changes:
+            _log.info('Done: no changes to import.')
+            return 0
+
+        commit_message = self._commit_message(chromium_commit, import_commit)
+        self._commit_changes(commit_message)
+        _log.info('Done: changes imported and committed.')
+
+        if options.auto_update:
             commit_successful = self.do_auto_update()
             if not commit_successful:
                 return 1
         return 0
 
     def parse_args(self, argv):
         parser = argparse.ArgumentParser()
         parser.description = __doc__
         parser.add_argument('-v', '--verbose', action='store_true',
                             help='log what we are doing')
@@ skipping 141 matching lines @@
 
         if not keep_w3c_repos_around:
             _log.info('Deleting temp repo directory %s.', temp_repo_path)
             self.rmtree(temp_repo_path)
 
         _log.info('Updating TestExpectations for any removed or renamed tests.')
         self.update_all_test_expectations_files(self._list_deleted_tests(), self._list_renamed_tests())
 
         return '%s@%s' % (dest_dir_name, master_commitish)
 
-    def commit_changes_if_needed(self, chromium_commitish, import_commitish):
-        if self.run(['git', 'diff', '--quiet', 'HEAD'], exit_on_failure=False)[0]:
-            _log.info('Committing changes.')
-            commit_msg = ('Import %s\n'
-                          '\n'
-                          'Using update-w3c-deps in Chromium %s.\n'
-                          % (import_commitish, chromium_commitish))
-            path_to_commit_msg = self.path_from_webkit_base('commit_msg')
-            _log.debug('cat > %s <<EOF', path_to_commit_msg)
-            _log.debug(commit_msg)
-            _log.debug('EOF')
-            self.fs.write_text_file(path_to_commit_msg, commit_msg)
-            self.run(['git', 'commit', '-a', '-F', path_to_commit_msg])
-            self.remove(path_to_commit_msg)
-            _log.info('Done: changes imported and committed.')
-            return True
-        else:
-            _log.info('Done: no changes to import.')
-            return False
+    def _commit_changes(self, commit_message):
+        _log.info('Committing changes.')
+        path_to_commit_msg = self.path_from_webkit_base('commit_msg')
+        _log.debug('cat > %s <<EOF', path_to_commit_msg)
+        _log.debug(commit_message)
+        _log.debug('EOF')
+        self.fs.write_text_file(path_to_commit_msg, commit_message)
+        self.run(['git', 'commit', '-a', '-F', path_to_commit_msg])

[jeffcarp, 2017/01/18 19:45:39]

Not sure if this is preferable, but you can also pipe the commit message to the
stdin of git commit, e.g.

self.run(['git', 'commit', '-a', '-F', '-'], stdin=commit_message)

Docs:
https://git-scm.com/docs/git-commit#git-commit--Fltfilegt

+        self.remove(path_to_commit_msg)
+
+    def _has_changes(self):
+        return_code, _ = self.run(['git', 'diff', '--quiet', 'HEAD'], exit_on_failure=False)
+        return return_code == 1
+
+    def _commit_message(self, chromium_commit, import_commit):
+        return ('Import %s\n\nUsing update-w3c-deps in Chromium %s.\n\n' %
+                (import_commit, chromium_commit))
 
     @staticmethod
     def is_baseline(basename):
         return basename.endswith('-expected.txt')
 
     def run(self, cmd, exit_on_failure=True, cwd=None):
         _log.debug('Running command: %s', ' '.join(cmd))
 
         cwd = cwd or self.finder.webkit_base()
         proc = self.executive.popen(cmd, stdout=self.executive.PIPE, stderr=self.executive.PIPE, cwd=cwd)
@@ skipping 195 matching lines @@
         """Returns a dict mapping source to dest name for layout tests that have been renamed."""
         out = self.check_run(['git', 'diff', 'origin/master', '-M100%', '--diff-filter=R', '--name-status'])
         renamed_tests = {}
         for line in out.splitlines():
             _, source_path, dest_path = line.split()
             source_test = self.finder.layout_test_name(source_path)
             dest_test = self.finder.layout_test_name(dest_path)
             if source_test and dest_test:
                 renamed_tests[source_test] = dest_test
         return renamed_tests