Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unpickled Git object has broken version_info #1836

Closed
EliahKagan opened this issue Feb 22, 2024 · 0 comments · Fixed by #1838
Closed

Unpickled Git object has broken version_info #1836

EliahKagan opened this issue Feb 22, 2024 · 0 comments · Fixed by #1838

Comments

@EliahKagan
Copy link
Contributor

A Git instance produced by pickle deserialization always has a version_info of None. This prevents version information from being accessed through version_info. It also causes the version_info property not to satisfy its type annotation as a tuple of ints (independently of #1830). This is the case whether or not version_info has been accessed on the original instance before it was pickled:

>>> import pickle, git
>>> g = git.Git()
>>> g.version_info
(2, 34, 1)
>>> g2 = pickle.loads(pickle.dumps(g))
>>> g2.version_info
>>>
>>> import pickle, git
>>> g = git.Git()
>>> g2 = pickle.loads(pickle.dumps(g))
>>> g2.version_info
>>>

This is due to an inconsistency in how the state of not yet having computed version_info is represented in the backing attribute _version_info:

  • Git uses LazyMixin caching for _version_info, which treats the attribute as uncomputed when absent and computed when present.
  • Git defines __getstate__ and __setstate__ for pickling and unpickling to represent state pickling does not preserve by setting attributes to None.

The _version_info attribute is set to None on a Git object created by unpickling, so when accessing the version_info property delegates to the _version_info attribute, its value of None is returned and the if attr == "_version_info" logic in the _set_cache_ method (inherited from LazyMixin and overridden) is never run.

The intention is that an unpickled Git object have version_info in the uncomputed state. This works for the other two excluded attributes, which use the same None convention as in the __getstate__ and __setstate__ implementations, and which do not use any LazyMixin functionality for their caching:

_excluded_ = ("cat_file_all", "cat_file_header", "_version_info")

GitPython/git/cmd.py

Lines 319 to 323 in afa5754

def __getstate__(self) -> Dict[str, Any]:
return slots_to_dict(self, exclude=self._excluded_)
def __setstate__(self, d: Dict[str, Any]) -> None:
dict_to_slots_and__excluded_are_none(self, d, excluded=self._excluded_)

Unlike _version_info, which is managed by _set_cache_, the cat_file_all and cat_file_header attributes are initialized to None in Git.__init__:

GitPython/git/cmd.py

Lines 786 to 788 in afa5754

# Cached command slots
self.cat_file_header: Union[None, TBD] = None
self.cat_file_all: Union[None, TBD] = None

This is natural to fix at the same time as #1829, because a fix for that should change the way caching works. LazyMixin caching supports attributes that are computed once and never become stale, so version_info/_version_info should implement caching in a different way to fix #1829, and that can fix this too if consistent with __getstate__/__setstate__.

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Feb 22, 2024
The new test doesn't pass yet, as gitpython-developers#1836 is not yet fixed.
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Feb 22, 2024
For gitpython-developers#1836.

This uses a property with the same logic as before for version_info
caching, except that the _version_info backing attribute holds the
value None, rather than being unset, for the uncomputed state. This
rectifies the inconistency between the property's behavior and the
way the associated states are represented by its __setstate__ and
__getstate__ methods for pickling.

Because the Git class only used LazyMixin as an implementation
detail of the version_info attribute, it is removed as a subclass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants