Change scm.SVN.Capture to redirect stderr and throw an exception on call failure.

Added try/except to the places where errors are tolerated.

Renamed scm.SVN.CaptureBaseRevision to CaptureRevision and removed CaptureHeadRevision.

BUG=54084
TEST=none

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

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

gclient_scm.py

@@ -742,12 +742,12 @@ class SVNWrapper(SCMWrapper):
       return
 
     # Get the existing scm url and the revision number of the current checkout.
-    from_info = scm.SVN.CaptureInfo(os.path.join(self.checkout_path, '.'), '.')
-    if not from_info:
-      raise gclient_utils.Error(('Can\'t update/checkout %r if an unversioned '
-                                 'directory is present. Delete the directory '
-                                 'and try again.') %
-                                self.checkout_path)
+    try:
+      from_info = scm.SVN.CaptureInfo(os.path.join(self.checkout_path, '.'))
+    except gclient_utils.Error:
+      raise gclient_utils.Error(
+          ('Can\'t update/checkout %s if an unversioned directory is present. '
+           'Delete the directory and try again.') % self.checkout_path)
 
     # Look for locked directories.
     dir_info = scm.SVN.CaptureStatus(os.path.join(self.checkout_path, '.'))
@@ -758,14 +758,15 @@ class SVNWrapper(SCMWrapper):
 
     # Retrieve the current HEAD version because svn is slow at null updates.
     if options.manually_grab_svn_rev and not revision:
-      from_info_live = scm.SVN.CaptureInfo(from_info['URL'], '.')
+      from_info_live = scm.SVN.CaptureInfo(from_info['URL'])
       revision = str(from_info_live['Revision'])
       rev_str = ' at %s' % revision
 
     if from_info['URL'] != base_url:
       # The repository url changed, need to switch.
-      to_info = scm.SVN.CaptureInfo(url, '.')
-      if not to_info.get('Repository Root') or not to_info.get('UUID'):
+      try:
+        to_info = scm.SVN.CaptureInfo(url)
+      except gclient_utils.Error:
         # The url is invalid or the server is not accessible, it's safer to bail
         # out right now.
         raise gclient_utils.Error('This url is unreachable: %s' % url)
@@ -919,7 +920,10 @@ class SVNWrapper(SCMWrapper):
 
   def revinfo(self, options, args, file_list):
     """Display revision"""
-    return scm.SVN.CaptureBaseRevision(self.checkout_path)
+    try:
+      return scm.SVN.CaptureRevision(self.checkout_path)
+    except gclient_utils.Error:
+      return None
 
   def runhooks(self, options, args, file_list):
     self.status(options, args, file_list)

scm.py

@@ -306,21 +306,12 @@ class SVN(object):
   current_version = None
 
   @staticmethod
-  def Capture(args, in_directory=None, print_error=True):
-    """Runs svn, capturing output sent to stdout as a string.
+  def Capture(args, **kwargs):
+    """Always redirect stderr.
 
-    Args:
-      args: A sequence of command line parameters to be passed to svn.
-      in_directory: The directory where svn is to be run.
-
-    Returns:
-      The output sent to stdout as a string.
-    """
-    stderr = None
-    if not print_error:
-      stderr = subprocess.PIPE
-    return gclient_utils.Popen(['svn'] + args, cwd=in_directory,
-        stdout=subprocess.PIPE, stderr=stderr).communicate()[0]
+    Throws an exception if non-0 is returned."""
+    return gclient_utils.CheckCall(['svn'] + args, print_error=False,
+        **kwargs)[0]
 
   @staticmethod
   def RunAndGetFileList(verbose, args, cwd, file_list, stdout=None):
@@ -425,15 +416,11 @@ class SVN(object):
       break
 
   @staticmethod
-  def CaptureInfo(relpath, in_directory=None, print_error=True):
+  def CaptureInfo(cwd):
     """Returns a dictionary from the svn info output for the given file.
 
-    Args:
-      relpath: The directory where the working copy resides relative to
-        the directory given by in_directory.
-      in_directory: The directory where svn is to be run.
-    """
-    output = SVN.Capture(["info", "--xml", relpath], in_directory, print_error)
+    Throws an exception if svn info fails."""
+    output = SVN.Capture(['info', '--xml', cwd])
     dom = gclient_utils.ParseXML(output)
     result = {}
     if dom:
@@ -468,24 +455,13 @@ class SVN(object):
     return result
 
   @staticmethod
-  def CaptureHeadRevision(url):
-    """Get the head revision of a SVN repository.
-
-    Returns:
-      Int head revision
-    """
-    info = SVN.Capture(["info", "--xml", url], os.getcwd())
-    dom = xml.dom.minidom.parseString(info)
-    return dom.getElementsByTagName('entry')[0].getAttribute('revision')
-
-  @staticmethod
-  def CaptureBaseRevision(cwd):
+  def CaptureRevision(cwd):
     """Get the base revision of a SVN repository.
 
     Returns:
       Int base revision
     """
-    info = SVN.Capture(["info", "--xml"], cwd)
+    info = SVN.Capture(['info', '--xml'], cwd=cwd)
     dom = xml.dom.minidom.parseString(info)
     return dom.getElementsByTagName('entry')[0].getAttribute('revision')
 
@@ -539,8 +515,9 @@ class SVN(object):
           if xml_item_status in status_letter:
             statuses[0] = status_letter[xml_item_status]
           else:
-            raise Exception('Unknown item status "%s"; please implement me!' %
-                            xml_item_status)
+            raise gclient_utils.Error(
+                'Unknown item status "%s"; please implement me!' %
+                    xml_item_status)
           # Col 1
           xml_props_status = wc_status[0].getAttribute('props')
           if xml_props_status == 'modified':
@@ -551,8 +528,9 @@ class SVN(object):
                 xml_props_status == 'normal'):
             pass
           else:
-            raise Exception('Unknown props status "%s"; please implement me!' %
-                            xml_props_status)
+            raise gclient_utils.Error(
+                'Unknown props status "%s"; please implement me!' %
+                    xml_props_status)
           # Col 2
           if wc_status[0].getAttribute('wc-locked') == 'true':
             statuses[2] = 'L'
@@ -592,12 +570,10 @@ class SVN(object):
       is not set on the file.  If the file is not under version control, the
       empty string is also returned.
     """
-    output = SVN.Capture(["propget", property_name, filename])
-    if (output.startswith("svn: ") and
-        output.endswith("is not under version control")):
-      return ""
-    else:
-      return output
+    try:
+      return SVN.Capture(['propget', property_name, filename])
+    except gclient_utils.Error:
+      return ''
 
   @staticmethod
   def DiffItem(filename, full_move=False, revision=None):
@@ -662,7 +638,7 @@ class SVN(object):
       else:
         if info.get("Node Kind") != "directory":
           # svn diff on a mv/cp'd file outputs nothing if there was no change.
-          data = SVN.Capture(command, None)
+          data = SVN.Capture(command)
           if not data:
             # We put in an empty Index entry so upload.py knows about them.
             data = "Index: %s\n" % filename.replace(os.sep, '/')
@@ -670,7 +646,7 @@ class SVN(object):
     else:
       if info.get("Node Kind") != "directory":
         # Normal simple case.
-        data = SVN.Capture(command, None)
+        data = SVN.Capture(command)
       # Otherwise silently ignore directories.
     return data
 
@@ -761,14 +737,15 @@ class SVN(object):
   @staticmethod
   def GetEmail(repo_root):
     """Retrieves the svn account which we assume is an email address."""
-    infos = SVN.CaptureInfo(repo_root)
-    uuid = infos.get('UUID')
-    root = infos.get('Repository Root')
-    if not root:
+    try:
+      infos = SVN.CaptureInfo(repo_root)
+    except gclient_utils.Error:
       return None
 
     # Should check for uuid but it is incorrectly saved for https creds.
+    root = infos['Repository Root']
     realm = root.rsplit('/', 1)[0]
+    uuid = infos['UUID']
     if root.startswith('https') or not uuid:
       regexp = re.compile(r'<%s:\d+>.*' % realm)
     else:
