bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage)

[Kai Tetzlaff]

"Herbert J. Skuhra" <herbert@gojira.at> writes:

> On Wed, Jan 18, 2023 at 09:17:59PM +0200, Eli Zaretskii wrote:
>> > this is strange because I can reproduce it easily on different systems:
>> > 
>> > - master on FreeBSD 13.1-STABLE 
>> > - emacs-29 and master on macOS 12.6.2
>> > - master on WLS2/Windows11 (Ubuntu)

I can now reproduce the error, too. The problem was that at the time Lars
closed the bug report by applying the attached patch (after quite a long
time of it sitting dormant), I had some additional local changes for
sieve.el and sieve-manage.el on a branch which I didn't get to submit.
And when I tried to reproduce the error, I've still been using this
branch without realizing it. Sorry for that.

>> ...
>> Thanks.  The error is here:
>> 
>> (defun sieve-manage--append-to-log (&rest args)
>>   "Append ARGS to sieve-manage log buffer.
>> 
>> ARGS can be a string or a list of strings.
>> The buffer to use for logging is specifified via
>> `sieve-manage-log'. If it is nil, logging is disabled."
>>   (when sieve-manage-log
>>     (with-current-buffer (or (get-buffer sieve-manage-log)
>>                              (with-current-buffer  <<<<<<<<<<<<<<<<<<<<<<
>>                                  (get-buffer-create sieve-manage-log)
>>                                (set-buffer-multibyte nil)
>>                                (buffer-disable-undo)))
>> 
>> And I admit that I don't understand this code.  What is it trying to
>> do?  Shouldn't it be just
>> 
>>   (when sieve-manage-log
>>     (with-current-buffer (get-buffer-create sieve-manage-log)
>>       (set-buffer-multibyte nil)
>>       (buffer-disable-undo)))
>> 
>> Kai, am I missing something?

The additional '(or ...' was meant to only run

  (set-buffer-multibyte nil)
  (buffer-disable-undo)

once, when creating the log buffer (not everytime something gets
appended to the log). What is missing in my code is an additional
`current-buffer'. Here's the complete fixed function:

(defun sieve-manage--append-to-log (&rest args)
  "Append ARGS to sieve-manage log buffer.

ARGS can be a string or a list of strings.
The buffer to use for logging is specifified via
`sieve-manage-log'. If it is nil, logging is disabled."
  (when sieve-manage-log
    (with-current-buffer (or (get-buffer sieve-manage-log)
                             (with-current-buffer
                                 (get-buffer-create sieve-manage-log)
                               (set-buffer-multibyte nil)
                               (buffer-disable-undo)
                               (current-buffer)))
      (goto-char (point-max))
      (apply #'insert args))))

>> Herbert, if you make the change above, does the problem go away?
>
> Yes, this change resolves the issue:
>
> diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
> index 5bee4f4c4a..636c7cbc5b 100644
> --- a/lisp/net/sieve-manage.el
> +++ b/lisp/net/sieve-manage.el
> @@ -174,11 +174,9 @@ sieve-manage--append-to-log
>  The buffer to use for logging is specifified via
>  `sieve-manage-log'. If it is nil, logging is disabled."
>    (when sieve-manage-log
> -    (with-current-buffer (or (get-buffer sieve-manage-log)
> -                             (with-current-buffer
> -                                 (get-buffer-create sieve-manage-log)
> +    (with-current-buffer (get-buffer-create sieve-manage-log)
>                                 (set-buffer-multibyte nil)
> -                               (buffer-disable-undo)))
> +                               (buffer-disable-undo)
>        (goto-char (point-max))
>        (apply #'insert args))))
>

Here's a patch which preserves the logic of the original code:

>From 4198e776da13b603c56acbae0ae89cd9d31cf207 Mon Sep 17 00:00:00 2001
From: Kai Tetzlaff <emacs@tetzco.de>
Date: Thu, 19 Jan 2023 03:16:14 +0100
Subject: [PATCH] Fix bug in sieve-manage--append-to-log

* lisp/net/sieve-manage.el
(sieve-manage--append-to-log): Fix log buffer creation
---
 lisp/net/sieve-manage.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
index 5bee4f4c4ad..ab22294a272 100644
--- a/lisp/net/sieve-manage.el
+++ b/lisp/net/sieve-manage.el
@@ -178,7 +178,8 @@ sieve-manage--append-to-log
                              (with-current-buffer
                                  (get-buffer-create sieve-manage-log)
                                (set-buffer-multibyte nil)
-                               (buffer-disable-undo)))
+                               (buffer-disable-undo)
+                               (current-buffer)))
       (goto-char (point-max))
       (apply #'insert args))))
 
-- 
2.39.0

The additional changes I mentioned above solve the problem in a different
way by introducing a helper function. The also add some other improvements
including a new test for handling multibyte characters in sieve server
responses. I'm attaching the additional patches below. They might be
too large for the current emacs-29 branch. But maybe they can be applied
to master?

0001-Fix-bug-in-sieve-manage-append-to-log-improve-sieve-.patch
Description: fix sieve-manage--append-to-log

0002-Handle-BYE-in-sieve-manage-server-responses.patch
Description: handle bye in sieve-manage.el

0003-Add-test-lisp-net-sieve-manage-tests.el.patch
Description: test for sieve-manage multibyte character handling

0004-Some-minor-fixes-in-lisp-net-sieve.el.patch
Description: minor fixes in lisp/net/sieve.el