Refactor source_control.py and add a test.

tools/auto_bisect/source_control.py

Index: tools/auto_bisect/source_control.py
diff --git a/tools/auto_bisect/source_control.py b/tools/auto_bisect/source_control.py
index 10b978074c225685e0686cae354f86d646b4461a..766293724db660fc39fdbd5d94630676ae974608 100644
--- a/tools/auto_bisect/source_control.py
+++ b/tools/auto_bisect/source_control.py
@@ -2,7 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""This module contains the SourceControl class and related functions."""
+"""This module contains functions for performing source control operations."""
 
 import os
 
@@ -11,284 +11,263 @@ import bisect_utils
 CROS_VERSION_PATTERN = 'new version number from %s'
 
 
-def DetermineAndCreateSourceControl(opts):
-  """Attempts to determine the underlying source control workflow and returns
-  a SourceControl object.
+def IsInGitRepository():
+  output, _ = bisect_utils.RunGit(['rev-parse', '--is-inside-work-tree'])
+  return output.strip() == 'true'

[qyearsley, 2014/10/14 17:48:34]

This function is used to make the same check that was made when creating a
SourceControl object. Now in the main() method of bisect_perf_regression.py we
can check this before starting.

+
+
+def SyncToRevisionWithGClient(revision):
+  """Uses gclient to sync to the specified revision.
+
+  This is like running gclient sync --revision <revision>.
+
+  Args:
+    revision: A git SHA1 hash or SVN revision number (depending on workflow).
 
   Returns:
-    An instance of a SourceControl object, or None if the current workflow
-    is unsupported.
+    The return code of the call.
   """
-  (output, _) = bisect_utils.RunGit(['rev-parse', '--is-inside-work-tree'])
+  return bisect_utils.RunGClient(
+      ['sync', '--verbose', '--reset', '--force',
+      '--delete_unversioned_trees', '--nohooks', '--revision', revision])

[qyearsley, 2014/10/14 17:48:34]

Sorry that the alignment in this diff is off -- most of these functions are
unchanged from how they were originally. SyncToRevisoinWithGClient starts on
line 39 in the "before" version.

 
-  if output.strip() == 'true':
-    return GitSourceControl(opts)
 
-  return None
+def SyncToRevisionWithRepo(timestamp):
+  return bisect_utils.RunRepoSyncAtTimestamp(timestamp)
 
 
-# TODO(qyearsley): Almost all of the methods below could be top-level functions
-# (or class methods). Refactoring may make this simpler.
-# pylint: disable=R0201
-class SourceControl(object):
-  """SourceControl is an abstraction over the source control system."""
+def GetRevisionList(end_revision_hash, start_revision_hash, cwd=None):
+  """Retrieves a list of git commit hashes in a range.
 
-  def __init__(self):
-    super(SourceControl, self).__init__()
+  Args:
+    end_revision_hash: The SHA1 for the end of the range, inclusive.
+    start_revision_hash: The SHA1 for the beginning of the range, inclusive.
 
-  def SyncToRevisionWithGClient(self, revision):
-    """Uses gclient to sync to the specified revision.
+  Returns:
+    A list of the git commit hashes in the range, in reverse time order --
+    that is, starting with |end_revision_hash|.
+  """
+  revision_range = '%s..%s' % (start_revision_hash, end_revision_hash)
+  cmd = ['log', '--format=%H', '-10000', '--first-parent', revision_range]
+  log_output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
 
-    This is like running gclient sync --revision <revision>.
+  revision_hash_list = log_output.split()
+  revision_hash_list.append(start_revision_hash)
 
-    Args:
-      revision: A git SHA1 hash or SVN revision number (depending on workflow).
+  return revision_hash_list
 
-    Returns:
-      The return code of the call.
-    """
-    return bisect_utils.RunGClient(['sync', '--verbose', '--reset', '--force',
-        '--delete_unversioned_trees', '--nohooks', '--revision', revision])
 
