Revert "Reland "[gclient] Read submodule status information""

This reverts commit 452cb7b6cb8603ea05f80acd252c8d7f05b9b88a.

Reason for revert: v8 deps roll failed, gclient revinfo is
misbehaving.

Original change's description:
> Reland "[gclient] Read submodule status information"
>
> This reverts commit c0366630f7810d63aff8e02ea38faf7530491f0f.
>
> Original change's description:
> > Revert "[gclient] Read submodule status information"
> >
> > This reverts commit 39501ba8c935b272c49853d2fa587a5e618968b2.
> >
> > Reason for revert: broke submodule update builder: gclient revinfo
> >
> >
> >
> > Original change's description:
> > > [gclient] Read submodule status information
> > >
> > > This allow us to skip sync if we know the state is correct.
> > >
> > > R=gavinmak@google.com
> > >
> > > Bug: 40283612, 40942309
> > > Change-Id: I30fd5bfb9ca8ab0f7dcce567e2a5cb4aebdc7b2f
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5480172
> > > Commit-Queue: Josip Sokcevic <sokcevic@chromium.org>
> > > Reviewed-by: Gavin Mak <gavinmak@google.com>
> >
> > Bug: 40283612, 40942309
> > Change-Id: Iaf478c65eb6f18b19afc3fdda2996190bdc58613
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5506653
> > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> > Auto-Submit: Josip Sokcevic <sokcevic@chromium.org>
> > Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
>
> Bug: 40283612, 40942309
> Change-Id: I707b0eb1d3850c415e474df8cd43f561054017cb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5506833
> Commit-Queue: Josip Sokcevic <sokcevic@chromium.org>
> Reviewed-by: Gavin Mak <gavinmak@google.com>

Bug: 40283612, 40942309
Change-Id: Id667a0e71e6e0a0e74c98ea96952a96c024e3289
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5513187
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Josip Sokcevic <sokcevic@chromium.org>

gclient.py

@@ -295,19 +295,7 @@ class DependencySettings(object):
         # These are not mutable:
         self._parent = parent
         self._deps_file = deps_file
-
-        # Post process the url to remove trailing slashes.
-        if isinstance(url, str):
-            # urls are sometime incorrectly written as proto://host/path/@rev.
-            # Replace it to proto://host/path@rev.
-            self._url = url.replace('/@', '@')
-        elif isinstance(url, (None.__class__)):
-            self._url = url
-        else:
-            raise gclient_utils.Error(
-                ('dependency url must be either string or None, '
-                 'instead of %s') % url.__class__.__name__)
-
+        self._url = url
         # The condition as string (or None). Useful to keep e.g. for flatten.
         self._condition = condition
         # 'managed' determines whether or not this dependency is synced/updated
@@ -333,6 +321,16 @@ class DependencySettings(object):
         self._custom_deps = custom_deps or {}
         self._custom_hooks = custom_hooks or []
 
+        # Post process the url to remove trailing slashes.
+        if isinstance(self.url, str):
+            # urls are sometime incorrectly written as proto://host/path/@rev.
+            # Replace it to proto://host/path@rev.
+            self.set_url(self.url.replace('/@', '@'))
+        elif not isinstance(self.url, (None.__class__)):
+            raise gclient_utils.Error(
+                ('dependency url must be either string or None, '
+                 'instead of %s') % self.url.__class__.__name__)
+
         # Make any deps_file path platform-appropriate.
         if self._deps_file:
             for sep in ['/', '\\']:
@@ -487,9 +485,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         # custom_deps, if any.
         self._should_sync = True
 
-        self._known_dependency_diff = None
-        self._dependency_index_state = None
-
         self._OverrideUrl()
         # This is inherited from WorkItem.  We want the URL to be a resource.
         if self.url and isinstance(self.url, str):
@@ -593,14 +588,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         ])
         return s
 
-    @property
-    def known_dependency_diff(self):
-        return self._known_dependency_diff
-
-    @property
-    def dependency_index_state(self):
-        return self._dependency_index_state
-
     @property
     def requirements(self):
         """Calculate the list of requirements."""
@@ -944,12 +931,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         if self.git_dependencies_state == gclient_eval.SUBMODULES:
             deps.update(self.ParseGitSubmodules())
 
-        if self.git_dependencies_state != gclient_eval.DEPS:
-            # Git submodules are used - get their state.
-            self._known_dependency_diff = self.CreateSCM().GetSubmoduleDiff()
-            self._dependency_index_state = self.CreateSCM(
-            ).GetSubmoduleStateFromIndex()
-
         deps_to_add = self._deps_to_objects(
             self._postprocess_deps(deps, rel_prefix), self._use_relative_paths)
 
@@ -1177,20 +1158,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
                 try:
                     start = time.time()
                     sync_status = metrics_utils.SYNC_STATUS_FAILURE
-                    if self.parent and self.parent.known_dependency_diff is not None:
-                        if self._use_relative_paths:
-                            path = self.name
-                        else:
-                            path = self.name[len(self.parent.name) + 1:]
-                        current_revision = None
-                        if path in self.parent.dependency_index_state:
-                            current_revision = self.parent.dependency_index_state[
-                                path]
-                        if path in self.parent.known_dependency_diff:
-                            current_revision = self.parent.known_dependency_diff[
-                                path][1]
-                        self._used_scm.current_revision = current_revision
-
                     self._got_revision = self._used_scm.RunCommand(
                         command, options, args, file_list)
                     latest_commit = self._got_revision
@@ -2155,21 +2122,6 @@ it or fix the checkout.
 
         removed_cipd_entries = []
         read_entries = self._ReadEntries()
-        # Add known dependency state
-        queue = list(self.dependencies)
-        while len(queue) > 0:
-            dep = queue.pop()
-            queue.extend(dep.dependencies)
-            if not dep._known_dependency_diff:
-                continue
-
-            for k, v in dep._known_dependency_diff.items():
-                path = f'{dep.name}/{k}'
-                if path in read_entries:
-                    continue
-                read_entries[path] = f'https://unknown@{v[1]}'
-
-
         # We process entries sorted in reverse to ensure a child dir is
         # always deleted before its parent dir.
         # This is especially important for submodules with pinned revisions

gclient_scm.py

@@ -229,7 +229,6 @@ class GitWrapper(SCMWrapper):
             filter_kwargs['predicate'] = self.out_cb
         self.filter = gclient_utils.GitFilter(**filter_kwargs)
         self._running_under_rosetta = None
-        self.current_revision = None
 
     def GetCheckoutRoot(self):
         return scm.GIT.GetCheckoutRoot(self.checkout_path)
@@ -251,69 +250,6 @@ class GitWrapper(SCMWrapper):
                     ['-c', 'core.quotePath=false', 'diff', '--name-only', base])
             )).split()
 
-    def GetSubmoduleStateFromIndex(self):
-        """Returns a map where keys are submodule names and values are commit
-        hashes. It reads data from the Git index, so only committed values are
-        present."""
-        out = self._Capture(['ls-files', '-s'])
-        result = {}
-        for l in out.split('\n'):
-            if not l.startswith('160000'):
-                # Not a submodule
-                continue
-            (_, commit, _, filepath) = l.split(maxsplit=3)
-            result[filepath] = commit
-        return result
-
-    def GetSubmoduleDiff(self):
-        """Returns a map where keys are submodule names and values are tuples of
-        (old_commit_hash, new_commit_hash). old_commit_hash matches the Git
-        index, whereas new_commit_hash matches currently checked out commit
-        hash."""
-        out = self._Capture([
-            'diff', '--no-prefix', '--no-ext-diff', '--no-color',
-            '--ignore-submodules=dirty', '--submodule=short'
-        ])
-        NO_COMMIT = 40 * '0'
-        committed_submodule = None
-        checked_submodule = None
-        filepath = None
-        state = 0
-        diff = {}
-        # Parsing git diff uses simple state machine. States:
-        # 0 - start state
-        # 1 - diff file/line detected, ready to process content
-        # 2 - gitlink detected, ready to process gitlink past and current
-        # content.
-        # 3 - past gitlink content detected. It contains a commit hash that's in
-        # git index.
-        # 4 - new gitlink content detected. It contains currently checked
-        # commit. At this point, we have all information needed, and we can
-        # reset state to 0.
-        for l in out.split('\n'):
-            if l.startswith('diff --git'):
-                # New file detected, reset state.
-                state = 1
-            elif state == 1 and l.startswith('index') and l.endswith('160000'):
-                # We detected gitlink
-                state = 2
-            elif state == 2 and l.startswith('+++ '):
-                # This line contains filename
-                filepath = l[4:]
-                state = 3
-            elif state == 3 and l.startswith('-Subproject commit '):
-                # This line contains what commit hash Git index expects
-                # (ls-files).
-                committed_submodule = l.split(' ')[-1]
-                state = 4
-            elif state == 4 and l.startswith('+Subproject commit '):
-                # This line contains currently checked out commit for this submodule.
-                checked_submodule = l.split(' ')[-1]
-                if NO_COMMIT not in (committed_submodule, checked_submodule):
-                    diff[filepath] = (committed_submodule, checked_submodule)
-                state = 0
-        return diff
-
     def diff(self, options, _args, _file_list):
         _, revision = gclient_utils.SplitUrlRevision(self.url)
         if not revision:
@@ -702,6 +638,7 @@ class GitWrapper(SCMWrapper):
             raise gclient_utils.Error("Unsupported argument(s): %s" %
                                       ",".join(args))
 
+        current_revision = None
         url, deps_revision = gclient_utils.SplitUrlRevision(self.url)
         revision = deps_revision
         managed = True
@@ -777,11 +714,11 @@ class GitWrapper(SCMWrapper):
                 self._UpdateMirrorIfNotContains(mirror, options, rev_type,
                                                 revision)
             try:
-                self.current_revision = self._Clone(revision, url, options)
+                current_revision = self._Clone(revision, url, options)
             except subprocess2.CalledProcessError as e:
                 logging.warning('Clone failed due to: %s', e)
                 self._DeleteOrMove(options.force)
-                self.current_revision = self._Clone(revision, url, options)
+                current_revision = self._Clone(revision, url, options)
             if file_list is not None:
                 files = self._Capture(
                     ['-c', 'core.quotePath=false', 'ls-files']).splitlines()
@@ -805,17 +742,6 @@ class GitWrapper(SCMWrapper):
                        self.relpath)
             return self._Capture(['rev-parse', '--verify', 'HEAD'])
 
-        # Special case for rev_type = hash. If we use submodules, we can check
-        # information already.
-        if rev_type == 'hash':
-            if self.current_revision == revision:
-                if verbose:
-                    self.Print('Using submodule information to skip check')
-                if options.reset or options.force:
-                    self._Scrub('HEAD', options)
-
-                return revision
-
         self._maybe_break_locks(options)
 
         if mirror:

tests/gclient_scm_test.py

@@ -139,7 +139,6 @@ Personalized
 from :3
 M 100644 :4 a
 M 100644 :5 b
-M 160000 1111111111111111111111111111111111111111 submodule
 
 blob
 mark :7
@@ -268,7 +267,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         scm.revert(options, self.args, file_list)
         self.assertEqual(file_list, [])
         self.assertEqual(scm.revinfo(options, self.args, None),
-                         '4091c7d010ca99d0f2dd416d4b70b758ae432187')
+                         'a7142dc9f0009350b96a11f372b6ea658592aa95')
         sys.stdout.close()
 
     def testRevertModified(self):
@@ -288,7 +287,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         scm.diff(options, self.args, file_list)
         self.assertEqual(file_list, [])
         self.assertEqual(scm.revinfo(options, self.args, None),
-                         '4091c7d010ca99d0f2dd416d4b70b758ae432187')
+                         'a7142dc9f0009350b96a11f372b6ea658592aa95')
         sys.stdout.close()
 
     def testRevertNew(self):
@@ -310,7 +309,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         scm.diff(options, self.args, file_list)
         self.assertEqual(file_list, [])
         self.assertEqual(scm.revinfo(options, self.args, None),
-                         '4091c7d010ca99d0f2dd416d4b70b758ae432187')
+                         'a7142dc9f0009350b96a11f372b6ea658592aa95')
         sys.stdout.close()
 
     def testStatusRef(self):
@@ -390,16 +389,14 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         if not self.enabled:
             return
         options = self.Options()
-        expected_file_list = [
-            join(self.base_path, x) for x in ['a', 'b', 'submodule']
-        ]
+        expected_file_list = [join(self.base_path, x) for x in ['a', 'b']]
         scm = gclient_scm.GitWrapper(self.url, self.root_dir, self.relpath)
         file_list = []
 
         scm.update(options, (), file_list)
         self.assertEqual(file_list, expected_file_list)
         self.assertEqual(scm.revinfo(options, (), None),
-                         '4091c7d010ca99d0f2dd416d4b70b758ae432187')
+                         'a7142dc9f0009350b96a11f372b6ea658592aa95')
         self.assertEqual(
             scm._Capture(['config', '--get', 'diff.ignoreSubmodules']), 'dirty')
         self.assertEqual(
@@ -421,13 +418,12 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         rev = scm.revinfo(options, (), None)
         file_list = []
         scm.update(options, (), file_list)
-        self.assertEqual(
-            file_list,
-            [join(self.base_path, x) for x in ['a', 'b', 'c', 'submodule']])
+        self.assertEqual(file_list,
+                         [join(self.base_path, x) for x in ['a', 'b', 'c']])
         # The actual commit that is created is unstable, so we verify its tree
         # and parents instead.
         self.assertEqual(scm._Capture(['rev-parse', 'HEAD:']),
-                         '3a3ba72731fa208d37b06598a129ba93970325df')
+                         'd2e35c10ac24d6c621e14a1fcadceb533155627d')
         parent = 'HEAD^' if sys.platform != 'win32' else 'HEAD^^'
         self.assertEqual(scm._Capture(['rev-parse', parent + '1']), rev)
         self.assertEqual(scm._Capture(['rev-parse', parent + '2']),
@@ -449,13 +445,12 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
             '(y)es / (q)uit / (s)kip : ', 'y')
         file_list = []
         scm.update(options, (), file_list)
