Package: emacs;
Reported by: "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de>
Date: Fri, 25 Feb 2022 09:20:01 UTC
Severity: normal
Tags: patch
Found in version 29.0.50
Fixed in version 29.1
Done: Lars Ingebrigtsen <larsi <at> gnus.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54154 in the body.
You can then email your comments to 54154 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Fri, 25 Feb 2022 09:20:01 GMT) Full text and rfc822 format available."Kai Tetzlaff" <kai.tetzlaff <at> t-online.de>:bug-gnu-emacs <at> gnu.org.
(Fri, 25 Feb 2022 09:20:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> To: bug-gnu-emacs <at> gnu.org Subject: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Fri, 25 Feb 2022 10:04:47 +0100
[Message part 1 (text/plain, inline)]
The sieve-manage package uses the managesieve protocol (s. https://datatracker.ietf.org/doc/html/draft-ietf-sieve-managesieve) to communicate with a sieve server process. When the sieve-manage client retrieves a script from the server it uses the `sieve-manage-getscript' function to send command `GETSCRIPT "<scriptname>"<CRLF>` to the server and tries to parse the response. If the downloaded sieve script contains multibyte characters the attempt to parse the response results in an infloop (in `sieve-manage-parse-okno'). To reproduce, save the following code
[sieve-manage-getscript-minimal-example.el (application/emacs-lisp, inline)]
[Message part 3 (text/plain, inline)]
to a file and run: emacs -Q -l <file>
* Detailed analysis:
The example code sets up a response buffer for a successful managesieve
`response-getscript` defined as:
response-getscript = (sieve-script CRLF response-ok)
Here's the buffer content:
```
1: {32}<CRLF>
2: if body :matches "ä" { stop; }<LF>
3: <CRLF>
4: OK "Getscript completed."<CRLF>
```
It comprises:
1. lines 1-2 (`sieve-script`): encoded as a managesieve `literal-s2c`
string which:
a. starts with a length in the form '{<OCTETS>}<CRLF>' (i.e. 32)
b. followed by the string data (i.e. the actual script: 'if
body :matches "ä" { stop;}<LF>') using UTF-8 encoding
2. line 3 (`CRLF`)
3. line 4 (`response-ok`): 'OK' SP "Getscript completed." (the latter is
an optional `quoted` string which can be shown to the user)
The sieve-manage code is parsing the length into an integer and uses it
to skip over `sieve-script` to get to the start of line 3 (<CRLF> -
empty) which is then also skipped to get to line 4 in order to parse the
result ('OK').
Now the problem:
Since sieve-manage explicitly enables multibyte support in the response
buffer (by calling '(mm-enable-multibyte)' in
`sieve-manage-make-process-buffer`) and uses `goto-char' for the purpose
of skipping/jumping over `sieve-script`, each multibyte character in
`sieve-script` causes the jump to go 1 (2, 3) character(s) too far. In
the example above there's only a single 2 byte character (`ä`), so
instead of skipping to the beginning of line 3, we land in the middle of
<CRLF>: <CR(point)LF>.
This causes the following attempt to parse the result code (i.e. the 'OK
"Getscript completed."<CRLF>' line) to infloop in
`sieve-manage-parse-okno'.
* An attempt of a fix:
As far as I can tell, the attached patch fixes the issue for the
GETSCRIPT command.
[sieve-manage-getscript-multibyte-fix.patch (text/x-diff, inline)]
diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
index 50342b9105..8020e6fdca 100644
--- a/lisp/net/sieve-manage.el
+++ b/lisp/net/sieve-manage.el
@@ -449,10 +449,19 @@ sieve-manage-deletescript
(defun sieve-manage-getscript (name output-buffer &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "GETSCRIPT \"%s\"" name))
+ (set-buffer-multibyte nil)
(let ((script (sieve-manage-parse-string)))
+ (set-buffer-multibyte t)
(sieve-manage-parse-crlf)
(with-current-buffer output-buffer
- (insert script))
+ (insert (decode-coding-string
+ script
+ ;; not sure if using `buffer-file-coding-system' is
+ ;; the right approach, it might be better to hardcode
+ ;; it to utf-8-* (managesieve requires UTF-8
+ ;; encoding) but in that case, which variant of
+ ;; utf-8-unix/dos/... is to be used?
+ buffer-file-coding-system t)))
(sieve-manage-parse-okno))))
(defun sieve-manage-setactive (name &optional buffer)
[Message part 5 (text/plain, inline)]
* Additional remarks:
There might be more problems. E.g. `sieve-manage-putscript' contains the
following comment:
;; Here we assume that the coding-system will
;; replace each char with a single byte.
;; This is always the case if `content' is
;; a unibyte string.
which seems to indicate that it might also have an issue with multibyte
content (even though I have not experienced any uploading issues). I
will try do some more testing to check that.
In general, it is also not clear to me why the response (or process)
buffer needs to be multibyte enabled at all as it should only be used
for the line/byte oriented protocol data. But the commit message of
8e16fb987df9b which introduced the multibyte handling states:
commit 8e16fb987df9b80b8328e9dbf80351a5f9d85bbb
Author: Albert Krewinkel <krewinkel <at> moltkeplatz.de>
Date: 2013-06-11 07:32:25 +0000
...
* Enable Multibyte for SieveManage buffers: The parser won't properly
handle umlauts and line endings unless multibyte is turned on in the
process buffer.
...
so this was obviously done on purpose. I contacted Albert about this but
he couldn't remember the details (it's been nearly 10 years).
In GNU Emacs 29.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.31, cairo version 1.16.0)
of 2022-02-18 built on moka
Repository revision: 51e51ce2df46fc0c6e17a97e74b00366bb9c09d8
Repository branch: master
System Description: Debian GNU/Linux bookworm/sid
Configured using:
'configure --with-pgtk --with-native-compilation'
Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS XIM GTK3 ZLIB
Important settings:
value of $LANG: en_US.UTF-8
locale-coding-system: utf-8-unix
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Fri, 25 Feb 2022 12:21:01 GMT) Full text and rfc822 format available.Message #8 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Fri, 25 Feb 2022 13:19:51 +0100
"Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> writes: > (with-current-buffer (or buffer (current-buffer)) > (sieve-manage-send (format "GETSCRIPT \"%s\"" name)) > + (set-buffer-multibyte nil) > (let ((script (sieve-manage-parse-string))) > + (set-buffer-multibyte t) Changing multibyteness in a buffer like this is (virtually) never the right thing to do -- it usually leads to obscure breakages. > In general, it is also not clear to me why the response (or process) > buffer needs to be multibyte enabled at all as it should only be used > for the line/byte oriented protocol data. But the commit message of > 8e16fb987df9b which introduced the multibyte handling states: > > commit 8e16fb987df9b80b8328e9dbf80351a5f9d85bbb > Author: Albert Krewinkel <krewinkel <at> moltkeplatz.de> > Date: 2013-06-11 07:32:25 +0000 > ... > * Enable Multibyte for SieveManage buffers: The parser won't properly > handle umlauts and line endings unless multibyte is turned on in the > process buffer. > ... > > so this was obviously done on purpose. I contacted Albert about this but > he couldn't remember the details (it's been nearly 10 years). I don't see why this buffer should be multibyte, either. The communication with the server is done using bytes, not characters. When we need to have characters, we should decode the data and put it in a multibyte buffer. So can you try to back out that commit and see whether it fixes the problem instead? -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Fri, 25 Feb 2022 13:12:02 GMT) Full text and rfc822 format available.Message #11 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Fri, 25 Feb 2022 14:10:56 +0100
The mail bounced with:
kai.tetzlaff <at> t-online.de
host mx03.t-online.de [194.25.134.73]
SMTP error from remote mail server after initial connection:
554 IP=95.216.78.240 - A problem occurred. (Ask your postmaster for help or to contact tosa <at> rx.t-online.de to clarify.)
Trying to send via a different SMTP server...
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> writes:
>
>> (with-current-buffer (or buffer (current-buffer))
>> (sieve-manage-send (format "GETSCRIPT \"%s\"" name))
>> + (set-buffer-multibyte nil)
>> (let ((script (sieve-manage-parse-string)))
>> + (set-buffer-multibyte t)
>
> Changing multibyteness in a buffer like this is (virtually) never the
> right thing to do -- it usually leads to obscure breakages.
>
>> In general, it is also not clear to me why the response (or process)
>> buffer needs to be multibyte enabled at all as it should only be used
>> for the line/byte oriented protocol data. But the commit message of
>> 8e16fb987df9b which introduced the multibyte handling states:
>>
>> commit 8e16fb987df9b80b8328e9dbf80351a5f9d85bbb
>> Author: Albert Krewinkel <krewinkel <at> moltkeplatz.de>
>> Date: 2013-06-11 07:32:25 +0000
>> ...
>> * Enable Multibyte for SieveManage buffers: The parser won't properly
>> handle umlauts and line endings unless multibyte is turned on in the
>> process buffer.
>> ...
>>
>> so this was obviously done on purpose. I contacted Albert about this but
>> he couldn't remember the details (it's been nearly 10 years).
>
> I don't see why this buffer should be multibyte, either. The
> communication with the server is done using bytes, not characters. When
> we need to have characters, we should decode the data and put it in a
> multibyte buffer.
>
> So can you try to back out that commit and see whether it fixes the
> problem instead?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Fri, 25 Feb 2022 17:27:03 GMT) Full text and rfc822 format available.Message #14 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <kai <at> tetzlaff.eu> To: Lars Ingebrigtsen <larsi <at> gnus.org> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Fri, 25 Feb 2022 17:00:36 +0100
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> The mail bounced with:
>
> kai.tetzlaff <at> t-online.de
> host mx03.t-online.de [194.25.134.73]
> SMTP error from remote mail server after initial connection:
> 554 IP=95.216.78.240 - A problem occurred. (Ask your postmaster for help or to contact tosa <at> rx.t-online.de to clarify.)
Sorry, not sure what is happening there. Using a different From:
address, now (hopefully - using the t-online address was accidental
anyway).
>>> (with-current-buffer (or buffer (current-buffer))
>>> (sieve-manage-send (format "GETSCRIPT \"%s\"" name))
>>> + (set-buffer-multibyte nil)
>>> (let ((script (sieve-manage-parse-string)))
>>> + (set-buffer-multibyte t)
>>
>> Changing multibyteness in a buffer like this is (virtually) never the
>> right thing to do -- it usually leads to obscure breakages.
>>
>>> In general, it is also not clear to me why the response (or process)
>>> buffer needs to be multibyte enabled at all as it should only be used
>>> for the line/byte oriented protocol data. But the commit message of
>>> 8e16fb987df9b which introduced the multibyte handling states:
>>>
>>> commit 8e16fb987df9b80b8328e9dbf80351a5f9d85bbb
>>> Author: Albert Krewinkel <krewinkel <at> moltkeplatz.de>
>>> Date: 2013-06-11 07:32:25 +0000
>>> ...
>>> * Enable Multibyte for SieveManage buffers: The parser won't properly
>>> handle umlauts and line endings unless multibyte is turned on in the
>>> process buffer.
>>> ...
>>>
>>> so this was obviously done on purpose. I contacted Albert about this but
>>> he couldn't remember the details (it's been nearly 10 years).
>>
>> I don't see why this buffer should be multibyte, either. The
>> communication with the server is done using bytes, not characters. When
>> we need to have characters, we should decode the data and put it in a
>> multibyte buffer.
>>
>> So can you try to back out that commit and see whether it fixes the
>> problem instead?
Most of the referenced commit was about changes related to STARTTLS
handling. Here's the full commit message:
lisp/gnus/sievel-manage.el: fully support STARTTLS, fix bit rot
* Make sieve-manage-open work with STARTTLS: shorten stream managing
functions by using open-protocol-stream to do most of the work.
Has the nice benefit of enabling STARTTLS.
* Remove unneeded functions and options: the following functions and
options are neither in the API, nor called by any other function,
so they are deleted:
- sieve-manage-network-p
- sieve-manage-network-open
- sieve-manage-starttls-p
- sieve-manage-starttls-open
- sieve-manage-forward
- sieve-manage-streams
- sieve-manage-stream-alist
The options could not be applied in a meaningful way anymore; they
didn't happen to have much effect before.
* Cosmetic changes and code clean-up
* Enable Multibyte for SieveManage buffers: The parser won't properly
handle umlauts and line endings unless multibyte is turned on in the
process buffer.
* Wait for capabilities after STARTTLS: following RFC5804, the server
sends new capabilities after successfully establishing a TLS
connection with the client. The client should update the cached list
of capabilities, but we just ignore the answer for now.
So just reverting it won't work. I will try to undo the parts relevant
to this issue.
For clarification: The original code before Alberts change was using
this macro (which seemingly contains an error in the doc string):
(defmacro sieve-manage-disable-multibyte ()
"Enable multibyte in the current buffer."
(unless (featurep 'xemacs)
'(set-buffer-multibyte nil)))
to *disable* multibyte handling in the response/protocol buffer. If
using `set-buffer-multibyte' is not the right thing, what should be used
instead?
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sat, 26 Feb 2022 15:08:02 GMT) Full text and rfc822 format available.Message #17 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: Kai Tetzlaff <kai <at> tetzlaff.eu> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Sat, 26 Feb 2022 16:07:14 +0100
Kai Tetzlaff <kai <at> tetzlaff.eu> writes: > So just reverting it won't work. I will try to undo the parts relevant > to this issue. Sounds good. > For clarification: The original code before Alberts change was using > this macro (which seemingly contains an error in the doc string): > > (defmacro sieve-manage-disable-multibyte () > "Enable multibyte in the current buffer." > (unless (featurep 'xemacs) > '(set-buffer-multibyte nil))) > > to *disable* multibyte handling in the response/protocol buffer. If > using `set-buffer-multibyte' is not the right thing, what should be used > instead? Using (set-buffer-multibyte nil) is the right thing to do to make a buffer unibyte -- but usually only when the buffer is empty. There's been some discussion about making `set-buffer-multibyte' signal an error if used in a non-empty buffer, because in 99.7% of the cases where people do that, it's the wrong thing to do. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 28 Feb 2022 12:36:02 GMT) Full text and rfc822 format available.Message #20 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> To: Lars Ingebrigtsen <larsi <at> gnus.org> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Mon, 28 Feb 2022 13:35:30 +0100
[Message part 1 (text/plain, inline)]
Oh - the previous version of the first patch in my last email still contained a bug (I forgot to re-run `git format-patch` before sending the mail). Lars Ingebrigtsen <larsi <at> gnus.org> writes: > Kai Tetzlaff <emacs <at> tetzco.de> writes: > >> So just reverting it won't work. I will try to undo the parts relevant >> to this issue. > > Sounds good. Ok, I'm attaching two patches which fix all issues I noticed. What I ended up with is quite a bit more than the initial attempt. Since these changes are non-trivial, I will need to do the copyright assignment. About a week ago I actually sent an email to assign <at> gnu.org to get the process started. But I haven't received a reply. Could you please send me the necessary papers? I'm in Germany, so my understanding is that it should be possible to do this electronically? The first (and major) set of fixes are in sieve-manage.el for the issues with multibyte characters in sieve scripts (sieve-manage-getscript/putscript). This also adds supports for multibyte characters in script names (sieve-manage-listscripts/getscript/putscript/havespace/deletescript/setactive). There is now also some handling of getscript errors reported by the server and improved logging.
[0001-Fix-mostly-multibyte-issues-in-sieve-manage.el-Bug-5.patch (text/x-diff, inline)]
From 3a4ecad9f680d130fba9e792b87824e1f5e6a6eb Mon Sep 17 00:00:00 2001
From: Kai Tetzlaff <emacs <at> tetzco.de>
Date: Mon, 28 Feb 2022 11:08:07 +0100
Subject: [PATCH 1/2] Fix (mostly multibyte) issues in sieve-manage.el
(Bug#54154)
The managesieve protocol (s. RFC5804) requires support for (a sightly
restricted variant of) UTF-8 in script content and script names. This
commit fixes/improves the handling of multibyte characters.
In addition, `sieve-manage-getscript' now properly handles NO
responses from the server instead of inflooping.
There are also some logging improvements.
* lisp/net/sieve-manage.el
(sieve-manage--append-to-log):
(sieve-manage--message):
(sieve-manage--error):
(sieve-manage-encode):
(sieve-manage-decode):
(sieve-manage-no-p): New functions.
(sieve-manage-make-process-buffer): Switch process buffer to unibyte.
(sieve-manage-open-server): Add `:coding 'raw-text-unix` to
`open-network-stream' call. Use unix EOLs in order to keep matching
CRLF (aka "\r\n") intact.
(sieve-manage-send): Make sure that UTF-8 multibyte characters are
properly encoded before sending data to the server.
(sieve-manage-getscript):
(sieve-manage-putscript): Use the changes above to fix down/uploading
scripts containing UTF-8 multibyte characters.
(sieve-manage-listscripts):
(sieve-manage-havespace)
(sieve-manage-getscript)
(sieve-manage-putscript):
(sieve-manage-deletescript):
(sieve-manage-setactive): Use the changes above to fix handling of
script names which contain UTF-8 multibyte characters.
(sieve-manage-parse-string):
(sieve-manage-getscript): Add handling of server responses with type
NO. Abort `sieve-manage-getscript' and show error message in message
area.
(sieve-manage-erase):
(sieve-manage-drop-next-answer):
(sieve-manage-parse-crlf): Return erased/dropped data (instead of nil).
(sieve-sasl-auth):
(sieve-manage-getscript):
(sieve-manage-erase):
(sieve-manage-open-server):
(sieve-manage-open):
(sieve-manage-send): Improve logging.
---
lisp/net/sieve-manage.el | 126 +++++++++++++++++++++++++++------------
1 file changed, 87 insertions(+), 39 deletions(-)
diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
index 50342b9105..4a36f94431 100644
--- a/lisp/net/sieve-manage.el
+++ b/lisp/net/sieve-manage.el
@@ -167,7 +167,53 @@ sieve-manage-process
(defvar sieve-manage-capability nil)
;; Internal utility functions
-(autoload 'mm-enable-multibyte "mm-util")
+(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))))
+
+(defun sieve-manage--message (format-string &rest args)
+ "Wrapper around `message' which also logs to sieve manage log.
+
+See `sieve-manage--append-to-log'."
+ (let ((ret (apply #'message
+ (concat "sieve-manage: " format-string)
+ args)))
+ (sieve-manage--append-to-log ret "\n")
+ ret))
+
+(defun sieve-manage--error (format-string &rest args)
+ "Wrapper around `error' which also logs to sieve manage log.
+
+See `sieve-manage--append-to-log'."
+ (let ((msg (apply #'format
+ (concat "sieve-manage/ERROR: " format-string)
+ args)))
+ (sieve-manage--append-to-log msg "\n")
+ (error msg)))
+
+(defun sieve-manage-encode (utf8-string)
+ "Convert UTF8-STRING to managesieve protocol octets."
+ (encode-coding-string utf8-string 'raw-text t))
+
+(defun sieve-manage-decode (octets &optional buffer)
+ "Convert managesieve protocol OCTETS to utf-8 string.
+
+If optional BUFFER is non-nil, insert decoded string into BUFFER."
+ (when octets
+ ;; eol type unix is required to preserve "\r\n"
+ (decode-coding-string octets 'utf-8-unix t buffer)))
(defun sieve-manage-make-process-buffer ()
(with-current-buffer
@@ -175,22 +221,19 @@ sieve-manage-make-process-buffer
sieve-manage-server
sieve-manage-port))
(mapc #'make-local-variable sieve-manage-local-variables)
- (mm-enable-multibyte)
+ (set-buffer-multibyte nil)
+ (setq-local after-change-functions nil)
(buffer-disable-undo)
(current-buffer)))
(defun sieve-manage-erase (&optional p buffer)
- (let ((buffer (or buffer (current-buffer))))
- (and sieve-manage-log
- (with-current-buffer (get-buffer-create sieve-manage-log)
- (mm-enable-multibyte)
- (buffer-disable-undo)
- (goto-char (point-max))
- (insert-buffer-substring buffer (with-current-buffer buffer
- (point-min))
- (or p (with-current-buffer buffer
- (point-max)))))))
- (delete-region (point-min) (or p (point-max))))
+ (with-current-buffer (or buffer (current-buffer))
+ (let* ((start (point-min))
+ (end (or p (point-max)))
+ (logdata (buffer-substring-no-properties start end)))
+ (sieve-manage--append-to-log logdata)
+ (delete-region start end)
+ logdata)))
(defun sieve-manage-open-server (server port &optional stream buffer)
"Open network connection to SERVER on PORT.
@@ -202,6 +245,8 @@ sieve-manage-open-server
(open-network-stream
"SIEVE" buffer server port
:type stream
+ ;; eol type unix is required to preserve "\r\n"
+ :coding 'raw-text-unix
:capability-command "CAPABILITY\r\n"
:end-of-command "^\\(OK\\|NO\\).*\n"
:success "^OK.*\n"
@@ -224,7 +269,7 @@ sieve-manage-open-server
;; Authenticators
(defun sieve-sasl-auth (buffer mech)
"Login to server using the SASL MECH method."
- (message "sieve: Authenticating using %s..." mech)
+ (sieve-manage--message "Authenticating using %s..." mech)
(with-current-buffer buffer
(let* ((auth-info (auth-source-search :host sieve-manage-server
:port "sieve"
@@ -275,11 +320,15 @@ sieve-sasl-auth
(if (and (setq step (sasl-next-step client step))
(setq data (sasl-step-data step)))
;; We got data for server but it's finished
- (error "Server not ready for SASL data: %s" data)
+ (sieve-manage--error
+ "Server not ready for SASL data: %s" data)
;; The authentication process is finished.
+ (sieve-manage--message "Logged in as %s using %s"
+ user-name mech)
(throw 'done t)))
(unless (stringp rsp)
- (error "Server aborted SASL authentication: %s" (caddr rsp)))
+ (sieve-manage--error
+ "Server aborted SASL authentication: %s" (caddr rsp)))
(sasl-step-set-data step (base64-decode-string rsp))
(setq step (sasl-next-step client step))
(sieve-manage-send
@@ -288,8 +337,7 @@ sieve-sasl-auth
(base64-encode-string (sasl-step-data step)
'no-line-break)
"\"")
- ""))))
- (message "sieve: Login using %s...done" mech))))
+ "")))))))
(defun sieve-manage-cram-md5-p (buffer)
(sieve-manage-capability "SASL" "CRAM-MD5" buffer))
@@ -353,7 +401,7 @@ sieve-manage-open
sieve-manage-default-stream)
sieve-manage-auth (or auth
sieve-manage-auth))
- (message "sieve: Connecting to %s..." sieve-manage-server)
+ (sieve-manage--message "Connecting to %s..." sieve-manage-server)
(sieve-manage-open-server sieve-manage-server
sieve-manage-port
sieve-manage-stream
@@ -368,7 +416,8 @@ sieve-manage-open
(setq sieve-manage-auth auth)
(cl-return)))
(unless sieve-manage-auth
- (error "Couldn't figure out authenticator for server")))
+ (sieve-manage--error
+ "Couldn't figure out authenticator for server")))
(sieve-manage-erase)
(current-buffer))))
@@ -433,11 +482,7 @@ sieve-manage-havespace
(defun sieve-manage-putscript (name content &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
- ;; Here we assume that the coding-system will
- ;; replace each char with a single byte.
- ;; This is always the case if `content' is
- ;; a unibyte string.
- (length content)
+ (length (sieve-manage-encode content))
sieve-manage-client-eol content))
(sieve-manage-parse-okno)))
@@ -449,11 +494,10 @@ sieve-manage-deletescript
(defun sieve-manage-getscript (name output-buffer &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "GETSCRIPT \"%s\"" name))
- (let ((script (sieve-manage-parse-string)))
- (sieve-manage-parse-crlf)
- (with-current-buffer output-buffer
- (insert script))
- (sieve-manage-parse-okno))))
+ (sieve-manage-decode (sieve-manage-parse-string)
+ output-buffer)
+ (sieve-manage-parse-crlf)
+ (sieve-manage-parse-okno)))
(defun sieve-manage-setactive (name &optional buffer)
(with-current-buffer (or buffer (current-buffer))
@@ -478,6 +522,9 @@ sieve-manage-drop-next-answer
(defun sieve-manage-ok-p (rsp)
(string= (downcase (or (car-safe rsp) "")) "ok"))
+(defun sieve-manage-no-p (rsp)
+ (string= (downcase (or (car-safe rsp) "")) "no"))
+
(defun sieve-manage-is-okno ()
(when (looking-at (concat
"^\\(OK\\|NO\\)\\( (\\([^)]+\\))\\)?\\( \\(.*\\)\\)?"
@@ -528,7 +575,11 @@ sieve-manage-parse-string
(while (null rsp)
(accept-process-output (get-buffer-process (current-buffer)) 1)
(goto-char (point-min))
- (setq rsp (sieve-manage-is-string)))
+ (unless (setq rsp (sieve-manage-is-string))
+ (when (sieve-manage-no-p (sieve-manage-is-okno))
+ ;; simple `error' is enough since `sieve-manage-erase'
+ ;; already adds the server response to the log
+ (error (sieve-manage-erase)))))
(sieve-manage-erase (point))
rsp))
@@ -540,7 +591,8 @@ sieve-manage-parse-listscripts
(let (tmp rsp data)
(while (null rsp)
(while (null (or (setq rsp (sieve-manage-is-okno))
- (setq tmp (sieve-manage-is-string))))
+ (setq tmp (sieve-manage-decode
+ (sieve-manage-is-string)))))
(accept-process-output (get-buffer-process (current-buffer)) 1)
(goto-char (point-min)))
(when tmp
@@ -559,13 +611,9 @@ sieve-manage-parse-listscripts
rsp)))
(defun sieve-manage-send (cmdstr)
- (setq cmdstr (concat cmdstr sieve-manage-client-eol))
- (and sieve-manage-log
- (with-current-buffer (get-buffer-create sieve-manage-log)
- (mm-enable-multibyte)
- (buffer-disable-undo)
- (goto-char (point-max))
- (insert cmdstr)))
+ (setq cmdstr (sieve-manage-encode
+ (concat cmdstr sieve-manage-client-eol)))
+ (sieve-manage--append-to-log cmdstr)
(process-send-string sieve-manage-process cmdstr))
(provide 'sieve-manage)
--
2.34.1
[Message part 3 (text/plain, inline)]
Both, the (internal) process/protocol buffer and the log buffer are now unibyte. The conversion to multibyte UTF-8 is only done for user visible (UI) buffers. To properly handle the protocol line termination (CRLF), I added `:coding 'raw-text-unix` (with explicit unix EOL convention) to the `open-network-stream' call (also in the new `manage-sieve-encode' function. This was needed to allow keep the various (looking-at "...\r\n" ...) calls working. This is something which still feels a bit weird, but I haven't found another way to do it. I did some tests with (setq-default buffer-file-coding-system 'utf-8-unix/'utf-8-dos) which did not show any issues. I would also add some ERT tests, probably in a separate commit? In addition, I found that `sieve-manage-quit' in sieve.el had the tendency to kill unrelated buffers in case of errors during earlier steps. For this, I created a sepate patch:
[0002-Improve-robustnes-of-sieve-manage-quit-in-case-of-er.patch (text/x-diff, inline)]
From 83ab45907c7b528ae4db98f33415e05e679c312e Mon Sep 17 00:00:00 2001 From: Kai Tetzlaff <emacs <at> tetzco.de> Date: Mon, 28 Feb 2022 11:33:56 +0100 Subject: [PATCH 2/2] Improve robustnes of `sieve-manage-quit' in case of errors * lisp/net/sieve.el (sieve-manage-quit): Avoid killing buffers it's not supposed to touch. --- lisp/net/sieve.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lisp/net/sieve.el b/lisp/net/sieve.el index 630ea04070..5680526389 100644 --- a/lisp/net/sieve.el +++ b/lisp/net/sieve.el @@ -154,7 +154,8 @@ sieve-manage-quit (interactive) (sieve-manage-close sieve-manage-buffer) (kill-buffer sieve-manage-buffer) - (kill-buffer (current-buffer))) + (when-let ((buffer (get-buffer sieve-buffer))) + (kill-buffer buffer))) (defun sieve-bury-buffer () "Bury the Manage Sieve buffer without closing the connection." -- 2.34.1
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 28 Feb 2022 13:08:01 GMT) Full text and rfc822 format available.Message #23 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Mon, 28 Feb 2022 14:06:56 +0100
"Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> writes: > Ok, I'm attaching two patches which fix all issues I noticed. The patches look good to me, but I haven't tried them myself, because I don't use sieve-manage. If somebody who does could try the patches, that would be helpful. Anybody? > What I ended up with is quite a bit more than the initial attempt. Since > these changes are non-trivial, I will need to do the copyright > assignment. About a week ago I actually sent an email to assign <at> gnu.org > to get the process started. But I haven't received a reply. Could you > please send me the necessary papers? I'm in Germany, so my understanding > is that it should be possible to do this electronically? It sometimes takes a while to get the process going -- if you don't get a response from the copyright clerk within a couple more days, send me an email and I'll get in touch with them to see what's up.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 28 Feb 2022 13:09:01 GMT) Full text and rfc822 format available.Message #26 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: emacs <at> tetzco.de Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Mon, 28 Feb 2022 14:08:06 +0100
(Re-sending because the previous mail went to to-online.de, which rejected it.) "Kai Tetzlaff" <kai.tetzlaff <at> t-online.de> writes: > Ok, I'm attaching two patches which fix all issues I noticed. The patches look good to me, but I haven't tried them myself, because I don't use sieve-manage. If somebody who does could try the patches, that would be helpful. Anybody? > What I ended up with is quite a bit more than the initial attempt. Since > these changes are non-trivial, I will need to do the copyright > assignment. About a week ago I actually sent an email to assign <at> gnu.org > to get the process started. But I haven't received a reply. Could you > please send me the necessary papers? I'm in Germany, so my understanding > is that it should be possible to do this electronically? It sometimes takes a while to get the process going -- if you don't get a response from the copyright clerk within a couple more days, send me an email and I'll get in touch with them to see what's up.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 28 Feb 2022 16:20:02 GMT) Full text and rfc822 format available.Message #29 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs <at> tetzco.de> To: Lars Ingebrigtsen <larsi <at> gnus.org> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Mon, 28 Feb 2022 14:03:19 +0100
[Message part 1 (text/plain, inline)]
Oh - the previous version of the first patch in my last email still contained a bug (I forgot to re-run `git format-patch` before sending the mail). Lars Ingebrigtsen <larsi <at> gnus.org> writes: > Kai Tetzlaff <emacs <at> tetzco.de> writes: > >> So just reverting it won't work. I will try to undo the parts relevant >> to this issue. > > Sounds good. Ok, I'm attaching two patches which fix all issues I noticed. What I ended up with is quite a bit more than the initial attempt. Since these changes are non-trivial, I will need to do the copyright assignment. About a week ago I actually sent an email to assign <at> gnu.org to get the process started. But I haven't received a reply. Could you please send me the necessary papers? I'm in Germany, so my understanding is that it should be possible to do this electronically? The first (and major) set of fixes are in sieve-manage.el for the issues with multibyte characters in sieve scripts (sieve-manage-getscript/putscript). This also adds supports for multibyte characters in script names (sieve-manage-listscripts/getscript/putscript/havespace/deletescript/setactive). There is now also some handling of getscript errors reported by the server and improved logging.
[0001-Fix-mostly-multibyte-issues-in-sieve-manage.el-Bug-5.patch (text/x-diff, inline)]
From 3a4ecad9f680d130fba9e792b87824e1f5e6a6eb Mon Sep 17 00:00:00 2001
From: Kai Tetzlaff <emacs <at> tetzco.de>
Date: Mon, 28 Feb 2022 11:08:07 +0100
Subject: [PATCH 1/2] Fix (mostly multibyte) issues in sieve-manage.el
(Bug#54154)
The managesieve protocol (s. RFC5804) requires support for (a sightly
restricted variant of) UTF-8 in script content and script names. This
commit fixes/improves the handling of multibyte characters.
In addition, `sieve-manage-getscript' now properly handles NO
responses from the server instead of inflooping.
There are also some logging improvements.
* lisp/net/sieve-manage.el
(sieve-manage--append-to-log):
(sieve-manage--message):
(sieve-manage--error):
(sieve-manage-encode):
(sieve-manage-decode):
(sieve-manage-no-p): New functions.
(sieve-manage-make-process-buffer): Switch process buffer to unibyte.
(sieve-manage-open-server): Add `:coding 'raw-text-unix` to
`open-network-stream' call. Use unix EOLs in order to keep matching
CRLF (aka "\r\n") intact.
(sieve-manage-send): Make sure that UTF-8 multibyte characters are
properly encoded before sending data to the server.
(sieve-manage-getscript):
(sieve-manage-putscript): Use the changes above to fix down/uploading
scripts containing UTF-8 multibyte characters.
(sieve-manage-listscripts):
(sieve-manage-havespace)
(sieve-manage-getscript)
(sieve-manage-putscript):
(sieve-manage-deletescript):
(sieve-manage-setactive): Use the changes above to fix handling of
script names which contain UTF-8 multibyte characters.
(sieve-manage-parse-string):
(sieve-manage-getscript): Add handling of server responses with type
NO. Abort `sieve-manage-getscript' and show error message in message
area.
(sieve-manage-erase):
(sieve-manage-drop-next-answer):
(sieve-manage-parse-crlf): Return erased/dropped data (instead of nil).
(sieve-sasl-auth):
(sieve-manage-getscript):
(sieve-manage-erase):
(sieve-manage-open-server):
(sieve-manage-open):
(sieve-manage-send): Improve logging.
---
lisp/net/sieve-manage.el | 126 +++++++++++++++++++++++++++------------
1 file changed, 87 insertions(+), 39 deletions(-)
diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
index 50342b9105..4a36f94431 100644
--- a/lisp/net/sieve-manage.el
+++ b/lisp/net/sieve-manage.el
@@ -167,7 +167,53 @@ sieve-manage-process
(defvar sieve-manage-capability nil)
;; Internal utility functions
-(autoload 'mm-enable-multibyte "mm-util")
+(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))))
+
+(defun sieve-manage--message (format-string &rest args)
+ "Wrapper around `message' which also logs to sieve manage log.
+
+See `sieve-manage--append-to-log'."
+ (let ((ret (apply #'message
+ (concat "sieve-manage: " format-string)
+ args)))
+ (sieve-manage--append-to-log ret "\n")
+ ret))
+
+(defun sieve-manage--error (format-string &rest args)
+ "Wrapper around `error' which also logs to sieve manage log.
+
+See `sieve-manage--append-to-log'."
+ (let ((msg (apply #'format
+ (concat "sieve-manage/ERROR: " format-string)
+ args)))
+ (sieve-manage--append-to-log msg "\n")
+ (error msg)))
+
+(defun sieve-manage-encode (utf8-string)
+ "Convert UTF8-STRING to managesieve protocol octets."
+ (encode-coding-string utf8-string 'raw-text t))
+
+(defun sieve-manage-decode (octets &optional buffer)
+ "Convert managesieve protocol OCTETS to utf-8 string.
+
+If optional BUFFER is non-nil, insert decoded string into BUFFER."
+ (when octets
+ ;; eol type unix is required to preserve "\r\n"
+ (decode-coding-string octets 'utf-8-unix t buffer)))
(defun sieve-manage-make-process-buffer ()
(with-current-buffer
@@ -175,22 +221,19 @@ sieve-manage-make-process-buffer
sieve-manage-server
sieve-manage-port))
(mapc #'make-local-variable sieve-manage-local-variables)
- (mm-enable-multibyte)
+ (set-buffer-multibyte nil)
+ (setq-local after-change-functions nil)
(buffer-disable-undo)
(current-buffer)))
(defun sieve-manage-erase (&optional p buffer)
- (let ((buffer (or buffer (current-buffer))))
- (and sieve-manage-log
- (with-current-buffer (get-buffer-create sieve-manage-log)
- (mm-enable-multibyte)
- (buffer-disable-undo)
- (goto-char (point-max))
- (insert-buffer-substring buffer (with-current-buffer buffer
- (point-min))
- (or p (with-current-buffer buffer
- (point-max)))))))
- (delete-region (point-min) (or p (point-max))))
+ (with-current-buffer (or buffer (current-buffer))
+ (let* ((start (point-min))
+ (end (or p (point-max)))
+ (logdata (buffer-substring-no-properties start end)))
+ (sieve-manage--append-to-log logdata)
+ (delete-region start end)
+ logdata)))
(defun sieve-manage-open-server (server port &optional stream buffer)
"Open network connection to SERVER on PORT.
@@ -202,6 +245,8 @@ sieve-manage-open-server
(open-network-stream
"SIEVE" buffer server port
:type stream
+ ;; eol type unix is required to preserve "\r\n"
+ :coding 'raw-text-unix
:capability-command "CAPABILITY\r\n"
:end-of-command "^\\(OK\\|NO\\).*\n"
:success "^OK.*\n"
@@ -224,7 +269,7 @@ sieve-manage-open-server
;; Authenticators
(defun sieve-sasl-auth (buffer mech)
"Login to server using the SASL MECH method."
- (message "sieve: Authenticating using %s..." mech)
+ (sieve-manage--message "Authenticating using %s..." mech)
(with-current-buffer buffer
(let* ((auth-info (auth-source-search :host sieve-manage-server
:port "sieve"
@@ -275,11 +320,15 @@ sieve-sasl-auth
(if (and (setq step (sasl-next-step client step))
(setq data (sasl-step-data step)))
;; We got data for server but it's finished
- (error "Server not ready for SASL data: %s" data)
+ (sieve-manage--error
+ "Server not ready for SASL data: %s" data)
;; The authentication process is finished.
+ (sieve-manage--message "Logged in as %s using %s"
+ user-name mech)
(throw 'done t)))
(unless (stringp rsp)
- (error "Server aborted SASL authentication: %s" (caddr rsp)))
+ (sieve-manage--error
+ "Server aborted SASL authentication: %s" (caddr rsp)))
(sasl-step-set-data step (base64-decode-string rsp))
(setq step (sasl-next-step client step))
(sieve-manage-send
@@ -288,8 +337,7 @@ sieve-sasl-auth
(base64-encode-string (sasl-step-data step)
'no-line-break)
"\"")
- ""))))
- (message "sieve: Login using %s...done" mech))))
+ "")))))))
(defun sieve-manage-cram-md5-p (buffer)
(sieve-manage-capability "SASL" "CRAM-MD5" buffer))
@@ -353,7 +401,7 @@ sieve-manage-open
sieve-manage-default-stream)
sieve-manage-auth (or auth
sieve-manage-auth))
- (message "sieve: Connecting to %s..." sieve-manage-server)
+ (sieve-manage--message "Connecting to %s..." sieve-manage-server)
(sieve-manage-open-server sieve-manage-server
sieve-manage-port
sieve-manage-stream
@@ -368,7 +416,8 @@ sieve-manage-open
(setq sieve-manage-auth auth)
(cl-return)))
(unless sieve-manage-auth
- (error "Couldn't figure out authenticator for server")))
+ (sieve-manage--error
+ "Couldn't figure out authenticator for server")))
(sieve-manage-erase)
(current-buffer))))
@@ -433,11 +482,7 @@ sieve-manage-havespace
(defun sieve-manage-putscript (name content &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
- ;; Here we assume that the coding-system will
- ;; replace each char with a single byte.
- ;; This is always the case if `content' is
- ;; a unibyte string.
- (length content)
+ (length (sieve-manage-encode content))
sieve-manage-client-eol content))
(sieve-manage-parse-okno)))
@@ -449,11 +494,10 @@ sieve-manage-deletescript
(defun sieve-manage-getscript (name output-buffer &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "GETSCRIPT \"%s\"" name))
- (let ((script (sieve-manage-parse-string)))
- (sieve-manage-parse-crlf)
- (with-current-buffer output-buffer
- (insert script))
- (sieve-manage-parse-okno))))
+ (sieve-manage-decode (sieve-manage-parse-string)
+ output-buffer)
+ (sieve-manage-parse-crlf)
+ (sieve-manage-parse-okno)))
(defun sieve-manage-setactive (name &optional buffer)
(with-current-buffer (or buffer (current-buffer))
@@ -478,6 +522,9 @@ sieve-manage-drop-next-answer
(defun sieve-manage-ok-p (rsp)
(string= (downcase (or (car-safe rsp) "")) "ok"))
+(defun sieve-manage-no-p (rsp)
+ (string= (downcase (or (car-safe rsp) "")) "no"))
+
(defun sieve-manage-is-okno ()
(when (looking-at (concat
"^\\(OK\\|NO\\)\\( (\\([^)]+\\))\\)?\\( \\(.*\\)\\)?"
@@ -528,7 +575,11 @@ sieve-manage-parse-string
(while (null rsp)
(accept-process-output (get-buffer-process (current-buffer)) 1)
(goto-char (point-min))
- (setq rsp (sieve-manage-is-string)))
+ (unless (setq rsp (sieve-manage-is-string))
+ (when (sieve-manage-no-p (sieve-manage-is-okno))
+ ;; simple `error' is enough since `sieve-manage-erase'
+ ;; already adds the server response to the log
+ (error (sieve-manage-erase)))))
(sieve-manage-erase (point))
rsp))
@@ -540,7 +591,8 @@ sieve-manage-parse-listscripts
(let (tmp rsp data)
(while (null rsp)
(while (null (or (setq rsp (sieve-manage-is-okno))
- (setq tmp (sieve-manage-is-string))))
+ (setq tmp (sieve-manage-decode
+ (sieve-manage-is-string)))))
(accept-process-output (get-buffer-process (current-buffer)) 1)
(goto-char (point-min)))
(when tmp
@@ -559,13 +611,9 @@ sieve-manage-parse-listscripts
rsp)))
(defun sieve-manage-send (cmdstr)
- (setq cmdstr (concat cmdstr sieve-manage-client-eol))
- (and sieve-manage-log
- (with-current-buffer (get-buffer-create sieve-manage-log)
- (mm-enable-multibyte)
- (buffer-disable-undo)
- (goto-char (point-max))
- (insert cmdstr)))
+ (setq cmdstr (sieve-manage-encode
+ (concat cmdstr sieve-manage-client-eol)))
+ (sieve-manage--append-to-log cmdstr)
(process-send-string sieve-manage-process cmdstr))
(provide 'sieve-manage)
--
2.34.1
[Message part 3 (text/plain, inline)]
Both, the (internal) process/protocol buffer and the log buffer are now unibyte. The conversion to multibyte UTF-8 is only done for user visible (UI) buffers. To properly handle the protocol line termination (CRLF), I added `:coding 'raw-text-unix` (with explicit unix EOL convention) to the `open-network-stream' call (also in the new `manage-sieve-encode' function. This was needed to allow keep the various (looking-at "...\r\n" ...) calls working. This is something which still feels a bit weird, but I haven't found another way to do it. I did some tests with (setq-default buffer-file-coding-system 'utf-8-unix/'utf-8-dos) which did not show any issues. I would also add some ERT tests, probably in a separate commit? In addition, I found that `sieve-manage-quit' in sieve.el had the tendency to kill unrelated buffers in case of errors during earlier steps. For this, I created a sepate patch:
[0002-Improve-robustnes-of-sieve-manage-quit-in-case-of-er.patch (text/x-diff, inline)]
From 83ab45907c7b528ae4db98f33415e05e679c312e Mon Sep 17 00:00:00 2001 From: Kai Tetzlaff <emacs <at> tetzco.de> Date: Mon, 28 Feb 2022 11:33:56 +0100 Subject: [PATCH 2/2] Improve robustnes of `sieve-manage-quit' in case of errors * lisp/net/sieve.el (sieve-manage-quit): Avoid killing buffers it's not supposed to touch. --- lisp/net/sieve.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lisp/net/sieve.el b/lisp/net/sieve.el index 630ea04070..5680526389 100644 --- a/lisp/net/sieve.el +++ b/lisp/net/sieve.el @@ -154,7 +154,8 @@ sieve-manage-quit (interactive) (sieve-manage-close sieve-manage-buffer) (kill-buffer sieve-manage-buffer) - (kill-buffer (current-buffer))) + (when-let ((buffer (get-buffer sieve-buffer))) + (kill-buffer buffer))) (defun sieve-bury-buffer () "Bury the Manage Sieve buffer without closing the connection." -- 2.34.1
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 28 Feb 2022 16:20:02 GMT) Full text and rfc822 format available.Message #32 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <kai <at> tetzlaff.eu> To: Lars Ingebrigtsen <larsi <at> gnus.org> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Mon, 28 Feb 2022 13:27:42 +0100
[Message part 1 (text/plain, inline)]
Lars Ingebrigtsen <larsi <at> gnus.org> writes: > Kai Tetzlaff <kai <at> tetzlaff.eu> writes: > >> So just reverting it won't work. I will try to undo the parts relevant >> to this issue. > > Sounds good. Ok, I'm attaching two patches which fix all issues I noticed. What I ended up with is quite a bit more than the initial attempt. Since these changes are non-trivial, I will need to do the copyright assignment. About a week ago I actually sent an email to assign <at> gnu.org to get the process started. But I haven't received a reply. Could you please send me the necessary papers? I'm in Germany, so my understanding is that it should be possible to do this electronically? The first (and major) set of fixes are in sieve-manage.el for the issues with multibyte characters in sieve scripts (sieve-manage-getscript/putscript). This also adds supports for multibyte characters in script names (sieve-manage-listscripts/getscript/putscript/havespace/deletescript/setactive). There is now also some handling of getscript errors reported by the server and improved logging.
[0001-Fix-mostly-multibyte-issues-in-sieve-manage.el-Bug-5.patch (text/x-diff, inline)]
From fd18929ce2004f7448ab997bc86e206afdbd8673 Mon Sep 17 00:00:00 2001
From: Kai Tetzlaff <emacs <at> tetzco.de>
Date: Mon, 28 Feb 2022 11:08:07 +0100
Subject: [PATCH 1/2] Fix (mostly multibyte) issues in sieve-manage.el
(Bug#54154)
The managesieve protocol (s. RFC5804) requires support for (a sightly
restricted variant of) UTF-8 in script content and script names. This
commit fixes/improves the handling of multibyte characters.
In addition, `sieve-manage-getscript' now properly handles NO
responses from the server instead of inflooping.
There are also some logging improvements.
* lisp/net/sieve-manage.el
(sieve-manage--append-to-log):
(sieve-manage--message):
(sieve-manage--error):
(sieve-manage-encode):
(sieve-manage-decode):
(sieve-manage-no-p): New functions.
(sieve-manage-make-process-buffer): Switch process buffer to unibyte.
(sieve-manage-open-server): Add `:coding 'raw-text-unix` to
`open-network-stream' call. Use unix EOLs in order to keep matching
CRLF (aka "\r\n") intact.
(sieve-manage-send): Make sure that UTF-8 multibyte characters are
properly encoded before sending data to the server.
(sieve-manage-getscript):
(sieve-manage-putscript): Use the changes above to fix down/uploading
scripts containing UTF-8 multibyte characters.
(sieve-manage-listscripts):
(sieve-manage-havespace)
(sieve-manage-getscript)
(sieve-manage-putscript):
(sieve-manage-deletescript):
(sieve-manage-setactive): Use the changes above to fix handling of
script names which contain UTF-8 multibyte characters.
(sieve-manage-parse-string):
(sieve-manage-getscript): Add handling of server responses with type
NO. Abort `sieve-manage-getscript' and show error message in message
area.
(sieve-manage-erase):
(sieve-manage-drop-next-answer):
(sieve-manage-parse-crlf): Return erased/dropped data (instead of nil).
(sieve-sasl-auth):
(sieve-manage-getscript):
(sieve-manage-erase):
(sieve-manage-open-server):
(sieve-manage-open):
(sieve-manage-send): Improve logging.
---
lisp/net/sieve-manage.el | 125 +++++++++++++++++++++++++++------------
1 file changed, 86 insertions(+), 39 deletions(-)
diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
index 50342b9105..a57d81efcd 100644
--- a/lisp/net/sieve-manage.el
+++ b/lisp/net/sieve-manage.el
@@ -167,7 +167,52 @@ sieve-manage-process
(defvar sieve-manage-capability nil)
;; Internal utility functions
-(autoload 'mm-enable-multibyte "mm-util")
+(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)))
+ (goto-char (point-max))
+ (apply #'insert args))))
+
+(defun sieve-manage--message (format-string &rest args)
+ "Wrapper around `message' which also logs to sieve manage log.
+
+See `sieve-manage--append-to-log'."
+ (let ((ret (apply #'message
+ (concat "sieve-manage: " format-string)
+ args)))
+ (sieve-manage--append-to-log ret "\n")
+ ret))
+
+(defun sieve-manage--error (format-string &rest args)
+ "Wrapper around `error' which also logs to sieve manage log.
+
+See `sieve-manage--append-to-log'."
+ (let ((msg (apply #'format
+ (concat "sieve-manage/ERROR: " format-string)
+ args)))
+ (sieve-manage--append-to-log msg "\n")
+ (error msg)))
+
+(defun sieve-manage-encode (utf8-string)
+ "Convert UTF8-STRING to managesieve protocol octets."
+ (encode-coding-string utf8-string 'raw-text t))
+
+(defun sieve-manage-decode (octets &optional buffer)
+ "Convert managesieve protocol OCTETS to utf-8 string.
+
+If optional BUFFER is non-nil, insert decoded string into BUFFER."
+ (when octets
+ ;; eol type unix is required to preserve "\r\n"
+ (decode-coding-string octets 'utf-8-unix t buffer)))
(defun sieve-manage-make-process-buffer ()
(with-current-buffer
@@ -175,22 +220,19 @@ sieve-manage-make-process-buffer
sieve-manage-server
sieve-manage-port))
(mapc #'make-local-variable sieve-manage-local-variables)
- (mm-enable-multibyte)
+ (set-buffer-multibyte nil)
+ (setq-local after-change-functions nil)
(buffer-disable-undo)
(current-buffer)))
(defun sieve-manage-erase (&optional p buffer)
- (let ((buffer (or buffer (current-buffer))))
- (and sieve-manage-log
- (with-current-buffer (get-buffer-create sieve-manage-log)
- (mm-enable-multibyte)
- (buffer-disable-undo)
- (goto-char (point-max))
- (insert-buffer-substring buffer (with-current-buffer buffer
- (point-min))
- (or p (with-current-buffer buffer
- (point-max)))))))
- (delete-region (point-min) (or p (point-max))))
+ (with-current-buffer (or buffer (current-buffer))
+ (let* ((start (point-min))
+ (end (or p (point-max)))
+ (logdata (buffer-substring-no-properties start end)))
+ (sieve-manage--append-to-log logdata)
+ (delete-region start end)
+ logdata)))
(defun sieve-manage-open-server (server port &optional stream buffer)
"Open network connection to SERVER on PORT.
@@ -202,6 +244,8 @@ sieve-manage-open-server
(open-network-stream
"SIEVE" buffer server port
:type stream
+ ;; eol type unix is required to preserve "\r\n"
+ :coding 'raw-text-unix
:capability-command "CAPABILITY\r\n"
:end-of-command "^\\(OK\\|NO\\).*\n"
:success "^OK.*\n"
@@ -224,7 +268,7 @@ sieve-manage-open-server
;; Authenticators
(defun sieve-sasl-auth (buffer mech)
"Login to server using the SASL MECH method."
- (message "sieve: Authenticating using %s..." mech)
+ (sieve-manage--message "Authenticating using %s..." mech)
(with-current-buffer buffer
(let* ((auth-info (auth-source-search :host sieve-manage-server
:port "sieve"
@@ -275,11 +319,15 @@ sieve-sasl-auth
(if (and (setq step (sasl-next-step client step))
(setq data (sasl-step-data step)))
;; We got data for server but it's finished
- (error "Server not ready for SASL data: %s" data)
+ (sieve-manage--error
+ "Server not ready for SASL data: %s" data)
;; The authentication process is finished.
+ (sieve-manage--message "Logged in as %s using %s"
+ user-name mech)
(throw 'done t)))
(unless (stringp rsp)
- (error "Server aborted SASL authentication: %s" (caddr rsp)))
+ (sieve-manage--error
+ "Server aborted SASL authentication: %s" (caddr rsp)))
(sasl-step-set-data step (base64-decode-string rsp))
(setq step (sasl-next-step client step))
(sieve-manage-send
@@ -288,8 +336,7 @@ sieve-sasl-auth
(base64-encode-string (sasl-step-data step)
'no-line-break)
"\"")
- ""))))
- (message "sieve: Login using %s...done" mech))))
+ "")))))))
(defun sieve-manage-cram-md5-p (buffer)
(sieve-manage-capability "SASL" "CRAM-MD5" buffer))
@@ -353,7 +400,7 @@ sieve-manage-open
sieve-manage-default-stream)
sieve-manage-auth (or auth
sieve-manage-auth))
- (message "sieve: Connecting to %s..." sieve-manage-server)
+ (sieve-manage--message "Connecting to %s..." sieve-manage-server)
(sieve-manage-open-server sieve-manage-server
sieve-manage-port
sieve-manage-stream
@@ -368,7 +415,8 @@ sieve-manage-open
(setq sieve-manage-auth auth)
(cl-return)))
(unless sieve-manage-auth
- (error "Couldn't figure out authenticator for server")))
+ (sieve-manage--error
+ "Couldn't figure out authenticator for server")))
(sieve-manage-erase)
(current-buffer))))
@@ -433,11 +481,7 @@ sieve-manage-havespace
(defun sieve-manage-putscript (name content &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
- ;; Here we assume that the coding-system will
- ;; replace each char with a single byte.
- ;; This is always the case if `content' is
- ;; a unibyte string.
- (length content)
+ (length (sieve-manage-encode content))
sieve-manage-client-eol content))
(sieve-manage-parse-okno)))
@@ -449,11 +493,10 @@ sieve-manage-deletescript
(defun sieve-manage-getscript (name output-buffer &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "GETSCRIPT \"%s\"" name))
- (let ((script (sieve-manage-parse-string)))
- (sieve-manage-parse-crlf)
- (with-current-buffer output-buffer
- (insert script))
- (sieve-manage-parse-okno))))
+ (sieve-manage-decode (sieve-manage-parse-string)
+ output-buffer)
+ (sieve-manage-parse-crlf)
+ (sieve-manage-parse-okno)))
(defun sieve-manage-setactive (name &optional buffer)
(with-current-buffer (or buffer (current-buffer))
@@ -478,6 +521,9 @@ sieve-manage-drop-next-answer
(defun sieve-manage-ok-p (rsp)
(string= (downcase (or (car-safe rsp) "")) "ok"))
+(defun sieve-manage-no-p (rsp)
+ (string= (downcase (or (car-safe rsp) "")) "no"))
+
(defun sieve-manage-is-okno ()
(when (looking-at (concat
"^\\(OK\\|NO\\)\\( (\\([^)]+\\))\\)?\\( \\(.*\\)\\)?"
@@ -528,7 +574,11 @@ sieve-manage-parse-string
(while (null rsp)
(accept-process-output (get-buffer-process (current-buffer)) 1)
(goto-char (point-min))
- (setq rsp (sieve-manage-is-string)))
+ (unless (setq rsp (sieve-manage-is-string))
+ (when (sieve-manage-no-p (sieve-manage-is-okno))
+ ;; simple `error' is enough since `sieve-manage-erase'
+ ;; already adds the server response to the log
+ (error (sieve-manage-erase)))))
(sieve-manage-erase (point))
rsp))
@@ -540,7 +590,8 @@ sieve-manage-parse-listscripts
(let (tmp rsp data)
(while (null rsp)
(while (null (or (setq rsp (sieve-manage-is-okno))
- (setq tmp (sieve-manage-is-string))))
+ (setq tmp (sieve-manage-decode
+ (sieve-manage-is-string)))))
(accept-process-output (get-buffer-process (current-buffer)) 1)
(goto-char (point-min)))
(when tmp
@@ -559,13 +610,9 @@ sieve-manage-parse-listscripts
rsp)))
(defun sieve-manage-send (cmdstr)
- (setq cmdstr (concat cmdstr sieve-manage-client-eol))
- (and sieve-manage-log
- (with-current-buffer (get-buffer-create sieve-manage-log)
- (mm-enable-multibyte)
- (buffer-disable-undo)
- (goto-char (point-max))
- (insert cmdstr)))
+ (setq cmdstr (sieve-manage-encode
+ (concat cmdstr sieve-manage-client-eol)))
+ (sieve-manage--append-to-log cmdstr)
(process-send-string sieve-manage-process cmdstr))
(provide 'sieve-manage)
--
2.34.1
[Message part 3 (text/plain, inline)]
Both, the (internal) process/protocol buffer and the log buffer are now unibyte. The conversion to multibyte UTF-8 is only done for user visible (UI) buffers. To properly handle the protocol line termination (CRLF), I added `:coding 'raw-text-unix` (with explicit unix EOL convention) to the `open-network-stream' call (also in the new `manage-sieve-encode' function. This was needed to allow keep the various (looking-at "...\r\n" ...) calls working. This is something which still feels a bit weird, but I haven't found another way to do it. I did some tests with (setq-default buffer-file-coding-system 'utf-8-unix/'utf-8-dos) which did not show any issues. I would also add some ERT tests, probably in a separate commit? In addition, I found that `sieve-manage-quit' in sieve.el had the tendency to kill unrelated buffers in case of errors during earlier steps. For this, I created a sepate patch:
[0002-Improve-robustnes-of-sieve-manage-quit-in-case-of-er.patch (text/x-diff, inline)]
From 559ce20b4c9b75f67bef3a1e23b7501bdeaa98d2 Mon Sep 17 00:00:00 2001 From: Kai Tetzlaff <emacs <at> tetzco.de> Date: Mon, 28 Feb 2022 11:33:56 +0100 Subject: [PATCH 2/2] Improve robustnes of `sieve-manage-quit' in case of errors * lisp/net/sieve.el (sieve-manage-quit): Avoid killing buffers it's not supposed to touch. --- lisp/net/sieve.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lisp/net/sieve.el b/lisp/net/sieve.el index 630ea04070..5680526389 100644 --- a/lisp/net/sieve.el +++ b/lisp/net/sieve.el @@ -154,7 +154,8 @@ sieve-manage-quit (interactive) (sieve-manage-close sieve-manage-buffer) (kill-buffer sieve-manage-buffer) - (kill-buffer (current-buffer))) + (when-let ((buffer (get-buffer sieve-buffer))) + (kill-buffer buffer))) (defun sieve-bury-buffer () "Bury the Manage Sieve buffer without closing the connection." -- 2.34.1
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Tue, 06 Sep 2022 11:35:02 GMT) Full text and rfc822 format available.Message #35 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: Kai Tetzlaff <kai <at> tetzlaff.eu> Cc: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Date: Tue, 06 Sep 2022 13:34:13 +0200
Kai Tetzlaff <kai <at> tetzlaff.eu> writes: > Ok, I'm attaching two patches which fix all issues I noticed. Sorry; looks like I forgot about this. Now finally pushed to Emacs 29.
Lars Ingebrigtsen <larsi <at> gnus.org>
to control <at> debbugs.gnu.org.
(Tue, 06 Sep 2022 11:35:03 GMT) Full text and rfc822 format available.Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Wed, 05 Oct 2022 11:24:07 GMT) Full text and rfc822 format available.Kai Tetzlaff <emacs+bug <at> tetzco.de>
to control <at> debbugs.gnu.org.
(Wed, 18 Jan 2023 15:09:01 GMT) Full text and rfc822 format available.bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Wed, 18 Jan 2023 18:34:02 GMT) Full text and rfc822 format available.Message #44 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Herbert J. Skuhra" <herbert <at> gojira.at> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: larsi <larsi <at> gnus.org>, 54154 <at> debbugs.gnu.org Subject: Re: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Wed, 18 Jan 2023 19:28:40 +0100
[Message part 1 (text/plain, inline)]
On Wed, Jan 18, 2023 at 10:09:33AM +0100, Kai Tetzlaff wrote: > Hello Herbert, > > it seems that I'm responsible for this issue. Unfortunately, I cannot > reproduce it with my imap/sieve server setup. However, if you're willing > to provide some additional info, we should hopefully be able to find the > bug. Hello, 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) > "Herbert J. Skuhra" writes: > > I think commit ae963e80a79f5a9184daabfc8197f211a39b136d is causing the > > following issue: > > > > 1. build master or emacs-29 > > 2. run emacs -Q > > 3. M-x sieve-manage and enter imap server. > In my case, after entering the server address, I do get prompted for a > username followed by a password prompt. > > > The following error is displayed: > > sieve-manage: Connecting to <imap server>... > > sieve-manage--message: Wrong type argument: stringp, t > > 4. Repeat step 3 and this time sieve-manage will connect and prompt for username/password > As I wrote above, I already get these prompts after 3. So somehow, my > setup is different from the one you're using. What imap server are you using? > Is the connection to the server using SSL/TLS (in my case it is)? I use imap.mailbox.org and the connection is encrypted (using STARTTLS). But tcpdump doesn't capture any packets when I run sieve-manage for the first time. > Could you re-run the steps above with the following additional steps > before 3.: > > 2a) M-x find-library sieve-manage > 2b) M-x eval-buffer > 2c) M-x find-library sieve > 2d) M-x eval-buffer > 2e) M-x toggle-debug-on-error > > to get a full backtrace and send it to me? Backtrace attached. On Wed, Jan 18, 2023 at 10:09:33AM +0100, Kai Tetzlaff wrote: > Hello Herbert, > > a small update: Please also send the content of the '*sieve-manage-log*' > buffer. There is no *sieve-manage-log* buffer after running sieve-manage for the first time. > (I also added a - hopefully working - email address for Lars.) Sorry, copy&paste error. :-( Thanks. -- Herbert
[sieve-manage.txt (text/plain, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Wed, 18 Jan 2023 19:18:02 GMT) Full text and rfc822 format available.Message #47 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Wed, 18 Jan 2023 21:17:59 +0200
> Cc: larsi <larsi <at> gnus.org>, 54154 <at> debbugs.gnu.org
> Date: Wed, 18 Jan 2023 19:28:40 +0100
> From: "Herbert J. Skuhra" <herbert <at> gojira.at>
>
> On Wed, Jan 18, 2023 at 10:09:33AM +0100, Kai Tetzlaff wrote:
> > Hello Herbert,
> >
> > it seems that I'm responsible for this issue. Unfortunately, I cannot
> > reproduce it with my imap/sieve server setup. However, if you're willing
> > to provide some additional info, we should hopefully be able to find the
> > bug.
>
> Hello,
>
> 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)
Is this problem still relevant? I thought that Lars closed the bug
report back in September?
> > Could you re-run the steps above with the following additional steps
> > before 3.:
> >
> > 2a) M-x find-library sieve-manage
> > 2b) M-x eval-buffer
> > 2c) M-x find-library sieve
> > 2d) M-x eval-buffer
> > 2e) M-x toggle-debug-on-error
> >
> > to get a full backtrace and send it to me?
>
> Backtrace attached.
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?
Herbert, if you make the change above, does the problem go away?
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 07:46:02 GMT) Full text and rfc822 format available.Message #50 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 09:45:21 +0200
> From: Kai Tetzlaff <emacs+bug <at> tetzco.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org
> Date: Thu, 19 Jan 2023 05:06:01 +0100
>
> >> 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))))
Thanks. The duplicate use of with-current-buffer is sub-optimal,
IMO. What about the simpler code below:
(when sieve-manage-log
(let* ((existing-log-buffer (get-buffer sieve-manage-log))
(log-buffer (or existing-log-buffer
(get-buffer-create sieve-manage-log))))
(with-current-buffer log-buffer
(unless existing-log-buffer
;; Do this only once, when creating the log buffer.
(set-buffer-multibyte nil)
(buffer-disable-undo))
(goto-char (point-max))
(apply #'insert args)))))
> ;; Internal utility functions
> +(defun sieve-manage--set-internal-buffer-properties (buffer)
> + "Set BUFFER properties for internally used buffers.
> +
> +Used for process and log buffers, this function makes sure that
> +those buffers keep received and sent data intact by:
> +- setting the coding system to 'sieve-manage--coding-system',
> +- setting `after-change-functions' to nil to avoid those
> + functions messing with buffer content.
> +Also disables undo (to save a bit of memory and improve
> +performance).
> +
> +Returns BUFFER."
> + (with-current-buffer buffer
> + (set-buffer-file-coding-system sieve-manage--coding-system)
> + (setq-local after-change-functions nil)
> + (buffer-disable-undo)
> + (current-buffer)))
> +
> (defun sieve-manage--append-to-log (&rest args)
> "Append ARGS to sieve-manage log buffer.
>
> @@ -175,10 +202,8 @@ sieve-manage--append-to-log
> `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)))
> + (sieve-manage--set-internal-buffer-properties
> + (get-buffer-create sieve-manage-log)))
> (goto-char (point-max))
> (apply #'insert args))))
This still uses a less-than-elegant implementation that calls
with-current-buffer twice.
> (defun sieve-manage-encode (utf8-string)
> "Convert UTF8-STRING to managesieve protocol octets."
> - (encode-coding-string utf8-string 'raw-text t))
> + (encode-coding-string utf8-string sieve-manage--coding-system t))
Why is the argument called utf8-string? If it's indeed a string
encoded in UTF-8, why do you need to encode it again with
raw-text-unix? it should be a no-op in that case. So please tell more
about the underlying issue.
> @@ -244,8 +267,7 @@ sieve-manage-open-server
> (open-network-stream
> "SIEVE" buffer server port
> :type stream
> - ;; eol type unix is required to preserve "\r\n"
> - :coding 'raw-text-unix
> + :coding `(binary . ,sieve-manage--coding-system)
Same question about encoding with raw-text-unix here: using it means
some other code will need to encode the text with a real encoding,
which in this case is UTF-8 (AFAIU the managesieve protocol RFC). So
why not use utf-8-unix here?
Should the addition of BYE support be mentioned in NEWS?
On balance, I think the additional patches should go to master,
indeed. But let's resolve the issues mentioned above first.
Thanks.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 08:22:02 GMT) Full text and rfc822 format available.Message #53 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Herbert J. Skuhra" <herbert <at> gojira.at> To: Eli Zaretskii <eliz <at> gnu.org> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 00:22:44 +0100
On Wed, Jan 18, 2023 at 09:17:59PM +0200, Eli Zaretskii wrote:
> > Cc: larsi <larsi <at> gnus.org>, 54154 <at> debbugs.gnu.org
> > Date: Wed, 18 Jan 2023 19:28:40 +0100
> > From: "Herbert J. Skuhra" <herbert <at> gojira.at>
> >
> > On Wed, Jan 18, 2023 at 10:09:33AM +0100, Kai Tetzlaff wrote:
> > > Hello Herbert,
> > >
> > > it seems that I'm responsible for this issue. Unfortunately, I cannot
> > > reproduce it with my imap/sieve server setup. However, if you're willing
> > > to provide some additional info, we should hopefully be able to find the
> > > bug.
> >
> > Hello,
> >
> > 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)
>
> Is this problem still relevant? I thought that Lars closed the bug
> report back in September?
>
> > > Could you re-run the steps above with the following additional steps
> > > before 3.:
> > >
> > > 2a) M-x find-library sieve-manage
> > > 2b) M-x eval-buffer
> > > 2c) M-x find-library sieve
> > > 2d) M-x eval-buffer
> > > 2e) M-x toggle-debug-on-error
> > >
> > > to get a full backtrace and send it to me?
> >
> > Backtrace attached.
>
> 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?
>
> 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))))
Thanks.
--
Herbert
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 08:22:03 GMT) Full text and rfc822 format available.Message #56 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 05:06:01 +0100
[Message part 1 (text/plain, inline)]
"Herbert J. Skuhra" <herbert <at> 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:
[0001-Fix-bug-in-sieve-manage-append-to-log.patch (text/x-diff, inline)]
From 4198e776da13b603c56acbae0ae89cd9d31cf207 Mon Sep 17 00:00:00 2001
From: Kai Tetzlaff <emacs <at> 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
[Message part 3 (text/plain, inline)]
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 (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 08:22:03 GMT) Full text and rfc822 format available.Message #59 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: [update] bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 05:50:01 +0100
[Message part 1 (text/plain, inline)]
Sorry, the patch with the new test/lisp/net/sieve-manage-tests.el was
incomplete and contained a bug. The attached updated patch(es) should
fix this (the only change is in
0003-Add-test-lisp-net-sieve-manage-tests.el.patch).
"Herbert J. Skuhra" <herbert <at> 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:
[0001-Fix-bug-in-sieve-manage-append-to-log.patch (text/x-diff, inline)]
From 4198e776da13b603c56acbae0ae89cd9d31cf207 Mon Sep 17 00:00:00 2001
From: Kai Tetzlaff <emacs <at> 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
[Message part 3 (text/plain, inline)]
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 (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 12:39:02 GMT) Full text and rfc822 format available.Message #62 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 13:38:13 +0100
[Message part 1 (text/plain, inline)]
Hello Eli, thanks for looking into this! Eli Zaretskii <eliz <at> gnu.org> writes: > The duplicate use of with-current-buffer is sub-optimal, > IMO. What about the simpler code below: > > (when sieve-manage-log > (let* ((existing-log-buffer (get-buffer sieve-manage-log)) > (log-buffer (or existing-log-buffer > (get-buffer-create sieve-manage-log)))) > (with-current-buffer log-buffer > (unless existing-log-buffer > ;; Do this only once, when creating the log buffer. > (set-buffer-multibyte nil) > (buffer-disable-undo)) > (goto-char (point-max)) > (apply #'insert args))))) Yes, that provides more insight into what the code intends to do. Here's the patch (with additional updated doc string):
[0001-Fix-bug-in-sieve-manage-append-to-log-emacs-29-only.patch (text/x-diff, inline)]
From 62d03f302125c0b1aab2e3ae4f5b12b531d30d74 Mon Sep 17 00:00:00 2001 From: Kai Tetzlaff <emacs <at> tetzco.de> Date: Thu, 19 Jan 2023 03:16:14 +0100 Subject: [PATCH] ; Fix bug in sieve-manage--append-to-log (emacs-29 only) This is emacs-29 only, use more elaborate fix for Emacs 30.x (master). * lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix log buffer creation. --- lisp/net/sieve-manage.el | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el index 5bee4f4c4ad..4866f788bff 100644 --- a/lisp/net/sieve-manage.el +++ b/lisp/net/sieve-manage.el @@ -168,19 +168,25 @@ sieve-manage-capability ;; Internal utility functions (defun sieve-manage--append-to-log (&rest args) - "Append ARGS to sieve-manage log buffer. + "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." +The buffer to use for logging is specifified via `sieve-manage-log'. +If it is nil, logging is disabled. + +When the `sieve-manage-log' buffer doesn't exist, it gets created (and +configured with some initial settings)." (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))) - (goto-char (point-max)) - (apply #'insert args)))) + (let* ((existing-log-buffer (get-buffer sieve-manage-log)) + (log-buffer (or existing-log-buffer + (get-buffer-create sieve-manage-log)))) + (with-current-buffer log-buffer + (unless existing-log-buffer + ;; Do this only once, when creating the log buffer. + (set-buffer-multibyte nil) + (buffer-disable-undo)) + (goto-char (point-max)) + (apply #'insert args))))) (defun sieve-manage--message (format-string &rest args) "Wrapper around `message' which also logs to sieve manage log. -- 2.39.0
[Message part 3 (text/plain, inline)]
>> ;; Internal utility functions
>> +(defun sieve-manage--set-internal-buffer-properties (buffer)
>> + "Set BUFFER properties for internally used buffers.
>> +
>> +Used for process and log buffers, this function makes sure that
>> +those buffers keep received and sent data intact by:
>> +- setting the coding system to 'sieve-manage--coding-system',
>> +- setting `after-change-functions' to nil to avoid those
>> + functions messing with buffer content.
>> +Also disables undo (to save a bit of memory and improve
>> +performance).
>> +
>> +Returns BUFFER."
>> + (with-current-buffer buffer
>> + (set-buffer-file-coding-system sieve-manage--coding-system)
>> + (setq-local after-change-functions nil)
>> + (buffer-disable-undo)
>> + (current-buffer)))
>> +
>> (defun sieve-manage--append-to-log (&rest args)
>> "Append ARGS to sieve-manage log buffer.
>>
>> @@ -175,10 +202,8 @@ sieve-manage--append-to-log
>> `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)))
>> + (sieve-manage--set-internal-buffer-properties
>> + (get-buffer-create sieve-manage-log)))
>> (goto-char (point-max))
>> (apply #'insert args))))
>
> This still uses a less-than-elegant implementation that calls
> with-current-buffer twice.
Yes, true. But since `sieve-manage--set-internal-buffer-properties' is
used in two different places, the more elegant solution you suggested
above would require duplicating the body of the function in those
places. I just didn't see a better way. In particular because
`set-buffer-file-coding-system' and `setq-local' only work with
(current-buffer). If you can point me to code which can replace these
with something that takes BUFFER arguments, I can rewrite
`sieve-manage--set-internal-buffer-properties' and avoid using
`with-current-buffer'.
>> (defun sieve-manage-encode (utf8-string)
>> "Convert UTF8-STRING to managesieve protocol octets."
>> - (encode-coding-string utf8-string 'raw-text t))
>> + (encode-coding-string utf8-string sieve-manage--coding-system t))
>
> Why is the argument called utf8-string? If it's indeed a string
> encoded in UTF-8, why do you need to encode it again with
> raw-text-unix? it should be a no-op in that case. So please tell more
> about the underlying issue.
I chose the name as a hint to the user that the incoming string should
be UTF-8 encoded. But that is probably misleading since the string
itself doesn't have an encoding? So let's change the function to:
(defun sieve-manage-encode (str)
"Convert STR to managesieve protocol octets."
(encode-coding-string str sieve-manage--coding-system t))
Regarding the potential double encoding: When sending data over the
network connection, `sieve-manage-encode' intends to make sure that
`utf8-string' data is converted to a byte/octet representation. I tried
to explain that in the doc string of `sieve-manage--coding-system':
(defconst sieve-manage--coding-system 'raw-text-unix
"Use 'raw-text-unix coding system for (network) communication.
Sets the coding system used for the internal (process, log)
buffers and the network stream created to communicate with the
managesieve server. Using 'raw-text encoding enables unibyte
mode and makes sure that sent/received octets (bytes) remain
untouched by the coding system. The explicit use of `-unix`
avoids EOL conversions (and thus keeps CRLF (\"\\r\\n\") intact).")
The original problem was that when communicating with the sievemanage
server, we need to handle length elements where we need make sure that
calculated values take into account that UTF-8 characters may comprise
multiple octets.
Even after reading the relevant sections of the documentation multiple
times I was (and am still) not sure what exactly the various coding
system settings do and how they interact with buffers and networking
functions. So forgive me if what I'm doing there looks weird to your
expert eyes.
When working on the original patch, I had several uhoh moments where
data sent to or received from the network seemed to have been
automatically modified by the coding system (unfortunately, I don't
remember the exact details). So I tried to eliminate any such automatic
modifications by using 'binary or 'raw-text encodings on code paths
which handle network data. Basically, my thinking was: 'better do things
twice/thrice/... before introducing new points of failure'.
>> @@ -244,8 +267,7 @@ sieve-manage-open-server
>> (open-network-stream
>> "SIEVE" buffer server port
>> :type stream
>> - ;; eol type unix is required to preserve "\r\n"
>> - :coding 'raw-text-unix
>> + :coding `(binary . ,sieve-manage--coding-system)
>
> Same question about encoding with raw-text-unix here: using it means
> some other code will need to encode the text with a real encoding,
> which in this case is UTF-8 (AFAIU the managesieve protocol RFC). So
> why not use utf-8-unix here?
Same as above: I'm just not sure that this is the right thing.
But after thinking about it some more, I made the following changes (as
an experiment):
1. set `sieve-manage--coding-system' to 'utf-8-unix and
2. changed the call to `open-network-stream' in the patch above to
>> @@ -244,8 +267,7 @@ sieve-manage-open-server
>> (open-network-stream
>> "SIEVE" buffer server port
>> :type stream
>> - ;; eol type unix is required to preserve "\r\n"
>> - :coding 'raw-text-unix
>> + :coding sieve-manage--coding-system
instead of the previous, asymmetric mix of `binary and
`sieve-manage--coding-system'.
With these changes, there are still no issues when connecting to my
managesieve server which still contains a script with a name that
contains utf-8 multibyte characters. Also, the test I wrote still works
with that change.
So if you think that this makes things clearer, I'm happy to make these
changes. I'm just don't feel confident enough to do this without
additional guidance.
I was also experimenting with some additional changes with the hope to
to just use coding system settings instead of calling
`sieve-manage-encode'/`sieve-manage-decode'. But I couldn't get that to
work.
I added an updated set of Emacs 30.x patches with the changes described
above (plus an additional change in sieve.el which makes sure that the
sieve-manage buffer containing the list of available sieve scripts is
also using coding system 'utf-8-unix).
> Should the addition of BYE support be mentioned in NEWS?
I can certainly do that if you think that this is useful. It just seems
to be more of an internal detail which probably doesn't mean much to
most users.
> On balance, I think the additional patches should go to master,
> indeed. But let's resolve the issues mentioned above first.
Ok, awaiting further input...
[0001-Fix-bug-in-sieve-manage-append-to-log-improve-sieve-.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 13:23:01 GMT) Full text and rfc822 format available.Message #65 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 14:22:39 +0100
Hmm, my previous mail with the updated patches was sent to early. The changes I made now cause issues when actually downloading a script from the server. So ignore my last message for now. I will send an updated version.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 14:10:02 GMT) Full text and rfc822 format available.Message #68 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 16:08:58 +0200
> From: Kai Tetzlaff <emacs+bug <at> tetzco.de> > Cc: herbert <at> gojira.at, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org > Date: Thu, 19 Jan 2023 13:38:13 +0100 > > Eli Zaretskii <eliz <at> gnu.org> writes: > > The duplicate use of with-current-buffer is sub-optimal, > > IMO. What about the simpler code below: > > > > (when sieve-manage-log > > (let* ((existing-log-buffer (get-buffer sieve-manage-log)) > > (log-buffer (or existing-log-buffer > > (get-buffer-create sieve-manage-log)))) > > (with-current-buffer log-buffer > > (unless existing-log-buffer > > ;; Do this only once, when creating the log buffer. > > (set-buffer-multibyte nil) > > (buffer-disable-undo)) > > (goto-char (point-max)) > > (apply #'insert args))))) > > Yes, that provides more insight into what the code intends to do. Here's > the patch (with additional updated doc string): Thanks, installed on the emacs-29 branch. > > This still uses a less-than-elegant implementation that calls > > with-current-buffer twice. > > Yes, true. But since `sieve-manage--set-internal-buffer-properties' is > used in two different places, the more elegant solution you suggested > above would require duplicating the body of the function in those > places. I just didn't see a better way. I'm not sure why you need to force the encoding of the process buffer, when you already set the coding-system to be used for decoding stuff from the process. Is that really needed? But if you really need this, then just make the insertion of the text into the buffer you create optional: then for the process-buffer pass nil as the text to insert, and you can do the with-current-buffer dance only inside that function. > >> (defun sieve-manage-encode (utf8-string) > >> "Convert UTF8-STRING to managesieve protocol octets." > >> - (encode-coding-string utf8-string 'raw-text t)) > >> + (encode-coding-string utf8-string sieve-manage--coding-system t)) > > > > Why is the argument called utf8-string? If it's indeed a string > > encoded in UTF-8, why do you need to encode it again with > > raw-text-unix? it should be a no-op in that case. So please tell more > > about the underlying issue. > > I chose the name as a hint to the user that the incoming string should > be UTF-8 encoded. But that is probably misleading since the string > itself doesn't have an encoding? So let's change the function to: > > (defun sieve-manage-encode (str) > "Convert STR to managesieve protocol octets." > (encode-coding-string str sieve-manage--coding-system t)) > > Regarding the potential double encoding: When sending data over the > network connection, `sieve-manage-encode' intends to make sure that > `utf8-string' data is converted to a byte/octet representation. I tried > to explain that in the doc string of `sieve-manage--coding-system': > > (defconst sieve-manage--coding-system 'raw-text-unix > "Use 'raw-text-unix coding system for (network) communication. > > Sets the coding system used for the internal (process, log) > buffers and the network stream created to communicate with the > managesieve server. Using 'raw-text encoding enables unibyte > mode and makes sure that sent/received octets (bytes) remain > untouched by the coding system. The explicit use of `-unix` > avoids EOL conversions (and thus keeps CRLF (\"\\r\\n\") intact).") > > The original problem was that when communicating with the sievemanage > server, we need to handle length elements where we need make sure that > calculated values take into account that UTF-8 characters may comprise > multiple octets. > > Even after reading the relevant sections of the documentation multiple > times I was (and am still) not sure what exactly the various coding > system settings do and how they interact with buffers and networking > functions. So forgive me if what I'm doing there looks weird to your > expert eyes. > > When working on the original patch, I had several uhoh moments where > data sent to or received from the network seemed to have been > automatically modified by the coding system (unfortunately, I don't > remember the exact details). So I tried to eliminate any such automatic > modifications by using 'binary or 'raw-text encodings on code paths > which handle network data. Basically, my thinking was: 'better do things > twice/thrice/... before introducing new points of failure'. Since you seem to be encoding and decoding to/from UTF-8 by hand in sieve-manage-encode/decode, you should use 'binary' as the process-codings-system for the network connection to the server, and that's it. I see no reason to encode again using raw-text-unix. What you should do is call sieve-manage-encode inside sieve-manage-send, and count the bytes there after encoding the payload. > >> @@ -244,8 +267,7 @@ sieve-manage-open-server > >> (open-network-stream > >> "SIEVE" buffer server port > >> :type stream > >> - ;; eol type unix is required to preserve "\r\n" > >> - :coding 'raw-text-unix > >> + :coding `(binary . ,sieve-manage--coding-system) > > > > Same question about encoding with raw-text-unix here: using it means > > some other code will need to encode the text with a real encoding, > > which in this case is UTF-8 (AFAIU the managesieve protocol RFC). So > > why not use utf-8-unix here? > > Same as above: I'm just not sure that this is the right thing. See above. > But after thinking about it some more, I made the following changes (as > an experiment): > > 1. set `sieve-manage--coding-system' to 'utf-8-unix and > 2. changed the call to `open-network-stream' in the patch above to > >> @@ -244,8 +267,7 @@ sieve-manage-open-server > >> (open-network-stream > >> "SIEVE" buffer server port > >> :type stream > >> - ;; eol type unix is required to preserve "\r\n" > >> - :coding 'raw-text-unix > >> + :coding sieve-manage--coding-system > instead of the previous, asymmetric mix of `binary and > `sieve-manage--coding-system'. This could work, but AFAIU, you need to specify the content length in bytes for the PUTSCRIPT command, so you must encode the content yourself. Thus my suggestion to use 'binary' in the :coding attribute of the process, and instead encode/decode using sieve-manage-encode/decode to/from UTF-8 inside sieve-manage-send and sieve-manage-parse-* functions. > > Should the addition of BYE support be mentioned in NEWS? > > I can certainly do that if you think that this is useful. It just seems > to be more of an internal detail which probably doesn't mean much to > most users. Isn't BYE provides some capabilities that user/callers would like to know about?
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 14:17:01 GMT) Full text and rfc822 format available.Message #71 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 15:16:16 +0100
[Message part 1 (text/plain, inline)]
Here's the update. As a summary, this change: 2. changed the call to `open-network-stream' in the patch above to >> @@ -244,8 +267,7 @@ sieve-manage-open-server >> (open-network-stream >> "SIEVE" buffer server port >> :type stream >> - ;; eol type unix is required to preserve "\r\n" >> - :coding 'raw-text-unix >> + :coding sieve-manage--coding-system needed to be reverted to what I had earlier: >> @@ -244,8 +267,7 @@ sieve-manage-open-server >> (open-network-stream >> "SIEVE" buffer server port >> :type stream >> - ;; eol type unix is required to preserve "\r\n" >> - :coding 'raw-text-unix >> + :coding `(binary . ,sieve-manage--coding-system) I updated the text below and the patches accordingly. Hello Eli, thanks for looking into this! Eli Zaretskii <eliz <at> gnu.org> writes: > The duplicate use of with-current-buffer is sub-optimal, > IMO. What about the simpler code below: > > (when sieve-manage-log > (let* ((existing-log-buffer (get-buffer sieve-manage-log)) > (log-buffer (or existing-log-buffer > (get-buffer-create sieve-manage-log)))) > (with-current-buffer log-buffer > (unless existing-log-buffer > ;; Do this only once, when creating the log buffer. > (set-buffer-multibyte nil) > (buffer-disable-undo)) > (goto-char (point-max)) > (apply #'insert args))))) Yes, that provides more insight into what the code intends to do. Here's the patch (with additional updated doc string):
[0001-Fix-bug-in-sieve-manage-append-to-log-emacs-29-only.patch (text/x-diff, inline)]
From 62d03f302125c0b1aab2e3ae4f5b12b531d30d74 Mon Sep 17 00:00:00 2001 From: Kai Tetzlaff <emacs <at> tetzco.de> Date: Thu, 19 Jan 2023 03:16:14 +0100 Subject: [PATCH] ; Fix bug in sieve-manage--append-to-log (emacs-29 only) This is emacs-29 only, use more elaborate fix for Emacs 30.x (master). * lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix log buffer creation. --- lisp/net/sieve-manage.el | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el index 5bee4f4c4ad..4866f788bff 100644 --- a/lisp/net/sieve-manage.el +++ b/lisp/net/sieve-manage.el @@ -168,19 +168,25 @@ sieve-manage-capability ;; Internal utility functions (defun sieve-manage--append-to-log (&rest args) - "Append ARGS to sieve-manage log buffer. + "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." +The buffer to use for logging is specifified via `sieve-manage-log'. +If it is nil, logging is disabled. + +When the `sieve-manage-log' buffer doesn't exist, it gets created (and +configured with some initial settings)." (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))) - (goto-char (point-max)) - (apply #'insert args)))) + (let* ((existing-log-buffer (get-buffer sieve-manage-log)) + (log-buffer (or existing-log-buffer + (get-buffer-create sieve-manage-log)))) + (with-current-buffer log-buffer + (unless existing-log-buffer + ;; Do this only once, when creating the log buffer. + (set-buffer-multibyte nil) + (buffer-disable-undo)) + (goto-char (point-max)) + (apply #'insert args))))) (defun sieve-manage--message (format-string &rest args) "Wrapper around `message' which also logs to sieve manage log. -- 2.39.0
[Message part 3 (text/plain, inline)]
>> ;; Internal utility functions
>> +(defun sieve-manage--set-internal-buffer-properties (buffer)
>> + "Set BUFFER properties for internally used buffers.
>> +
>> +Used for process and log buffers, this function makes sure that
>> +those buffers keep received and sent data intact by:
>> +- setting the coding system to 'sieve-manage--coding-system',
>> +- setting `after-change-functions' to nil to avoid those
>> + functions messing with buffer content.
>> +Also disables undo (to save a bit of memory and improve
>> +performance).
>> +
>> +Returns BUFFER."
>> + (with-current-buffer buffer
>> + (set-buffer-file-coding-system sieve-manage--coding-system)
>> + (setq-local after-change-functions nil)
>> + (buffer-disable-undo)
>> + (current-buffer)))
>> +
>> (defun sieve-manage--append-to-log (&rest args)
>> "Append ARGS to sieve-manage log buffer.
>>
>> @@ -175,10 +202,8 @@ sieve-manage--append-to-log
>> `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)))
>> + (sieve-manage--set-internal-buffer-properties
>> + (get-buffer-create sieve-manage-log)))
>> (goto-char (point-max))
>> (apply #'insert args))))
>
> This still uses a less-than-elegant implementation that calls
> with-current-buffer twice.
Yes, true. But since `sieve-manage--set-internal-buffer-properties' is
used in two different places, the more elegant solution you suggested
above would require duplicating the body of the function in those
places. I just didn't see a better way. In particular because
`set-buffer-file-coding-system' and `setq-local' only work with
(current-buffer). If you can point me to code which can replace these
with something that takes BUFFER arguments, I can rewrite
`sieve-manage--set-internal-buffer-properties' and avoid using
`with-current-buffer'.
>> (defun sieve-manage-encode (utf8-string)
>> "Convert UTF8-STRING to managesieve protocol octets."
>> - (encode-coding-string utf8-string 'raw-text t))
>> + (encode-coding-string utf8-string sieve-manage--coding-system t))
>
> Why is the argument called utf8-string? If it's indeed a string
> encoded in UTF-8, why do you need to encode it again with
> raw-text-unix? it should be a no-op in that case. So please tell more
> about the underlying issue.
I chose the name as a hint to the user that the incoming string should
be UTF-8 encoded. But that is probably misleading since the string
itself doesn't have an encoding? So let's change the function to:
(defun sieve-manage-encode (str)
"Convert STR to managesieve protocol octets."
(encode-coding-string str sieve-manage--coding-system t))
Regarding the potential double encoding: When sending data over the
network connection, `sieve-manage-encode' intends to make sure that
`utf8-string' data is converted to a byte/octet representation. I tried
to explain that in the doc string of `sieve-manage--coding-system':
(defconst sieve-manage--coding-system 'raw-text-unix
"Use 'raw-text-unix coding system for (network) communication.
Sets the coding system used for the internal (process, log)
buffers and the network stream created to communicate with the
managesieve server. Using 'raw-text encoding enables unibyte
mode and makes sure that sent/received octets (bytes) remain
untouched by the coding system. The explicit use of `-unix`
avoids EOL conversions (and thus keeps CRLF (\"\\r\\n\") intact).")
The original problem was that when communicating with the sievemanage
server, we need to handle length elements where we need make sure that
calculated values take into account that UTF-8 characters may comprise
multiple octets.
Even after reading the relevant sections of the documentation multiple
times I was (and am still) not sure what exactly the various coding
system settings do and how they interact with buffers and networking
functions. So forgive me if what I'm doing there looks weird to your
expert eyes.
When working on the original patch, I had several uhoh moments where
data sent to or received from the network seemed to have been
automatically modified by the coding system (unfortunately, I don't
remember the exact details). So I tried to eliminate any such automatic
modifications by using 'binary or 'raw-text encodings on code paths
which handle network data. Basically, my thinking was: 'better do things
twice/thrice/... before introducing new points of failure'.
>> @@ -244,8 +267,7 @@ sieve-manage-open-server
>> (open-network-stream
>> "SIEVE" buffer server port
>> :type stream
>> - ;; eol type unix is required to preserve "\r\n"
>> - :coding 'raw-text-unix
>> + :coding `(binary . ,sieve-manage--coding-system)
>
> Same question about encoding with raw-text-unix here: using it means
> some other code will need to encode the text with a real encoding,
> which in this case is UTF-8 (AFAIU the managesieve protocol RFC). So
> why not use utf-8-unix here?
Same as above: I'm just not sure that this is the right thing.
But after thinking about it some more, I made the following change (as
an experiment):
1. set `sieve-manage--coding-system' to 'utf-8-unix (and update
the doc string)
With this change, there are still no issues when connecting to my
managesieve server which still contains a script with a name that
contains utf-8 multibyte characters. Also, the test I wrote still works
with that change.
So if you think that this makes things clearer, I'm happy to make the
change. I just don't feel confident enough to do this without
additional guidance.
I was also experimenting with some additional changes with the hope to
to just use coding system settings instead of calling
`sieve-manage-encode'/`sieve-manage-decode'. But I couldn't get that to
work.
I added an updated set of Emacs 30.x patches with the changes described
above (plus an additional change in sieve.el which makes sure that the
sieve-manage buffer containing the list of available sieve scripts is
also using coding system 'utf-8-unix from `sieve-manage--coding-system').
> Should the addition of BYE support be mentioned in NEWS?
I can certainly do that if you think that this is useful. It just seems
to be more of an internal detail which probably doesn't mean much to
most users.
> On balance, I think the additional patches should go to master,
> indeed. But let's resolve the issues mentioned above first.
Ok, awaiting further input...
[0001-Fix-bug-in-sieve-manage-append-to-log-improve-sieve-.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 16:00:02 GMT) Full text and rfc822 format available.Message #74 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 16:59:36 +0100
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > This still uses a less-than-elegant implementation that calls
>> > with-current-buffer twice.
>>
>> Yes, true. But since `sieve-manage--set-internal-buffer-properties' is
>> used in two different places, the more elegant solution you suggested
>> above would require duplicating the body of the function in those
>> places. I just didn't see a better way.
>
> I'm not sure why you need to force the encoding of the process buffer,
> when you already set the coding-system to be used for decoding stuff
> from the process. Is that really needed?
Not sure if it is really needed. But I wanted to make sure that both,
the process buffer and the log buffer use identical settings. Otherwise,
the content of the log buffer might be misleading.
> But if you really need this, then just make the insertion of the text
> into the buffer you create optional: then for the process-buffer pass
> nil as the text to insert, and you can do the with-current-buffer
> dance only inside that function.
Sorry, you lost me there. I don't understand what you want to tell me.
Which (optional) text in which buffer?
> Since you seem to be encoding and decoding to/from UTF-8 by hand in
> sieve-manage-encode/decode, you should use 'binary' as the
> process-codings-system for the network connection to the server, and
> that's it.
That works. Done.
> What you should do is call sieve-manage-encode inside
> sieve-manage-send, and count the bytes there after encoding the
> payload.
Unfortunately, that is too late since the sent data - in case that the
sent text may contain CRLF sequences - contains its own length. So in
order to insert the correct length, I need to encode before sending.
See:
(defun sieve-manage-putscript (name content &optional buffer)
(with-current-buffer (or buffer (current-buffer))
(sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
(length (sieve-manage-encode content))
sieve-manage-client-eol content))
(sieve-manage-parse-oknobye)))
>> 1. set `sieve-manage--coding-system' to 'utf-8-unix and
>> 2. changed the call to `open-network-stream' in the patch above to
>> >> @@ -244,8 +267,7 @@ sieve-manage-open-server
>> >> (open-network-stream
>> >> "SIEVE" buffer server port
>> >> :type stream
>> >> - ;; eol type unix is required to preserve "\r\n"
>> >> - :coding 'raw-text-unix
>> >> + :coding sieve-manage--coding-system
>> instead of the previous, asymmetric mix of `binary and
>> `sieve-manage--coding-system'.
>
> This could work, but AFAIU, you need to specify the content length in
> bytes for the PUTSCRIPT command, so you must encode the content
> yourself. Thus my suggestion to use 'binary' in the :coding attribute
> of the process, and instead encode/decode using
> sieve-manage-encode/decode to/from UTF-8 inside sieve-manage-send and
> sieve-manage-parse-* functions.
Yes, that's what I explained above (before reading this part of your
reply). Unfortunately, just using `sieve-manage--coding-system' for the
:coding property didn't work, but I'm now using 'binary' encoding for
both directions.
>> > Should the addition of BYE support be mentioned in NEWS?
>>
>> I can certainly do that if you think that this is useful. It just seems
>> to be more of an internal detail which probably doesn't mean much to
>> most users.
>
> Isn't BYE provides some capabilities that user/callers would like to
> know about?
From RFC5804:
response-nobye = ("NO" / "BYE") [SP "(" resp-code ")"]
[SP string] CRLF
;; The string contains human-readable text
;; encoded as UTF-8.
As far as I understand, the difference between NO and BYE is that BYE is
just a different (and more drastic, because the server will also
disconnect) way of signalling an error. Fortunately, the human readable
<string> is typically included in these responses and will be shown to
the user.
Here is some more info about where BYE SHOULD (not MUST) be used:
The BYE response SHOULD be used if the server wishes to close the
connection. A server may wish to do this because the client was idle
for too long or there were too many failed authentication attempts.
This response can be issued at any time and should be immediately
followed by a server hang-up of the connection. ...
If I remember correctly, the timeout case was the reason why I added the
BYE handling (since during my experiments, I sometimes used the debugger
to understand what's going on which introduced long delays between
connection establishment/authentication and sending the first request
and resulted in a BYE instead of a NO).
There's also one additional (more interesting) case:
REFERRAL
This response code may be returned with a BYE result from any
command, and includes a mandatory parameter that indicates what
server to access to manage this user's Sieve scripts. The server
will be specified by a Sieve URL (see Section 3). The scriptname
portion of the URL MUST NOT be specified. The client should
authenticate to the specified server and use it for all further
commands in the current session.
However, even my updated sieve-manage code doesn't handle REFERRALs.
So I still think that to understand the difference between a BYE and a
NO would require the user to take a (deeper) dive into the RFC. But to
avoid a longer discussion, how about:
* Changes in Specialized Modes and Packages in Emacs 30.1
...
** sieve-manage
--
*** Added (partial) handling of BYE responses
The managesieve client in `sieve-manage' now handles BYE responses
sent be the server (in addition to OK and NO). This makes the
implementation more robust in case of e.g. timeouts and
authentication failures.
Note: The special case of a REFERRAL/BYE responses is still not
handled by the client (s. RFC5804 for more details).
[0001-Fix-bug-in-sieve-manage-append-to-log-improve-sieve-.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 17:42:02 GMT) Full text and rfc822 format available.Message #77 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 19:41:13 +0200
> From: Kai Tetzlaff <emacs+bug <at> tetzco.de>
> Cc: herbert <at> gojira.at, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org
> Date: Thu, 19 Jan 2023 16:59:36 +0100
>
> >> Yes, true. But since `sieve-manage--set-internal-buffer-properties' is
> >> used in two different places, the more elegant solution you suggested
> >> above would require duplicating the body of the function in those
> >> places. I just didn't see a better way.
> >
> > I'm not sure why you need to force the encoding of the process buffer,
> > when you already set the coding-system to be used for decoding stuff
> > from the process. Is that really needed?
>
> Not sure if it is really needed. But I wanted to make sure that both,
> the process buffer and the log buffer use identical settings. Otherwise,
> the content of the log buffer might be misleading.
I don't think it could mislead, but OK.
> > But if you really need this, then just make the insertion of the text
> > into the buffer you create optional: then for the process-buffer pass
> > nil as the text to insert, and you can do the with-current-buffer
> > dance only inside that function.
>
> Sorry, you lost me there. I don't understand what you want to tell me.
> Which (optional) text in which buffer?
I meant this:
(defun sieve-manage--set-buffer-and-append-text (buffer-name &rest args)
(let ((existing-buffer (get-buffer buffer-name))
new-buffer)
(if existing-buffer
(setq new-buffer existing-buffer)
(setq new-buffer (get-buffer-create buffer-name)))
(with-current-buffer new-buffer
(when (not existing-buffer)
(set-buffer-file-coding-system sieve-manage--coding-system)
(setq-local after-change-functions nil)
(buffer-disable-undo)
; What happened to set-buffer-multibyte?
)
(goto-char (point-max))
(apply #'insert args))))
Then you can call it from sieve-manage-make-process-buffer like this:
(sieve-manage--set-buffer-and-append-text
(format " *sieve %s:%s*" sieve-manage-server sieve-manage-port)
"")
i.e. with an empty string, so nothing gets inserted into the process
buffer. Or you could instead change the signature to accept a single
&optional argument that is a list, and then you could make the last
two lines in the function above conditional on that argument being
non-nil.
> > Since you seem to be encoding and decoding to/from UTF-8 by hand in
> > sieve-manage-encode/decode, you should use 'binary' as the
> > process-codings-system for the network connection to the server, and
> > that's it.
>
> That works. Done.
>
> > What you should do is call sieve-manage-encode inside
> > sieve-manage-send, and count the bytes there after encoding the
> > payload.
>
> Unfortunately, that is too late since the sent data - in case that the
> sent text may contain CRLF sequences - contains its own length. So in
> order to insert the correct length, I need to encode before sending.
> See:
>
> (defun sieve-manage-putscript (name content &optional buffer)
> (with-current-buffer (or buffer (current-buffer))
> (sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
> (length (sieve-manage-encode content))
> sieve-manage-client-eol content))
> (sieve-manage-parse-oknobye)))
This is because you pass both the text and the number to 'format'.
But that is not carved in stone: the "%d" part can never produce any
non-ASCII characters, so there's no need to encode it together with
CONTENT. You could do this instead:
(defun sieve-manage-send (command &optional payload)
(let ((encoded (if payload (encode-coding-string payload 'utf-8-unix)))
size cmdstr)
(if encoded
(setq size (format " {%d+}%s"
(length encoded) sieve-manage-client-eol)))
(setq cmdstr (concat command size encoded))
(sieve-manage--append-to-log cmdstr)
(process-send-string sieve-manage-process cmdstr)))
And then you call this like below:
(sieve-manage-send (format "PUTSCRIPT \"%s\"" name) content)
(sieve-manage-send (format "HAVESPACE \"%s\" %s" name size))
I hope this clarifies my proposal.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Thu, 19 Jan 2023 21:34:01 GMT) Full text and rfc822 format available.Message #80 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 22:33:14 +0100
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Kai Tetzlaff <emacs+bug <at> tetzco.de>
>> Cc: herbert <at> gojira.at, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org
>> Date: Thu, 19 Jan 2023 16:59:36 +0100
>>
>> >> Yes, true. But since `sieve-manage--set-internal-buffer-properties' is
>> >> used in two different places, the more elegant solution you suggested
>> >> above would require duplicating the body of the function in those
>> >> places. I just didn't see a better way.
>> >
>> > I'm not sure why you need to force the encoding of the process buffer,
>> > when you already set the coding-system to be used for decoding stuff
>> > from the process. Is that really needed?
>>
>> Not sure if it is really needed. But I wanted to make sure that both,
>> the process buffer and the log buffer use identical settings. Otherwise,
>> the content of the log buffer might be misleading.
>
> I don't think it could mislead, but OK.
>
>> > But if you really need this, then just make the insertion of the text
>> > into the buffer you create optional: then for the process-buffer pass
>> > nil as the text to insert, and you can do the with-current-buffer
>> > dance only inside that function.
>>
>> Sorry, you lost me there. I don't understand what you want to tell me.
>> Which (optional) text in which buffer?
>
> I meant this:
>
> (defun sieve-manage--set-buffer-and-append-text (buffer-name &rest args)
> (let ((existing-buffer (get-buffer buffer-name))
> new-buffer)
> (if existing-buffer
> (setq new-buffer existing-buffer)
> (setq new-buffer (get-buffer-create buffer-name)))
> (with-current-buffer new-buffer
> (when (not existing-buffer)
> (set-buffer-file-coding-system sieve-manage--coding-system)
> (setq-local after-change-functions nil)
> (buffer-disable-undo)
> ; What happened to set-buffer-multibyte?
> )
> (goto-char (point-max))
> (apply #'insert args))))
>
> Then you can call it from sieve-manage-make-process-buffer like this:
>
> (sieve-manage--set-buffer-and-append-text
> (format " *sieve %s:%s*" sieve-manage-server sieve-manage-port)
> "")
>
> i.e. with an empty string, so nothing gets inserted into the process
> buffer. Or you could instead change the signature to accept a single
> &optional argument that is a list, and then you could make the last
> two lines in the function above conditional on that argument being
> non-nil.
Ok, I now implemented it like this:
(defun sieve-manage--set-buffer-maybe-append-text (buffer-name
&rest args)
"Append ARGS to buffer named BUFFER-NAME and return buffer.
To be used with process and log buffers. When the buffer doesn't
exist, it gets created and configure as follows:
- set coding system to 'sieve-manage--coding-system',
- set buffer to single-byte mode,
- set `after-change-functions' to nil to avoid those
functions messing with buffer content,
- disable undo (to save a bit of memory and improve
performance).
ARGS can be a nil, a string or a list of strings. If no
ARGS are provided, the content of buffer will not be
modified."
(let* ((existing-buffer (get-buffer buffer-name))
(buffer (or existing-buffer
(get-buffer-create buffer-name))))
(with-current-buffer buffer
(unless existing-buffer
(set-buffer-file-coding-system sieve-manage--coding-system)
(set-buffer-multibyte nil)
(setq-local after-change-functions nil)
(buffer-disable-undo))
(when args
(goto-char (point-max))
(apply #'insert args)))
buffer))
(defun sieve-manage--append-to-log (&rest args)
"Append ARGS to `sieve-manage-log' buffer.
If `sieve-manage-log' is nil, logging is disabled. See also
`sieve-manage--set-buffer-maybe-append-text'."
(when sieve-manage-log
(apply #'sieve-manage--set-buffer-maybe-append-text
sieve-manage-log
args)))
(defun sieve-manage-make-process-buffer ()
(let ((buffer (sieve-manage--set-buffer-maybe-append-text
(format " *sieve %s:%s*"
sieve-manage-server
sieve-manage-port))))
(with-current-buffer buffer
(mapc #'make-local-variable sieve-manage-local-variables))
buffer))
Is that better, now? I also added the (set-buffer-multibyte nil) back
into the mix. My understanding was that it was not needed when using the
'raw-text-unix coding system but it is now after switching to
'utf-8-unix?
>> > What you should do is call sieve-manage-encode inside
>> > sieve-manage-send, and count the bytes there after encoding the
>> > payload.
>>
>> Unfortunately, that is too late since the sent data - in case that the
>> sent text may contain CRLF sequences - contains its own length. So in
>> order to insert the correct length, I need to encode before sending.
>> See:
>>
>> (defun sieve-manage-putscript (name content &optional buffer)
>> (with-current-buffer (or buffer (current-buffer))
>> (sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
>> (length (sieve-manage-encode content))
>> sieve-manage-client-eol content))
>> (sieve-manage-parse-oknobye)))
>
> This is because you pass both the text and the number to 'format'.
> But that is not carved in stone: the "%d" part can never produce any
> non-ASCII characters, so there's no need to encode it together with
> CONTENT. You could do this instead:
>
> (defun sieve-manage-send (command &optional payload)
> (let ((encoded (if payload (encode-coding-string payload 'utf-8-unix)))
> size cmdstr)
> (if encoded
> (setq size (format " {%d+}%s"
> (length encoded) sieve-manage-client-eol)))
> (setq cmdstr (concat command size encoded))
> (sieve-manage--append-to-log cmdstr)
> (process-send-string sieve-manage-process cmdstr)))
>
> And then you call this like below:
>
> (sieve-manage-send (format "PUTSCRIPT \"%s\"" name) content)
> (sieve-manage-send (format "HAVESPACE \"%s\" %s" name size))
>
> I hope this clarifies my proposal.
Yes, it does. Actually, I like it.
RFC5804 specifies three variants for string encoding:
string = quoted / literal-c2s / literal-s2c
Only the first two are relevant for `sieve-menage-send' ('literal-s2c'
is for messages from s(server) to c(lient)).
For PUTSCRIPT, we need to use 'literal-c2s' which requires the
additional length element (since 'quoted' is a) limited in the character
set and b) may not exceed 1024 characters). So I would just modify the
your `sieve-manage-send' like this:
(defun sieve-manage-send (command &optional payload-str)
"Send COMMAND with optional PAYLOAD-STR.
If non-nil, PAYLOAD-STR will be appended to COMMAND using the
\\='literal-s2c\\' representation according to RFC5804."
(let ((encoded (when payload-str (sieve-manage-encode payload-str)))
literal-c2s cmdstr)
(when encoded
(setq literal-c2s (format " {%d+}%s%s"
(length encoded)
sieve-manage-client-eol
encoded)))
(setq cmdstr (concat (sieve-manage-encode command)
literal-c2s
sieve-manage-client-eol))
(sieve-manage--append-to-log cmdstr)
(process-send-string sieve-manage-process cmdstr)))
Apart from renaming some of the variables, this adds encoding of
`command' itself (since command may contain multibyte characters in
script names) and an additional `sieve-manage-client-eol' at the end
of `cmdstr'.
As before, updated patches are attached.
[0001-Fix-bug-in-sieve-manage-append-to-log-improve-sieve-.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Fri, 20 Jan 2023 06:55:01 GMT) Full text and rfc822 format available.Message #83 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Fri, 20 Jan 2023 07:54:15 +0100
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
Sorry, the first patch in the last email was outdated. Please check the
updated ones below.
>> From: Kai Tetzlaff <emacs+bug <at> tetzco.de>
>> Cc: herbert <at> gojira.at, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org
>> Date: Thu, 19 Jan 2023 16:59:36 +0100
>>
>> >> Yes, true. But since `sieve-manage--set-internal-buffer-properties' is
>> >> used in two different places, the more elegant solution you suggested
>> >> above would require duplicating the body of the function in those
>> >> places. I just didn't see a better way.
>> >
>> > I'm not sure why you need to force the encoding of the process buffer,
>> > when you already set the coding-system to be used for decoding stuff
>> > from the process. Is that really needed?
>>
>> Not sure if it is really needed. But I wanted to make sure that both,
>> the process buffer and the log buffer use identical settings. Otherwise,
>> the content of the log buffer might be misleading.
>
> I don't think it could mislead, but OK.
>
>> > But if you really need this, then just make the insertion of the text
>> > into the buffer you create optional: then for the process-buffer pass
>> > nil as the text to insert, and you can do the with-current-buffer
>> > dance only inside that function.
>>
>> Sorry, you lost me there. I don't understand what you want to tell me.
>> Which (optional) text in which buffer?
>
> I meant this:
>
> (defun sieve-manage--set-buffer-and-append-text (buffer-name &rest args)
> (let ((existing-buffer (get-buffer buffer-name))
> new-buffer)
> (if existing-buffer
> (setq new-buffer existing-buffer)
> (setq new-buffer (get-buffer-create buffer-name)))
> (with-current-buffer new-buffer
> (when (not existing-buffer)
> (set-buffer-file-coding-system sieve-manage--coding-system)
> (setq-local after-change-functions nil)
> (buffer-disable-undo)
> ; What happened to set-buffer-multibyte?
> )
> (goto-char (point-max))
> (apply #'insert args))))
>
> Then you can call it from sieve-manage-make-process-buffer like this:
>
> (sieve-manage--set-buffer-and-append-text
> (format " *sieve %s:%s*" sieve-manage-server sieve-manage-port)
> "")
>
> i.e. with an empty string, so nothing gets inserted into the process
> buffer. Or you could instead change the signature to accept a single
> &optional argument that is a list, and then you could make the last
> two lines in the function above conditional on that argument being
> non-nil.
Ok, I now implemented it like this:
(defun sieve-manage--set-buffer-maybe-append-text (buffer-name
&rest args)
"Append ARGS to buffer named BUFFER-NAME and return buffer.
To be used with process and log buffers. When the buffer doesn't
exist, it gets created and configure as follows:
- set coding system to 'sieve-manage--coding-system',
- set buffer to single-byte mode,
- set `after-change-functions' to nil to avoid those
functions messing with buffer content,
- disable undo (to save a bit of memory and improve
performance).
ARGS can be a nil, a string or a list of strings. If no
ARGS are provided, the content of buffer will not be
modified."
(let* ((existing-buffer (get-buffer buffer-name))
(buffer (or existing-buffer
(get-buffer-create buffer-name))))
(with-current-buffer buffer
(unless existing-buffer
(set-buffer-file-coding-system sieve-manage--coding-system)
(set-buffer-multibyte nil)
(setq-local after-change-functions nil)
(buffer-disable-undo))
(when args
(goto-char (point-max))
(apply #'insert args)))
buffer))
(defun sieve-manage--append-to-log (&rest args)
"Append ARGS to `sieve-manage-log' buffer.
If `sieve-manage-log' is nil, logging is disabled. See also
`sieve-manage--set-buffer-maybe-append-text'."
(when sieve-manage-log
(apply #'sieve-manage--set-buffer-maybe-append-text
sieve-manage-log
args)))
(defun sieve-manage-make-process-buffer ()
(let ((buffer (sieve-manage--set-buffer-maybe-append-text
(format " *sieve %s:%s*"
sieve-manage-server
sieve-manage-port))))
(with-current-buffer buffer
(mapc #'make-local-variable sieve-manage-local-variables))
buffer))
Is that better, now? I also added the (set-buffer-multibyte nil) back
into the mix. My understanding was that it was not needed when using the
'raw-text-unix coding system but it is now after switching to
'utf-8-unix?
>> > What you should do is call sieve-manage-encode inside
>> > sieve-manage-send, and count the bytes there after encoding the
>> > payload.
>>
>> Unfortunately, that is too late since the sent data - in case that the
>> sent text may contain CRLF sequences - contains its own length. So in
>> order to insert the correct length, I need to encode before sending.
>> See:
>>
>> (defun sieve-manage-putscript (name content &optional buffer)
>> (with-current-buffer (or buffer (current-buffer))
>> (sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
>> (length (sieve-manage-encode content))
>> sieve-manage-client-eol content))
>> (sieve-manage-parse-oknobye)))
>
> This is because you pass both the text and the number to 'format'.
> But that is not carved in stone: the "%d" part can never produce any
> non-ASCII characters, so there's no need to encode it together with
> CONTENT. You could do this instead:
>
> (defun sieve-manage-send (command &optional payload)
> (let ((encoded (if payload (encode-coding-string payload 'utf-8-unix)))
> size cmdstr)
> (if encoded
> (setq size (format " {%d+}%s"
> (length encoded) sieve-manage-client-eol)))
> (setq cmdstr (concat command size encoded))
> (sieve-manage--append-to-log cmdstr)
> (process-send-string sieve-manage-process cmdstr)))
>
> And then you call this like below:
>
> (sieve-manage-send (format "PUTSCRIPT \"%s\"" name) content)
> (sieve-manage-send (format "HAVESPACE \"%s\" %s" name size))
>
> I hope this clarifies my proposal.
Yes, it does. Actually, I like it.
RFC5804 specifies three variants for string encoding:
string = quoted / literal-c2s / literal-s2c
Only the first two are relevant for `sieve-menage-send' ('literal-s2c'
is for messages from s(server) to c(lient)).
For PUTSCRIPT, we need to use 'literal-c2s' which requires the
additional length element (since 'quoted' is a) limited in the character
set and b) may not exceed 1024 characters). So I would just modify the
your `sieve-manage-send' like this:
(defun sieve-manage-send (command &optional payload-str)
"Send COMMAND with optional PAYLOAD-STR.
If non-nil, PAYLOAD-STR will be appended to COMMAND using the
\\='literal-s2c\\' representation according to RFC5804."
(let ((encoded (when payload-str (sieve-manage-encode payload-str)))
literal-c2s cmdstr)
(when encoded
(setq literal-c2s (format " {%d+}%s%s"
(length encoded)
sieve-manage-client-eol
encoded)))
(setq cmdstr (concat (sieve-manage-encode command)
literal-c2s
sieve-manage-client-eol))
(sieve-manage--append-to-log cmdstr)
(process-send-string sieve-manage-process cmdstr)))
Apart from renaming some of the variables, this adds encoding of
`command' itself (since command may contain multibyte characters in
script names) and an additional `sieve-manage-client-eol' at the end
of `cmdstr'.
As before, updated patches are attached.
[0001-Fix-bug-in-sieve-manage-append-to-log-and-do-some-re.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sun, 22 Jan 2023 02:13:02 GMT) Full text and rfc822 format available.Message #86 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Sun, 22 Jan 2023 03:12:00 +0100
[Message part 1 (text/plain, inline)]
I've now added an additional patch which automatically handles unix/dos eol-types when downloading/uploading sieve scripts. So far, if a script downloaded from the server contained CRLF EOLs, the script buffer was full of '^M's. With the additional patch (0005-Autodetect-eol-type-of-sieve-manage-scripts), the EOL type is detected and used for decoding during script download (and subsequently also for encoding during upload). For that, I changed the interface between 'sieve-upload' (in sieve.el), and 'manage-sieve-putscript' (plus 'sieve-manage-decode' and 'sieve-manage-send' in sieve-manage.el). Instead of transferring the script data as a string, the functions are now using the actual script buffer. The eol-type detection is done in the new function 'sieve-manage--guess-buffer-coding-system'. But I would assume, that this functionality already exists somewhere else. E.g. 'find-file' must do a similar, much more detailed analysis. However, that seems to happen in the C implementation, so it's not directly usable in sieve-manage. Or am I missing something?
[0001-Fix-bug-in-sieve-manage-append-to-log-and-do-some-re.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
[0005-Autodetect-eol-type-of-sieve-manage-scripts.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 01:00:02 GMT) Full text and rfc822 format available.Message #89 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 01:59:09 +0100
[Message part 1 (text/plain, inline)]
Just a quick update (also answering my own question(s) below): Regarding the status of this bug report: The fix for emacs-29 which was initially intended to be emacs-29 only has (inadvertently?) found its way to master. So I think we can close the report. The discussion about the additional changes I made (I'm still working on that s. below) could be moved elsewhere (emacs-devel?). Even though, from my side there is no urgent need for that. I really appreciate the feedback I got from Eli and hope to get some more. > I've now added an additional patch which automatically handles unix/dos > eol-types when downloading/uploading sieve scripts. So far, if a script > downloaded from the server contained CRLF EOLs, the script buffer was > full of '^M's. With the additional patch > (0005-Autodetect-eol-type-of-sieve-manage-scripts), the EOL type is > detected and used for decoding during script download (and subsequently > also for encoding during upload). > > For that, I changed the interface between 'sieve-upload' (in sieve.el), > and 'manage-sieve-putscript' (plus 'sieve-manage-decode' and > 'sieve-manage-send' in sieve-manage.el). Instead of transferring the > script data as a string, the functions are now using the actual script > buffer. > > The eol-type detection is done in the new function > 'sieve-manage--guess-buffer-coding-system'. But I would assume, that > this functionality already exists somewhere else. E.g. 'find-file' must > do a similar, much more detailed analysis. However, that seems to happen > in the C implementation, so it's not directly usable in sieve-manage. Or > am I missing something? In the meantime, I found `detect-coding-region'/`detect-coding-string' which in combination with `coding-system-eol-type' do exactly what I was missing. I've now started to refactor the encoding and sending functions in sieve-manage.el with the intent to improve the readability and testability of the code. I'm also adding some more tests. These additional changes are in yet another patch (0006-WiP-new-encode-tested-OK.patch). I also added NEWS entries to patches 0002-Handle-BYE... and 0005-Autodetect-eol-type... (these were already present in the patches of the previous mail).
[0001-Fix-bug-in-sieve-manage-append-to-log-and-do-some-re.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0004-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
[0005-Autodetect-eol-type-of-sieve-manage-scripts.patch (text/x-diff, attachment)]
[0006-WiP-new-encode-tested-OK.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 12:48:02 GMT) Full text and rfc822 format available.Message #92 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Herbert J. Skuhra" <herbert <at> gojira.at> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 13:47:19 +0100
On Mon, Jan 23, 2023 at 01:59:09AM +0100, Kai Tetzlaff wrote: > Just a quick update (also answering my own question(s) below): > > Regarding the status of this bug report: The fix for emacs-29 which was > initially intended to be emacs-29 only has (inadvertently?) found its way > to master. No, the change was not merged to master. Thanks. -- Herbert
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 13:02:02 GMT) Full text and rfc822 format available.Message #95 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 14:01:02 +0100
"Herbert J. Skuhra" <herbert <at> gojira.at> writes:
> On Mon, Jan 23, 2023 at 01:59:09AM +0100, Kai Tetzlaff wrote:
>> Just a quick update (also answering my own question(s) below):
>>
>> Regarding the status of this bug report: The fix for emacs-29 which was
>> initially intended to be emacs-29 only has (inadvertently?) found its way
>> to master.
>
> No, the change was not merged to master.
Hmm, what about:
$ git log --grep "^Fix bug in 'sieve-manage--append-to-log" origin/master
commit 12d7670b90f66f1d45a8c69d9acfc25238a65b02
Author: Kai Tetzlaff <emacs <at> tetzco.de>
Date: 2023-01-19 03:16:14 +0100
Fix bug in 'sieve-manage--append-to-log'
* lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix
log buffer creation. (Bug#54154) Do not merge to master.
BR, Kai
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 13:37:02 GMT) Full text and rfc822 format available.Message #98 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Herbert J. Skuhra" <herbert <at> gojira.at> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 14:36:44 +0100
On Mon, Jan 23, 2023 at 02:01:02PM +0100, Kai Tetzlaff wrote:
>
> Hmm, what about:
>
> $ git log --grep "^Fix bug in 'sieve-manage--append-to-log" origin/master
> commit 12d7670b90f66f1d45a8c69d9acfc25238a65b02
> Author: Kai Tetzlaff <emacs <at> tetzco.de>
> Date: 2023-01-19 03:16:14 +0100
>
> Fix bug in 'sieve-manage--append-to-log'
>
> * lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix
> log buffer creation. (Bug#54154) Do not merge to master.
Well, I don't see this change in my checkout (and even in a fresh clone)
and sieve-manage still produces the reported error.
Last entry of 'git log lisp/net/sieve-manage.el' is:
commit cae528457cb862dc886a34240c9d4c73035b6659
Author: Eli Zaretskii
Date: Sun Jan 1 05:31:12 2023 -0500
; Add 2023 to copyright years.
--
Herbert
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 13:41:01 GMT) Full text and rfc822 format available.Message #101 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 15:40:26 +0200
> From: Kai Tetzlaff <emacs+bug <at> tetzco.de> > Cc: Eli Zaretskii <eliz <at> gnu.org>, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org > Date: Mon, 23 Jan 2023 14:01:02 +0100 > > "Herbert J. Skuhra" <herbert <at> gojira.at> writes: > > > On Mon, Jan 23, 2023 at 01:59:09AM +0100, Kai Tetzlaff wrote: > >> Just a quick update (also answering my own question(s) below): > >> > >> Regarding the status of this bug report: The fix for emacs-29 which was > >> initially intended to be emacs-29 only has (inadvertently?) found its way > >> to master. > > > > No, the change was not merged to master. > Hmm, what about: > > $ git log --grep "^Fix bug in 'sieve-manage--append-to-log" origin/master > commit 12d7670b90f66f1d45a8c69d9acfc25238a65b02 > Author: Kai Tetzlaff <emacs <at> tetzco.de> > Date: 2023-01-19 03:16:14 +0100 > > Fix bug in 'sieve-manage--append-to-log' > > * lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix > log buffer creation. (Bug#54154) Do not merge to master. The "Do not merge to master" part prevents its merging to the master branch. This was according to your request: you intended (and still do, AFAIU) to fix this differently on the master branch, for Emacs 30.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 13:58:02 GMT) Full text and rfc822 format available.Message #104 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 14:57:19 +0100
"Herbert J. Skuhra" <herbert <at> gojira.at> writes: > On Mon, Jan 23, 2023 at 02:01:02PM +0100, Kai Tetzlaff wrote: >> >> Hmm, what about: >> >> $ git log --grep "^Fix bug in 'sieve-manage--append-to-log" origin/master >> commit 12d7670b90f66f1d45a8c69d9acfc25238a65b02 >> Author: Kai Tetzlaff <emacs <at> tetzco.de> >> Date: 2023-01-19 03:16:14 +0100 >> >> Fix bug in 'sieve-manage--append-to-log' >> >> * lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix >> log buffer creation. (Bug#54154) Do not merge to master. > > > Well, I don't see this change in my checkout (and even in a fresh clone) > and sieve-manage still produces the reported error. > > Last entry of 'git log lisp/net/sieve-manage.el' is: > > commit cae528457cb862dc886a34240c9d4c73035b6659 > Author: Eli Zaretskii > Date: Sun Jan 1 05:31:12 2023 -0500 > > ; Add 2023 to copyright years. You're right! And I learned something about git again. `git log --grep ...` apparently silently ignores my origin/master ref and greps through all available refs.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 14:28:02 GMT) Full text and rfc822 format available.Message #107 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Andreas Schwab <schwab <at> suse.de> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: larsi <at> gnus.org, "Herbert J. Skuhra" <herbert <at> gojira.at>, 54154 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org> Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 15:27:49 +0100
On Jan 23 2023, Kai Tetzlaff wrote: > And I learned something about git again. `git log --grep ...` apparently > silently ignores my origin/master ref and greps through all available > refs. It searches through all commit messages that you would see without the --grep option. -- Andreas Schwab, SUSE Labs, schwab <at> suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 16:23:01 GMT) Full text and rfc822 format available.Message #110 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 17:22:13 +0100
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes: >> From: Kai Tetzlaff <emacs+bug <at> tetzco.de> >> Cc: Eli Zaretskii <eliz <at> gnu.org>, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org >> Date: Mon, 23 Jan 2023 14:01:02 +0100 >> >> "Herbert J. Skuhra" <herbert <at> gojira.at> writes: >> >> > On Mon, Jan 23, 2023 at 01:59:09AM +0100, Kai Tetzlaff wrote: >> >> Just a quick update (also answering my own question(s) below): >> >> >> >> Regarding the status of this bug report: The fix for emacs-29 which was >> >> initially intended to be emacs-29 only has (inadvertently?) found its way >> >> to master. >> > >> > No, the change was not merged to master. >> Hmm, what about: >> >> $ git log --grep "^Fix bug in 'sieve-manage--append-to-log" origin/master >> commit 12d7670b90f66f1d45a8c69d9acfc25238a65b02 >> Author: Kai Tetzlaff <emacs <at> tetzco.de> >> Date: 2023-01-19 03:16:14 +0100 >> >> Fix bug in 'sieve-manage--append-to-log' >> >> * lisp/net/sieve-manage.el (sieve-manage--append-to-log): Fix >> log buffer creation. (Bug#54154) Do not merge to master. > > The "Do not merge to master" part prevents its merging to the master > branch. Yes, I just was under the wrong assumption that this didn't work. > This was according to your request: you intended (and still do, AFAIU) > to fix this differently on the master branch, for Emacs 30. Correct, I'm still working on additional sieve-manage changes for master. However, I've now updated/reordered the patches. And from my POV, the following are ready to be applied to master: 1. 0001-Fix-bug-in-sieve-manage-append-to-log-and-do-some-re.patch: This is the master version of the patch which has been added to emacs-29. 2. 0002-Handle-BYE-in-sieve-manage-server-responses.patch: Adds support for BYE in server responses. 3. 0003-Some-minor-fixes-in-lisp-net-sieve.el.patch: Fixes some minor usability issues in sieve.el. 4. 0004-Autodetect-eol-type-of-sieve-manage-scripts.patch: Adds eol-type autodetection. Avoids showing '^M's in sieve script buffers for scripts which use CRLF EOLs. For the other two: 5. 0005-Add-test-lisp-net-sieve-manage-tests.el.patch: This adds a first test which verifies that multibyte script names and scripts can be downloaded without issues. I wrote this already last year, but then Lars committed what was on debbugs before I got to add it there. However, since I've already added additional tests in patch 0006-*, it might not be worth adding this one right now. Up to you... 6. 0006-WiP-new-encode-tested-OK.patch: This is contains my current WiP status. It works but is not ready/finished. It will probably take me one or two weeks to finish it. Some of the changes affect code which got updated/added in patches 0001/0002/0004. And there are a lot of new tests. Eli, how do you want to handle this? Apply the first patches now? Wait until I'm done with all all of them? Or, ...? Feedback appreciated! Thanks, Kai
[0001-Fix-bug-in-sieve-manage-append-to-log-and-do-some-re.patch (text/x-diff, attachment)]
[0002-Handle-BYE-in-sieve-manage-server-responses.patch (text/x-diff, attachment)]
[0003-Some-minor-fixes-in-lisp-net-sieve.el.patch (text/x-diff, attachment)]
[0004-Autodetect-eol-type-of-sieve-manage-scripts.patch (text/x-diff, attachment)]
[0005-Add-test-lisp-net-sieve-manage-tests.el.patch (text/x-diff, attachment)]
[0006-WiP-new-encode-tested-OK.patch (text/x-diff, attachment)]
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 16:50:01 GMT) Full text and rfc822 format available.Message #113 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 18:49:54 +0200
> From: Kai Tetzlaff <emacs+bug <at> tetzco.de> > Cc: herbert <at> gojira.at, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org > Date: Mon, 23 Jan 2023 17:22:13 +0100 > > Eli, how do you want to handle this? Apply the first patches now? Wait > until I'm done with all all of them? Or, ...? Feedback appreciated! I have yet to review the final version of this. So if you are still working on the changes, I prefer to wait until you have the final version, and review then. Thanks.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 17:08:01 GMT) Full text and rfc822 format available.Message #116 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Andreas Schwab <schwab <at> suse.de> Cc: larsi <at> gnus.org, "Herbert J. Skuhra" <herbert <at> gojira.at>, 54154 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org> Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 18:07:12 +0100
Andreas Schwab <schwab <at> suse.de> writes:
> On Jan 23 2023, Kai Tetzlaff wrote:
>
>> And I learned something about git again. `git log --grep ...` apparently
>> silently ignores my origin/master ref and greps through all available
>> refs.
>
> It searches through all commit messages that you would see without the
> --grep option.
And right again. This really stumped me. So after some investigation on
my part, here's where my confusion originated. To check git logs, I
typically use an alias (l2) which adds the `--graph` option to git log.
And `git log --pretty=oneline --graph` then actually results in:
...
| * 0d3b6518e39a28774e4e70ed9bb7ef4aa009c0cf (ruby-ts--indent-rules): Indent inside empty parens properly
| * 7fb69ce233b8a655af63d4c47b7359c43660acf6 (emacs-29) ; * doc/emacs/modes.texi (Choosing Modes): Add index entries.
* | ede5e82418a0b8cfce2bf96b2a3255ca86b65000 ; Merge from origin/emacs-29
|\|
| * 12d7670b90f66f1d45a8c69d9acfc25238a65b02 Fix bug in 'sieve-manage--append-to-log'
* | e9ceeee1198aa10cac3cd61ff9537b64640455c2 Merge from origin/emacs-29
|\|
| * 21be03cccb611ea9e6c73fb04e578c48edf49a25 CC Mode: Prevent two classes of "type" prematurely entering c-found-types
* | 117f90865adca03eab84778db0370ddc05ba8ae7 Add new command `kill-matching-buffers-no-ask' (bug#60714)
...
Which seems to indicate that commit
12d7670b90f66f1d45a8c69d9acfc25238a65b02 Fix bug in 'sieve-manage--append-to-log'
has been merged to master.
However, here's the actual merge commit:
$ git log --patch -1 ede5e82418a0b8cfce2bf96b2a3255ca86b65000
commit ede5e82418a0b8cfce2bf96b2a3255ca86b65000
Merge: e9ceeee1198 12d7670b90f
Author: Stefan Kangas <stefankangas <at> gmail.com>
Date: 2023-01-20 11:30:22 +0100
; Merge from origin/emacs-29
The following commit was skipped:
12d7670b90f Fix bug in 'sieve-manage--append-to-log'
That's it. I.e. it is completely empty because the only commit which was
actually (supposed to be) merged was then just skipped. I guess that this is
needed for git to recognize that the commit was purposely skipped (so
that git will not include it in subsequent merges).
So, as ever so often, the problem was sitting in front of the monitor. I
must admit that I still find this behaviour surprising/unexpected. But
good to know! Beware of `git log --graph` ...
BR, Kai
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 17:13:02 GMT) Full text and rfc822 format available.Message #119 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Kai Tetzlaff <emacs+bug <at> tetzco.de> To: Eli Zaretskii <eliz <at> gnu.org> Cc: herbert <at> gojira.at, 54154 <at> debbugs.gnu.org, larsi <at> gnus.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 18:12:23 +0100
Eli Zaretskii <eliz <at> gnu.org> writes: >> From: Kai Tetzlaff <emacs+bug <at> tetzco.de> >> Cc: herbert <at> gojira.at, larsi <at> gnus.org, 54154 <at> debbugs.gnu.org >> Date: Mon, 23 Jan 2023 17:22:13 +0100 >> >> Eli, how do you want to handle this? Apply the first patches now? Wait >> until I'm done with all all of them? Or, ...? Feedback appreciated! > > I have yet to review the final version of this. So if you are still > working on the changes, I prefer to wait until you have the final > version, and review then. Ok, fine with me. Thank, Kai
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Mon, 23 Jan 2023 17:54:02 GMT) Full text and rfc822 format available.Message #122 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Andreas Schwab <schwab <at> suse.de> To: Kai Tetzlaff <emacs+bug <at> tetzco.de> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, "Herbert J. Skuhra" <herbert <at> gojira.at>, Eli Zaretskii <eliz <at> gnu.org> Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Mon, 23 Jan 2023 18:53:42 +0100
On Jan 23 2023, Kai Tetzlaff wrote: > That's it. I.e. it is completely empty because the only commit which was > actually (supposed to be) merged was then just skipped. In other words, it is an evil merge. -- Andreas Schwab, SUSE Labs, schwab <at> suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Tue, 21 Feb 2023 12:24:06 GMT) Full text and rfc822 format available."Herbert J. Skuhra" <herbert <at> gojira.at>
to control <at> debbugs.gnu.org.
(Sun, 29 Sep 2024 09:12:02 GMT) Full text and rfc822 format available.bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sun, 29 Sep 2024 09:14:01 GMT) Full text and rfc822 format available.Message #129 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Herbert J. Skuhra" <herbert <at> gojira.at> To: 54154 <at> debbugs.gnu.org Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Sun, 29 Sep 2024 11:11:52 +0200
Hi,
Unfortunately I still see the following issue in master and emacs-30:
- Run emacs -Q
- M-x sieve-manage
- Input IMAP server (e.g. imap.mailbox.org)
Debugger entered--Lisp error: (wrong-type-argument stringp t)
sieve-manage--append-to-log("sieve-manage: Connecting to imap.mailbox.org..." "\n")
sieve-manage--message("Connecting to %s..." "imap.mailbox.org")
sieve-manage-open("imap.mailbox.org" nil)
sieve-open-server("imap.mailbox.org" nil)
sieve-manage("imap.mailbox.org")
funcall-interactively(sieve-manage "imap.mailbox.org")
command-execute(sieve-manage record)
execute-extended-command(nil "sieve-manage" "sieve-ma")
funcall-interactively(execute-extended-command nil "sieve-manage" "sieve-ma")
command-execute(execute-extended-command)
On 2nd try it works!
--
Herber
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sun, 29 Sep 2024 09:31:01 GMT) Full text and rfc822 format available.Message #132 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Sun, 29 Sep 2024 12:29:39 +0300
> Date: Sun, 29 Sep 2024 11:03:41 +0200
> From: "Herbert J. Skuhra" <herbert <at> gojira.at>
> Cc: Kai Tetzlaff <emacs+bug <at> tetzco.de>,
> larsi <at> gnus.org,
> 54154 <at> debbugs.gnu.org,
> Eli Zaretskii <eliz <at> gnu.org>
>
> Hi,
>
> Unfortunately I still see the following issue in master and emacs-30:
>
> - Run emacs -Q
> - M-x sieve-manage
> - Input IMAP server (e.g. imap.mailbox.org)
>
> Debugger entered--Lisp error: (wrong-type-argument stringp t)
> sieve-manage--append-to-log("sieve-manage: Connecting to imap.mailbox.org..." "\n")
> sieve-manage--message("Connecting to %s..." "imap.mailbox.org")
> sieve-manage-open("imap.mailbox.org" nil)
> sieve-open-server("imap.mailbox.org" nil)
> sieve-manage("imap.mailbox.org")
> funcall-interactively(sieve-manage "imap.mailbox.org")
> command-execute(sieve-manage record)
> execute-extended-command(nil "sieve-manage" "sieve-ma")
> funcall-interactively(execute-extended-command nil "sieve-manage" "sieve-ma")
> command-execute(execute-extended-command)
>
> On 2nd try it works!
The fix was not merged to the-then master branch, so it is not in
Emacs 30. Therefore, it is not a surprise you still see the problem:
it was only fixed in Emacs 29.
I lost the context long ago, so I hope Kai will suggest how to handle
this.
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sun, 29 Sep 2024 10:24:02 GMT) Full text and rfc822 format available.Message #135 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: herbert <at> gojira.at Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Sun, 29 Sep 2024 13:23:03 +0300
> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de
> Date: Sun, 29 Sep 2024 12:29:39 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> > - Run emacs -Q
> > - M-x sieve-manage
> > - Input IMAP server (e.g. imap.mailbox.org)
> >
> > Debugger entered--Lisp error: (wrong-type-argument stringp t)
> > sieve-manage--append-to-log("sieve-manage: Connecting to imap.mailbox.org..." "\n")
> > sieve-manage--message("Connecting to %s..." "imap.mailbox.org")
> > sieve-manage-open("imap.mailbox.org" nil)
> > sieve-open-server("imap.mailbox.org" nil)
> > sieve-manage("imap.mailbox.org")
> > funcall-interactively(sieve-manage "imap.mailbox.org")
> > command-execute(sieve-manage record)
> > execute-extended-command(nil "sieve-manage" "sieve-ma")
> > funcall-interactively(execute-extended-command nil "sieve-manage" "sieve-ma")
> > command-execute(execute-extended-command)
> >
> > On 2nd try it works!
>
> The fix was not merged to the-then master branch, so it is not in
> Emacs 30. Therefore, it is not a surprise you still see the problem:
> it was only fixed in Emacs 29.
>
> I lost the context long ago, so I hope Kai will suggest how to handle
> this.
However, it's quite possible that the following band-aid should fix
this particular issue:
diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
index 0faeb02..da2167c 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))))
Can you test this?
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sun, 29 Sep 2024 12:18:01 GMT) Full text and rfc822 format available.Message #138 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: "Herbert J. Skuhra" <herbert <at> gojira.at> To: Eli Zaretskii <eliz <at> gnu.org> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Sun, 29 Sep 2024 14:15:47 +0200
On Sun, 29 Sep 2024 12:23:03 +0200,
Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de
> > Date: Sun, 29 Sep 2024 12:29:39 +0300
> > From: Eli Zaretskii <eliz <at> gnu.org>
> >
> > > - Run emacs -Q
> > > - M-x sieve-manage
> > > - Input IMAP server (e.g. imap.mailbox.org)
> > >
> > > Debugger entered--Lisp error: (wrong-type-argument stringp t)
> > > sieve-manage--append-to-log("sieve-manage: Connecting to imap.mailbox.org..." "\n")
> > > sieve-manage--message("Connecting to %s..." "imap.mailbox.org")
> > > sieve-manage-open("imap.mailbox.org" nil)
> > > sieve-open-server("imap.mailbox.org" nil)
> > > sieve-manage("imap.mailbox.org")
> > > funcall-interactively(sieve-manage "imap.mailbox.org")
> > > command-execute(sieve-manage record)
> > > execute-extended-command(nil "sieve-manage" "sieve-ma")
> > > funcall-interactively(execute-extended-command nil "sieve-manage" "sieve-ma")
> > > command-execute(execute-extended-command)
> > >
> > > On 2nd try it works!
> >
> > The fix was not merged to the-then master branch, so it is not in
> > Emacs 30. Therefore, it is not a surprise you still see the problem:
> > it was only fixed in Emacs 29.
> >
> > I lost the context long ago, so I hope Kai will suggest how to handle
> > this.
>
> However, it's quite possible that the following band-aid should fix
> this particular issue:
>
> diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
> index 0faeb02..da2167c 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))))
Yes, this works. Thanks.
As manage-sieve logs credentials maybe logging should be disabled by
default and/or credentials shouldn't be logged when logging is
enabled?
bug-gnu-emacs <at> gnu.org:bug#54154; Package emacs.
(Sun, 29 Sep 2024 12:45:03 GMT) Full text and rfc822 format available.Message #141 received at 54154 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: "Herbert J. Skuhra" <herbert <at> gojira.at> Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de Subject: Re: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Sun, 29 Sep 2024 15:43:37 +0300
> Date: Sun, 29 Sep 2024 14:15:47 +0200
> From: "Herbert J. Skuhra" <herbert <at> gojira.at>
> Cc: larsi <at> gnus.org,
> 54154 <at> debbugs.gnu.org,
> emacs+bug <at> tetzco.de
>
> On Sun, 29 Sep 2024 12:23:03 +0200,
> Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> > > Cc: larsi <at> gnus.org, 54154 <at> debbugs.gnu.org, emacs+bug <at> tetzco.de
> > > Date: Sun, 29 Sep 2024 12:29:39 +0300
> > > From: Eli Zaretskii <eliz <at> gnu.org>
> > >
> > > > - Run emacs -Q
> > > > - M-x sieve-manage
> > > > - Input IMAP server (e.g. imap.mailbox.org)
> > > >
> > > > Debugger entered--Lisp error: (wrong-type-argument stringp t)
> > > > sieve-manage--append-to-log("sieve-manage: Connecting to imap.mailbox.org..." "\n")
> > > > sieve-manage--message("Connecting to %s..." "imap.mailbox.org")
> > > > sieve-manage-open("imap.mailbox.org" nil)
> > > > sieve-open-server("imap.mailbox.org" nil)
> > > > sieve-manage("imap.mailbox.org")
> > > > funcall-interactively(sieve-manage "imap.mailbox.org")
> > > > command-execute(sieve-manage record)
> > > > execute-extended-command(nil "sieve-manage" "sieve-ma")
> > > > funcall-interactively(execute-extended-command nil "sieve-manage" "sieve-ma")
> > > > command-execute(execute-extended-command)
> > > >
> > > > On 2nd try it works!
> > >
> > > The fix was not merged to the-then master branch, so it is not in
> > > Emacs 30. Therefore, it is not a surprise you still see the problem:
> > > it was only fixed in Emacs 29.
> > >
> > > I lost the context long ago, so I hope Kai will suggest how to handle
> > > this.
> >
> > However, it's quite possible that the following band-aid should fix
> > this particular issue:
> >
> > diff --git a/lisp/net/sieve-manage.el b/lisp/net/sieve-manage.el
> > index 0faeb02..da2167c 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))))
>
> Yes, this works. Thanks.
Thanks, installed on the emacs-30 branch.
> As manage-sieve logs credentials maybe logging should be disabled by
> default and/or credentials shouldn't be logged when logging is
> enabled?
I'll let Kai and others comment on that.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Mon, 28 Oct 2024 11:24:10 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.