Changed the behaviour of '--transitive' in gclient.py to use revision instead...

Changed the behaviour of '--transitive' in gclient.py to use revision instead of timestamp for identical repositories.

Here's some background why we need this:
We discovered that google code defines the timestamp of a revision to be the time when a commit was started
rather than when it was finished (apache subversion takes the timestamp when the commit transaction is finished).
This can result in a situation where revision R(i-1) has a higher timestamp than Ri.
See bug: https://code.google.com/p/support/issues/detail?id=30419

When using 'gclient --transitive' we effectively do date-based checkouts.
If a parent has a dependency (without a ...@revision) and that dependency lives in the same repository as the parent does
we'd like to checkout the exact same revision as the parent (if we do a date-based checkout as we do now the google code
bug can result in a situation where we don't get the same revision).

Review URL: https://chromiumcodereview.appspot.com/13814012

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

gclient.py

@@ -298,6 +298,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     self._processed = False
     # This dependency had its hook run
     self._hooks_ran = False
+    # This is the scm used to checkout self.url. It may be used by dependencies
+    # to get the datetime of the revision we checked out.
+    self._used_scm = None
 
     if not self.name and self.parent:
       raise gclient_utils.Error('Dependency without name')
@@ -538,42 +541,42 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         self.add_dependency(dep)
     self._mark_as_parsed(hooks)
 
-  @staticmethod
   def maybeGetParentRevision(
-      command, options, parsed_url, parent_name, revision_overrides):
-    """If we are performing an update and --transitive is set, set the
-    revision to the parent's revision. If we have an explicit revision
-    do nothing."""
+      self, command, options, parsed_url, parent_name, revision_overrides):
+    """Uses revision/timestamp of parent if no explicit revision was specified.
+
+    If we are performing an update and --transitive is set, use
+    - the parent's revision if 'self.url' is in the same repository
+    - the parent's timestamp otherwise
+    to update 'self.url'. The used revision/timestamp will be set in
+    'options.revision'.
+    If we have an explicit revision do nothing.
+    """
     if command == 'update' and options.transitive and not options.revision:
       _, revision = gclient_utils.SplitUrlRevision(parsed_url)
       if not revision:
         options.revision = revision_overrides.get(parent_name)
-        if options.verbose and options.revision:
-          print("Using parent's revision date: %s" % options.revision)
-        # If the parent has a revision override, then it must have been
-        # converted to date format.
-        assert (not options.revision or
-                gclient_utils.IsDateRevision(options.revision))
-
-  @staticmethod
-  def maybeConvertToDateRevision(
-      command, options, name, scm, revision_overrides):
-    """If we are performing an update and --transitive is set, convert the
-    revision to a date-revision (if necessary). Instead of having
-    -r 101 replace the revision with the time stamp of 101 (e.g.
-    "{2011-18-04}").
-    This way dependencies are upgraded to the revision they had at the
-    check-in of revision 101."""
-    if (command == 'update' and
-        options.transitive and
-        options.revision and
-        not gclient_utils.IsDateRevision(options.revision)):
-      revision_date = scm.GetRevisionDate(options.revision)
-      revision = gclient_utils.MakeDateRevision(revision_date)
-      if options.verbose:
-        print("Updating revision override from %s to %s." %
-              (options.revision, revision))
-      revision_overrides[name] = revision
+        if (options.revision and
+            not gclient_utils.IsDateRevision(options.revision)):
+          assert self.parent and self.parent.used_scm
+          # If this dependency is in the same repository as parent it's url will
+          # start with a slash. If so we take the parent revision instead of
+          # it's timestamp.
+          # (The timestamps of commits in google code are broken -- which can
+          # result in dependencies to be checked out at the wrong revision)
+          if self.url.startswith('/'):
+            if options.verbose:
+              print('Using parent\'s revision %s since we are in the same '
+                    'repository.' % options.revision)
+          else:
+            parent_revision_date = self.parent.used_scm.GetRevisionDate(
+                options.revision)
+            options.revision = gclient_utils.MakeDateRevision(
+                parent_revision_date)
+            if options.verbose:
+              print('Using parent\'s revision date %s since we are in a '
+                    'different repository.' % options.revision)
+          revision_overrides[self.name] = options.revision
 
   # Arguments number differs from overridden method
   # pylint: disable=W0221
