Description

tucked

opened on Feb 5, 2023

Issue body actions

# sscce.py

import git

git.Repo(".").tags[None]

$ git init
Initialized empty Git repository in /tmp/tmp.Xoz9gZndOi/.git/
$ venv/bin/python sscce.py
Traceback (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

GitPython/git/util.py

Line 1087 in c84dde2

assert isinstance(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.py
Traceback (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.)

bandit can catch this kind of thing:

$ 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.html
1086
1087            assert isinstance(index, (int, str, slice)), "Index of IterableList should be an int or str"
1088
1089            if isinstance(index, int):
...

Activity

1. Byron

   added

   acknowledged

   help wanted

   on Feb 6, 2023
2. Byron commented on Feb 6, 2023

   

   Byron

   on Feb 6, 2023

   Member

   More actions

   Thanks for bringing this up! A PR with a fix is definitely welcome.

   Reacted by David Tucker
3. EliahKagan commented on Jan 24, 2024

   

   EliahKagan

   on Jan 24, 2024

   Member

   More actions

   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.
4. Byron commented on Jan 24, 2024

   

   Byron

   on Jan 24, 2024

   Member

   More actions

   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.

   Reacted by David Tucker
5. Byron

   closed this as completedin #2028on May 23, 2025

Issue actions