From 592ec8492326ccef7b4af022bffcc385eac8adb8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 21 Sep 2023 06:57:27 -0400 Subject: [PATCH 1/3] Fix rollback bug in SymbolicReference.set_reference This fixes the initialization of the "ok" flag by setting it to False before the operation that might fail is attempted. (This is a fix for the *specific* problem reported in #1669 about how the pattern SymbolicReference.set_reference attempts to use with LockedFD is not correctly implemented.) --- git/refs/symbolic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/refs/symbolic.py b/git/refs/symbolic.py index 5c293aa7b..6361713de 100644 --- a/git/refs/symbolic.py +++ b/git/refs/symbolic.py @@ -370,7 +370,7 @@ def set_reference( lfd = LockedFD(fpath) fd = lfd.open(write=True, stream=True) - ok = True + ok = False try: fd.write(write_value.encode("utf-8") + b"\n") lfd.commit() From ff84b26445b147ee9e2c75d82903b0c6b09e2b7a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 21 Sep 2023 03:00:04 -0400 Subject: [PATCH 2/3] Refactor try-finally cleanup in git/ This is, in part, to help avoid (or be able to notice) other bugs like the rollback bug that affected SymbolicReference. However, it also includes a simplification of try-(try-except)-finally to try-except-finally. --- git/config.py | 17 ++++++++--------- git/index/base.py | 8 +++----- git/refs/symbolic.py | 8 +++----- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/git/config.py b/git/config.py index 880eb9301..76b149179 100644 --- a/git/config.py +++ b/git/config.py @@ -406,15 +406,14 @@ def release(self) -> None: return try: - try: - self.write() - except IOError: - log.error("Exception during destruction of GitConfigParser", exc_info=True) - except ReferenceError: - # This happens in PY3 ... and usually means that some state cannot be written - # as the sections dict cannot be iterated - # Usually when shutting down the interpreter, don'y know how to fix this - pass + self.write() + except IOError: + log.error("Exception during destruction of GitConfigParser", exc_info=True) + except ReferenceError: + # This happens in PY3 ... and usually means that some state cannot be + # written as the sections dict cannot be iterated + # Usually when shutting down the interpreter, don't know how to fix this + pass finally: if self._lock is not None: self._lock._release_lock() diff --git a/git/index/base.py b/git/index/base.py index cf016df6a..0cdeb1ce5 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -224,13 +224,11 @@ def write( lfd = LockedFD(file_path or self._file_path) stream = lfd.open(write=True, stream=True) - ok = False try: self._serialize(stream, ignore_extension_data) - ok = True - finally: - if not ok: - lfd.rollback() + except BaseException: + lfd.rollback() + raise lfd.commit() diff --git a/git/refs/symbolic.py b/git/refs/symbolic.py index 6361713de..734bf32d8 100644 --- a/git/refs/symbolic.py +++ b/git/refs/symbolic.py @@ -370,14 +370,12 @@ def set_reference( lfd = LockedFD(fpath) fd = lfd.open(write=True, stream=True) - ok = False try: fd.write(write_value.encode("utf-8") + b"\n") lfd.commit() - ok = True - finally: - if not ok: - lfd.rollback() + except BaseException: + lfd.rollback() + raise # Adjust the reflog if logmsg is not None: self.log_append(oldbinsha, logmsg) From e480985aa4d358d0cc167d4552910e85944b8966 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 21 Sep 2023 07:05:40 -0400 Subject: [PATCH 3/3] Tweak rollback logic in log.to_file This modifies the exception handling in log.to_file so it catches BaseException rather than Exception and rolls back. Ordinarily we do not want to catch BaseException, since this means catching things like SystemExit, KeyboardInterupt, etc., but the other cases of rolling back with LockedFD do it that strongly (both before when try-finally was used with a flag, and now with try-except catching BaseException to roll back the temporary-file write and reraise). Having this behave subtly different does not appear intentional. (This is also closer to what will happen if LockedFD becomes a context manager and these pieces of code use it in a with-statement: even exceptions not inheriting from Exception will cause __exit__ to be called.) --- git/refs/log.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/git/refs/log.py b/git/refs/log.py index 1f86356a4..9acc0e360 100644 --- a/git/refs/log.py +++ b/git/refs/log.py @@ -262,8 +262,7 @@ def to_file(self, filepath: PathLike) -> None: try: self._serialize(fp) lfd.commit() - except Exception: - # on failure it rolls back automatically, but we make it clear + except BaseException: lfd.rollback() raise # END handle change