-  def SyncToRevisionWithRepo(self, timestamp):
-    """Uses the repo command to sync all the underlying git depots to the
-    specified time.
+def SyncToRevision(revision, sync_client=None):
+  """Syncs to the specified revision.

[ojan, 2014/10/14 22:59:09]

IMO, docstring comments like this should be removed. They don't tell me useful
information. They just say what the code obviously does and get out of sync with
the code (e.g. in this one, the argument name is wrong).

[qyearsley, 2014/10/14 23:14:55]

Ah, you're right. Docstring removed.

In some other cases, the first sentence of the docstring doesn't add much, but
the other parts (details about the arguments and return value) do explain
something. In that case, would you still add a first line?


Note, here's what the Google python style guide says on the subject:

A function must have a docstring, unless it meets all of the following criteria:
 * not externally visible
 * very short
 * obvious
A docstring should give enough information to write a call to the function
without reading the function's code...

https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme...

 
-    Args:
-      timestamp: The Unix timestamp to sync to.
+  Args:
+    revision: The revision to sync to.
+    use_gclient: Specifies whether or not we should sync using gclient or
+      just use source control directly.
 
-    Returns:
-      The return code of the call.
-    """
-    return bisect_utils.RunRepoSyncAtTimestamp(timestamp)
+  Returns:
+    True if successful.
+  """
+  if not sync_client:
+    _, return_code = bisect_utils.RunGit(['checkout', revision])[1]
+  elif sync_client == 'gclient':
+    return_code = SyncToRevisionWithGClient(revision)
+  elif sync_client == 'repo':
+    return_code = SyncToRevisionWithRepo(revision)
+  else:
+    raise NotImplementedError('Unsupported sync_client: "%s"' % sync_client)
+
+  return not return_code
+
+
+def ResolveToRevision(revision_to_check, depot, depot_deps_dict,
+                      search, cwd=None):

[qyearsley, 2014/10/14 17:48:34]

This is the only function in here that I made a non-trivial change to. I split
the CrOS-related part out and added a comment.

+  """Tries to resolve an SVN revision or commit position to a git SHA1.
+
+  Args:
+    revision_to_check: The user supplied revision string that may need to be
+        resolved to a git commit hash. This may be an SVN revision, git commit
+        position, or a git commit hash.
+    depot: The depot (dependency repository) that |revision_to_check| is from.
+    depot_deps_dict: A dictionary with information about different depots.
+    search: How many revisions forward or backward to search. If the value is
+        negative, the function will search backwards chronologically, otherwise
+        it will search forward.
 
+  Returns:
+    A string containing a git SHA1 hash, otherwise None.
+  """
+  # Android-chrome is git only, so no need to resolve this to anything else.
+  if depot == 'android-chrome':
+    return revision_to_check
+
+  if depot == 'cros':
+    return ResolveToRevisionCrOS(revision_to_check, cwd)
+
+  # If the given revision can't be parsed as an integer, then it may already
+  # be a git commit hash.
+  if not bisect_utils.IsStringInt(revision_to_check):
+    return revision_to_check
+
+  depot_svn = 'svn://svn.chromium.org/chrome/trunk/src'
+
+  if depot != 'chromium':
+    depot_svn = depot_deps_dict[depot]['svn']
+  svn_revision = int(revision_to_check)
+  git_revision = None
+
+  if search > 0:
+    search_range = xrange(svn_revision, svn_revision + search, 1)
+  else:
+    search_range = xrange(svn_revision, svn_revision + search, -1)
+
+  for i in search_range:
+    # NOTE: Checking for the git-svn-id footer is for backwards compatibility.
+    # When we can assume that all the revisions we care about are from after
+    # git commit positions started getting added, we don't need to check this.
+    svn_pattern = 'git-svn-id: %s@%d' % (depot_svn, i)
+    commit_position_pattern = '^Cr-Commit-Position: .*@{#%d}' % i
+    cmd = ['log', '--format=%H', '-1', '--grep', svn_pattern,
+           '--grep', commit_position_pattern, 'origin/master']
+    log_output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
+    log_output = log_output.strip()
 
-class GitSourceControl(SourceControl):
-  """GitSourceControl is used to query the underlying source control."""
+    if log_output:
+      git_revision = log_output
+      break
 
-  def __init__(self, opts):
-    super(GitSourceControl, self).__init__()
-    self.opts = opts
+  return git_revision
 
-  def IsGit(self):
-    return True
 
-  def GetRevisionList(self, revision_range_end, revision_range_start, cwd=None):
-    """Retrieves a list of revisions between |revision_range_start| and
-    |revision_range_end|.
+def ResolveToRevisionCrOS(revision_to_check, cwd=None):
+  """Return a git commit hash corresponding to the give version or revision.
 
-    Args:
-      revision_range_end: The SHA1 for the end of the range.
-      revision_range_start: The SHA1 for the beginning of the range.
+  TODO(qyearsley): Either verify that this works or delete it.
+  """
+  if bisect_utils.IsStringInt(revision_to_check):
+    return int(revision_to_check)
 
-    Returns:
-      A list of the revisions between |revision_range_start| and
-      |revision_range_end| (inclusive).
-    """
-    revision_range = '%s..%s' % (revision_range_start, revision_range_end)
-    cmd = ['log', '--format=%H', '-10000', '--first-parent', revision_range]
-    log_output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
+  cwd = os.getcwd()
+  os.chdir(os.path.join(os.getcwd(), 'src', 'third_party',
+                        'chromiumos-overlay'))
+  pattern = CROS_VERSION_PATTERN % revision_to_check
+  cmd = ['log', '--format=%ct', '-1', '--grep', pattern]
 
-    revision_hash_list = log_output.split()
-    revision_hash_list.append(revision_range_start)
+  git_revision = None
 
-    return revision_hash_list
+  log_output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
+  if log_output:
+    git_revision = log_output
+    git_revision = int(log_output.strip())
+  os.chdir(cwd)
 
