Enforces using cwd in all svn calls.

That helps weed out some issues faces with svn plus helped me figure out some
misuses.

Most of the commands have been implicitly depending on os.getcwd(). This change
makes it always consistent and clear when dependence on the current directory is
needed.

Remove default arguments to scm.SVN.GenerateDiff and a few other calls to be
sure the refactoring was done right.

R=dpranke@chromium.org
BUG=
TEST=make sure most commands aren't broke


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

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

gcl.py

@@ -87,7 +87,10 @@ def CheckHomeForFile(filename):
 
 def UnknownFiles():
   """Runs svn status and returns unknown files."""
-  return [item[1] for item in SVN.CaptureStatus([]) if item[0][0] == '?']
+  return [
+      item[1] for item in SVN.CaptureStatus([], GetRepositoryRoot())
+      if item[0][0] == '?'
+  ]
 
 
 def GetRepositoryRoot():
@@ -138,7 +141,7 @@ def GetCachedFile(filename, max_age=60*60*24*3, use_root=False):
       return None
     if (not os.path.exists(cached_file) or
         (time.time() - os.stat(cached_file).st_mtime) > max_age):
-      dir_info = SVN.CaptureInfo('.')
+      dir_info = SVN.CaptureLocalInfo([], '.')
       repo_root = dir_info['Repository Root']
       if use_root:
         url_path = repo_root
@@ -548,8 +551,7 @@ class ChangeInfo(object):
     files = values['files']
     if update_status:
       for item in files[:]:
-        filename = os.path.join(local_root, item[1])
-        status_result = SVN.CaptureStatus(filename)
+        status_result = SVN.CaptureStatus(item[1], local_root)
         if not status_result or not status_result[0][0]:
           # File has been reverted.
           save = True
@@ -678,7 +680,7 @@ def GetModifiedFiles():
       files_in_cl[filename] = change_info.name
 
   # Get all the modified files.
-  status_result = SVN.CaptureStatus(None)
+  status_result = SVN.CaptureStatus(None, GetRepositoryRoot())
   for line in status_result:
     status = line[0]
     filename = line[1]
@@ -733,8 +735,9 @@ def ListFiles(show_unknown_files):
   return 0
 
 
-def GenerateDiff(files, root=None):
-  return SVN.GenerateDiff(files, root=root)
+def GenerateDiff(files):
+  return SVN.GenerateDiff(
+      files, GetRepositoryRoot(), full_move=False, revision=None)
 
 
 def OptionallyDoPresubmitChecks(change_info, committing, args):
