René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"SymbolicReference.set_reference may not roll back properly on error","articleBody":"`SymbolicReference.set_reference` contains this code, which is meant to attempt a transactional write:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/a5a646494393478c65f26cd3a921f3505219d3e1/git/refs/symbolic.py#L373-L380\r\n\r\nHowever, because the `ok` flag is *initialized* to `True` instead of `False`--which strongly appears unintentional--it is always `True` even if the `ok = True` statement in the `try` block is not reached. Consequently, `lfd.rollback()` in the `finally` block is never called. This seems on first glance like a serious bug, but as detailed below I believe it might actually be quite minor in the context of what kind of cleanup GitPython generally [does or does not guarantee](https://github.com/gitpython-developers/GitPython?tab=readme-ov-file#leakage-of-system-resources).\r\n\r\nThis can be contrasted to another place where that pattern is used correctly:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/a5a646494393478c65f26cd3a921f3505219d3e1/git/index/base.py#L227-L233\r\n\r\nThis is very simple to fix, since it is sufficient to change `True` to `False` in the initial assignment. So most of the work to fix it would be figuring out where a regression test should go and how it should manage to produce a case where cleanup doesn't happen immediately.\r\n\r\n(Besides that simple fix, other fixes that also reduce the risk of such bugs coming back and allow the code to be simplified are possible. In particular, the `LockedFD` object represents a non-memory resource that always can and should be managed deterministically according to where control flow is, so it ought to implement the context manager protocol.)\r\n\r\nI am unsure if this bug causes any problems in practice. If I understand [`LockedFD`](https://github.com/gitpython-developers/gitdb/blob/master/gitdb/util.py#L269) correctly, the write that is not rolled back is to a temporary file separate from the real target, accessed through a file object specific to that `LockedFD` object. The lock file name is determined from the target file name, so if a separate attempt to write to the target is made, that could cause problems. `LockedFD` has a `__del__` method that should be called eventually and performs the rollback. On CPython, which has reference counting as its primary garbage collection strategy, cleanup would often happen immediately as a result... though not always, even in the absence of reference cycles, because, in exception handling, an exception object holds a reference to a traceback object which holds references to all stack frames in its call stack, through which local variables thus remain accessible until control leaves the handling `except` block.\r\n\r\nStill, it may be that this shall not be considered a bug--or at least not a bug of its own, or at least not a bug that merits a regression test accompanying its bugfix--on the grounds that **it might be considered a special case of the [Leakage of System Resources](https://github.com/gitpython-developers/GitPython?tab=readme-ov-file#leakage-of-system-resources) limitation** documented prominently in the readme. In support of this view, the third place in the code of GitPython where `ldf.rollback` is called uses a different pattern and its comments suggest that cleanup via `LockedFD.__del__` is considered acceptable to rely on:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/a5a646494393478c65f26cd3a921f3505219d3e1/git/refs/log.py#L261-L268\r\n\r\n(Interestingly, that is not exactly equivalent to the behavior of the `try`-`finally` pattern used in `git/index/base.py`. If it is intended to be, then it should catch `BaseException` rather than `Exception`.)","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-09-21T08:14:24.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":7},"url":"https://github.com/1669/GitPython/issues/1669"}