-        self.assertEqual(
-            file_list,
-            [join(self.base_path, x) for x in ['a', 'b', 'c', 'submodule']])
+        self.assertEqual(file_list,
+                         [join(self.base_path, x) for x in ['a', 'b', 'c']])
         # The actual commit that is created is unstable, so we verify its tree
         # and parent instead.
         self.assertEqual(scm._Capture(['rev-parse', 'HEAD:']),
-                         '3a3ba72731fa208d37b06598a129ba93970325df')
+                         'd2e35c10ac24d6c621e14a1fcadceb533155627d')
         parent = 'HEAD^' if sys.platform != 'win32' else 'HEAD^^'
         self.assertEqual(scm._Capture(['rev-parse', parent + '1']),
                          scm._Capture(['rev-parse', 'origin/main']))
@@ -806,15 +801,14 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
         self.relpath = '.'
         self.base_path = join(self.root_dir, self.relpath)
         url_with_commit_ref = origin_root_dir +\
-                              '@4091c7d010ca99d0f2dd416d4b70b758ae432187'
+                              '@a7142dc9f0009350b96a11f372b6ea658592aa95'
 
         scm = gclient_scm.GitWrapper(url_with_commit_ref, self.root_dir,
                                      self.relpath)
 
         expected_file_list = [
             join(self.base_path, "a"),
-            join(self.base_path, "b"),
-            join(self.base_path, "submodule"),
+            join(self.base_path, "b")
         ]
         file_list = []
         options.revision = 'unmanaged'
@@ -822,11 +816,11 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
         self.assertEqual(file_list, expected_file_list)
         self.assertEqual(scm.revinfo(options, (), None),
-                         '4091c7d010ca99d0f2dd416d4b70b758ae432187')
+                         'a7142dc9f0009350b96a11f372b6ea658592aa95')
         # indicates detached HEAD
         self.assertEqual(self.getCurrentBranch(), None)
         self.checkInStdout(
-            'Checked out 4091c7d010ca99d0f2dd416d4b70b758ae432187 to a detached HEAD'
+            'Checked out a7142dc9f0009350b96a11f372b6ea658592aa95 to a detached HEAD'
         )
 
     def testUpdateCloneOnBranch(self):
@@ -1608,57 +1602,6 @@ class CheckDiffTest(fake_repos.FakeReposTestBase):
         self.assertFalse(scm.check_diff(self.githash('repo_1', 2)))
 
 
-class Submodules(BaseGitWrapperTestCase):
-    submodule_hash = '1111111111111111111111111111111111111111'
-
-    def testGetSubmoduleClean(self):
-        scm = gclient_scm.GitWrapper(self.url, self.root_dir, self.relpath)
-        options = self.Options()
-        scm.update(options, None, [])
-        self.assertEqual(scm.GetSubmoduleStateFromIndex(),
-                         {'submodule': self.submodule_hash})
-        self.assertEqual(scm.GetSubmoduleDiff(), {})
-
-    def testGetSubmoduleModified(self):
-        scm = gclient_scm.GitWrapper(self.url, self.root_dir, self.relpath)
-        options = self.Options()
-        scm.update(options, None, [])
-
-        # Create submodule diff
-        submodule_dir = os.path.join(self.root_dir, 'submodule')
-        subprocess2.check_output(['git', '-C', submodule_dir, 'init'])
-        subprocess2.check_output([
-            'git', '-C', submodule_dir, 'commit', '-m', 'foo', '--allow-empty'
-        ])
-        new_rev = subprocess2.check_output(
-            ['git', '-C', submodule_dir, 'rev-parse',
-             'HEAD']).decode('utf-8').strip()
-
-        # And file diff
-        with open(os.path.join(self.root_dir, 'a'), 'w') as f:
-            f.write('foo')
-
-        self.assertEqual(scm.GetSubmoduleStateFromIndex(),
-                         {'submodule': self.submodule_hash})
-
-        self.assertEqual(scm.GetSubmoduleDiff(),
-                         {'submodule': (self.submodule_hash, new_rev)})
-
-    def testGetSubmoduleDeleted(self):
-        scm = gclient_scm.GitWrapper(self.url, self.root_dir, self.relpath)
-        options = self.Options()
-        scm.update(options, None, [])
-        subprocess2.check_output(
-            ['git', '-C', self.root_dir, 'rm', 'submodule'])
-
-        # When git removes submodule, it's autmatically staged and content is
-        # unavailable. Therefore, the index shouldn't have any entries and diff
-        # should be empty.
-        self.assertEqual(scm.GetSubmoduleStateFromIndex(), {})
-
-        self.assertEqual(scm.GetSubmoduleDiff(), {})
-
-
 if 'unittest.util' in __import__('sys').modules:
     # Show full diff in self.assertEqual.
     __import__('sys').modules['unittest.util']._MAX_LENGTH = 999999999