@@ -1037,7 +1040,14 @@ def CMDcommit(change_info, args):
 def CMDchange(args):
   """Creates or edits a changelist.
 
-  Only scans the current directory and subdirectories."""
+  Only scans the current directory and subdirectories.
+  """
+  # Verify the user is running the change command from a read-write checkout.
+  svn_info = SVN.CaptureLocalInfo([], '.')
+  if not svn_info:
+    ErrorExit("Current checkout is unversioned.  Please retry with a versioned "
+              "directory.")
+
   if len(args) == 0:
     # Generate a random changelist name.
     changename = GenerateChangeName()
@@ -1047,12 +1057,6 @@ def CMDchange(args):
     changename = args[0]
   change_info = ChangeInfo.Load(changename, GetRepositoryRoot(), False, True)
 
-  # Verify the user is running the change command from a read-write checkout.
-  svn_info = SVN.CaptureInfo('.')
-  if not svn_info:
-    ErrorExit("Current checkout is unversioned.  Please retry with a versioned "
-              "directory.")
-
   if len(args) == 2:
     if not os.path.isfile(args[1]):
       ErrorExit('The change "%s" doesn\'t exist.' % args[1])
@@ -1439,6 +1443,7 @@ def main(argv):
   except upload.ClientLoginError, e:
     print >> sys.stderr, 'Got an exception logging in to Rietveld'
     print >> sys.stderr, str(e)
+    return 1
   except urllib2.HTTPError, e:
     if e.code != 500:
       raise

gclient_scm.py

@@ -711,8 +711,9 @@ class SVNWrapper(SCMWrapper):
   def GetRevisionDate(self, revision):
     """Returns the given revision's date in ISO-8601 format (which contains the
     time zone)."""
-    date = scm.SVN.Capture(['propget', '--revprop', 'svn:date', '-r', revision,
-                            os.path.join(self.checkout_path, '.')])
+    date = scm.SVN.Capture(
+        ['propget', '--revprop', 'svn:date', '-r', revision],
+        os.path.join(self.checkout_path, '.'))
     return date.strip()
 
   def cleanup(self, options, args, file_list):
@@ -796,7 +797,8 @@ class SVNWrapper(SCMWrapper):
 
     # Get the existing scm url and the revision number of the current checkout.
     try:
-      from_info = scm.SVN.CaptureInfo(os.path.join(self.checkout_path, '.'))
+      from_info = scm.SVN.CaptureLocalInfo(
+          [], os.path.join(self.checkout_path, '.'))
     except (gclient_utils.Error, subprocess2.CalledProcessError):
       raise gclient_utils.Error(
           ('Can\'t update/checkout %s if an unversioned directory is present. '
@@ -809,14 +811,16 @@ class SVNWrapper(SCMWrapper):
             self.checkout_path, from_info))
 
     # Look for locked directories.
-    dir_info = scm.SVN.CaptureStatus(os.path.join(self.checkout_path, '.'))
+    dir_info = scm.SVN.CaptureStatus(
+        None, os.path.join(self.checkout_path, '.'))
     if any(d[0][2] == 'L' for d in dir_info):
       try:
         self._Run(['cleanup', self.checkout_path], options)
       except subprocess2.CalledProcessError, e:
         # Get the status again, svn cleanup may have cleaned up at least
         # something.
-        dir_info = scm.SVN.CaptureStatus(os.path.join(self.checkout_path, '.'))
+        dir_info = scm.SVN.CaptureStatus(
+            None, os.path.join(self.checkout_path, '.'))
 
         # Try to fix the failures by removing troublesome files.
         for d in dir_info:
@@ -832,14 +836,14 @@ 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.CaptureRemoteInfo(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.
       try:
-        to_info = scm.SVN.CaptureInfo(url)
+        to_info = scm.SVN.CaptureRemoteInfo(url)
       except (gclient_utils.Error, subprocess2.CalledProcessError):
         # The url is invalid or the server is not accessible, it's safer to bail
         # out right now.
@@ -868,7 +872,7 @@ class SVNWrapper(SCMWrapper):
       else:
         if not options.force and not options.reset:
           # Look for local modifications but ignore unversioned files.
-          for status in scm.SVN.CaptureStatus(self.checkout_path):
+          for status in scm.SVN.CaptureStatus(None, self.checkout_path):
             if status[0] != '?':
               raise gclient_utils.Error(
                   ('Can\'t switch the checkout to %s; UUID don\'t match and '

presubmit_support.py

@@ -302,9 +302,8 @@ class InputApi(object):
 
       Remember to check for the None case and show an appropriate error!
     """
-    local_path = scm.SVN.CaptureInfo(depot_path).get('Path')
-    if local_path:
-      return local_path
+    return scm.SVN.CaptureLocalInfo([depot_path], self.change.RepositoryRoot()
+        ).get('Path')
 
   def LocalToDepotPath(self, local_path):
     """Translate a local path to a depot path.
@@ -315,9 +314,8 @@ class InputApi(object):
     Returns:
       The depot path (SVN URL) of the file if mapped, otherwise None.
     """
-    depot_path = scm.SVN.CaptureInfo(local_path).get('URL')
-    if depot_path:
-      return depot_path
+    return scm.SVN.CaptureLocalInfo([local_path], self.change.RepositoryRoot()
+        ).get('URL')
 
   def AffectedFiles(self, include_dirs=False, include_deletes=True,
                     file_filter=None):
@@ -429,7 +427,7 @@ class AffectedFile(object):
   """Representation of a file in a change."""
   # Method could be a function
   # pylint: disable=R0201
-  def __init__(self, path, action, repository_root=''):
+  def __init__(self, path, action, repository_root):
     self._path = path
     self._action = action
     self._local_root = repository_root
@@ -563,8 +561,8 @@ class SvnAffectedFile(AffectedFile):
 
   def ServerPath(self):
     if self._server_path is None:
-      self._server_path = scm.SVN.CaptureInfo(
-          self.AbsoluteLocalPath()).get('URL', '')
+      self._server_path = scm.SVN.CaptureLocalInfo(
+          [self.LocalPath()], self._local_root).get('URL', '')
     return self._server_path
 
   def IsDirectory(self):
@@ -575,14 +573,15 @@ class SvnAffectedFile(AffectedFile):
         # querying subversion, especially on Windows.
         self._is_directory = os.path.isdir(path)
       else:
