[no-sync] Store previous sync commit in memory.

- Instead of expecting the caller to run `rev-parse HEAD` before
  calling gclient, we can just store the latest commit on disk
  - this gets around the problem where gclient might git reset to the
     base checkout after applying patch-refs, so calling rev-parse
     before calling gclient sync might not actually return the latest
     commit of the last sync.
- also moving os.environ[PREVIOUS_CUSTOM_VARS] setting to earlier when
  we only have the solutions in self.dependencies because all other
  dependencies inherit from the solutions so there's no point storing
  custom_vars for the rest.

Bug: 1339472
Change-Id: I6a3570f09153bd8087bbe6bdab7ece4949856aae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3750491
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Aravind Vasudevan <aravindvasudev@google.com>
Commit-Queue: Joanna Wang <jojwang@chromium.org>

gclient.py

@@ -132,6 +132,7 @@ UNSET_CACHE_DIR = object()
 
 
 PREVIOUS_CUSTOM_VARS = 'GCLIENT_PREVIOUS_CUSTOM_VARS'
+PREVIOUS_SYNC_COMMITS = 'GCLIENT_PREVIOUS_SYNC_COMMITS'
 
 
 class GNException(Exception):
@@ -457,7 +458,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
 
     # Whether we should sync git/cipd dependencies and hooks from the
     # DEPS file.
-    # This is set based on options.skip_sync_revisions and must be done
+    # This is set based on skip_sync_revisions and must be done
     # after the patch refs are applied.
     # If this is False, we will still run custom_hooks and process
     # custom_deps, if any.
@@ -1010,13 +1011,20 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
             'sync_status': sync_status,
           })
 
-      patch_repo = self.url.split('@')[0]
-      patch_ref = patch_refs.pop(self.FuzzyMatchUrl(patch_refs), None)
-      target_branch = target_branches.pop(
-          self.FuzzyMatchUrl(target_branches), None)
-      if command == 'update' and patch_ref is not None:
-        self._used_scm.apply_patch_ref(patch_repo, patch_ref, target_branch,
-                                       options, file_list)
+      if isinstance(self, GitDependency) and command == 'update':
+        patch_repo = self.url.split('@')[0]
+        patch_ref = patch_refs.pop(self.FuzzyMatchUrl(patch_refs), None)
+        target_branch = target_branches.pop(
+            self.FuzzyMatchUrl(target_branches), None)
+        if patch_ref:
+          latest_commit = self._used_scm.apply_patch_ref(
+              patch_repo, patch_ref, target_branch, options, file_list)
+        else:
+          latest_commit = self._used_scm.revinfo(None, None, None)
+        existing_sync_commits = json.loads(
+            os.environ.get(PREVIOUS_SYNC_COMMITS, '{}'))
+        existing_sync_commits[self.name] = latest_commit
+        os.environ[PREVIOUS_SYNC_COMMITS] = json.dumps(existing_sync_commits)
 
       if file_list:
         file_list = [os.path.join(self.name, f.strip()) for f in file_list]
@@ -1637,15 +1645,6 @@ it or fix the checkout.
               ', '.join(s.name for s in client.dependencies[1:])),
           file=sys.stderr)
 
-    if any('@' not in r for r in options.skip_sync_revisions):
-      raise gclient_utils.Error(
-          "You must specify the full solution name like --revision src@abc")
-    skip_sync_names = [rev.split('@')[0] for rev in options.skip_sync_revisions]
-    sol_names = [s.name for s in client.dependencies]
-    if any(name not in sol_names for name in skip_sync_names):
-      raise gclient_utils.Error(
-          "--skip_sync_revisions are only allowed for solutions.")
-
     return client
 
   def SetDefaultConfig(self, solution_name, deps_file, solution_url,
@@ -1702,12 +1701,15 @@ it or fix the checkout.
   def _EnforceSkipSyncRevisions(self, patch_refs):
     # type: (Mapping[str, str]) -> Mapping[str, str]
     """Checks for and enforces revisions for skipping deps syncing."""
-    if not self._options.skip_sync_revisions:
+    previous_sync_commits = json.loads(
+        os.environ.get(PREVIOUS_SYNC_COMMITS, '{}'))
+
+    if not previous_sync_commits:
       return {}
 
     # Current `self.dependencies` only contain solutions. If a patch_ref is
     # not for a solution, then it is for a solution's dependency or recursed
-    # dependency which we cannot support with skip_sync_revisions.
+    # dependency which we cannot support while skipping sync.
     if patch_refs:
       unclaimed_prs = []
       candidates = []
@@ -1719,9 +1721,9 @@ it or fix the checkout.
           unclaimed_prs.append(patch_repo)
       if unclaimed_prs:
         print(
-            'Ignoring all --skip-sync-revisions. It cannot be used when there '
-            'are --patch-refs flags for non-solution dependencies. To skip '
-            'syncing remove patch_refs for: \n%s' % '\n'.join(unclaimed_prs))
+            'We cannot skip syncs when there are --patch-refs flags for '
+            'non-solution dependencies. To skip syncing, remove patch_refs '
+            'for: \n%s' % '\n'.join(unclaimed_prs))
         return {}
 
     # We cannot skip syncing if there are custom_vars that differ from the
