Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kai Tetzlaff <emacs+bug@tetzco.de>
>> Cc: herbert@gojira.at,  larsi@gnus.org,  54154@debbugs.gnu.org
>> Date: Thu, 19 Jan 2023 16:59:36 +0100
>> 
>> >> 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.
>> >
>> > I'm not sure why you need to force the encoding of the process buffer,
>> > when you already set the coding-system to be used for decoding stuff
>> > from the process.  Is that really needed?
>> 
>> Not sure if it is really needed. But I wanted to make sure that both,
>> the process buffer and the log buffer use identical settings. Otherwise,
>> the content of the log buffer might be misleading.
>
> I don't think it could mislead, but OK.
>
>> > But if you really need this, then just make the insertion of the text
>> > into the buffer you create optional: then for the process-buffer pass
>> > nil as the text to insert, and you can do the with-current-buffer
>> > dance only inside that function.
>> 
>> Sorry, you lost me there. I don't understand what you want to tell me.
>> Which (optional) text in which buffer?
>
> I meant this:
>
>   (defun sieve-manage--set-buffer-and-append-text (buffer-name &rest args)
>     (let ((existing-buffer (get-buffer buffer-name))
>           new-buffer)
>       (if existing-buffer
>           (setq new-buffer existing-buffer)
>         (setq new-buffer (get-buffer-create buffer-name)))
>       (with-current-buffer new-buffer
>         (when (not existing-buffer)
>           (set-buffer-file-coding-system sieve-manage--coding-system)
>           (setq-local after-change-functions nil)
>           (buffer-disable-undo)
>           ; What happened to set-buffer-multibyte?
>           )
>         (goto-char (point-max))
>         (apply #'insert args))))
>
> Then you can call it from sieve-manage-make-process-buffer like this:
>
>     (sieve-manage--set-buffer-and-append-text
>       (format " *sieve %s:%s*" sieve-manage-server sieve-manage-port)
>       "")
>
> i.e. with an empty string, so nothing gets inserted into the process
> buffer.  Or you could instead change the signature to accept a single
> &optional argument that is a list, and then you could make the last
> two lines in the function above conditional on that argument being
> non-nil.

Ok, I now implemented it like this:

    (defun sieve-manage--set-buffer-maybe-append-text (buffer-name
                                                       &rest args)
      "Append ARGS to buffer named BUFFER-NAME and return buffer.
    
    To be used with process and log buffers. When the buffer doesn't
    exist, it gets created and configure as follows:
    - set coding system to 'sieve-manage--coding-system',
    - set buffer to single-byte mode,
    - set `after-change-functions' to nil to avoid those
      functions messing with buffer content,
    - disable undo (to save a bit of memory and improve
      performance).
    
    ARGS can be a nil, a string or a list of strings. If no
    ARGS are provided, the content of buffer will not be
    modified."
      (let* ((existing-buffer (get-buffer buffer-name))
             (buffer (or existing-buffer
                         (get-buffer-create buffer-name))))
        (with-current-buffer buffer
          (unless existing-buffer
            (set-buffer-file-coding-system sieve-manage--coding-system)
            (set-buffer-multibyte nil)
            (setq-local after-change-functions nil)
            (buffer-disable-undo))
          (when args
            (goto-char (point-max))
            (apply #'insert args)))
        buffer))

    (defun sieve-manage--append-to-log (&rest args)
      "Append ARGS to `sieve-manage-log' buffer.
    
    If `sieve-manage-log' is nil, logging is disabled. See also
    `sieve-manage--set-buffer-maybe-append-text'."
      (when sieve-manage-log
        (apply #'sieve-manage--set-buffer-maybe-append-text
               sieve-manage-log
               args)))

    (defun sieve-manage-make-process-buffer ()
      (let ((buffer (sieve-manage--set-buffer-maybe-append-text
                     (format " *sieve %s:%s*"
                             sieve-manage-server
                             sieve-manage-port))))
        (with-current-buffer buffer
          (mapc #'make-local-variable sieve-manage-local-variables))
        buffer))

Is that better, now? I also added the (set-buffer-multibyte nil) back
into the mix. My understanding was that it was not needed when using the
'raw-text-unix coding system but it is now after switching to
'utf-8-unix?

>> > What you should do is call sieve-manage-encode inside
>> > sieve-manage-send, and count the bytes there after encoding the
>> > payload.
>> 
>> Unfortunately, that is too late since the sent data - in case that the
>> sent text may contain CRLF sequences - contains its own length. So in
>> order to insert the correct length, I need to encode before sending.
>> See:
>> 
>>     (defun sieve-manage-putscript (name content &optional buffer)
>>       (with-current-buffer (or buffer (current-buffer))
>>         (sieve-manage-send (format "PUTSCRIPT \"%s\" {%d+}%s%s" name
>>                                    (length (sieve-manage-encode content))
>>                                    sieve-manage-client-eol content))
>>         (sieve-manage-parse-oknobye)))
>
> This is because you pass both the text and the number to 'format'.
> But that is not carved in stone: the "%d" part can never produce any
> non-ASCII characters, so there's no need to encode it together with
> CONTENT.  You could do this instead:
>
>   (defun sieve-manage-send (command &optional payload)
>     (let ((encoded (if payload (encode-coding-string payload 'utf-8-unix)))
>           size cmdstr)
>       (if encoded
>           (setq size (format " {%d+}%s"
> 	                     (length encoded) sieve-manage-client-eol)))
>       (setq cmdstr (concat command size encoded))
>       (sieve-manage--append-to-log cmdstr)
>       (process-send-string sieve-manage-process cmdstr)))
>
> And then you call this like below:
>
>   (sieve-manage-send (format "PUTSCRIPT \"%s\"" name) content)
>   (sieve-manage-send (format "HAVESPACE \"%s\" %s" name size))
>
> I hope this clarifies my proposal.

Yes, it does. Actually, I like it.

RFC5804 specifies three variants for string encoding:

    string                = quoted / literal-c2s / literal-s2c

Only the first two are relevant for `sieve-menage-send' ('literal-s2c'
is for messages from s(server) to c(lient)).

For PUTSCRIPT, we need to use 'literal-c2s' which requires the
additional length element (since 'quoted' is a) limited in the character
set and b) may not exceed 1024 characters). So I would just modify the
your `sieve-manage-send' like this:

    (defun sieve-manage-send (command &optional payload-str)
      "Send COMMAND with optional PAYLOAD-STR.
    
    If non-nil, PAYLOAD-STR will be appended to COMMAND using the
    \\='literal-s2c\\' representation according to RFC5804."
      (let ((encoded (when payload-str (sieve-manage-encode payload-str)))
            literal-c2s cmdstr)
        (when encoded
          (setq literal-c2s (format " {%d+}%s%s"
                                    (length encoded)
                                    sieve-manage-client-eol
                                    encoded)))
        (setq cmdstr (concat (sieve-manage-encode command)
                             literal-c2s
                             sieve-manage-client-eol))
        (sieve-manage--append-to-log cmdstr)
        (process-send-string sieve-manage-process cmdstr)))

Apart from renaming some of the variables, this adds encoding of
`command' itself (since command may contain multibyte characters in
script names) and an additional `sieve-manage-client-eol' at the end
of `cmdstr'.


As before, updated patches are attached.