[PATCH 0/5] Rework url-retrieve-synchronously wrapper

[PATCH 0/5] Rework url-retrieve-synchronously wrapper

[Kyle Meyer]

The main change is in the second patch, which brings piem's
url-retrieve-synchronously wrapper closer to url-insert-file-contents.
The other patches are small tweaks and cleanups in related code.

Note that this involves the removal of two functions,
piem-download-and-decompress and piem-check-gunzip.  I'd be surprised
if there are any outside callers at this point, but either way I don't
plan to add backward-compatibility kludges at this stage in
development.

  [1/5] b4: Check for message ID match when using current buffer's URL
  [2/5] Rework url-retrieve-synchronously wrapper
  [3/5] piem-gunzip-buffer: Check for gunzip executable
  [4/5] piem-gunzip-buffer: Absorb caching of gunzip check
  [5/5] piem-gunzip-buffer: Don't assume t.mbox.gz is being decompressed

 piem-b4.el |  18 +++++-----
 piem.el    | 104 ++++++++++++++++++++++++++---------------------------
 2 files changed, 60 insertions(+), 62 deletions(-)


base-commit: 2333819ee6b632909ba7333eb61be1653f5d8203
-- 
2.31.1

[PATCH 1/5] b4: Check for message ID match when using current buffer's URL

[Kyle Meyer]

If piem-mid-to-thread-functions fails to generate the thread,
piem-b4--get-am-files tries to download the mbox from the URL
associated with the current buffer.  However, it uses the message ID
returned by piem-mid rather than the message ID passed by
piem-b4-am-from-mid.  That's not a safe assumption for non-interactive
calls to piem-b4-am-from-mid.

Construct the URL with the message ID passed by piem-b4-am-from-mid,
and skip the download completely if that message ID doesn't match the
one for the current buffer.
---
 piem-b4.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/piem-b4.el b/piem-b4.el
index f25a676..48c8bf4 100644
--- a/piem-b4.el
+++ b/piem-b4.el
@@ -72,8 +72,8 @@ (defun piem-b4--get-am-files (mid coderepo args)
     ;; try to download it from a URL at `piem-inboxes'.  Finally, fall
     ;; back to b4's configuration.
     (unless local-mbox-p
-      (when-let ((url (piem-inbox-get :url))
-                 (mid (piem-mid))
+      (when-let ((url (and (equal mid (piem-mid))
+                           (piem-inbox-get :url)))
                  (buffer (condition-case nil
                              (piem-download-and-decompress
                               (concat url (piem-escape-mid mid) "/t.mbox.gz"))
-- 
2.31.1

[PATCH 2/5] Rework url-retrieve-synchronously wrapper

[Kyle Meyer]

piem-download-and-decompress calls url-retrieve-synchronously, checks
for a 200 status, and then manually removes the header.  This works
okay, but it'd be good for the error handling to match what's done by
url-insert-file-contents.

Introduce a new macro that largely copies what is done by
url-insert-file-contents.  The main difference is that url-insert is
used instead of url-insert-buffer-contents so that the contents can be
inserted literally.

This approach is based on Emacs's 5f9671e57e
(lisp/emacs-lisp/package.el: Fix decoding of downloaded files,
2019-05-18).
---
 piem-b4.el | 17 +++++------
 piem.el    | 88 ++++++++++++++++++++++++++++--------------------------
 2 files changed, 54 insertions(+), 51 deletions(-)

diff --git a/piem-b4.el b/piem-b4.el
index 48c8bf4..92ddea8 100644
--- a/piem-b4.el
+++ b/piem-b4.el
@@ -73,15 +73,14 @@ (defun piem-b4--get-am-files (mid coderepo args)
     ;; back to b4's configuration.
     (unless local-mbox-p
       (when-let ((url (and (equal mid (piem-mid))
-                           (piem-inbox-get :url)))
-                 (buffer (condition-case nil
-                             (piem-download-and-decompress
-                              (concat url (piem-escape-mid mid) "/t.mbox.gz"))
-                           (error nil))))
-        (with-current-buffer buffer
-          (write-region nil nil mbox-thread))
-        (kill-buffer buffer)
-        (setq local-mbox-p t)))
+                           (piem-check-gunzip)
+                           (piem-inbox-get :url))))
+        (ignore-errors
+          (piem-with-url-contents
+              (concat url (piem-escape-mid mid) "/t.mbox.gz")
+            (piem-gunzip-buffer)
+            (write-region nil nil mbox-thread))
+          (setq local-mbox-p t))))
     ;; Move to the coderepo so that we pick up any b4 configuration
     ;; from there.
     (condition-case err
diff --git a/piem.el b/piem.el
index 78536f1..7b98005 100644
--- a/piem.el
+++ b/piem.el
@@ -47,9 +47,9 @@ (require 'rfc2047)
 (require 'subr-x)
 (require 'transient)
 (require 'url)
+(require 'url-handlers)
+(require 'url-http)
 
-(defvar url-http-end-of-headers)
-(defvar url-http-response-status)
 
 \f
 ;;;; Options
@@ -535,29 +535,39 @@ (defun piem-check-gunzip ()
     (setq piem--has-gunzip (executable-find "gunzip")))
   piem--has-gunzip)
 
-(defun piem--url-remove-header ()
-  (goto-char (1+ url-http-end-of-headers))
-  (delete-region (point-min) (point)))
-
-(defun piem--url-decompress ()
+(defun piem-gunzip-buffer ()
+  (goto-char (point-min))
   (unless (= 0 (call-process-region nil nil "gunzip" nil t))
     (error "Decompressing t.mbox.gz failed"))
   (delete-region (point) (point-max))
   (goto-char (point-min)))
 
-(defun piem-download-and-decompress (url)
-  "Retrieve gzipped content at URL and decompress it.
-A buffer with the decompressed content is returned."
-  (unless (piem-check-gunzip)
-    (user-error "gunzip executable not found"))
-  (when-let ((buffer (url-retrieve-synchronously url 'silent)))
-    (with-current-buffer buffer
-      (if (/= url-http-response-status 200)
-          (progn (kill-buffer buffer)
-                 (error "Download of %s failed" url))
-        (piem--url-remove-header)
-        (piem--url-decompress))
-      buffer)))
+(defmacro piem-with-url-contents (url &rest body)
+  "Insert URL contents literally into temporary buffer and evaluate BODY."
+  (declare (indent 1) (debug t))
+  (let ((u (cl-gensym "url")))
+    `(with-temp-buffer
+       (set-buffer-multibyte nil)
+       ;; This mostly copies `url-insert-file-contents', but it embeds
+       ;; `url-http--insert-file-helper' and uses `url-insert' rather
+       ;; than `url-insert-buffer-contents' to insert the contents
+       ;; literally.
+       (let* ((,u ,url)
+              (buffer (url-retrieve-synchronously ,u)))
+         (unless buffer (signal 'file-error (list ,u "No Data")))
+         ;; This error handling follows what's in
+         ;; `url-http--insert-file-helper'.
+         (with-current-buffer buffer
+           (when (bound-and-true-p url-http-response-status)
+             (unless (or (and (>= url-http-response-status 200)
+                              (< url-http-response-status 300))
+                         (= url-http-response-status 304))
+               (let ((desc (nth 2 (assq url-http-response-status url-http-codes))))
+                 (kill-buffer buffer)
+                 (signal 'file-error (list ,u desc))))))
+         (url-insert buffer))
+       (goto-char (point-min))
+       ,@body)))
 
 \f
 ;;;; Maildir injection
@@ -623,28 +633,22 @@ (defun piem-inject-thread-into-maildir (mid &optional message-only)
        "Does not look like a Maildir directory: %s" maildir-directory))
      ((not (or message-only (piem-check-gunzip)))
       (user-error "gunzip executable not found")))
-    (when-let ((url (concat (piem-mid-url mid)
-                            (if message-only "/raw" "/t.mbox.gz")))
-               (buffer (url-retrieve-synchronously url 'silent)))
-      (unwind-protect
-          (with-current-buffer buffer
-            (if (/= url-http-response-status 200)
-                (error "Download of %s failed" url)
-              (piem--url-remove-header)
-              (unless message-only
-                (piem--url-decompress))
-              (pcase-let ((`(,added-count . ,skipped-count)
-                           (piem--write-mbox-to-maildir maildir-directory)))
-                (message "Added %d message%s%s for %s to %s"
-                         added-count
-                         (if (= added-count 1) "" "s")
-                         (if (> skipped-count 0)
-                             (format " (skipping %d)" skipped-count)
-                           "")
-                         mid
-                         (abbreviate-file-name maildir-directory)))
-              (run-hook-with-args 'piem-after-mail-injection-functions mid)))
-        (kill-buffer buffer)))))
+    (let ((url (concat (piem-mid-url mid)
+                       (if message-only "/raw" "/t.mbox.gz"))))
+      (piem-with-url-contents url
+        (unless message-only
+          (piem-gunzip-buffer))
+        (pcase-let ((`(,added-count . ,skipped-count)
+                     (piem--write-mbox-to-maildir maildir-directory)))
+          (message "Added %d message%s%s for %s to %s"
+                   added-count
+                   (if (= added-count 1) "" "s")
+                   (if (> skipped-count 0)
+                       (format " (skipping %d)" skipped-count)
+                     "")
+                   mid
+                   (abbreviate-file-name maildir-directory))))
+      (run-hook-with-args 'piem-after-mail-injection-functions mid))))
 
 \f
 ;;;; Patch handling
-- 
2.31.1

[PATCH 3/5] piem-gunzip-buffer: Check for gunzip executable

[Kyle Meyer]

Make piem-gunzip-buffer handle the executable check so that callers
don't have to worry about it.
---
 piem-b4.el | 1 -
 piem.el    | 6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/piem-b4.el b/piem-b4.el
index 92ddea8..e76953e 100644
--- a/piem-b4.el
+++ b/piem-b4.el
@@ -73,7 +73,6 @@ (defun piem-b4--get-am-files (mid coderepo args)
     ;; back to b4's configuration.
     (unless local-mbox-p
       (when-let ((url (and (equal mid (piem-mid))
-                           (piem-check-gunzip)
                            (piem-inbox-get :url))))
         (ignore-errors
           (piem-with-url-contents
diff --git a/piem.el b/piem.el
index 7b98005..20f2f8b 100644
--- a/piem.el
+++ b/piem.el
@@ -536,6 +536,8 @@ (defun piem-check-gunzip ()
   piem--has-gunzip)
 
 (defun piem-gunzip-buffer ()
+  (unless (piem-check-gunzip)
+    (user-error "gunzip executable not found"))
   (goto-char (point-min))
   (unless (= 0 (call-process-region nil nil "gunzip" nil t))
     (error "Decompressing t.mbox.gz failed"))
@@ -630,9 +632,7 @@ (defun piem-inject-thread-into-maildir (mid &optional message-only)
       (user-error "No directory returned by `piem-inbox-maildir-directory'"))
      ((not (piem-maildir-dir-is-maildir-p maildir-directory))
       (user-error
-       "Does not look like a Maildir directory: %s" maildir-directory))
-     ((not (or message-only (piem-check-gunzip)))
-      (user-error "gunzip executable not found")))
+       "Does not look like a Maildir directory: %s" maildir-directory)))
     (let ((url (concat (piem-mid-url mid)
                        (if message-only "/raw" "/t.mbox.gz"))))
       (piem-with-url-contents url
-- 
2.31.1

[PATCH 4/5] piem-gunzip-buffer: Absorb caching of gunzip check

[Kyle Meyer]

Now that piem-gunzip-buffer handles the check for the gunzip
executable, there's not much point in having a dedicated function.
---
 piem.el | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/piem.el b/piem.el
index 20f2f8b..25b7afe 100644
--- a/piem.el
+++ b/piem.el
@@ -529,14 +529,10 @@ (defun piem-copy-mid-url (&optional browse)
 ;;;; Download helpers
 
 (defvar piem--has-gunzip)
-(defun piem-check-gunzip ()
-  "Return non-nil if gunzip is available."
-  (unless (boundp 'piem--has-gunzip)
-    (setq piem--has-gunzip (executable-find "gunzip")))
-  piem--has-gunzip)
-
 (defun piem-gunzip-buffer ()
-  (unless (piem-check-gunzip)
+  (unless (if (boundp 'piem--has-gunzip)
+              piem--has-gunzip
+            (setq piem--has-gunzip (executable-find "gunzip")))
     (user-error "gunzip executable not found"))
   (goto-char (point-min))
   (unless (= 0 (call-process-region nil nil "gunzip" nil t))
-- 
2.31.1

[PATCH 5/5] piem-gunzip-buffer: Don't assume t.mbox.gz is being decompressed

[Kyle Meyer]

If the gunzip call fails, piem-gunzip-buffer says "Decompressing
t.mbox.gz failed".  All the piem-gunzip-buffer callers at the moment
do use to decompress t.mbox.gz downloads, but that's of course not
something this function should assume or know about.
---
 piem.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/piem.el b/piem.el
index 25b7afe..2626643 100644
--- a/piem.el
+++ b/piem.el
@@ -536,7 +536,7 @@ (defun piem-gunzip-buffer ()
     (user-error "gunzip executable not found"))
   (goto-char (point-min))
   (unless (= 0 (call-process-region nil nil "gunzip" nil t))
-    (error "Decompressing t.mbox.gz failed"))
+    (error "Decompressing buffer failed"))
   (delete-region (point) (point-max))
   (goto-char (point-min)))
 
-- 
2.31.1