You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
$ git initInitialized empty Git repository in /tmp/tmp.Xoz9gZndOi/.git/
$ venv/bin/python sscce.pyTraceback (most recent call last): File "sscce.py", line 5, in <module> git.Repo(".").tags[None] File "/tmp/tmp.Xoz9gZndOi/venv/lib/python3.8/site-packages/git/util.py", line 1087, in __getitem__ assert isinstance(index, (int, str, slice)), "Index of IterableList should be an int or str"AssertionError: Index of IterableList should be an int or str
assertisinstance(index, (int, str, slice)), "Index of IterableList should be an int or str"
assert should not be used in product code because it can be ignored with -O:
$ venv/bin/python -O sscce.pyTraceback (most recent call last): File "sscce.py", line 5, in <module> git.Repo(".").tags[None] File "/tmp/tmp.Xoz9gZndOi/venv/lib/python3.8/site-packages/git/util.py", line 1095, in __getitem__ return getattr(self, index)TypeError: getattr(): attribute name must be string
(In fact, that behavior is probably better since a TypeError is more semantically meaningful.)
$ venv/bin/bandit venv/lib/python3.8/site-packages/git/util.py...>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Severity: Low Confidence: High CWE: CWE-703 (https://cwe.mitre.org/data/definitions/703.html) Location: venv/lib/python3.8/site-packages/git/util.py:1087:8 More Info: https://bandit.readthedocs.io/en/1.7.4/plugins/b101_assert_used.html10861087 assert isinstance(index, (int, str, slice)), "Index of IterableList should be an int or str"10881089 if isinstance(index, int):...
The text was updated successfully, but these errors were encountered:
What kind of fix should be done for this? As in the example given above where TypeError would be preferable to AssertionError, the type of exception that should be raised, when raising an exception to validate arguments passed to a public function, is usually not AssertionError.
Changing it may break existing code that uses GitPython, delegates validating those preconditions to the function being called, and catches AssertionError. On the other hand, unless the behavior of raising an exception (or any particular exception type) is documented, I think calling code should not assume that will happen, at least not when it can feasibly know what needs to be validated. So arguably it is a bug for callers to do that, and causing different exceptions to be raised here might not be a breaking change.
Such breakage could nonetheless be avoided--assuming the behavior without assert disabled is the one that is to be kept--by raising AssertionError or a type that derives from AssertionError. But that would dig in deeper on something that may be better to change.
I'd also think that exceptions shouldn't be assumed as part of the API unless they are documented, which would make it possible remove the assertion entirely.
From my todays perspective, I'd also think one shouldn't try to build a runtime-type-checker with assertions, but instead leave it to the python runtime to emit exceptions instead. They are probably less clear, but it's a trade-off to unnecessarily forbidding otherwise usable types with assertions.
Thus, removing the these type assertions would be preferred here.
GitPython/git/util.py
Line 1087 in c84dde2
assert
should not be used in product code because it can be ignored with-O
:(In fact, that behavior is probably better since a
TypeError
is more semantically meaningful.)bandit
can catch this kind of thing:The text was updated successfully, but these errors were encountered: