bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage)

[Eli Zaretskii]

> 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.