improve retrieving submodule entries from trees/commits

ryan-williams · ryan-williams · commit 7946cb8727b9 · 2020-11-26T22:42:35.000-05:00
diff --git a/git/objects/commit.py b/git/objects/commit.py
@@ -132,6 +132,9 @@ def __init__(self, repo, binsha, tree=None, author=None, authored_date=None, aut
         if gpgsig is not None:
             self.gpgsig = gpgsig
 
+    def __getitem__(self, item):
+        return self.tree.join(item, parent_commit=self)
+
     @classmethod
     def _get_intermediate_items(cls, commit):
         return commit.parents
diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
@@ -2,6 +2,7 @@
 from io import BytesIO
 import logging
 import os
+import re
 import stat
 from unittest import SkipTest
 import uuid
@@ -24,6 +25,7 @@
     BadName
 )
 from git.objects.base import IndexObject, Object
+from git.objects.submodule.util import SM_SECTION_NAME_REGEX
 from git.objects.util import Traversable
 from git.util import (
     Iterable,
@@ -116,20 +118,28 @@ def __init__(self, repo, binsha, mode=None, path=None, name=None, parent_commit=
             self._name = name
 
     def _set_cache_(self, attr):
-        if attr in ('path', '_url', '_branch_path'):
+        if attr in ('path', '_url', '_branch_path', '_name'):
             reader = self.config_reader()
             # default submodule values
             try:
                 self.path = reader.get('path')
             except cp.NoSectionError as e:
-                raise ValueError("This submodule instance does not exist anymore in '%s' file"
-                                 % osp.join(self.repo.working_tree_dir, '.gitmodules')) from e
+                if self.repo.working_tree_dir:
+                    raise ValueError("This submodule instance does not exist anymore in '%s' file"
+                                     % osp.join(self.repo.working_tree_dir, '.gitmodules')) from e
+                else:
+                    raise ValueError("This submodule instance does not exist anymore in bare repo at '%s'"
+                                     % self.repo.git_dir) from e
             # end
             self._url = reader.get('url')
             # git-python extension values - optional
             self._branch_path = reader.get_value(self.k_head_option, git.Head.to_full_path(self.k_head_default))
-        elif attr == '_name':
-            raise AttributeError("Cannot retrieve the name of a submodule if it was not set initially")
+            section_name = reader._section_name
+            m = re.match(SM_SECTION_NAME_REGEX, section_name)
+            if not m:
+                raise RuntimeError('Unexpected submodule section name in %s: %s' % (reader.file_or_files, section_name))
+            name = m['name']
+            self._name = name
         else:
             super(Submodule, self)._set_cache_(attr)
         # END handle attribute name
@@ -362,9 +372,7 @@ def add(cls, repo, name, path, url=None, branch=None, no_checkout=False, depth=N
         if sm.exists():
             # reretrieve submodule from tree
             try:
-                sm = repo.head.commit.tree[path]
-                sm._name = name
-                return sm
+                return repo.head.commit[path]
             except KeyError:
                 # could only be in index
                 index = repo.index
@@ -927,7 +935,7 @@ def remove(self, module=True, force=False, configuration=True, dry_run=False):
 
         return self
 
-    def set_parent_commit(self, commit, check=True):
+    def set_parent_commit(self, commit, check=True, pcommit_name=None):
         """Set this instance to use the given commit whose tree is supposed to
         contain the .gitmodules blob.
 
@@ -953,20 +961,17 @@ def set_parent_commit(self, commit, check=True):
 
         prev_pc = self._parent_commit
         self._parent_commit = pcommit
+        if pcommit_name:
+            self._name = pcommit_name
 
         if check:
-            parser = self._config_parser(self.repo, self._parent_commit, read_only=True)
-            if not parser.has_section(sm_section(self.name)):
-                self._parent_commit = prev_pc
-                raise ValueError("Submodule at path %r did not exist in parent commit %s" % (self.path, commit))
-            # END handle submodule did not exist
-        # END handle checking mode
+            self._set_cache_('path')
 
         # update our sha, it could have changed
         # If check is False, we might see a parent-commit that doesn't even contain the submodule anymore.
         # in that case, mark our sha as being NULL
         try:
-            self.binsha = pctree[self.path].binsha
+            self.binsha = pctree.join(self.path, parent_commit=pcommit).binsha
         except KeyError:
             self.binsha = self.NULL_BIN_SHA
         # end
@@ -1140,6 +1145,14 @@ def parent_commit(self):
             return self.repo.commit()
         return self._parent_commit
 
+    @parent_commit.setter
+    def parent_commit(self, parent_commit):
+        assert parent_commit
+        if self._parent_commit != parent_commit:
+            self._clear_cache()
+        self._parent_commit = parent_commit
+
+
     @property
     def name(self):
         """:return: The name of this submodule. It is used to identify it within the
@@ -1195,7 +1208,7 @@ def iter_items(cls, repo, parent_commit='HEAD'):
             # get the binsha
             index = repo.index
             try:
-                sm = rt[p]
+                sm = pc[p]
             except KeyError:
                 # try the index, maybe it was just added
                 try:
diff --git a/git/objects/submodule/util.py b/git/objects/submodule/util.py
@@ -10,6 +10,8 @@
 #{ Utilities
 
 
+SM_SECTION_NAME_REGEX = '^submodule "(?P<name>.*)"$'
+
 def sm_section(name):
     """:return: section title used in .gitmodules configuration file"""
     return 'submodule "%s"' % name
diff --git a/git/objects/tree.py b/git/objects/tree.py
@@ -207,7 +207,7 @@ def _iter_convert_to_object(self, iterable):
                 raise TypeError("Unknown mode %o found in tree data for path '%s'" % (mode, path)) from e
         # END for each item
 
-    def join(self, file):
+    def join(self, file, parent_commit=None):
         """Find the named object in this tree's contents
         :return: ``git.Blob`` or ``git.Tree`` or ``git.Submodule``
 
@@ -218,7 +218,7 @@ def join(self, file):
             item = self
             tokens = file.split('/')
             for i, token in enumerate(tokens):
-                item = tree[token]
+                item = tree.join(token, parent_commit=parent_commit)
                 if item.type == 'tree':
                     tree = item
                 else:
@@ -234,8 +234,15 @@ def join(self, file):
         else:
             for info in self._cache:
                 if info[2] == file:     # [2] == name
-                    return self._map_id_to_type[info[1] >> 12](self.repo, info[0], info[1],
-                                                               join_path(self.path, info[2]))
+                    type_id = info[1] >> 12
+                    typ = self._map_id_to_type[type_id]
+                    if typ == Submodule:
+                        binsha = info[0]
+                        mode = info[1]
+                        path = join_path(self.path, info[2])
+                        return typ(self.repo, binsha=binsha, mode=mode, path=path, parent_commit=parent_commit)
+                    else:
+                        return typ(self.repo, info[0], info[1], join_path(self.path, info[2]))
             # END for each obj
             raise KeyError(msg % file)
         # END handle long paths
diff --git a/test/test_submodule.py b/test/test_submodule.py
@@ -52,9 +52,7 @@ def tearDown(self):
     def _do_base_tests(self, rwrepo):
         """Perform all tests in the given repository, it may be bare or nonbare"""
         # manual instantiation
-        smm = Submodule(rwrepo, "\0" * 20)
-        # name needs to be set in advance
-        self.assertRaises(AttributeError, getattr, smm, 'name')
+        Submodule(rwrepo, b"\0" * 20, name='foo')
 
         # iterate - 1 submodule
         sms = Submodule.list_items(rwrepo, self.k_subm_current)
@@ -118,8 +116,13 @@ def _do_base_tests(self, rwrepo):
 
         # make the old into a new - this doesn't work as the name changed
         self.assertRaises(ValueError, smold.set_parent_commit, self.k_subm_current)
+
+        # passing the module name explicitly works
+        smold.set_parent_commit(self.k_subm_current, pcommit_name='gitdb')
+        assert smold.hexsha == 'f2233fbf40f3f69309ce5cc714e99fcbdcd33ec3'
         # the sha is properly updated
-        smold.set_parent_commit(self.k_subm_changed + "~1")
+        smold.set_parent_commit(self.k_subm_changed + "~1", pcommit_name='lib/git/ext/gitdb')
+        assert smold.hexsha == '18152febd428e67b86bb4fb68ec1691d4de75a9c'
         assert smold.binsha != sm.binsha
 
         # raises if the sm didn't exist in new parent - it keeps its
@@ -148,7 +151,7 @@ def _do_base_tests(self, rwrepo):
             ###########
             # preliminary tests
             # adding existing returns exactly the existing
-            sma = Submodule.add(rwrepo, sm.name, sm.path)
+            sma = Submodule.add(rwrepo, sm.name, sm.path, url=sm.path)
             assert sma.path == sm.path
 
             # no url and no module at path fails
@@ -713,7 +716,10 @@ def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
         sm2.move(sm2.path + '_moved')
 
         parent.index.commit("moved submodules")
-
+        # Below we'll test writing to the submodule's config, but first we update its `parent_commit` attr, otherwise it
+        # will still point to the previous commit, which is now in the parent's history (i.e. no longer HEAD), so
+        # attempting to write to it will fail
+        sm.parent_commit = parent.commit()
         with sm.config_writer() as writer:
             writer.set_value('user.email', 'example@example.com')
             writer.set_value('user.name', 'me')
@@ -945,3 +951,25 @@ def test_depth(self, rwdir):
         sm_depth = 1
         sm = parent.create_submodule(sm_name, sm_name, url=self._small_repo_url(), depth=sm_depth)
         self.assertEqual(len(list(sm.module().iter_commits())), sm_depth)
+
+    @with_rw_directory
+    def test_submodule_name(self, rwdir):
+        parent_dir = osp.join(rwdir, 'test_tree')
+        parent = git.Repo.init(parent_dir)
+        sm_name = 'test_module'
+        sm_path = 'mymodules/myname'
+        sm_dir = osp.join(parent_dir, sm_path)
+        parent.create_submodule(sm_name, sm_path, url=self._small_repo_url())
+        sm_repo = git.Repo(sm_dir)
+
+        sm_repo.git.checkout('v3.0.2')
+        parent.git.commit('-a','-m','add test submodule: smmap v3.0.2')
+        tree = parent.tree()
+        sm_tree = tree['mymodules/myname']
+        assert sm_tree.hexsha == 'f4d7a58b4d96200cd057a38a0758d3c84901f57e'
+
+        sm_repo.git.checkout('v3.0.4')
+        parent.git.commit('-a','-m','update test submodule to v3.0.4')
+        tree = parent.tree()
+        sm_tree = tree['mymodules/myname']
+        assert sm_tree.hexsha == '5e5f940dff80beaa3eedf9342ef502f5e630d5ed'