@@ -596,22 +599,19 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
           # Sadly, pylint doesn't realize that parsed_url is of FileImpl.
           # pylint: disable=E1103
           options.revision = parsed_url.GetRevision()
-          scm = gclient_scm.SVNWrapper(parsed_url.GetPath(),
-                                       self.root.root_dir,
-                                       self.name)
-          scm.RunCommand('updatesingle', options,
-                         args + [parsed_url.GetFilename()],
-                         file_list)
+          self._used_scm = gclient_scm.SVNWrapper(
+              parsed_url.GetPath(), self.root.root_dir, self.name)
+          self._used_scm.RunCommand('updatesingle',
+              options, args + [parsed_url.GetFilename()], file_list)
       else:
         # Create a shallow copy to mutate revision.
         options = copy.copy(options)
         options.revision = revision_overrides.get(self.name)
         self.maybeGetParentRevision(
             command, options, parsed_url, self.parent.name, revision_overrides)
-        scm = gclient_scm.CreateSCM(parsed_url, self.root.root_dir, self.name)
-        scm.RunCommand(command, options, args, file_list)
-        self.maybeConvertToDateRevision(
-            command, options, self.name, scm, revision_overrides)
+        self._used_scm = gclient_scm.CreateSCM(
+            parsed_url, self.root.root_dir, self.name)
+        self._used_scm.RunCommand(command, options, args, file_list)
         file_list = [os.path.join(self.name, f.strip()) for f in file_list]
 
       # TODO(phajdan.jr): We should know exactly when the paths are absolute.
@@ -827,6 +827,11 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
   def file_list(self):
     return tuple(self._file_list)
 
+  @property
+  def used_scm(self):
+    """SCMWrapper instance for this dependency or None if not processed yet."""
+    return self._used_scm
+
   @property
   def file_list_and_children(self):
     result = list(self.file_list)

testing_support/fake_repos.py

@@ -161,8 +161,6 @@ def wait_for_port_to_free(host, port):
   assert False, '%d is still bound' % port
 
 
-_FAKE_LOADED = False
-
 class FakeReposBase(object):
   """Generate both svn and git repositories to test gclient functionality.
 
@@ -181,11 +179,6 @@ class FakeReposBase(object):
   ]
 
   def __init__(self, host=None):
-    global _FAKE_LOADED
-    if _FAKE_LOADED:
-      raise Exception('You can only start one FakeRepos at a time.')
-    _FAKE_LOADED = True
-
     self.trial = trial_dir.TrialDir('repos')
     self.host = host or '127.0.0.1'
     # Format is [ None, tree, tree, ...]
@@ -321,6 +314,17 @@ class FakeReposBase(object):
     text += ''.join('%s = %s\n' % (usr, pwd) for usr, pwd in self.USERS)
     write(join(self.svn_repo, 'conf', 'passwd'), text)
 
+    # Necessary to be able to change revision properties
+    revprop_hook_filename = join(self.svn_repo, 'hooks', 'pre-revprop-change')
+    if sys.platform == 'win32':
+      # TODO(kustermann): Test on Windows one day.
+      write("%s.bat" % revprop_hook_filename, "")
+    else:
+      write(revprop_hook_filename,
+          '#!/bin/sh\n'
+          'exit 0\n')
+      os.chmod(revprop_hook_filename, 0755)
+
     # Mac 10.6 ships with a buggy subversion build and we need this line
     # to work around the bug.
     write(join(self.svn_repo, 'db', 'fsfs.conf'),
@@ -392,6 +396,14 @@ class FakeReposBase(object):
       new_tree = tree.copy()
     self.svn_revs.append(new_tree)
 
+  def _set_svn_commit_date(self, revision, date):
+    subprocess2.check_output(
+        ['svn', 'propset', 'svn:date', '--revprop', '-r', revision, date,
+         self.svn_base,
+         '--username', self.USERS[0][0],
+         '--password', self.USERS[0][1],
+         '--non-interactive'])
+
   def _commit_git(self, repo, tree):
     repo_root = join(self.git_root, repo)
     self._genTree(repo_root, tree)
@@ -642,22 +654,67 @@ hooks = [
     })
 
 
+class FakeRepoTransitive(FakeReposBase):
+  """Implements populateSvn()"""
+
+  def populateSvn(self):
+    """Creates a few revisions of changes including a DEPS file."""
+    # Repos
+    subprocess2.check_call(
+        ['svn', 'checkout', self.svn_base, self.svn_checkout,
+         '-q', '--non-interactive', '--no-auth-cache',
+         '--username', self.USERS[0][0], '--password', self.USERS[0][1]])
+    assert os.path.isdir(join(self.svn_checkout, '.svn'))
+
+    def file_system(rev):
+      DEPS = """deps = {
+                'src/different_repo': '%(svn_base)strunk/third_party',
+                'src/different_repo_fixed': '%(svn_base)strunk/third_party@1',
+                'src/same_repo': '/trunk/third_party',
+                'src/same_repo_fixed': '/trunk/third_party@1',
+             }""" % { 'svn_base': self.svn_base }
+      return {
+        'trunk/src/DEPS': DEPS,
+        'trunk/src/origin': 'svn/trunk/src@%(rev)d' % { 'rev': rev },
+        'trunk/third_party/origin':
+            'svn/trunk/third_party@%(rev)d' % { 'rev': rev },
+      }
+
+    # We make three commits. We use always the same DEPS contents but
+    # - 'trunk/src/origin' contains 'svn/trunk/src/origin@rX'
+    # - 'trunk/third_party/origin' contains 'svn/trunk/third_party/origin@rX'
+    # where 'X' is the revision number.
+    # So the 'origin' files will change in every commit.
+    self._commit_svn(file_system(1))
+    self._commit_svn(file_system(2))
+    self._commit_svn(file_system(3))
+    # We rewrite the timestamps so we can test that '--transitive' will take the
+    # parent timestamp on different repositories and the parent revision
+    # otherwise.
+    self._set_svn_commit_date('1', '2011-10-01T03:00:00.000000Z')
+    self._set_svn_commit_date('2', '2011-10-09T03:00:00.000000Z')
+    self._set_svn_commit_date('3', '2011-10-02T03:00:00.000000Z')
+
+  def populateGit(self):
+    pass
+
+
 class FakeReposTestBase(trial_dir.TestCase):
   """This is vaguely inspired by twisted."""
-  # static FakeRepos instance. Lazy loaded.
-  FAKE_REPOS = None
+  # Static FakeRepos instances. Lazy loaded.
+  CACHED_FAKE_REPOS = {}
   # Override if necessary.
   FAKE_REPOS_CLASS = FakeRepos
 
   def setUp(self):
     super(FakeReposTestBase, self).setUp()
-    if not FakeReposTestBase.FAKE_REPOS:
-      # Lazy create the global instance.
-      FakeReposTestBase.FAKE_REPOS = self.FAKE_REPOS_CLASS()
+    if not self.FAKE_REPOS_CLASS in self.CACHED_FAKE_REPOS:
+      self.CACHED_FAKE_REPOS[self.FAKE_REPOS_CLASS] = self.FAKE_REPOS_CLASS()
+    self.FAKE_REPOS = self.CACHED_FAKE_REPOS[self.FAKE_REPOS_CLASS]
     # No need to call self.FAKE_REPOS.setUp(), it will be called by the child
     # class.
     # Do not define tearDown(), since super's version does the right thing and
-    # FAKE_REPOS is kept across tests.
+    # self.FAKE_REPOS is kept across tests.
 
   @property
   def svn_base(self):

tests/gclient_smoketest.py

@@ -20,7 +20,9 @@ import unittest
 ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.insert(0, ROOT_DIR)
 
-from testing_support.fake_repos import join, write, FakeReposTestBase
+from testing_support.fake_repos import join, write
+from testing_support.fake_repos import FakeReposTestBase, FakeRepoTransitive
+
 import gclient_utils
 
 import subprocess2
@@ -348,43 +350,6 @@ class GClientSmokeSVN(GClientSmokeBase):
     tree['src/svn_hooked1'] = 'svn_hooked1'
     self.assertTree(tree)
 
-  def testSyncTransitive(self):
-    # TODO(maruel): safesync.
-    if not self.enabled:
-      return
-    self.gclient(['config', self.svn_base + 'trunk/src/'])
-
-    # Make sure we can populate a new repository with --transitive.
-    self.parseGclient(
-        ['sync', '--transitive', '--revision', 'src@1', '--deps', 'mac',
-          '--jobs', '1'],
-        ['running', 'running', 'running', 'running'])
-    tree = self.mangle_svn_tree(
-        ('trunk/src@1', 'src'),
-        ('trunk/third_party/foo@1', 'src/third_party/fpp'),
-        ('trunk/other@1', 'src/other'),
-        ('trunk/third_party/foo@1', 'src/third_party/prout'))
-
-    # Get up to date, so we can test synching back.
-    self.gclient(['sync', '--deps', 'mac', '--jobs', '1'])
-
-    # Manually remove svn_hooked1 before synching to make sure it's not
-    # recreated.
-    os.remove(join(self.root_dir, 'src', 'svn_hooked1'))
-
-    self.parseGclient(
-        ['sync', '--transitive', '--revision', 'src@1', '--deps', 'mac',
-          '--delete_unversioned_trees', '--jobs', '1'],
-        ['running', 'running', 'running', 'running', 'deleting'])
-    tree = self.mangle_svn_tree(
-        ('trunk/src@1', 'src'),
-        ('trunk/third_party/foo@1', 'src/third_party/fpp'),
-        ('trunk/other@1', 'src/other'),
-        ('trunk/third_party/foo@1', 'src/third_party/prout'))
-    tree['src/file/other/DEPS'] = (
-        self.FAKE_REPOS.svn_revs[2]['trunk/other/DEPS'])
-    self.assertTree(tree)
-
   def testSyncIgnoredSolutionName(self):
     """TODO(maruel): This will become an error soon."""
     if not self.enabled:
@@ -801,6 +766,62 @@ class GClientSmokeSVN(GClientSmokeBase):
     self.assertEquals(0, self.gclient(cmd)[-1])
 
 
+class GClientSmokeSVNTransitive(GClientSmokeBase):
+  FAKE_REPOS_CLASS = FakeRepoTransitive
+
+  def setUp(self):
+    super(GClientSmokeSVNTransitive, self).setUp()
+    self.enabled = self.FAKE_REPOS.set_up_svn()
+
+  def testSyncTransitive(self):
+    if not self.enabled:
+      return
+
+    self.gclient(['config', self.svn_base + 'trunk/src/'])
+
+    def test_case(parent, timestamp, fixed, output):
+      # We check out revision 'parent' and expect the following:
+      #  - src/ is checked out at r'parent'
+      #  - src/same_repo is checked out at r'parent' (due to --transitive)
+      #  - src/same_repo_fixed is checked out at r'fixed'
+      #  - src/different_repo is checked out at r'timestamp'
+      #    (due to --transitive)
+      #  - src/different_repo_fixed is checked out at r'fixed'
+
+      revisions = self.FAKE_REPOS.svn_revs
+      self.parseGclient(
+          ['sync', '--transitive', '--revision', 'src@%d' % parent,
+           '--jobs', '1'], output)
+      self.assertTree({
+        'src/origin': revisions[parent]['trunk/src/origin'],
+        'src/DEPS': revisions[parent]['trunk/src/DEPS'],
+        'src/same_repo/origin': revisions[parent]['trunk/third_party/origin'],
+        'src/same_repo_fixed/origin':
+            revisions[fixed]['trunk/third_party/origin'],
+        'src/different_repo/origin':
+            revisions[timestamp]['trunk/third_party/origin'],
+        'src/different_repo_fixed/origin':
+            revisions[fixed]['trunk/third_party/origin'],
+      })
+
+    # Here are the test cases for checking out 'trunk/src' at r1, r2 and r3
+    # r1: Everything is normal
+    test_case(parent=1, timestamp=1, fixed=1,
+              output=['running', 'running', 'running', 'running', 'running'])
+    # r2: Svn will scan from r1 upwards until it finds a revision matching the
+    # given timestamp or it takes the next smallest one (which is r2 in this
+    # case).
+    test_case(parent=2, timestamp=2, fixed=1,
+              output=['running', 'running', 'running'])
+    # r3: Svn will scan from r1 upwards until it finds a revision matching the
+    # given timestamp or it takes the next smallest one. Since
+    # timestamp(r3) < timestamp(r2) svn will checkout r1.
+    # This happens often on http://googlecode.com but is unexpected to happen
+    # with svnserve (unless you manually change 'svn:date')
+    test_case(parent=3, timestamp=1, fixed=1,
+              output=['running', 'running', 'running'])
+
+
 class GClientSmokeGIT(GClientSmokeBase):
   def setUp(self):
     super(GClientSmokeGIT, self).setUp()