Submodule.__init__ parent_commit conversion/validation is implied but not done

The parent_commit parameter of Submodule.__init__ is documented this way:

Lines 135 to 136 in edb8d26

    
                   :param parent_commit: 
        
                       See :meth:`set_parent_commit`.

Submodule.__init__ binds this directly to the private _parent_commit attribute:

GitPython/git/objects/submodule/base.py

Line 147 in edb8d26

self._parent_commit = parent_commit

But this is at odds with the documented relationship to Submodule.set_parent_commit. That method's commit parameter corresponds to the parent_commit parameter of Submodule.__init__, in that its commit parameter is used to identify the commit, if any, to set as _parent_commit. However, set_parent_commit performs both conversion and validation.

This is the relevant fragment of set_parent_commit's docstring:

GitPython/git/objects/submodule/base.py

Lines 1253 to 1255 in edb8d26

    
                   :param commit: 
        
                       Commit-ish reference pointing at the root_tree, or ``None`` to always point 
        
                       to the most recent commit.

When commit is None, it sets None to _parent_commit. Otherwise, however, commit may not already be a Commit object, and that is okay, because a commit is looked up from it:

GitPython/git/objects/submodule/base.py

Line 1274 in edb8d26

pcommit = self.repo.commit(commit)

That's the conversion. Then validation is performed, with _parent_commit ending up set to the commit that commit identified only if there is such a suitable commit:

GitPython/git/objects/submodule/base.py

Lines 1275 to 1289 in edb8d26

    
           pctree = pcommit.tree 
        
           if self.k_modules_file not in pctree: 
        
               raise ValueError("Tree of commit %s did not contain the %s file" % (commit, self.k_modules_file)) 
        
           # END handle exceptions 
        
           prev_pc = self._parent_commit 
        
           self._parent_commit = pcommit 
        
           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

The type annotations do not reveal the intent, as they are among those using Commit_ish that need to be updated with the fix for Commit_ish, and that I am fixing up in #1859. My immediate motivation for opening this issue is that I'm having trouble figuring out how to annotate them, because due to the inconsistency between the docstring and the implementations, I don't know what is intended to be accepted.

Either the documentation should be updated, which could be part of #1859, or the code should be fixed to perform any expected validation and conversion and a test case added to check that this is working, which would be best done separately from #1859 (lest its scope expand ever further). I am not sure which. For #1859, it is likely sufficient for me to know what is intended, so full progress on this is not needed to finish #1859. It is my hope and also strong guess that this issue is not a blocker for #1859.

This should not be confused with #1866, which is about the parent_commits parameter of IndexFile.commit rather than the parent_commit parameter of Submodule.__init__ (and which, unlike this, really is about annotations).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submodule.init `parent_commit` conversion/validation is implied but not done #1869

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	:param commit:
	Commit-ish reference pointing at the root_tree, or ``None`` to always point
	to the most recent commit.

	pctree = pcommit.tree
	if self.k_modules_file not in pctree:
	raise ValueError("Tree of commit %s did not contain the %s file" % (commit, self.k_modules_file))
	# END handle exceptions

	prev_pc = self._parent_commit
	self._parent_commit = pcommit

	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

Submodule.__init__ parent_commit conversion/validation is implied but not done #1869

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Submodule.init `parent_commit` conversion/validation is implied but not done #1869