|
From: | Kai Tetzlaff |
Subject: | bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) |
Date: | Thu, 19 Jan 2023 13:38:13 +0100 |
User-agent: | Gnus/5.13 (Gnus v5.13) |
Hello Eli, thanks for looking into this! Eli Zaretskii <eliz@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):
>From 62d03f302125c0b1aab2e3ae4f5b12b531d30d74 Mon Sep 17 00:00:00 2001 From: Kai Tetzlaff <emacs@tetzco.de> Date: Thu, 19 Jan 2023 03:16:14 +0100 Subject: [PATCH] ; Fix bug in sieve-manage--append-to-log (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
>> ;; 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
Description: Text Data
0002-Handle-BYE-in-sieve-manage-server-responses.patch
Description: Text Data
0003-Add-test-lisp-net-sieve-manage-tests.el.patch
Description: Text Data
0004-Some-minor-fixes-in-lisp-net-sieve.el.patch
Description: Text Data
[Prev in Thread] | Current Thread | [Next in Thread] |