@@ -820,15 +797,16 @@ class SVN(object):
     The directory is returned as an absolute path.
     """
     directory = os.path.abspath(directory)
-    infos = SVN.CaptureInfo(directory, print_error=False)
-    cur_dir_repo_root = infos.get("Repository Root")
-    if not cur_dir_repo_root:
+    try:
+      cur_dir_repo_root = SVN.CaptureInfo(directory)['Repository Root']
+    except gclient_utils.Error:
       return None
-
     while True:
       parent = os.path.dirname(directory)
-      if (SVN.CaptureInfo(parent, print_error=False).get(
-              "Repository Root") != cur_dir_repo_root):
+      try:
+        if SVN.CaptureInfo(parent)['Repository Root'] != cur_dir_repo_root:
+          break
+      except gclient_utils.Error:
         break
       directory = parent
     return GetCasedPath(directory)

tests/scm_unittest.py

@@ -28,9 +28,10 @@ class BaseTestCase(SuperMoxTestBase):
 class BaseSCMTestCase(BaseTestCase):
   def setUp(self):
     BaseTestCase.setUp(self)
-    self.mox.StubOutWithMock(scm.gclient_utils, 'Popen')
+    self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCall')
     self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCallAndFilter')
     self.mox.StubOutWithMock(scm.gclient_utils, 'CheckCallAndFilterAndHeader')
+    self.mox.StubOutWithMock(scm.gclient_utils, 'Popen')
 
 
 class RootTestCase(BaseSCMTestCase):
@@ -153,13 +154,13 @@ class SVNTestCase(BaseSCMTestCase):
     self.args = self.Args()
     self.url = self.Url()
     self.relpath = 'asf'
+    self.mox.StubOutWithMock(scm.SVN, 'Capture')
 
   def testMembersChanged(self):
     self.mox.ReplayAll()
     members = [
-        'AssertVersion', 'Capture', 'CaptureBaseRevision',
-        'CaptureHeadRevision', 'CaptureInfo', 'CaptureStatus',
-        'current_version', 'DiffItem', 'GenerateDiff',
+        'AssertVersion', 'Capture', 'CaptureRevision', 'CaptureInfo',
+        'CaptureStatus', 'current_version', 'DiffItem', 'GenerateDiff',
         'GetCheckoutRoot', 'GetEmail', 'GetFileProperty', 'IsMoved',
         'IsMovedInfo', 'ReadSimpleAuth', 'RunAndGetFileList',
     ]
@@ -172,10 +173,10 @@ class SVNTestCase(BaseSCMTestCase):
     scm.os.path.abspath(self.root_dir + 'x').AndReturn(self.root_dir)
     scm.GetCasedPath(self.root_dir).AndReturn(self.root_dir)
     result1 = { "Repository Root": "Some root" }
-    scm.SVN.CaptureInfo(self.root_dir, print_error=False).AndReturn(result1)
+    scm.SVN.CaptureInfo(self.root_dir).AndReturn(result1)
     results2 = { "Repository Root": "A different root" }
-    scm.SVN.CaptureInfo(scm.os.path.dirname(self.root_dir),
-                        print_error=False).AndReturn(results2)
+    scm.SVN.CaptureInfo(
+        scm.os.path.dirname(self.root_dir)).AndReturn(results2)
     self.mox.ReplayAll()
     self.assertEquals(scm.SVN.GetCheckoutRoot(self.root_dir + 'x'),
                       self.root_dir)
@@ -196,8 +197,7 @@ class SVNTestCase(BaseSCMTestCase):
 </entry>
 </info>
 """ % self.url
-    self.mox.StubOutWithMock(scm.SVN, 'Capture')
-    scm.SVN.Capture(['info', '--xml', self.url], '.', True).AndReturn(xml_text)
+    scm.SVN.Capture(['info', '--xml', self.url]).AndReturn(xml_text)
     expected = {
       'URL': 'http://src.chromium.org/svn/trunk/src/chrome/app/d',
       'UUID': None,
@@ -211,7 +211,7 @@ class SVNTestCase(BaseSCMTestCase):
       'Node Kind': 'file',
     }
     self.mox.ReplayAll()
-    file_info = scm.SVN.CaptureInfo(self.url, '.', True)
+    file_info = scm.SVN.CaptureInfo(self.url)
     self.assertEquals(sorted(file_info.items()), sorted(expected.items()))
 
   def testCaptureInfo(self):
@@ -238,10 +238,9 @@ class SVNTestCase(BaseSCMTestCase):
 </entry>
 </info>
 """ % (self.url, self.root_dir)
-    self.mox.StubOutWithMock(scm.SVN, 'Capture')
-    scm.SVN.Capture(['info', '--xml', self.url], '.', True).AndReturn(xml_text)
+    scm.SVN.Capture(['info', '--xml', self.url]).AndReturn(xml_text)
     self.mox.ReplayAll()
-    file_info = scm.SVN.CaptureInfo(self.url, '.', True)
+    file_info = scm.SVN.CaptureInfo(self.url)
     expected = {
       'URL': self.url,
       'UUID': '7b9385f5-0452-0410-af26-ad4892b7a1fb',
@@ -293,12 +292,7 @@ class SVNTestCase(BaseSCMTestCase):
 </target>
 </status>
 """
-    proc = self.mox.CreateMockAnything()
-    scm.gclient_utils.Popen(['svn', 'status', '--xml', '.'],
-                            cwd=None,
-                            stderr=None,
-                            stdout=scm.subprocess.PIPE).AndReturn(proc)
-    proc.communicate().AndReturn((text, 0))
+    scm.SVN.Capture(['status', '--xml', '.']).AndReturn(text)
 
     self.mox.ReplayAll()
     info = scm.SVN.CaptureStatus('.')
@@ -318,12 +312,7 @@ class SVNTestCase(BaseSCMTestCase):
        path="perf">
        </target>
        </status>"""
-    proc = self.mox.CreateMockAnything()
-    scm.gclient_utils.Popen(['svn', 'status', '--xml'],
-                            cwd=None,
-                            stderr=None,
-                            stdout=scm.subprocess.PIPE).AndReturn(proc)
-    proc.communicate().AndReturn((text, 0))
+    scm.SVN.Capture(['status', '--xml']).AndReturn(text)
     self.mox.ReplayAll()
     info = scm.SVN.CaptureStatus(None)
     self.assertEquals(info, [])