Move all revision parsing code to _EnforceRevisions.

Make -r invalid@rev a warning and ignore the argument.
Make -r rev use the Nth solution.
Fix a bug in FakeRepos.assertTree().

TEST=updated the smoke test to reflect the new behavior which should be the most compatible with the current buildbot behavior
BUG=chromium-os:3465

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

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

gclient.py

@@ -506,31 +506,33 @@ solutions = [
   def _EnforceRevisions(self, solutions):
     """Checks for revision overrides."""
     revision_overrides = {}
-    solutions = [s['name'] for s in solutions]
-    if self._options.revisions:
-      revision = self._options.revisions[0]
-      # Ignore solution@
-      rev = revision
-      if '@' in revision:
-        rev = revision.split('@', 1)[1]
-      revision_overrides[solutions[0]] = rev
-
-    if len(self._options.revisions) > 1:
-      # Enforce solution@rev format for following params.
-      for revision in self._options.revisions[1:]:
-        if not '@' in revision:
-          raise gclient_utils.Error(
-              'Specify the full dependency when specifying a revision number '
-              'for non-primary solution.')
-        sol, rev = revision.split("@", 1)
-        # Disallow conflicting revs
-        if revision_overrides.get(sol, rev) != rev:
-          raise gclient_utils.Error(
-              'Conflicting revision numbers specified for %s: %s and %s.' %
-                  (sol, revision_overrides[sol], rev))
-        if not sol in solutions:
-          raise gclient_utils.Error('%s is not a valid solution.' % sol)
+    if self._options.head:
+      return revision_overrides
+    for s in solutions:
+      if not s.get('safesync_url', None):
+        continue
+      handle = urllib.urlopen(s['safesync_url'])
+      rev = handle.read().strip()
+      handle.close()
+      if len(rev):
+        self._options.revisions.append('%s@%s' % (s['name'], rev))
+    if not self._options.revisions:
+      return revision_overrides
+    # --revision will take over safesync_url.
+    solutions_names = [s['name'] for s in solutions]
+    index = 0
+    for revision in self._options.revisions:
+      if not '@' in revision:
+        # Support for --revision 123
+        revision = '%s@%s' % (solutions_names[index], revision)
+      sol, rev = revision.split("@", 1)
+      if not sol in solutions_names:
+        #raise gclient_utils.Error('%s is not a valid solution.' % sol)
+        print >> sys.stderr, ('Please fix your script, having invalid '
+                              '--revision flags will soon considered an error.')
+      else:
         revision_overrides[sol] = rev
+      index += 1
     return revision_overrides
 
   def RunOnDeps(self, command, args):
@@ -942,10 +944,11 @@ def CMDsync(parser, args):
                     help="don't run hooks after the update is complete")
   parser.add_option("-r", "--revision", action="append",
                     dest="revisions", metavar="REV", default=[],
-                    help="Enforces revision/hash for the primary solution. "
-                         "To modify a secondary solution, use it at least once "
-                         "for the primary solution and then use the format "
-                         "solution@rev/hash, e.g. -r src@123")
+                    help="Enforces revision/hash for the solutions with the "
+                         "format src@rev. The src@ part is optional and can be "
+                         "skipped. -r can be used multiple times when .gclient "
+                         "has multiple solutions configured and will work even "
+                         "if the src@ part is skipped.")
   parser.add_option("--head", action="store_true",
                     help="skips any safesync_urls specified in "
                          "configured solutions and sync to head instead")
@@ -967,29 +970,9 @@ def CMDsync(parser, args):
   if not client:
     raise gclient_utils.Error("client not configured; see 'gclient config'")
 
-  if not options.head:
-    solutions = client.GetVar('solutions')
-    if solutions:
-      first = True
-      for s in solutions:
-        if s.get('safesync_url', None):
-          # rip through revisions and make sure we're not over-riding
-          # something that was explicitly passed
-          has_key = False
-          r_first = True
-          for r in options.revisions:
-            if (first and r_first) or r.split('@', 1)[0] == s['name']:
-              has_key = True
-              break
-            r_first = False
-
-          if not has_key:
-            handle = urllib.urlopen(s['safesync_url'])
-            rev = handle.read().strip()
-            handle.close()
-            if len(rev):
-              options.revisions.append(s['name']+'@'+rev)
-        first = False
+  if options.revisions and options.head:
+    # TODO(maruel): Make it a parser.error if it doesn't break any builder.
+    print("Warning: you cannot use both --head and --revision")
 
   if options.verbose:
     # Print out the .gclient file.  This is longer than if we just printed the
@@ -1125,6 +1108,8 @@ def Main(argv):
     if not hasattr(options, 'revisions'):
       # GClient.RunOnDeps expects it even if not applicable.
       options.revisions = []
+    if not hasattr(options, 'head'):
+      options.head = None
     return (options, args)
   parser.parse_args = Parse
   # We don't want wordwrapping in epilog (usually examples)

tests/fake_repos.py

@@ -8,6 +8,7 @@
 import atexit
 import logging
 import os
+import pprint
 import re
 import shutil
 import subprocess
@@ -500,6 +501,7 @@ class FakeReposTestBase(unittest.TestCase):
       logging.debug('Actual %s\n%s' % (tree_root, pprint.pformat(actual)))
       logging.debug('Expected\n%s' % pprint.pformat(tree))
       logging.debug('Diff\n%s' % pprint.pformat(diff))
+      self.assertEquals(diff, [])
 
 
 def main(argv):

tests/gclient_smoketest.py

