Remove gclient-specific hacks from trychange into gcl.

Remove upload dependency.

This is towards making trychange.py work mostly standalone for the webkit try server.

TEST=unit tests and tested both gcl try and git-try.

Review URL: http://codereview.chromium.org/501143

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@35071 0039d316-1c4b-4281-b951-d872f2087c98

gcl.py

@@ -24,7 +24,7 @@ import breakpad
 from scm import SVN
 import gclient_utils
 
-__version__ = '1.1.2'
+__version__ = '1.1.3'
 
 
 CODEREVIEW_SETTINGS = {
@@ -858,8 +858,35 @@ def TryChange(change_info, args, swallow_exception):
       return
     ErrorExit("You need to install trychange.py to use the try server.")
 
+  def PathDifference(root, subpath):
+    """Returns the difference subpath minus root."""
+    root = os.path.realpath(root)
+    subpath = os.path.realpath(subpath)
+    if not subpath.startswith(root):
+      return None
+    # If the root does not have a trailing \ or /, we add it so the returned
+    # path starts immediately after the seperator regardless of whether it is
+    # provided.
+    root = os.path.join(root, '')
+    return subpath[len(root):]
+
+  # Try to find the gclient root.
+  def FindGclientRootDir(from_dir):
+    path = os.path.realpath(from_dir)
+    while not os.path.exists(os.path.join(path, '.gclient')):
+      next = os.path.split(path)
+      if not next[1]:
+        return None
+      path = next[0]
+    return path
+
+  trychange_args = []
+  gclient_root = FindGclientRootDir(GetRepositoryRoot())
+  if gclient_root:
+    trychange_args.extend(['--root', PathDifference(gclient_root,
+                                                    GetRepositoryRoot())])
   if change_info:
-    trychange_args = ['--name', change_info.name]
+    trychange_args.extend(['--name', change_info.name])
     if change_info.issue:
       trychange_args.extend(["--issue", str(change_info.issue)])
     if change_info.patchset:
@@ -870,7 +897,8 @@ def TryChange(change_info, args, swallow_exception):
                         swallow_exception=swallow_exception,
                         prog='gcl try')
   else:
-    trychange.TryChange(args,
+    trychange_args.extend(args)
+    trychange.TryChange(trychange_args,
                         file_list=None,
                         swallow_exception=swallow_exception,
                         prog='gcl try')

gclient_utils.py

@@ -40,12 +40,15 @@ def CheckCall(command, cwd=None):
 
   Works on python 2.4
   """
+  try:
     process = subprocess.Popen(command, cwd=cwd,
                                shell=sys.platform.startswith('win'),
                                stdout=subprocess.PIPE)
     output = process.communicate()[0]
-  if process.retcode:
+  except OSError, e:
-    raise CheckCallError(command, cwd, process.retcode, output)
+    raise CheckCallError(command, cwd, errno, None)
+  if process.returncode:
+    raise CheckCallError(command, cwd, process.returncode, output)
   return output
 
 

tests/gclient_utils_test.py

@@ -29,7 +29,7 @@ class CheckCallTestCase(SuperMoxTestBase):
   def testCheckCallSuccess(self):
     command = ['boo', 'foo', 'bar']
     process = self.mox.CreateMockAnything()
-    process.retcode = 0
+    process.returncode = 0
     gclient_utils.subprocess.Popen(
         command, cwd=None,
         stdout=gclient_utils.subprocess.PIPE,
@@ -41,7 +41,7 @@ class CheckCallTestCase(SuperMoxTestBase):
   def testCheckCallFailure(self):
     command = ['boo', 'foo', 'bar']
     process = self.mox.CreateMockAnything()
-    process.retcode = 42
+    process.returncode = 42
     gclient_utils.subprocess.Popen(
         command, cwd=None,
         stdout=gclient_utils.subprocess.PIPE,

tests/trychange_unittest.py

@@ -14,20 +14,33 @@ from super_mox import mox, SuperMoxTestBase
 
 class TryChangeTestsBase(SuperMoxTestBase):
   """Setups and tear downs the mocks but doesn't test anything as-is."""
-  pass
+  def setUp(self):
+    SuperMoxTestBase.setUp(self)
+    self.mox.StubOutWithMock(trychange.gclient_utils, 'CheckCall')
+    self.mox.StubOutWithMock(trychange.scm.GIT, 'Capture')
+    self.mox.StubOutWithMock(trychange.scm.GIT, 'GetEmail')
+    self.mox.StubOutWithMock(trychange.scm.SVN, 'DiffItem')
+    self.mox.StubOutWithMock(trychange.scm.SVN, 'GetCheckoutRoot')
+    self.mox.StubOutWithMock(trychange.scm.SVN, 'GetEmail')
+    self.fake_root = self.Dir()
+    self.expected_files = ['foo.txt', 'bar.txt']
+    self.options = optparse.Values()
+    self.options.files = self.expected_files
+    self.options.diff = None
+    self.options.name = None
+    self.options.email = None
 
 
 class TryChangeUnittest(TryChangeTestsBase):
   """General trychange.py tests."""
   def testMembersChanged(self):
     members = [
-      'EscapeDot', 'GIT', 'GetSourceRoot',
+      'EscapeDot', 'GIT', 'GetTryServerSettings', 'GuessVCS',
-      'GetTryServerSettings', 'GuessVCS',
+      'HELP_STRING', 'InvalidScript', 'NoTryServerAccess',
-      'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'PathDifference',
       'SCM', 'SVN', 'TryChange', 'USAGE', 'breakpad',
       'datetime', 'gcl', 'gclient_utils', 'getpass', 'logging',
       'optparse', 'os', 'presubmit_support', 'scm', 'shutil', 'socket',
-      'subprocess', 'sys', 'tempfile', 'upload', 'urllib',
+      'subprocess', 'sys', 'tempfile', 'urllib',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(trychange, members)
@@ -35,64 +48,53 @@ class TryChangeUnittest(TryChangeTestsBase):
 
 class SVNUnittest(TryChangeTestsBase):
   """trychange.SVN tests."""
-  def setUp(self):
-    SuperMoxTestBase.setUp(self)
-    self.fake_root = '/fake_root'
-    self.expected_files = ['foo.txt', 'bar.txt']
-    change_info = trychange.gcl.ChangeInfo(
-        'test_change', 0, 0, 'desc',
-        [('M', f) for f in self.expected_files],
-        self.fake_root)
-    self.svn = trychange.SVN(None)
-    self.svn.change_info = change_info
-
   def testMembersChanged(self):
     members = [
-      'GenerateDiff', 'GetFileNames', 'GetLocalRoot', 'ProcessOptions',
+      'GetFileNames', 'GetLocalRoot',
-      'options'
     ]
     # If this test fails, you should add the relevant test.
-    self.compareMembers(trychange.SVN(None), members)
+    self.compareMembers(trychange.SVN, members)
-
+
-  def testGetFileNames(self):
+  def testBasic(self):
+    trychange.os.getcwd().AndReturn(self.fake_root)
+    trychange.scm.SVN.GetCheckoutRoot(self.fake_root).AndReturn(self.fake_root)
+    trychange.os.getcwd().AndReturn('pro')
+    trychange.os.chdir(self.fake_root)
+    trychange.scm.SVN.DiffItem(self.expected_files[0]).AndReturn('bleh')
+    trychange.scm.SVN.DiffItem(self.expected_files[1]).AndReturn('blew')
+    trychange.os.chdir('pro')
+    trychange.scm.SVN.GetEmail(self.fake_root).AndReturn('georges@example.com')
     self.mox.ReplayAll()
-    self.assertEqual(self.svn.GetFileNames(), self.expected_files)
+    svn = trychange.SVN(self.options)
-
+    self.assertEqual(svn.GetFileNames(), self.expected_files)
-  def testGetLocalRoot(self):
+    self.assertEqual(svn.GetLocalRoot(), self.fake_root)
-    self.mox.ReplayAll()
-    self.assertEqual(self.svn.GetLocalRoot(), self.fake_root)
 
 
 class GITUnittest(TryChangeTestsBase):
   """trychange.GIT tests."""
-  def setUp(self):
-    self.fake_root = trychange.os.path.join(
-        trychange.os.path.dirname(__file__), 'fake_root')
-    self.expected_files = ['foo.txt', 'bar.txt']
-    options = optparse.Values()
-    options.files = self.expected_files
-    self.git = trychange.GIT(options)
-    SuperMoxTestBase.setUp(self)
-
   def testMembersChanged(self):
     members = [
-      'GenerateDiff', 'GetFileNames', 'GetLocalRoot',
+      'GetFileNames', 'GetLocalRoot',
-      'GetPatchName', 'ProcessOptions', 'options'
     ]
     # If this test fails, you should add the relevant test.
-    self.compareMembers(trychange.GIT(None), members)
+    self.compareMembers(trychange.GIT, members)
-
-  def testGetFileNames(self):
-    self.mox.ReplayAll()
-    self.assertEqual(self.git.GetFileNames(), self.expected_files)
 
-  def testGetLocalRoot(self):
+  def testBasic(self):
-    self.mox.StubOutWithMock(trychange.upload, 'RunShell')
+    trychange.gclient_utils.CheckCall(
-    trychange.upload.RunShell(['git', 'rev-parse', '--show-cdup']).AndReturn(
+        ['git', 'rev-parse', '--show-cdup']).AndReturn(self.fake_root)
-        self.fake_root)
     trychange.os.path.abspath(self.fake_root).AndReturn(self.fake_root)
+    trychange.gclient_utils.CheckCall(
+        ['git', 'cl', 'upstream']).AndReturn('random branch')
+    trychange.gclient_utils.CheckCall(
+        ['git', 'diff-tree', '-p', '--no-prefix', 'random branch', 'HEAD']
+        ).AndReturn('This is a dummy diff\n+3\n-4\n')
+    trychange.gclient_utils.CheckCall(
+        ['git', 'symbolic-ref', 'HEAD']).AndReturn('refs/heads/another branch')
+    trychange.scm.GIT.GetEmail('.').AndReturn('georges@example.com')
     self.mox.ReplayAll()
-    self.assertEqual(self.git.GetLocalRoot(), self.fake_root)
+    git = trychange.GIT(self.options)
+    self.assertEqual(git.GetFileNames(), self.expected_files)
+    self.assertEqual(git.GetLocalRoot(), self.fake_root)
 
 
 if __name__ == '__main__':

trychange.py

@@ -25,9 +25,8 @@ import gcl
 import gclient_utils
 import scm
 import presubmit_support
-import upload
 
-__version__ = '1.1.2'
+__version__ = '1.2'
 
 
 # Constants
@@ -66,23 +65,6 @@ class NoTryServerAccess(Exception):
     return self.args[0] + '\n' + HELP_STRING
 
 
-def PathDifference(root, subpath):
-  """Returns the difference subpath minus root."""
-  if subpath.find(root) != 0:
-    return None
-  # If the root does not have a trailing \ or /, we add it so the returned path
-  # starts immediately after the seperator regardless of whether it is provided.
-  if not root.endswith(os.sep):
-    root += os.sep
-  return subpath[len(root):]
-
-
-def GetSourceRoot():
-  """Returns the absolute directory one level up from the repository root."""
-  # TODO(maruel): This is odd to assume that '..' is the source root.
-  return os.path.abspath(os.path.join(gcl.GetRepositoryRoot(), '..'))
-
-
 def GetTryServerSettings():
   """Grab try server settings local to the repository."""
   def _SafeResolve(host):
@@ -124,61 +106,61 @@ class SCM(object):
   def __init__(self, options):
     self.options = options
 
-  def ProcessOptions(self):
+  def GetFileNames(self):
-    raise NotImplementedError
+    """Return the list of files in the diff."""
+    return self.options.files
 
 
 class SVN(SCM):
   """Gathers the options and diff for a subversion checkout."""
-  def GenerateDiff(self, files, root):
+  def __init__(self, *args, **kwargs):
+    SCM.__init__(self, *args, **kwargs)
+    self.checkout_root = scm.SVN.GetCheckoutRoot(os.getcwd())
+    self.options.files
+    if not self.options.diff:
+      # Generate the diff from the scm.
+      self.options.diff = self._GenerateDiff()
+    if not self.options.email:
+      # Assumes the svn credential is an email address.
+      self.options.email = scm.SVN.GetEmail(self.checkout_root)
+
+  def _GenerateDiff(self):
     """Returns a string containing the diff for the given file list.
 
     The files in the list should either be absolute paths or relative to the
-    given root. If no root directory is provided, the repository root will be
+    given root.
-    used.
     """
     previous_cwd = os.getcwd()
-    os.chdir(root or scm.SVN.GetCheckoutRoot(previous_cwd))
+    os.chdir(self.checkout_root)
-
+    if not self.options.files:
+      self.options.files = [f[1] for f in scm.SVN.CaptureStatus(None)]
     # Directories will return None so filter them out.
-    diff = filter(None, [scm.SVN.DiffItem(f) for f in files])
+    diff = filter(None, [scm.SVN.DiffItem(f) for f in self.options.files])
     os.chdir(previous_cwd)
     return "".join(diff)
 
-  def GetFileNames(self):
-    """Return the list of files in the diff."""
-    return self.change_info.GetFileNames()
-
   def GetLocalRoot(self):
     """Return the path of the repository root."""
-    return self.change_info.GetLocalRoot()
+    return self.checkout_root
-
-  def ProcessOptions(self):
-    checkout_root = None
-    if not self.options.diff:
-      # Generate the diff with svn and write it to the submit queue path.  The
-      # files are relative to the repository root, but we need patches relative
-      # to one level up from there (i.e., 'src'), so adjust both the file
-      # paths and the root of the diff.
-      # TODO(maruel): Remove this hack.
-      source_root = GetSourceRoot()
-      checkout_root = scm.SVN.GetCheckoutRoot(os.getcwd())
-      prefix = PathDifference(source_root, checkout_root)
-      adjusted_paths = [os.path.join(prefix, x) for x in self.options.files]
-      self.options.diff = self.GenerateDiff(adjusted_paths, root=source_root)
-      self.change_info = gcl.LoadChangelistInfoForMultiple(self.options.name,
-          gcl.GetRepositoryRoot(), True, True)
-    if not self.options.email:
-      checkout_root = checkout_root or scm.SVN.GetCheckoutRoot(os.getcwd())
-      self.options.email = scm.SVN.GetEmail(checkout_root)
 
 
 class GIT(SCM):
   """Gathers the options and diff for a git checkout."""
-  def GenerateDiff(self):
+  def __init__(self, *args, **kwargs):
+    SCM.__init__(self, *args, **kwargs)
+    self.checkout_root = os.path.abspath(
+        gclient_utils.CheckCall(['git', 'rev-parse', '--show-cdup']).strip())
+    if not self.options.diff:
+      self.options.diff = self._GenerateDiff()
+    if not self.options.name:
+      self.options.name = self._GetPatchName()
+    if not self.options.email:
+      self.options.email = scm.GIT.GetEmail('.')
+
+  def _GenerateDiff(self):
     """Get the diff we'll send to the try server. We ignore the files list."""
-    branch = upload.RunShell(['git', 'cl', 'upstream']).strip()
+    branch = gclient_utils.CheckCall(['git', 'cl', 'upstream']).strip()
-    diff = upload.RunShell(['git', 'diff-tree', '-p', '--no-prefix',
+    diff = gclient_utils.CheckCall(['git', 'diff-tree', '-p', '--no-prefix',
                                     branch, 'HEAD']).splitlines(True)
     for i in range(len(diff)):
       # In the case of added files, replace /dev/null with the path to the
@@ -187,34 +169,20 @@ class GIT(SCM):
         diff[i] = '--- %s' % diff[i+1][4:]
     return ''.join(diff)
 
-  def GetFileNames(self):
+  def _GetPatchName(self):
-    """Return the list of files in the diff."""
-    return self.options.files
-
-  def GetLocalRoot(self):
-    """Return the path of the repository root."""
-    # TODO: check for errors here?
-    root = upload.RunShell(['git', 'rev-parse', '--show-cdup']).strip()
-    return os.path.abspath(root)
-
-  def GetPatchName(self):
     """Construct a name for this patch."""
     # TODO: perhaps include the hash of the current commit, to distinguish
     # patches?
-    branch = upload.RunShell(['git', 'symbolic-ref', 'HEAD']).strip()
+    branch = gclient_utils.CheckCall(['git', 'symbolic-ref', 'HEAD']).strip()
     if not branch.startswith('refs/heads/'):
       # TODO(maruel): Find a better type.
       raise NoTryServerAccess("Couldn't figure out branch name")
     branch = branch[len('refs/heads/'):]
     return branch
 
-  def ProcessOptions(self):
+  def GetLocalRoot(self):
-    if not self.options.diff:
+    """Return the path of the repository root."""
-      self.options.diff = self.GenerateDiff()
+    return self.checkout_root
-    if not self.options.name:
-      self.options.name = self.GetPatchName()
-    if not self.options.email:
-      self.options.email = scm.GIT.GetEmail('.')
 
 
 def _ParseSendChangeOptions(options):
@@ -365,17 +333,12 @@ def GuessVCS(options):
   # Git has a command to test if you're in a git tree.
   # Try running it, but don't die if we don't have git installed.
   try:
-    out, returncode = subprocess.Popen(
+    gclient_utils.CheckCall(["git", "rev-parse", "--is-inside-work-tree"])
-        ["git", "rev-parse", "--is-inside-work-tree"],
-        shell=sys.platform.startswith('win'),
-        stdout=subprocess.PIPE).communicate()[0]
-    if returncode == 0:
     logging.info("Guessed VCS = Git")
     return GIT(options)
-  except OSError, (errno, message):
+  except gclient_utils.CheckCallError, e:
-    if errno != 2:  # ENOENT -- they don't have git installed.
+    if e.retcode != 2:  # ENOENT -- they don't have git installed.
       raise
-
   raise NoTryServerAccess("Could not guess version control system. "
                           "Are you in a working copy directory?")
 
@@ -520,7 +483,6 @@ def TryChange(argv,
     # Process the VCS in any case at least to retrieve the email address.
     try:
       options.scm = GuessVCS(options)
-      options.scm.ProcessOptions()
     except NoTryServerAccess, e:
       # If we got the diff, we don't care.
       if not options.diff: