|
From: | Kai Tetzlaff |
Subject: | bug#54154: Emacs commit ae963e80a79f5a9184daabfc8197f211a39b136d (sieve-manage) |
Date: | Fri, 20 Jan 2023 07:54:15 +0100 |
User-agent: | Gnus/5.13 (Gnus v5.13) |
Eli Zaretskii <eliz@gnu.org> writes: Sorry, the first patch in the last email was outdated. Please check the updated ones below. >> 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.
0001-Fix-bug-in-sieve-manage-append-to-log-and-do-some-re.patch
Description: Text Data
0002-Handle-BYE-in-sieve-manage-server-responses.patch
Description: Text Data
0003-Add-test-lisp-net-sieve-manage-tests.el.patch
Description: Text Data
0004-Some-minor-fixes-in-lisp-net-sieve.el.patch
Description: Text Data
[Prev in Thread] | Current Thread | [Next in Thread] |