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

gclient.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Meta checkout manager supporting both Subversion and GIT.
 
 Files
   .gclient      : Current client configuration, written by 'config' command.
                   Format is a Python script defining 'solutions', a list whose
@@ skipping 280 matching lines @@
     # A cache of the files affected by the current operation, necessary for
     # hooks.
     self._file_list = []
     # If it is not set to True, the dependency wasn't processed for its child
     # dependency, i.e. its DEPS wasn't read.
     self._deps_parsed = False
     # This dependency has been processed, i.e. checked out
     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

[M-A Ruel, 2013/04/12 15:28:17]

I actually like the idea of keeping an initialized Scm instance per dependency.
Not sure why I didn't think about it before.

 
     if not self.name and self.parent:
       raise gclient_utils.Error('Dependency without name')
 
   @property
   def requirements(self):
     """Calculate the list of requirements."""
     requirements = set()
     # self.parent is implicitly a requirement. This will be recursive by
     # definition.
@@ skipping 220 matching lines @@
     self.add_dependencies_and_close(deps_to_add, local_scope.get('hooks', []))
     logging.info('ParseDepsFile(%s) done' % self.name)
 
   def add_dependencies_and_close(self, deps_to_add, hooks):
     """Adds the dependencies, hooks and mark the parsing as done."""
     for dep in deps_to_add:
       if dep.verify_validity():
         self.add_dependency(dep)
     self._mark_as_parsed(hooks)
 
-  @staticmethod
-  def maybeGetParentRevision(
+  def maybeGetParentRevision(self,

[M-A Ruel, 2013/04/12 15:28:17]

keep self on the next line.

[kustermann, 2013/04/15 08:59:13]

Done.

       command, options, parsed_url, parent_name, revision_overrides):
     """If we are performing an update and --transitive is set, set the

[M-A Ruel, 2013/04/12 15:28:17]

This sentence is hard to read. Try to capture what it does in a single line with
a short sentence, then an empty, then a paragraph explaining more in details.

[kustermann, 2013/04/15 08:59:13]

I hope this is better.

-    revision to the parent's revision. If we have an explicit revision
-    do nothing."""
+    revision to the parent's revision (if self.url is the same repo) or to the
+    timestamp of the parent revision (if parent is in a different repo).
+    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 != None and self.parent.used_scm != None)

[M-A Ruel, 2013/04/12 15:28:17]

No need for () and checks for None, just do:
assert self.parent and self.parent.used_scm

[kustermann, 2013/04/15 08:59:13]

Done.

+          parent_revision_date = self.parent.used_scm.GetRevisionDate(
+              options.revision)
+          parent_revision = gclient_utils.MakeDateRevision(parent_revision_date)
+          # 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
+          # can result in dependencies to be checked out at the wrong revision)
+          if self.url.startswith('/'):
+            if options.verbose:
+              print("Not updating revision override from %s to %s. Using "

[M-A Ruel, 2013/04/12 15:28:17]

Use '' instead of "". (We are slowly replacing them to match the coding style)

[kustermann, 2013/04/15 08:59:13]

Done.

+                    "parent's revision %s since we are in the same repository."
+                    % (options.revision, parent_revision, options.revision))
+          else:
+            if options.verbose:
+              print("Updating revision override from %s to %s." %
+                    (options.revision, parent_revision))
+            revision_overrides[self.name] = parent_revision
+            options.revision = parent_revision
 
   # Arguments number differs from overridden method
   # pylint: disable=W0221
   def run(self, revision_overrides, command, args, work_queue, options):
     """Runs |command| then parse the DEPS file."""
     logging.info('Dependency(%s).run()' % self.name)
     assert self._file_list == []
     if not self.should_process:
       return
     # When running runhooks, there's no need to consult the SCM.
     # All known hooks are expected to run unconditionally regardless of working
     # copy state, so skip the SCM status check.
     run_scm = command not in ('runhooks', 'recurse', None)
     parsed_url = self.LateOverride(self.url)
     file_list = []
     if run_scm and parsed_url:
       if isinstance(parsed_url, self.FileImpl):
         # Special support for single-file checkout.
         if not command in (None, 'cleanup', 'diff', 'pack', 'status'):
           # 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(),

[M-A Ruel, 2013/04/12 15:28:17]

Example of why I dislike multi-line argument alignment not at +4. :)

[kustermann, 2013/04/15 08:59:13]

Done.

+                                                  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.
       # Convert all absolute paths to relative.
       for i in range(len(file_list)):
         # It depends on the command being executed (like runhooks vs sync).
         if not os.path.isabs(file_list[i]):
           continue
         prefix = os.path.commonprefix(
             [self.root.root_dir.lower(), file_list[i].lower()])
@@ skipping 196 matching lines @@
   @gclient_utils.lockedmethod
   def hooks_ran(self):
     return self._hooks_ran
 
   @property
   @gclient_utils.lockedmethod
   def file_list(self):
     return tuple(self._file_list)
 
   @property
+  def used_scm(self):

[M-A Ruel, 2013/04/12 15:28:17]

Add a docstring. It's actually poor that the others don't have one.

[kustermann, 2013/04/15 08:59:13]

Done.

+    return self._used_scm
+
+  @property
   def file_list_and_children(self):
     result = list(self.file_list)
     for d in self.dependencies:
       result.extend(d.file_list_and_children)
     return tuple(result)
 
   def __str__(self):
     out = []
     for i in ('name', 'url', 'parsed_url', 'safesync_url', 'custom_deps',
               'custom_vars', 'deps_hooks', 'file_list', 'should_process',
@@ skipping 934 matching lines @@
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, 'Error: %s' % str(e)
     return 1
 
 
 if '__main__' == __name__:
   fix_encoding.fix_encoding()
   sys.exit(Main(sys.argv[1:]))
 
 # vim: ts=2:sw=2:tw=80:et: