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

[J.P.]

Vasilij Schneidermann <mail@vasilij.de> writes:

>> 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)
>
> Thank you, this looks very good. I've tested it on my own code using the
> SQLite support and there don't appear to be any (obvious issues).

Not obvious on account of the indentation, but it seems the second

  (funcall ,func-var)

snuck outside the `if' form with that fix, making body run twice if you
have SQLite.