@@ -177,7 +177,7 @@ class GClientSmokeSVN(GClientSmokeBase):
     self.checkString('', results[1])
     self.assertEquals(0, results[2])
     tree = mangle_svn_tree(
-        (join('trunk', 'src'), 'src', self.FAKE_REPOS.svn_revs[-1]),
+        (join('trunk', 'src'), 'src', self.FAKE_REPOS.svn_revs[2]),
         (join('trunk', 'third_party', 'foo'), join('src', 'third_party', 'fpp'),
             self.FAKE_REPOS.svn_revs[2]),
         (join('trunk', 'third_party', 'foo'), join('src', 'third_party', 'foo'),
@@ -191,9 +191,29 @@ class GClientSmokeSVN(GClientSmokeBase):
     tree[join('src', 'svn_hooked1')] = 'svn_hooked1'
     self.assertTree(tree)
 
-  def SyncAtRev1(self, arg):
+  def testSyncIgnoredSolutionName(self):
+    """TODO(maruel): This will become an error soon."""
     self.gclient(['config', self.svn_base + 'trunk/src/'])
-    results = self.gclient(['sync', '--deps', 'mac', '-r', arg])
+    results = self.gclient(['sync', '--deps', 'mac', '-r', 'invalid@1'])
+    out = results[0].splitlines(False)
+    self.assertEquals(17, len(out))
+    self.checkString('Please fix your script, having invalid --revision flags '
+        'will soon considered an error.\n', results[1])
+    self.assertEquals(0, results[2])
+    tree = mangle_svn_tree(
+        (join('trunk', 'src'), 'src', self.FAKE_REPOS.svn_revs[2]),
+        (join('trunk', 'third_party', 'foo'), join('src', 'third_party', 'foo'),
+            self.FAKE_REPOS.svn_revs[1]),
+        (join('trunk', 'other'), join('src', 'other'),
+            self.FAKE_REPOS.svn_revs[2]),
+        )
+    tree[join('src', 'svn_hooked1')] = 'svn_hooked1'
+    self.assertTree(tree)
+
+  def testSyncNoSolutionName(self):
+    # When no solution name is provided, gclient uses the first solution listed.
+    self.gclient(['config', self.svn_base + 'trunk/src/'])
+    results = self.gclient(['sync', '--deps', 'mac', '-r', '1'])
     out = results[0].splitlines(False)
     self.assertEquals(19, len(out))
     self.checkString('', results[1])
@@ -210,12 +230,6 @@ class GClientSmokeSVN(GClientSmokeBase):
         )
     self.assertTree(tree)
 
-  def testSyncIgnoredSolutionName(self):
-    self.SyncAtRev1('ignored@1')
-
-  def testSyncNoSolutionName(self):
-    self.SyncAtRev1('1')
-
   def testRevertAndStatus(self):
     self.gclient(['config', self.svn_base + 'trunk/src/'])
     # Tested in testSync.
@@ -420,6 +434,54 @@ class GClientSmokeGIT(GClientSmokeBase):
     tree[join('src', 'git_hooked2')] = 'git_hooked2'
     self.assertTree(tree)
 
+  def testSyncIgnoredSolutionName(self):
+    """TODO(maruel): This will become an error soon."""
+    self.gclient(['config', self.git_base + 'repo_1', '--name', 'src'])
+    results = self.gclient([
+        'sync', '--deps', 'mac', '--revision',
+        'invalid@' + self.FAKE_REPOS.git_hashes['repo_1'][0][0],
+      ])
+    out = results[0].splitlines(False)
+
+    self.assertEquals(13, len(out))
+    # TODO(maruel): git shouldn't output to stderr...
+    self.checkString('Please fix your script, having invalid --revision flags '
+        'will soon considered an error.\nSwitched to a new branch \'%s\'\n' %
+            self.FAKE_REPOS.git_hashes['repo_2'][0][0][:7],
+        results[1])
+    self.assertEquals(0, results[2])
+    tree = mangle_git_tree(
+        ('src', self.FAKE_REPOS.git_hashes['repo_1'][1][1]),
+        (join('src', 'repo2'), self.FAKE_REPOS.git_hashes['repo_2'][0][1]),
+        (join('src', 'repo2', 'repo_renamed'),
+              self.FAKE_REPOS.git_hashes['repo_3'][1][1]),
+        )
+    tree[join('src', 'git_hooked1')] = 'git_hooked1'
+    tree[join('src', 'git_hooked2')] = 'git_hooked2'
+    self.assertTree(tree)
+
+  def testSyncNoSolutionName(self):
+    # When no solution name is provided, gclient uses the first solution listed.
+    self.gclient(['config', self.git_base + 'repo_1', '--name', 'src'])
+    results = self.gclient([
+        'sync', '--deps', 'mac', '--revision',
+        self.FAKE_REPOS.git_hashes['repo_1'][0][0],
+      ])
+    out = results[0].splitlines(False)
+    self.assertEquals(12, len(out))
+    # TODO(maruel): git shouldn't output to stderr...
+    self.checkString('Switched to a new branch \'%s\'\n'
+        % self.FAKE_REPOS.git_hashes['repo_1'][0][0], results[1])
+    self.assertEquals(0, results[2])
+    tree = mangle_git_tree(
+        ('src', self.FAKE_REPOS.git_hashes['repo_1'][0][1]),
+        (join('src', 'repo2'), self.FAKE_REPOS.git_hashes['repo_2'][1][1]),
+        (join('src', 'repo2', 'repo3'),
+            self.FAKE_REPOS.git_hashes['repo_3'][1][1]),
+        (join('src', 'repo4'), self.FAKE_REPOS.git_hashes['repo_4'][1][1]),
+        )
+    self.assertTree(tree)
+
   def testRevertAndStatus(self):
     """TODO(maruel): Remove this line once this test is fixed."""
     self.gclient(['config', self.git_base + 'repo_1', '--name', 'src'])