bug#67142: 29.1; with-sqlite-transaction commits on exception rather than rolling back

[Eli Zaretskii]

> Date: Fri, 17 Nov 2023 10:08:36 +0100
> From: Vasilij Schneidermann <mail@vasilij.de>
> Cc: Eli Zaretskii <eliz@gnu.org>, 67142@debbugs.gnu.org
> 
> > db-var and func-var are uninterned symbols in the macro expansion but
> > the variables 'result' and 'commit' marked below aren't.  If the intent
> > is not to expose these variables to the macro's BODY, the inner let
> > should also uninterned symbols right?
> 
> Correct, this is what I was hinting at. As annoying as it is, when
> writing unhygienic macros one should use uninterned symbols to avoid
> exposing additional variables to the BODY argument.
> 
> Alternatively, the pattern of `(let ((return (...))) ... return)` can be
> replaced with `(prog1 (...) ...)`.
> 
> @Eli: The new patch looks better and closer to how the issue is solved
> in the Ruby sqlite3 gem. I'm still not sure about this use of
> `unwind-protect` being correct, but it does preserve the backtrace in
> case of an error better than when I used `(condition-case e (...) (error
> (apply #'signal e)))`.

OK, how about the below?

diff --git a/lisp/sqlite.el b/lisp/sqlite.el
index aad0aa4..8a52573 100644
--- a/lisp/sqlite.el
+++ b/lisp/sqlite.el
@@ -24,19 +24,28 @@
 ;;; Code:
 
 (defmacro with-sqlite-transaction (db &rest body)
-  "Execute BODY while holding a transaction for DB."
+  "Execute BODY while holding a transaction for DB.
+If BODY completes normally, commit the changes and return
+the value of BODY.
+If BODY signals an error, or transaction commit fails, roll
+back the transaction changes."
   (declare (indent 1) (debug (form body)))
   (let ((db-var (gensym))
-        (func-var (gensym)))
+        (func-var (gensym))
+        (res-var (gensym))
+        (commit-var (gensym)))
     `(let ((,db-var ,db)
-           (,func-var (lambda () ,@body)))
+           (,func-var (lambda () ,@body))
+           ,res-var ,commit-var)
        (if (sqlite-available-p)
            (unwind-protect
                (progn
                  (sqlite-transaction ,db-var)
-                 (funcall ,func-var))
-             (sqlite-commit ,db-var))
-         (funcall ,func-var)))))
+                 (setq ,res-var (funcall ,func-var))
+                 (setq ,commit-var (sqlite-commit ,db-var))
+                 ,res-var)
+             (or ,commit-var (sqlite-rollback ,db-var))))
+         (funcall ,func-var))))
 
 (provide 'sqlite)