@@ -1729,16 +1731,16 @@ it or fix the checkout.
     previous_custom_vars = json.loads(os.environ.get(PREVIOUS_CUSTOM_VARS,
                                                      '{}'))
     cvs_by_name = {s.name: s.custom_vars for s in self.dependencies}
+
     skip_sync_revisions = {}
-    for revision in self._options.skip_sync_revisions:
-      name, rev = revision.split('@', 1)
+    for name, commit in previous_sync_commits.items():
       previous_vars = previous_custom_vars.get(name, {})
       if previous_vars == cvs_by_name.get(name):
-        skip_sync_revisions[name] = rev
+        skip_sync_revisions[name] = commit
       else:
-        print('--skip-sync-revisions cannot be used for solutions where '
-              'custom_vars is different from custom_vars of the last run on '
-              'this machine.\nRemoving skip_sync_revision for:\n'
+        print('We cannot skip syncs when custom_vars for solutions have '
+              'changed since the last sync run on this machine.\n'
+              '\nRemoving skip_sync_revision for:\n'
               'solution: %s, current: %r, previous: %r.' %
               (name, cvs_by_name.get(name), previous_vars))
     return skip_sync_revisions
@@ -1924,6 +1926,14 @@ it or fix the checkout.
       # TODO(crbug.com/1339472): Pass skip_sync_revisions to flush()
       _skip_sync_revisions = self._EnforceSkipSyncRevisions(patch_refs)
 
+      # Store solutions' custom_vars on disk to compare in the next run.
+      # All dependencies added later are inherited from the current
+      # self.dependencies.
+      custom_vars = {}
+      for dep in self.dependencies:
+        custom_vars[dep.name] = dep.custom_vars
+      os.environ[PREVIOUS_CUSTOM_VARS] = json.dumps(sorted(custom_vars))
+
     # Disable progress for non-tty stdout.
     should_show_progress = (
         setup_color.IS_TTY and not self._options.verbose and progress)
@@ -1977,12 +1987,6 @@ it or fix the checkout.
         pm = Progress('Running hooks', 1)
       self.RunHooksRecursively(self._options, pm)
 
-    # Store custom_vars on disk to compare in the next run.
-    custom_vars = {}
-    for dep in self.dependencies:
-      custom_vars[dep.name] = dep.custom_vars
-    os.environ[PREVIOUS_CUSTOM_VARS] = json.dumps(sorted(custom_vars))
-
     return 0
 
   def PrintRevInfo(self):
@@ -3300,8 +3304,6 @@ class OptionParser(optparse.OptionParser):
     if not hasattr(options, 'revisions'):
       # GClient.RunOnDeps expects it even if not applicable.
       options.revisions = []
-    if not hasattr(options, 'skip_sync_revisions'):
-      options.skip_sync_revisions = []
     if not hasattr(options, 'head'):
       options.head = None
     if not hasattr(options, 'nohooks'):

gclient_scm.py

@@ -399,6 +399,7 @@ class GitWrapper(SCMWrapper):
 
   def apply_patch_ref(self, patch_repo, patch_rev, target_rev, options,
                       file_list):
+    # type: (str, str, str, optparse.Values, Collection[str]) -> str
     """Apply a patch on top of the revision we're synced at.
 
     The patch ref is given by |patch_repo|@|patch_rev|.
@@ -440,7 +441,7 @@ class GitWrapper(SCMWrapper):
     except subprocess2.CalledProcessError:
       pass
 
-    base_rev = self._Capture(['rev-parse', 'HEAD'])
+    base_rev = self.revinfo(None, None, None)
 
     if not target_rev:
       raise gclient_utils.Error('A target revision for the patch must be given')
@@ -533,8 +534,10 @@ class GitWrapper(SCMWrapper):
       if file_list is not None:
         file_list.extend(self._GetDiffFilenames(base_rev))
 
+    latest_commit = self.revinfo(None, None, None)
     if options.reset_patch_ref:
       self._Capture(['reset', '--soft', base_rev])
+    return latest_commit
 
   def check_diff(self, previous_commit, files=None):
     # type: (str, Optional[List[str]]) -> bool

tests/gclient_test.py

@@ -68,6 +68,9 @@ class SCMMock(object):
   def GetActualRemoteURL(self, _):
     return self.url
 
+  def revinfo(self, _, _a, _b):
+    return 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
+
 
 class GclientTest(trial_dir.TestCase):
   def setUp(self):
@@ -1546,44 +1549,23 @@ class GclientTest(trial_dir.TestCase):
     foo_sol = obj.dependencies[0]
     self.assertEqual('foo', foo_sol.FuzzyMatchUrl(['foo']))
 
-  def testLoadCurrentConfig_SkipSyncRevisions(self):
-    """Invalid skip_sync_revisions should raise an error."""
-    write(
-        '.gclient', 'solutions = [\n'
-        '  { "name": "foo", "url": "https://example.com/foo",\n'
-        '    "deps_file" : ".DEPS.git",\n'
-        '  },\n'
-        ']')
-    write(
-        os.path.join('foo', 'DEPS'), 'deps = {\n'
-        '  "bar": "https://example.com/bar.git@bar_version",\n'
-        '}')
-    options, _ = gclient.OptionParser().parse_args([])
-
-    options.skip_sync_revisions = ['1234']
-    with self.assertRaises(gclient_utils.Error):
-      gclient.GClient.LoadCurrentConfig(options)
-
-    options.skip_sync_revisions = ['notasolution@12345']
-    with self.assertRaises(gclient_utils.Error):
-      gclient.GClient.LoadCurrentConfig(options)
-
   def testEnforceSkipSyncRevisions_DepsPatchRefs(self):
     """Patch_refs for any deps removes all skip_sync_revisions."""
     write(
         '.gclient', 'solutions = [\n'
-        '  { "name": "foo", "url": "https://example.com/foo",\n'
+        '  { "name": "foo/src", "url": "https://example.com/foo",\n'
         '    "deps_file" : ".DEPS.git",\n'
         '  },\n'
         ']')
     write(
-        os.path.join('foo', 'DEPS'), 'deps = {\n'
+        os.path.join('foo/src', 'DEPS'), 'deps = {\n'
         '  "bar": "https://example.com/bar.git@bar_version",\n'
         '}')
     options, _ = gclient.OptionParser().parse_args([])
-    options.skip_sync_revisions = ['foo@1234']
+    os.environ[gclient.PREVIOUS_SYNC_COMMITS] = json.dumps(
+        {'foo/src': '1234'})
     client = gclient.GClient.LoadCurrentConfig(options)
-    patch_refs = {'foo': '1222', 'somedeps': '1111'}
+    patch_refs = {'foo/src': '1222', 'somedeps': '1111'}
     self.assertEqual({}, client._EnforceSkipSyncRevisions(patch_refs))
 
   def testEnforceSkipSyncRevisions_CustomVars(self):
@@ -1627,9 +1609,12 @@ class GclientTest(trial_dir.TestCase):
     options, _ = gclient.OptionParser().parse_args([])
 
     patch_refs = {'samevars': '1222'}
-    options.skip_sync_revisions = [
-        'samevars@10001', 'diffvars@10002', 'novars@10003'
-    ]
+    previous_sync_commits = {'samevars': '10001',
+                             'diffvars': '10002',
+                             'novars': '10003'}
+    os.environ[
+        gclient.PREVIOUS_SYNC_COMMITS] = json.dumps(previous_sync_commits)
+
     expected_skip_sync_revisions = {'samevars': '10001', 'novars': '10003'}
 
     client = gclient.GClient.LoadCurrentConfig(options)