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

gclient.py

Index: gclient.py
diff --git a/gclient.py b/gclient.py
index 85f144c1faa052298c3c082dbdd2dfd4d453b205..c79388badea35d3e2b60fd3e81f5bb78d7d339b8 100755
--- a/gclient.py
+++ b/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

[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')
@@ -538,42 +541,38 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         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
@@ -596,22 +595,21 @@ 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(),

[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.
@@ -828,6 +826,10 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     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: