From: Eli Zaretskii <eliz@gnu.org>
To: Kai Tetzlaff <emacs+bug@tetzco.de>
Cc: herbert@gojira.at, 54154@debbugs.gnu.org, larsi@gnus.org
Subject: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage)
Date: Thu, 19 Jan 2023 09:45:21 +0200 [thread overview]
Message-ID: <83tu0nynla.fsf@gnu.org> (raw)
In-Reply-To: <874jsngod2.fsf@tetzco.de> (message from Kai Tetzlaff on Thu, 19 Jan 2023 05:06:01 +0100)
> From: Kai Tetzlaff <emacs+bug@tetzco.de>
> Cc: Eli Zaretskii <eliz@gnu.org>, larsi@gnus.org, 54154@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.
next prev parent reply other threads:[~2023-01-19 7:45 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-25 9:04 bug#54154: 29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters Kai Tetzlaff
2022-02-25 12:19 ` Lars Ingebrigtsen
2022-02-25 13:10 ` Lars Ingebrigtsen
2022-02-25 16:00 ` Kai Tetzlaff
2022-02-26 15:07 ` Lars Ingebrigtsen
2022-02-28 12:27 ` Kai Tetzlaff
2022-09-06 11:34 ` Lars Ingebrigtsen
2022-02-28 12:35 ` Kai Tetzlaff
2022-02-28 13:06 ` Lars Ingebrigtsen
2022-02-28 13:08 ` Lars Ingebrigtsen
2022-02-28 13:03 ` Kai Tetzlaff
[not found] ` <87bkmwi0ut.fsf@tetzco.de>
[not found] ` <87fsc8i2c5.fsf@tetzco.de>
[not found] ` <87bkmw8b02.fsf@tetzco.de>
2023-01-18 18:28 ` bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Herbert J. Skuhra
2023-01-18 19:17 ` Eli Zaretskii
2023-01-18 23:22 ` Herbert J. Skuhra
2023-01-19 4:06 ` Kai Tetzlaff
2023-01-19 7:45 ` Eli Zaretskii [this message]
2023-01-19 12:38 ` Kai Tetzlaff
2023-01-19 14:08 ` Eli Zaretskii
2023-01-19 15:59 ` Kai Tetzlaff
2023-01-19 17:41 ` Eli Zaretskii
2023-01-19 21:33 ` Kai Tetzlaff
2023-01-20 6:54 ` Kai Tetzlaff
2023-01-22 2:12 ` Kai Tetzlaff
2023-01-23 0:59 ` Kai Tetzlaff
2023-01-23 12:47 ` Herbert J. Skuhra
2023-01-23 13:01 ` Kai Tetzlaff
2023-01-23 13:36 ` Herbert J. Skuhra
2023-01-23 13:57 ` Kai Tetzlaff
2023-01-23 14:27 ` Andreas Schwab
2023-01-23 17:07 ` Kai Tetzlaff
2023-01-23 17:53 ` Andreas Schwab
2023-01-23 13:40 ` Eli Zaretskii
2023-01-23 16:22 ` Kai Tetzlaff
2023-01-23 16:49 ` Eli Zaretskii
2023-01-23 17:12 ` Kai Tetzlaff
2023-01-19 13:22 ` Kai Tetzlaff
2023-01-19 14:16 ` Kai Tetzlaff
2023-01-19 4:50 ` bug#54154: [update] " Kai Tetzlaff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83tu0nynla.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=54154@debbugs.gnu.org \
--cc=emacs+bug@tetzco.de \
--cc=herbert@gojira.at \
--cc=larsi@gnus.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).