-  def SyncToRevision(self, revision, sync_client=None):
-    """Syncs to the specified revision.
+  return git_revision
 
-    Args:
-      revision: The revision to sync to.
-      use_gclient: Specifies whether or not we should sync using gclient or
-        just use source control directly.
-
-    Returns:
-      True if successful.
-    """
-
-    if not sync_client:
-      results = bisect_utils.RunGit(['checkout', revision])[1]
-    elif sync_client == 'gclient':
-      results = self.SyncToRevisionWithGClient(revision)
-    elif sync_client == 'repo':
-      results = self.SyncToRevisionWithRepo(revision)
-
-    return not results
-
-  def ResolveToRevision(self, revision_to_check, depot, depot_deps_dict,
-                        search, cwd=None):
-    """Tries to resolve an SVN revision or commit position to a git SHA1.
-
-    Args:
-      revision_to_check: The user supplied revision string that may need to be
-        resolved to a git SHA1.
-      depot: The depot the revision_to_check is from.
-      depot_deps_dict: A dictionary with information about different depots.
-      search: The number of changelists to try if the first fails to resolve
-        to a git hash. If the value is negative, the function will search
-        backwards chronologically, otherwise it will search forward.
-
-    Returns:
-      A string containing a git SHA1 hash, otherwise None.
-    """
-    # Android-chrome is git only, so no need to resolve this to anything else.
-    if depot == 'android-chrome':
-      return revision_to_check
-
-    if depot != 'cros':
-      if not bisect_utils.IsStringInt(revision_to_check):
-        return revision_to_check
-
-      depot_svn = 'svn://svn.chromium.org/chrome/trunk/src'
-
-      if depot != 'chromium':
-        depot_svn = depot_deps_dict[depot]['svn']
-
-      svn_revision = int(revision_to_check)
-      git_revision = None
-
-      if search > 0:
-        search_range = xrange(svn_revision, svn_revision + search, 1)
-      else:
-        search_range = xrange(svn_revision, svn_revision + search, -1)
-
-      for i in search_range:
-        svn_pattern = 'git-svn-id: %s@%d' % (depot_svn, i)
-        commit_position_pattern = '^Cr-Commit-Position: .*@{#%d}' % i
-        cmd = ['log', '--format=%H', '-1', '--grep', svn_pattern,
-               '--grep', commit_position_pattern, 'origin/master']
-
-        (log_output, return_code) = bisect_utils.RunGit(cmd, cwd=cwd)
-
-        assert not return_code, 'An error occurred while running'\
-                                ' "git %s"' % ' '.join(cmd)
-
-        if not return_code:
-          log_output = log_output.strip()
-
-          if log_output:
-            git_revision = log_output
-
-            break
-
-      return git_revision
-    else:
-      if bisect_utils.IsStringInt(revision_to_check):
-        return int(revision_to_check)
-      else:
-        cwd = os.getcwd()
-        os.chdir(os.path.join(os.getcwd(), 'src', 'third_party',
-            'chromiumos-overlay'))
-        pattern = CROS_VERSION_PATTERN % revision_to_check
-        cmd = ['log', '--format=%ct', '-1', '--grep', pattern]
-
-        git_revision = None
-
-        log_output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
-        if log_output:
-          git_revision = log_output
-          git_revision = int(log_output.strip())
-        os.chdir(cwd)
-
-        return git_revision
-
-  def IsInProperBranch(self):
-    """Confirms they're in the master branch for performing the bisection.
-    This is needed or gclient will fail to sync properly.
-
-    Returns:
-      True if the current branch on src is 'master'
-    """
-    cmd = ['rev-parse', '--abbrev-ref', 'HEAD']
-    log_output = bisect_utils.CheckRunGit(cmd)
-    log_output = log_output.strip()
 
-    return log_output == "master"
+def IsInProperBranch():
+  """Checks whether the current branch is "master"."""
+  cmd = ['rev-parse', '--abbrev-ref', 'HEAD']
+  log_output = bisect_utils.CheckRunGit(cmd)
+  log_output = log_output.strip()
+  return log_output == 'master'
 
-  def GetCommitPosition(self, git_revision, cwd=None):
-    """Finds git commit postion for the given git hash.
 
-    This function executes "git footer --position-num <git hash>" command to get
-    commit position the given revision.
+def GetCommitPosition(git_revision, cwd=None):
+  """Finds git commit postion for the given git hash.
 
-    Args:
-      git_revision: The git SHA1 to use.
-      cwd: Working directory to run the command from.
+  This function executes "git footer --position-num <git hash>" command to get
+  commit position the given revision.
 
-    Returns:
-      Git commit position as integer or None.
-    """
-    cmd = ['footers', '--position-num', git_revision]
-    output = bisect_utils.CheckRunGit(cmd, cwd)
-    commit_position = output.strip()
+  Args:
+    git_revision: The git SHA1 to use.
+    cwd: Working directory to run the command from.
 
-    if bisect_utils.IsStringInt(commit_position):
-      return int(commit_position)
+  Returns:
+    Git commit position as integer or None.
+  """
+  cmd = ['footers', '--position-num', git_revision]
+  output = bisect_utils.CheckRunGit(cmd, cwd)
+  commit_position = output.strip()
 
-    return None
+  if bisect_utils.IsStringInt(commit_position):
+    return int(commit_position)
 
