I've hit into a few cases where piem-notmuch-am-ready-mbox hasn't been able to find an attached patch. This series, in particular patch 2, should improve the situation. [1/3] piem-notmuch--with-current-message: Declare debug and indent specs [2/3] piem-notmuch-am-ready-mbox: Improve handling of attachments [3/3] gnus, notmuch: Absorb now-shared bits into patch attachment helper piem-gnus.el | 14 ++----------- piem-notmuch.el | 52 ++++++++++++++++++++++++++----------------------- piem.el | 21 ++++++++++++-------- 3 files changed, 43 insertions(+), 44 deletions(-) base-commit: 96da1c622caac904e3f60e306847a4e68ca15e0c -- 2.29.2
--- piem-notmuch.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/piem-notmuch.el b/piem-notmuch.el index 361b450..ff8654c 100644 --- a/piem-notmuch.el +++ b/piem-notmuch.el @@ -33,6 +33,7 @@ (require 'piem) (require 'subr-x) (defmacro piem-notmuch--with-current-message (&rest body) + (declare (indent 0) (debug (body))) (let ((rv (make-symbol "rv"))) `(let (,rv) (with-current-notmuch-show-message @@ -43,7 +44,7 @@ (defun piem-notmuch-get-inbox () "Return inbox name from a `notmuch-show-mode' buffer." (when (derived-mode-p 'notmuch-show-mode) (piem-notmuch--with-current-message - (piem-inbox-by-header-match)))) + (piem-inbox-by-header-match)))) (defun piem-notmuch-get-mid () "Return the message ID of a `notmuch-show-mode' buffer." -- 2.29.2
piem-notmuch-am-ready-mbox tries to get attached patches from the plist returned by notmuch-show-get-message-properties. This works okay for simple cases, but it doesn't find a patch if the content is encoded or if the MIME tree isn't structured as expected. Instead use mm-decode functions to get a list of handles and display their content. This new approach, as well as the previous one, probably isn't compatible with `format-patch --attach' patches, but I'm not going to worry about that until I actually see such a patch in the wild. --- piem-notmuch.el | 55 ++++++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/piem-notmuch.el b/piem-notmuch.el index ff8654c..94b1fa0 100644 --- a/piem-notmuch.el +++ b/piem-notmuch.el @@ -28,6 +28,7 @@ ;;; Code: +(require 'mm-decode) (require 'notmuch) (require 'piem) (require 'subr-x) @@ -78,29 +79,37 @@ (defun piem-notmuch-am-ready-mbox () those parts' contents (in order) as the mbox. Otherwise, use the message itself if it looks like a patch." (when (derived-mode-p 'notmuch-show-mode) - (let ((body (car (plist-get (notmuch-show-get-message-properties) - :body)))) - (pcase (plist-get body :content-type) - ((and "text/plain" - (guard (string-match-p piem-patch-subject-re - (notmuch-show-get-subject)))) - (let ((id (notmuch-show-get-message-id))) - (lambda () - (call-process notmuch-command nil t nil - "show" "--format=mbox" id)))) - ("multipart/mixed" - (when-let ((patches - (delq nil - (mapcar (lambda (part) - (and (piem-am-patch-attachment-p - (plist-get part :content-type) - (plist-get part :filename)) - (plist-get part :content))) - (plist-get body :content))))) - (cons (lambda () - (dolist (patch patches) - (insert patch))) - "mbox"))))))) + (if (string-prefix-p + "multipart/" + (plist-get + (car (plist-get (notmuch-show-get-message-properties) + :body)) + :content-type)) + (let (patches) + (piem-notmuch--with-current-message + (notmuch-foreach-mime-part + (lambda (p) + (and (piem-am-patch-attachment-p + (mm-handle-media-type p) + (mm-handle-filename p)) + (with-temp-buffer + (mm-display-inline p) + (push (buffer-substring-no-properties + (point-min) (point-max)) + patches)))) + (mm-dissect-buffer))) + (when patches + (setq patches (nreverse patches)) + (cons (lambda () + (dolist (patch patches) + (insert patch))) + "mbox"))) + (when (string-match-p piem-patch-subject-re + (notmuch-show-get-subject)) + (let ((id (notmuch-show-get-message-id))) + (lambda () + (call-process notmuch-command nil t nil + "show" "--format=mbox" id))))))) ;;;###autoload (define-minor-mode piem-notmuch-mode -- 2.29.2
With the previous commit, -notmuch more closely follows -gnus in its handling of attachments (e.g., getting the content with mm-display-inline). Replace piem-am-patch-attachment-p with a helper that has this shared logic. --- piem-gnus.el | 14 ++------------ piem-notmuch.el | 10 ++-------- piem.el | 21 +++++++++++++-------- 3 files changed, 17 insertions(+), 28 deletions(-) diff --git a/piem-gnus.el b/piem-gnus.el index c6b9d0c..5f10be8 100644 --- a/piem-gnus.el +++ b/piem-gnus.el @@ -66,18 +66,8 @@ (defun piem-gnus-am-ready-mbox () (when (derived-mode-p 'gnus-article-mode 'gnus-summary-mode) (cond (gnus-article-mime-handles - (when-let ((patches - (delq nil - (mapcar (lambda (handle) - (and (listp handle) - (piem-am-patch-attachment-p - (mm-handle-media-type handle) - (mm-handle-filename handle)) - (with-temp-buffer - (mm-display-inline handle) - (buffer-substring-no-properties - (point-min) (point-max))))) - gnus-article-mime-handles)))) + (when-let ((patches (delq nil (mapcar #'piem-am-extract-attached-patch + gnus-article-mime-handles)))) (cons (lambda () (dolist (patch patches) (insert patch))) diff --git a/piem-notmuch.el b/piem-notmuch.el index 94b1fa0..07f17d4 100644 --- a/piem-notmuch.el +++ b/piem-notmuch.el @@ -89,14 +89,8 @@ (defun piem-notmuch-am-ready-mbox () (piem-notmuch--with-current-message (notmuch-foreach-mime-part (lambda (p) - (and (piem-am-patch-attachment-p - (mm-handle-media-type p) - (mm-handle-filename p)) - (with-temp-buffer - (mm-display-inline p) - (push (buffer-substring-no-properties - (point-min) (point-max)) - patches)))) + (when-let ((patch (piem-am-extract-attached-patch p))) + (push patch patches))) (mm-dissect-buffer))) (when patches (setq patches (nreverse patches)) diff --git a/piem.el b/piem.el index bf79813..8c23e7b 100644 --- a/piem.el +++ b/piem.el @@ -40,6 +40,7 @@ (require 'cl-lib) (require 'mail-extr) (require 'message) +(require 'mm-decode) (require 'piem-maildir) (require 'rfc2047) (require 'subr-x) @@ -573,14 +574,18 @@ (defun piem-inject-thread-into-maildir (mid &optional message-only) \f ;;;; Patch handling -(defun piem-am-patch-attachment-p (type filename) - "Return non-nil if an attachment should be treated as a patch. -TYPE is a media type such as \"text/x-patch\". FILENAME is the -attachment file name, if any." - (or (member type '("text/x-diff" "text/x-patch")) - (and filename - (equal type "text/plain") - (string-match-p "\\.patch\\'" filename)))) +(defun piem-am-extract-attached-patch (handle) + "Return content for HANDLE if it looks like a patch." + (and (listp handle) + (let ((type (mm-handle-media-type handle)) + (filename (mm-handle-filename handle))) + (or (member type '("text/x-diff" "text/x-patch")) + (and filename + (equal type "text/plain") + (string-match-p "\\.patch\\'" filename)))) + (with-temp-buffer + (mm-display-inline handle) + (buffer-substring-no-properties (point-min) (point-max))))) (defun piem-extract-mbox-info (&optional buffer) "Collect information from message in BUFFER. -- 2.29.2
Kyle Meyer writes:
> + (if (string-prefix-p
> + "multipart/"
> + (plist-get
> + (car (plist-get (notmuch-show-get-message-properties)
> + :body))
> + :content-type))
This is problematic for an inline multipart/signed patch (which the
old/current logic doesn't handle properly either). That's not a big
deal, since such a patch can be sent through piem-b4-am-from-mid
instead, but it'd still be nice to support it.
v2 incoming...
I've hit into a few cases where piem-notmuch-am-ready-mbox hasn't been able to find an attached patch. This series, in particular patch 2, should improve the situation. v2: Fix handling of inline multipart/signed patches by using the attachment count rather than the top-level content-type to decide whether to go down the attachment or inline code path. [1/3] piem-notmuch--with-current-message: Declare debug and indent specs [2/3] piem-notmuch-am-ready-mbox: Improve handling of attachments [3/3] gnus, notmuch: Absorb now-shared bits into patch attachment helper piem-gnus.el | 14 ++------------ piem-notmuch.el | 49 +++++++++++++++++++++++++------------------------ piem.el | 21 +++++++++++++-------- 3 files changed, 40 insertions(+), 44 deletions(-) Range-diff against v1: 1: f4ff1f1 = 1: f4ff1f1 piem-notmuch--with-current-message: Declare debug and indent specs 2: 8eb290d ! 2: ca8d684 piem-notmuch-am-ready-mbox: Improve handling of attachments @@ piem-notmuch.el: (defun piem-notmuch-am-ready-mbox () - (dolist (patch patches) - (insert patch))) - "mbox"))))))) -+ (if (string-prefix-p -+ "multipart/" -+ (plist-get -+ (car (plist-get (notmuch-show-get-message-properties) -+ :body)) -+ :content-type)) -+ (let (patches) -+ (piem-notmuch--with-current-message -+ (notmuch-foreach-mime-part -+ (lambda (p) -+ (and (piem-am-patch-attachment-p -+ (mm-handle-media-type p) -+ (mm-handle-filename p)) -+ (with-temp-buffer -+ (mm-display-inline p) -+ (push (buffer-substring-no-properties -+ (point-min) (point-max)) -+ patches)))) -+ (mm-dissect-buffer))) -+ (when patches -+ (setq patches (nreverse patches)) -+ (cons (lambda () -+ (dolist (patch patches) -+ (insert patch))) -+ "mbox"))) -+ (when (string-match-p piem-patch-subject-re -+ (notmuch-show-get-subject)) -+ (let ((id (notmuch-show-get-message-id))) -+ (lambda () -+ (call-process notmuch-command nil t nil -+ "show" "--format=mbox" id))))))) ++ (let* ((handle (piem-notmuch--with-current-message ++ (mm-dissect-buffer))) ++ (n-attachments (notmuch-count-attachments handle)) ++ patches) ++ (if (= n-attachments 0) ++ (when (string-match-p piem-patch-subject-re ++ (notmuch-show-get-subject)) ++ (let ((id (notmuch-show-get-message-id))) ++ (lambda () ++ (call-process notmuch-command nil t nil ++ "show" "--format=mbox" id)))) ++ (notmuch-foreach-mime-part ++ (lambda (p) ++ (and (piem-am-patch-attachment-p ++ (mm-handle-media-type p) ++ (mm-handle-filename p)) ++ (with-temp-buffer ++ (mm-display-inline p) ++ (push (buffer-substring-no-properties ++ (point-min) (point-max)) ++ patches)))) ++ handle) ++ (when patches ++ (setq patches (nreverse patches)) ++ (cons (lambda () ++ (dolist (patch patches) ++ (insert patch))) ++ "mbox")))))) ;;;###autoload (define-minor-mode piem-notmuch-mode 3: 6bbde3b ! 3: ee314a5 gnus, notmuch: Absorb now-shared bits into patch attachment helper @@ piem-gnus.el: (defun piem-gnus-am-ready-mbox () ## piem-notmuch.el ## @@ piem-notmuch.el: (defun piem-notmuch-am-ready-mbox () - (piem-notmuch--with-current-message - (notmuch-foreach-mime-part - (lambda (p) -- (and (piem-am-patch-attachment-p -- (mm-handle-media-type p) -- (mm-handle-filename p)) -- (with-temp-buffer -- (mm-display-inline p) -- (push (buffer-substring-no-properties -- (point-min) (point-max)) -- patches)))) -+ (when-let ((patch (piem-am-extract-attached-patch p))) -+ (push patch patches))) - (mm-dissect-buffer))) - (when patches - (setq patches (nreverse patches)) + "show" "--format=mbox" id)))) + (notmuch-foreach-mime-part + (lambda (p) +- (and (piem-am-patch-attachment-p +- (mm-handle-media-type p) +- (mm-handle-filename p)) +- (with-temp-buffer +- (mm-display-inline p) +- (push (buffer-substring-no-properties +- (point-min) (point-max)) +- patches)))) ++ (when-let ((patch (piem-am-extract-attached-patch p))) ++ (push patch patches))) + handle) + (when patches + (setq patches (nreverse patches)) ## piem.el ## @@ base-commit: 96da1c622caac904e3f60e306847a4e68ca15e0c -- 2.29.2
--- piem-notmuch.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/piem-notmuch.el b/piem-notmuch.el index 361b450..ff8654c 100644 --- a/piem-notmuch.el +++ b/piem-notmuch.el @@ -33,6 +33,7 @@ (require 'piem) (require 'subr-x) (defmacro piem-notmuch--with-current-message (&rest body) + (declare (indent 0) (debug (body))) (let ((rv (make-symbol "rv"))) `(let (,rv) (with-current-notmuch-show-message @@ -43,7 +44,7 @@ (defun piem-notmuch-get-inbox () "Return inbox name from a `notmuch-show-mode' buffer." (when (derived-mode-p 'notmuch-show-mode) (piem-notmuch--with-current-message - (piem-inbox-by-header-match)))) + (piem-inbox-by-header-match)))) (defun piem-notmuch-get-mid () "Return the message ID of a `notmuch-show-mode' buffer." -- 2.29.2
piem-notmuch-am-ready-mbox tries to get attached patches from the plist returned by notmuch-show-get-message-properties. This works okay for simple cases, but it doesn't find a patch if the content is encoded or if the MIME tree isn't structured as expected. Instead use mm-decode functions to get a list of handles and display their content. This new approach, as well as the previous one, probably isn't compatible with `format-patch --attach' patches, but I'm not going to worry about that until I actually see such a patch in the wild. --- piem-notmuch.el | 52 +++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/piem-notmuch.el b/piem-notmuch.el index ff8654c..915675e 100644 --- a/piem-notmuch.el +++ b/piem-notmuch.el @@ -28,6 +28,7 @@ ;;; Code: +(require 'mm-decode) (require 'notmuch) (require 'piem) (require 'subr-x) @@ -78,29 +79,34 @@ (defun piem-notmuch-am-ready-mbox () those parts' contents (in order) as the mbox. Otherwise, use the message itself if it looks like a patch." (when (derived-mode-p 'notmuch-show-mode) - (let ((body (car (plist-get (notmuch-show-get-message-properties) - :body)))) - (pcase (plist-get body :content-type) - ((and "text/plain" - (guard (string-match-p piem-patch-subject-re - (notmuch-show-get-subject)))) - (let ((id (notmuch-show-get-message-id))) - (lambda () - (call-process notmuch-command nil t nil - "show" "--format=mbox" id)))) - ("multipart/mixed" - (when-let ((patches - (delq nil - (mapcar (lambda (part) - (and (piem-am-patch-attachment-p - (plist-get part :content-type) - (plist-get part :filename)) - (plist-get part :content))) - (plist-get body :content))))) - (cons (lambda () - (dolist (patch patches) - (insert patch))) - "mbox"))))))) + (let* ((handle (piem-notmuch--with-current-message + (mm-dissect-buffer))) + (n-attachments (notmuch-count-attachments handle)) + patches) + (if (= n-attachments 0) + (when (string-match-p piem-patch-subject-re + (notmuch-show-get-subject)) + (let ((id (notmuch-show-get-message-id))) + (lambda () + (call-process notmuch-command nil t nil + "show" "--format=mbox" id)))) + (notmuch-foreach-mime-part + (lambda (p) + (and (piem-am-patch-attachment-p + (mm-handle-media-type p) + (mm-handle-filename p)) + (with-temp-buffer + (mm-display-inline p) + (push (buffer-substring-no-properties + (point-min) (point-max)) + patches)))) + handle) + (when patches + (setq patches (nreverse patches)) + (cons (lambda () + (dolist (patch patches) + (insert patch))) + "mbox")))))) ;;;###autoload (define-minor-mode piem-notmuch-mode -- 2.29.2
With the previous commit, -notmuch more closely follows -gnus in its handling of attachments (e.g., getting the content with mm-display-inline). Replace piem-am-patch-attachment-p with a helper that has this shared logic. --- piem-gnus.el | 14 ++------------ piem-notmuch.el | 10 ++-------- piem.el | 21 +++++++++++++-------- 3 files changed, 17 insertions(+), 28 deletions(-) diff --git a/piem-gnus.el b/piem-gnus.el index c6b9d0c..5f10be8 100644 --- a/piem-gnus.el +++ b/piem-gnus.el @@ -66,18 +66,8 @@ (defun piem-gnus-am-ready-mbox () (when (derived-mode-p 'gnus-article-mode 'gnus-summary-mode) (cond (gnus-article-mime-handles - (when-let ((patches - (delq nil - (mapcar (lambda (handle) - (and (listp handle) - (piem-am-patch-attachment-p - (mm-handle-media-type handle) - (mm-handle-filename handle)) - (with-temp-buffer - (mm-display-inline handle) - (buffer-substring-no-properties - (point-min) (point-max))))) - gnus-article-mime-handles)))) + (when-let ((patches (delq nil (mapcar #'piem-am-extract-attached-patch + gnus-article-mime-handles)))) (cons (lambda () (dolist (patch patches) (insert patch))) diff --git a/piem-notmuch.el b/piem-notmuch.el index 915675e..37e695b 100644 --- a/piem-notmuch.el +++ b/piem-notmuch.el @@ -92,14 +92,8 @@ (defun piem-notmuch-am-ready-mbox () "show" "--format=mbox" id)))) (notmuch-foreach-mime-part (lambda (p) - (and (piem-am-patch-attachment-p - (mm-handle-media-type p) - (mm-handle-filename p)) - (with-temp-buffer - (mm-display-inline p) - (push (buffer-substring-no-properties - (point-min) (point-max)) - patches)))) + (when-let ((patch (piem-am-extract-attached-patch p))) + (push patch patches))) handle) (when patches (setq patches (nreverse patches)) diff --git a/piem.el b/piem.el index bf79813..8c23e7b 100644 --- a/piem.el +++ b/piem.el @@ -40,6 +40,7 @@ (require 'cl-lib) (require 'mail-extr) (require 'message) +(require 'mm-decode) (require 'piem-maildir) (require 'rfc2047) (require 'subr-x) @@ -573,14 +574,18 @@ (defun piem-inject-thread-into-maildir (mid &optional message-only) \f ;;;; Patch handling -(defun piem-am-patch-attachment-p (type filename) - "Return non-nil if an attachment should be treated as a patch. -TYPE is a media type such as \"text/x-patch\". FILENAME is the -attachment file name, if any." - (or (member type '("text/x-diff" "text/x-patch")) - (and filename - (equal type "text/plain") - (string-match-p "\\.patch\\'" filename)))) +(defun piem-am-extract-attached-patch (handle) + "Return content for HANDLE if it looks like a patch." + (and (listp handle) + (let ((type (mm-handle-media-type handle)) + (filename (mm-handle-filename handle))) + (or (member type '("text/x-diff" "text/x-patch")) + (and filename + (equal type "text/plain") + (string-match-p "\\.patch\\'" filename)))) + (with-temp-buffer + (mm-display-inline handle) + (buffer-substring-no-properties (point-min) (point-max))))) (defun piem-extract-mbox-info (&optional buffer) "Collect information from message in BUFFER. -- 2.29.2