From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) Date: Thu, 19 Jan 2023 09:45:21 +0200 Message-ID: <83tu0nynla.fsf@gnu.org> References: <87wnhj5nbk.fsf@tetzco.de> <835yd31wk8.fsf@gnu.org> <874jsngod2.fsf@tetzco.de> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="9288"; mail-complaints-to="usenet@ciao.gmane.io" Cc: herbert@gojira.at, 54154@debbugs.gnu.org, larsi@gnus.org To: Kai Tetzlaff Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Jan 19 08:46:30 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pIPcx-0002BM-KJ for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 19 Jan 2023 08:46:28 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pIPca-0005Zb-Vg; Thu, 19 Jan 2023 02:46:05 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pIPcZ-0005ZF-3H for bug-gnu-emacs@gnu.org; Thu, 19 Jan 2023 02:46:03 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pIPcY-0006zg-G7 for bug-gnu-emacs@gnu.org; Thu, 19 Jan 2023 02:46:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pIPcY-0007I2-1I for bug-gnu-emacs@gnu.org; Thu, 19 Jan 2023 02:46:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 19 Jan 2023 07:46:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 54154 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 54154-submit@debbugs.gnu.org id=B54154.167411431627957 (code B ref 54154); Thu, 19 Jan 2023 07:46:02 +0000 Original-Received: (at 54154) by debbugs.gnu.org; 19 Jan 2023 07:45:16 +0000 Original-Received: from localhost ([127.0.0.1]:42447 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pIPbn-0007Gq-MW for submit@debbugs.gnu.org; Thu, 19 Jan 2023 02:45:16 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:51694) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pIPbm-0007Ga-45 for 54154@debbugs.gnu.org; Thu, 19 Jan 2023 02:45:14 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pIPbe-0006os-GF; Thu, 19 Jan 2023 02:45:06 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=zy3EHALh5+TCCf0ru/gL2OxVoYtLdyY0rI06x6vXfUg=; b=kw9+oPuZ5cf6 40CyLdiJoyyQJUYGMhi6qR7DgQ2Dhj5m+V/HRsOCVc+t3pHCO+IfMWMP+NDm+UsCoyJ25O1wAUThm iBXni4L2JVf8WbZm7yGBVfeZdDPAS1F78W9BQny/K9IIWOzCMPWndL5jMzAiP2fPdOYDyarGJS4LF QUidF0NroDDv6QhwCr9vJIS/YLq5Y6W36+2yHYvjZTekS8V3b5sws1K6+EiikPBMST7hx9ds26S+4 kgAphKvOF87AaA/Xeb3v77Mbf9qwnZojRJrkj6gkKOA86aQkdT4PrD1YJRPcUJXVCM8ZlfvxsU5J6 RaNACmBLiukqSIzRxi7M2Q==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pIPbd-0001Mb-Fv; Thu, 19 Jan 2023 02:45:05 -0500 In-Reply-To: <874jsngod2.fsf@tetzco.de> (message from Kai Tetzlaff on Thu, 19 Jan 2023 05:06:01 +0100) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:253682 Archived-At: > From: Kai Tetzlaff > Cc: Eli Zaretskii , 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.