-  def QueryRevisionInfo(self, revision, cwd=None):
-    """Gathers information on a particular revision, such as author's name,
-    email, subject, and date.
+  return None
 
-    Args:
-      revision: Revision you want to gather information on.
 
-    Returns:
-      A dict in the following format:
-      {
-        'author': %s,
-        'email': %s,
-        'date': %s,
-        'subject': %s,
-        'body': %s,
-      }
-    """
-    commit_info = {}
+def QueryRevisionInfo(revision, cwd=None):
+  """Gathers information on a particular revision, such as author's name,
+  email, subject, and date.
 
-    formats = ['%aN', '%aE', '%s', '%cD', '%b']
-    targets = ['author', 'email', 'subject', 'date', 'body']
+  Args:
+    revision: Revision you want to gather information on; a git commit hash.
 
-    for i in xrange(len(formats)):
-      cmd = ['log', '--format=%s' % formats[i], '-1', revision]
-      output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
-      commit_info[targets[i]] = output.rstrip()
+  Returns:
+    A dict in the following format:
+    {
+      'author': %s,
+      'email': %s,
+      'date': %s,
+      'subject': %s,
+      'body': %s,
+    }
+  """
+  commit_info = {}
 
-    return commit_info
+  formats = ['%aN', '%aE', '%s', '%cD', '%b']
+  targets = ['author', 'email', 'subject', 'date', 'body']
 
-  def CheckoutFileAtRevision(self, file_name, revision, cwd=None):
-    """Performs a checkout on a file at the given revision.
+  for i in xrange(len(formats)):
+    cmd = ['log', '--format=%s' % formats[i], '-1', revision]
+    output = bisect_utils.CheckRunGit(cmd, cwd=cwd)
+    commit_info[targets[i]] = output.rstrip()
 
-    Returns:
-      True if successful.
-    """
-    return not bisect_utils.RunGit(
-        ['checkout', revision, file_name], cwd=cwd)[1]
+  return commit_info
 
-  def RevertFileToHead(self, file_name):
-    """Un-stages a file and resets the file's state to HEAD.
 
-    Returns:
-      True if successful.
-    """
-    # Reset doesn't seem to return 0 on success.
-    bisect_utils.RunGit(['reset', 'HEAD', file_name])
+def CheckoutFileAtRevision(file_name, revision, cwd=None):
+  """Performs a checkout on a file at the given revision.
 
-    return not bisect_utils.RunGit(['checkout', bisect_utils.FILE_DEPS_GIT])[1]
+  Returns:
+    True if successful.
+  """
+  command = ['checkout', revision, file_name]
+  _, return_code = bisect_utils.RunGit(command, cwd=cwd)
+  return not return_code
 
-  def QueryFileRevisionHistory(self, filename, revision_start, revision_end):
-    """Returns a list of commits that modified this file.
 
-    Args:
-        filename: Name of file.
-        revision_start: Start of revision range.
-        revision_end: End of revision range.
+def RevertFileToHead(file_name):
+  """Un-stages a file and resets the file's state to HEAD.
 
-    Returns:
-        Returns a list of commits that touched this file.
-    """
-    cmd = ['log', '--format=%H', '%s~1..%s' % (revision_start, revision_end),
-           '--', filename]
-    output = bisect_utils.CheckRunGit(cmd)
+  Returns:
+    True if successful.
+  """
+  # Reset doesn't seem to return 0 on success.
+  bisect_utils.RunGit(['reset', 'HEAD', file_name])
+  _, return_code = bisect_utils.RunGit(
+      ['checkout', bisect_utils.FILE_DEPS_GIT])
+  return not return_code
+
+
+def QueryFileRevisionHistory(filename, revision_start, revision_end):
+  """Returns a list of commits that modified this file.
+
+  Args:
+    filename: Name of file.
+    revision_start: Start of revision range (inclusive).
+    revision_end: End of revision range.
+
+  Returns:
+    Returns a list of commits that touched this file.
+  """
+  cmd = [
+      'log',
+      '--format=%H',
+      '%s~1..%s' % (revision_start, revision_end),
+      '--',
+      filename,
+  ]
+  output = bisect_utils.CheckRunGit(cmd)
+  lines = output.split('\n')
+  return [o for o in lines if o]
 
-    return [o for o in output.split('\n') if o]