GNU bug report logs - #54154
29.0.50; [PATCH] `sieve-manage-getscript' fails if script contains multibyte characters

Previous Next

Full log

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.