-        self._is_directory = scm.SVN.CaptureInfo(
-            path).get('Node Kind') in ('dir', 'directory')
+        self._is_directory = scm.SVN.CaptureLocalInfo(
+            [self.LocalPath()], self._local_root
+            ).get('Node Kind') in ('dir', 'directory')
     return self._is_directory
 
   def Property(self, property_name):
     if not property_name in self._properties:
       self._properties[property_name] = scm.SVN.GetFileProperty(
-          self.AbsoluteLocalPath(), property_name).rstrip()
+          self.LocalPath(), property_name, self._local_root).rstrip()
     return self._properties[property_name]
 
   def IsTextFile(self):
@@ -593,13 +592,14 @@ class SvnAffectedFile(AffectedFile):
       elif self.IsDirectory():
         self._is_text_file = False
       else:
-        mime_type = scm.SVN.GetFileProperty(self.AbsoluteLocalPath(),
-                                            'svn:mime-type')
+        mime_type = scm.SVN.GetFileProperty(
+            self.LocalPath(), 'svn:mime-type', self._local_root)
         self._is_text_file = (not mime_type or mime_type.startswith('text/'))
     return self._is_text_file
 
   def GenerateScmDiff(self):
-    return scm.SVN.GenerateDiff([self.AbsoluteLocalPath()])
+    return scm.SVN.GenerateDiff(
+        [self.LocalPath()], self._local_root, False, None)
 
 
 class GitAffectedFile(AffectedFile):

testing_support/fake_repos.py

@@ -63,7 +63,7 @@ def commit_svn(repo, usr, pwd):
   """Commits the changes and returns the new revision number."""
   to_add = []
   to_remove = []
-  for status, filepath in scm.SVN.CaptureStatus(repo):
+  for status, filepath in scm.SVN.CaptureStatus(None, repo):
     if status[0] == '?':
       to_add.append(filepath)
     elif status[0] == '!':

tests/gcl_unittest.py

@@ -24,7 +24,7 @@ class GclTestsBase(SuperMoxTestBase):
     SuperMoxTestBase.setUp(self)
     self.fake_root_dir = self.RootDir()
     self.mox.StubOutWithMock(gcl, 'RunShell')
-    self.mox.StubOutWithMock(gcl.SVN, 'CaptureInfo')
+    self.mox.StubOutWithMock(gcl.SVN, '_CaptureInfo')
     self.mox.StubOutWithMock(gcl.SVN, 'GetCheckoutRoot')
     self.mox.StubOutWithMock(gcl, 'tempfile')
     self.mox.StubOutWithMock(gcl.upload, 'RealMain')

tests/scm_unittest.py

@@ -20,6 +20,10 @@ import scm
 import subprocess2
 
 
+# Access to a protected member XXX of a client class
+# pylint: disable=W0212
+
+
 class BaseTestCase(SuperMoxTestBase):
   # Like unittest's assertRaises, but checks for Gclient.Error.
   def assertRaisesError(self, msg, fn, *args, **kwargs):
@@ -94,7 +98,8 @@ class SVNTestCase(BaseSCMTestCase):
   def testMembersChanged(self):
     self.mox.ReplayAll()
     members = [
-        'AssertVersion', 'Capture', 'CaptureRevision', 'CaptureInfo',
+        'AssertVersion', 'Capture', 'CaptureRevision', 'CaptureLocalInfo',
+        'CaptureRemoteInfo',
         'CaptureStatus', 'current_version', 'DiffItem', 'GenerateDiff',
         'GetCheckoutRoot', 'GetEmail', 'GetFileProperty', 'IsMoved',
         'IsMovedInfo', 'ReadSimpleAuth', 'Revert', 'RunAndGetFileList',
@@ -104,19 +109,19 @@ class SVNTestCase(BaseSCMTestCase):
 
   def testGetCheckoutRoot(self):
     # pylint: disable=E1103
-    self.mox.StubOutWithMock(scm.SVN, 'CaptureInfo')
+    self.mox.StubOutWithMock(scm.SVN, '_CaptureInfo')
     self.mox.StubOutWithMock(scm, 'GetCasedPath')
     scm.os.path.abspath = lambda x: x
     scm.GetCasedPath = lambda x: x
-    scm.SVN.CaptureInfo(self.root_dir + '/foo/bar').AndReturn({
+    scm.SVN._CaptureInfo([], self.root_dir + '/foo/bar').AndReturn({
         'Repository Root': 'svn://svn.chromium.org/chrome',
         'URL': 'svn://svn.chromium.org/chrome/trunk/src',
     })
-    scm.SVN.CaptureInfo(self.root_dir + '/foo').AndReturn({
+    scm.SVN._CaptureInfo([], self.root_dir + '/foo').AndReturn({
         'Repository Root': 'svn://svn.chromium.org/chrome',
         'URL': 'svn://svn.chromium.org/chrome/trunk',
     })
-    scm.SVN.CaptureInfo(self.root_dir).AndReturn({
+    scm.SVN._CaptureInfo([], self.root_dir).AndReturn({
         'Repository Root': 'svn://svn.chromium.org/chrome',
         'URL': 'svn://svn.chromium.org/chrome/trunk/tools/commit-queue/workdir',
     })
@@ -140,7 +145,7 @@ class SVNTestCase(BaseSCMTestCase):
 </entry>
 </info>
 """ % self.url
-    scm.SVN.Capture(['info', '--xml', self.url]).AndReturn(xml_text)
+    scm.SVN.Capture(['info', '--xml', self.url], None).AndReturn(xml_text)
     expected = {
       'URL': 'http://src.chromium.org/svn/trunk/src/chrome/app/d',
       'UUID': None,
@@ -154,7 +159,7 @@ class SVNTestCase(BaseSCMTestCase):
       'Node Kind': 'file',
     }
     self.mox.ReplayAll()
-    file_info = scm.SVN.CaptureInfo(self.url)
+    file_info = scm.SVN._CaptureInfo([self.url], None)
     self.assertEquals(sorted(file_info.items()), sorted(expected.items()))
 
   def testCaptureInfo(self):
@@ -181,9 +186,9 @@ class SVNTestCase(BaseSCMTestCase):
 </entry>
 </info>
 """ % (self.url, self.root_dir)
-    scm.SVN.Capture(['info', '--xml', self.url]).AndReturn(xml_text)
+    scm.SVN.Capture(['info', '--xml', self.url], None).AndReturn(xml_text)
     self.mox.ReplayAll()
-    file_info = scm.SVN.CaptureInfo(self.url)
+    file_info = scm.SVN._CaptureInfo([self.url], None)
     expected = {
       'URL': self.url,
       'UUID': '7b9385f5-0452-0410-af26-ad4892b7a1fb',
@@ -235,10 +240,10 @@ class SVNTestCase(BaseSCMTestCase):
 </target>
 </status>
 """
-    scm.SVN.Capture(['status', '--xml', '.']).AndReturn(text)
+    scm.SVN.Capture(['status', '--xml'], '.').AndReturn(text)
 
     self.mox.ReplayAll()
-    info = scm.SVN.CaptureStatus('.')
+    info = scm.SVN.CaptureStatus(None, '.')
     expected = [
       ('?      ', 'unversionned_file.txt'),
       ('M      ', 'build\\internal\\essential.vsprops'),
@@ -255,9 +260,9 @@ class SVNTestCase(BaseSCMTestCase):
        path="perf">
        </target>
        </status>"""
-    scm.SVN.Capture(['status', '--xml']).AndReturn(text)
+    scm.SVN.Capture(['status', '--xml'], None).AndReturn(text)
     self.mox.ReplayAll()
-    info = scm.SVN.CaptureStatus(None)
+    info = scm.SVN.CaptureStatus(None, None)
     self.assertEquals(info, [])
 
 

trychange.py

@@ -244,11 +244,7 @@ class SVN(SCM):
     return data
 
   def CaptureStatus(self):
-    previous_cwd = os.getcwd()
-    os.chdir(self.checkout_root)
-    result = scm.SVN.CaptureStatus(self.checkout_root)
-    os.chdir(previous_cwd)
-    return result
+    return scm.SVN.CaptureStatus(None, self.checkout_root)
 
   def GenerateDiff(self):
     """Returns a string containing the diff for the given file list.
@@ -468,8 +464,9 @@ def GuessVCS(options, path, file_list):
       # 128 = git error code when not in a repo.
       logging.warning('Unexpected error code: %s' % e.returncode)
       raise
-  raise NoTryServerAccess("Could not guess version control system. "
-                          "Are you in a working copy directory?")
+  raise NoTryServerAccess(
+      ( 'Could not guess version control system for %s.\n'
+        'Are you in a working copy directory?') % path)
 
 
 def GetMungedDiff(path_diff, diff):