[PATCH v2] Use notmuch-extract-patch if available

[PATCH v2] Use notmuch-extract-patch if available

[leo]

From: Leo <sourcehut@relevant-information.com>

notmuch-extract-patch is a command line tool from the elpa-mailscripts
package that extracts patches from a thread.  It's a useful way to
extract the latest patch series from an email thread and filter out the
replies and reviews.
---
I've extracted the functionality into its own function as requested, and 
added clarification on where the copied comment came from.  I'm not sure 
how the copyright notice should be formatted though. 


 piem-notmuch.el | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/piem-notmuch.el b/piem-notmuch.el
index 8b2a353..27a7d7c 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -1,6 +1,6 @@
 ;;; piem-notmuch.el --- Notmuch integration for piem  -*- lexical-binding: t; -*-
 
-;; Copyright (C) 2020-2021  all contributors <piem@inbox.kyleam.com>
+;; Copyright (C) 2020-2021  all contributors <piem@inbox.kyleam.com> (C) 2019 Sean Whitton <spwhitton@spwhitton.name>
 
 ;; Author: Kyle Meyer <kyle@kyleam.com>
 ;; Keywords: vc, tools
@@ -106,6 +106,40 @@ (defun piem-notmuch-am-ready-mbox ()
                     (insert patch)))
                 "mbox"))))))
 
+(defun piem-notmuch-extract-patch-am-ready-mbox ()
+  "Return a function that inserts an am-ready mbox.
+Use the message itself if it looks like a patch using
+notmuch-extract-patch to get the latest patch series from the
+notmuch thread."
+  (when (derived-mode-p 'notmuch-show-mode)
+    (let* ((handle (piem-notmuch--with-current-message
+                     (mm-dissect-buffer)))
+           (n-attachments (notmuch-count-attachments handle))
+           patches)
+      (when (= n-attachments 0)
+          (when (string-match-p piem-patch-subject-re
+                                (notmuch-show-get-subject))
+            (let ((id (notmuch-show-get-message-id))
+                  (thread-id notmuch-show-thread-id))
+              (lambda ()
+                (if-let ((cmd (executable-find "notmuch-extract-patch"))
+                         (tid
+                          ;; Copied from mailscripts.el
+                          ;;
+                          ;; If `notmuch-show' was called with a notmuch query rather
+                          ;; than a thread ID, as `org-notmuch-follow-link' in
+                          ;; org-notmuch.el does, then `notmuch-show-thread-id' might
+                          ;; be an arbitrary notmuch query instead of a thread ID.  We
+                          ;; need to wrap such a query in thread:{} before passing it
+                          ;; to notmuch-extract-patch(1), or we might not get a whole
+                          ;; thread extracted (e.g. if the query is just id:foo)
+                          (if (string= (substring thread-id 0 7) "thread:")
+                              thread-id
+                            (concat "thread:{" thread-id "}"))))
+                    (call-process cmd nil t nil
+                                  tid)
+                  (user-error "The executable notmuch-extract-patch was not found")))))))))
+
 (defun piem-notmuch-show-get-public-inbox-link (mid)
   "Given the message-id MID, return the public-inbox url.
 This will lookup the url in the inboxes returned by

base-commit: e519aa44d148d5b8f22d7fe8844dc566046b04c2
-- 
2.34.1

Re: [PATCH v2] Use notmuch-extract-patch if available

[Kyle Meyer]

Thanks for the update.

leo@relevant-information.com writes:

> From: Leo <sourcehut@relevant-information.com>

I think the patch subject needs an update for the current approach where
notmuch-extract-patch isn't used by default.  Perhaps something like

  Support preparing am-ready mbox via notmuch-extract-patch

?

> notmuch-extract-patch is a command line tool from the elpa-mailscripts
> package that extracts patches from a thread.  It's a useful way to
> extract the latest patch series from an email thread and filter out the
> replies and reviews.
> ---
> I've extracted the functionality into its own function as requested, and 
> added clarification on where the copied comment came from.  I'm not sure 
> how the copyright notice should be formatted though. 

Thanks.  For the copyright, please add it on its own line (i.e. repeat
";; Copyright (C) ...").

> +(defun piem-notmuch-extract-patch-am-ready-mbox ()
> +  "Return a function that inserts an am-ready mbox.
> +Use the message itself if it looks like a patch using
> +notmuch-extract-patch to get the latest patch series from the
> +notmuch thread."
> +  (when (derived-mode-p 'notmuch-show-mode)
> +    (let* ((handle (piem-notmuch--with-current-message
> +                     (mm-dissect-buffer)))
> +           (n-attachments (notmuch-count-attachments handle))
> +           patches)
> +      (when (= n-attachments 0)
> +          (when (string-match-p piem-patch-subject-re
> +                                (notmuch-show-get-subject))
> +            (let ((id (notmuch-show-get-message-id))
> +                  (thread-id notmuch-show-thread-id))

The `patches' and `id' bindings are unused and should be removed:

  $ make compile
  emacs --batch -Q -L . -L tests -f batch-byte-compile piem-notmuch.el
  
  In toplevel form:
  piem-notmuch.el:109:1:Warning: Unused lexical variable ‘id’
  piem-notmuch.el:109:1:Warning: Unused lexical variable ‘patches’

A purely cosmetic comment: you could compress the (when ... (when ...))
to

  (when (and (= n-attachments 0)
             (string-match-p piem-patch-subject-re
                             (notmuch-show-get-subject)))
   ....)

Or, rolling it all the way up to the top (and taking advantage of the
fact that, unlike in piem-notmuch-am-ready-mbox, `handle' isn't used
here):

  (when (and (derived-mode-p 'notmuch-show-mode)
             (string-match-p piem-patch-subject-re (notmuch-show-get-subject))
             (= (notmuch-count-attachments
                 (piem-notmuch--with-current-message
                   (mm-dissect-buffer)))
                0))
    ...)

Anyway, that's dealer's choice.

With the next iteration, I think this should be good to apply, adjusting
later as needed.  Once things sit for a bit, I can extend the
documentation to mention it.

Re: [PATCH v2] Use notmuch-extract-patch if available

[Kyle Meyer]

Kyle Meyer writes:

> Or, rolling it all the way up to the top (and taking advantage of the
> fact that, unlike in piem-notmuch-am-ready-mbox, `handle' isn't used
> here):

Not really important, but what I meant to say that `handle' isn't used
in the `let' body here.

[PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch

[sourcehut]

From: Leo <sourcehut@relevant-information.com>

notmuch-extract-patch is a command line tool from the elpa-mailscripts
package that extracts patches from a thread.  It's a useful way to
extract the latest patch series from an email thread and filter out the
replies and reviews.
---
 piem-notmuch.el | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/piem-notmuch.el b/piem-notmuch.el
index 8b2a353..1ee0b37 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -1,6 +1,7 @@
 ;;; piem-notmuch.el --- Notmuch integration for piem  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 2020-2021  all contributors <piem@inbox.kyleam.com>
+;;           (C) 2019 Sean Whitton <spwhitton@spwhitton.name>
 
 ;; Author: Kyle Meyer <kyle@kyleam.com>
 ;; Keywords: vc, tools
@@ -106,6 +107,37 @@ (defun piem-notmuch-am-ready-mbox ()
                     (insert patch)))
                 "mbox"))))))
 
+(defun piem-notmuch-extract-patch-am-ready-mbox ()
+  "Return a function that inserts an am-ready mbox.
+Use the message itself if it looks like a patch using
+notmuch-extract-patch to get the latest patch series from the
+notmuch thread."
+  (when (and (derived-mode-p 'notmuch-show-mode)
+             (string-match-p piem-patch-subject-re
+                             (notmuch-show-get-subject))
+             (= (notmuch-count-attachments
+                 (piem-notmuch--with-current-message
+                  (mm-dissect-buffer))) 0))
+    (let ((thread-id notmuch-show-thread-id))
+      (lambda ()
+        (if-let ((cmd (executable-find "notmuch-extract-patch"))
+                 (tid
+                  ;; Copied from mailscripts.el
+                  ;;
+                  ;; If `notmuch-show' was called with a notmuch query rather
+                  ;; than a thread ID, as `org-notmuch-follow-link' in
+                  ;; org-notmuch.el does, then `notmuch-show-thread-id' might
+                  ;; be an arbitrary notmuch query instead of a thread ID.  We
+                  ;; need to wrap such a query in thread:{} before passing it
+                  ;; to notmuch-extract-patch(1), or we might not get a whole
+                  ;; thread extracted (e.g. if the query is just id:foo)
+                  (if (string= (substring thread-id 0 7) "thread:")
+                      thread-id
+                    (concat "thread:{" thread-id "}"))))
+            (call-process cmd nil t nil
+                          tid)
+          (user-error "The executable notmuch-extract-patch was not found"))))))
+
 (defun piem-notmuch-show-get-public-inbox-link (mid)
   "Given the message-id MID, return the public-inbox url.
 This will lookup the url in the inboxes returned by
-- 
2.34.1

[PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch

[sourcehut]

From: Leo <sourcehut@relevant-information.com>

`notmuch-extract-patch` might not be available on PATH.  This can be the
case if the package wasn't installed through the distro package manager.
---
My laptop didn't have mailscripts in the package repos so I had to
install it from emacsmirror which didn't put the scripts in PATH.  I
thought this change might be useful for others as well so I decided to
include it.

 piem-notmuch.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/piem-notmuch.el b/piem-notmuch.el
index 1ee0b37..bc254f5 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -107,6 +107,10 @@ (defun piem-notmuch-am-ready-mbox ()
                     (insert patch)))
                 "mbox"))))))
 
+(defvar piem-notmuch-extract-patch-path nil
+  "The path to the executable notmuch-extract path.
+If nil try to find it on path.")
+
 (defun piem-notmuch-extract-patch-am-ready-mbox ()
   "Return a function that inserts an am-ready mbox.
 Use the message itself if it looks like a patch using
@@ -120,7 +124,7 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
                   (mm-dissect-buffer))) 0))
     (let ((thread-id notmuch-show-thread-id))
       (lambda ()
-        (if-let ((cmd (executable-find "notmuch-extract-patch"))
+        (if-let ((cmd (or piem-notmuch-extract-patch-path (executable-find "notmuch-extract-patch")))
                  (tid
                   ;; Copied from mailscripts.el
                   ;;
-- 
2.34.1

Re: [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch

[Kyle Meyer]

Thanks, pushed (3e553530) ...

>  ;; Copyright (C) 2020-2021  all contributors <piem@inbox.kyleam.com>
> +;;           (C) 2019 Sean Whitton <spwhitton@spwhitton.name>

... adding a "Copyright" on the second line (for no stronger reason than
that's what looks most familiar to me, something that's probably largely
influenced by Guix's practice).

Re: [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch

[Kyle Meyer]

sourcehut@relevant-information.com writes:

> From: Leo <sourcehut@relevant-information.com>
>
> `notmuch-extract-patch` might not be available on PATH.  This can be the
> case if the package wasn't installed through the distro package manager.
> ---
> My laptop didn't have mailscripts in the package repos so I had to
> install it from emacsmirror which didn't put the scripts in PATH.  I
> thought this change might be useful for others as well so I decided to
> include it.

I okay with the general idea, but I held off on pushing this to propose
that instead you add a notmuch-extract-patch-executable defcustom
consistent with other spots in piem (piem-git-executable,
piem-b4-b4-executable, piem-lei-lei-executable).

That can be set to "notmuch-extract-patch" by default then ...

>  (defun piem-notmuch-extract-patch-am-ready-mbox ()
>    "Return a function that inserts an am-ready mbox.
>  Use the message itself if it looks like a patch using
> @@ -120,7 +124,7 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
>                    (mm-dissect-buffer))) 0))
>      (let ((thread-id notmuch-show-thread-id))
>        (lambda ()
> -        (if-let ((cmd (executable-find "notmuch-extract-patch"))
> +        (if-let ((cmd (or piem-notmuch-extract-patch-path (executable-find "notmuch-extract-patch")))

... instead of worrying about the executable-find call here, just pass
notmuch-extract-patch-executable to call-process as is.  If call-process
can't find it, it will signal a clear enough error:

   Searching for program: No such file or directory, notmuch-extract-patch

[PATCH v4] Add user option for specifying path to notmuch-extract-patch

[Leo]

`notmuch-extract-patch` might not be available on PATH.  This can be the
case if the package wasn't installed through the distro package manager.
---
 piem-notmuch.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/piem-notmuch.el b/piem-notmuch.el
index b48cf70..34231a5 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -38,6 +38,10 @@ (defgroup piem-notmuch nil
   "Notmuch integration for piem."
   :group 'piem)
 
+(defcustom piem-notmuch-extract-patch-executable "notmuch-extract-patch"
+  "Which notmuch-extract-patch executable to use."
+  :type 'string)
+
 (defmacro piem-notmuch--with-current-message (&rest body)
   (declare (indent 0) (debug (body)))
   (let ((rv (make-symbol "rv")))
@@ -120,8 +124,7 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
                   (mm-dissect-buffer))) 0))
     (let ((thread-id notmuch-show-thread-id))
       (lambda ()
-        (if-let ((cmd (executable-find "notmuch-extract-patch"))
-                 (tid
+        (if-let ((tid
                   ;; Copied from mailscripts.el
                   ;;
                   ;; If `notmuch-show' was called with a notmuch query rather
@@ -134,7 +137,7 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
                   (if (string= (substring thread-id 0 7) "thread:")
                       thread-id
                     (concat "thread:{" thread-id "}"))))
-            (call-process cmd nil t nil
+            (call-process piem-notmuch-extract-patch-executable nil t nil
                           tid)
           (user-error "The executable notmuch-extract-patch was not found"))))))
 

base-commit: 3e5535304519c31453ec12037fcd33a4b4095a51
-- 
2.34.1

Re: [PATCH v4] Add user option for specifying path to notmuch-extract-patch

[Kyle Meyer]

Thanks for the update.

Leo writes:

> +(defcustom piem-notmuch-extract-patch-executable "notmuch-extract-patch"
> +  "Which notmuch-extract-patch executable to use."
> +  :type 'string)

Looks good.  I'll add a package-version keyword on apply.

> @@ -120,8 +124,7 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
>                    (mm-dissect-buffer))) 0))
>      (let ((thread-id notmuch-show-thread-id))
>        (lambda ()
> -        (if-let ((cmd (executable-find "notmuch-extract-patch"))
> -                 (tid
> +        (if-let ((tid
>                    ;; Copied from mailscripts.el
>                    ;;
>                    ;; If `notmuch-show' was called with a notmuch query rather
> @@ -134,7 +137,7 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
>                    (if (string= (substring thread-id 0 7) "thread:")
>                        thread-id
>                      (concat "thread:{" thread-id "}"))))
> -            (call-process cmd nil t nil
> +            (call-process piem-notmuch-extract-patch-executable nil t nil
>                            tid)
>            (user-error "The executable notmuch-extract-patch was not found"))))))

This user-error is no longer accurate because it would only be signaled
if notmuch-show-thread-id is unexpectedly nil.  I've amended your patch
to drop this if-let binding, moving the tid binding outside the lambda.

Pushed (955abe41) with the changes below on top.

diff --git a/piem-notmuch.el b/piem-notmuch.el
index 34231a50..71d56f7e 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -40,6 +40,7 @@ (defgroup piem-notmuch nil
 
 (defcustom piem-notmuch-extract-patch-executable "notmuch-extract-patch"
   "Which notmuch-extract-patch executable to use."
+  :package-version '(piem . "0.4.0")
   :type 'string)
 
 (defmacro piem-notmuch--with-current-message (&rest body)
@@ -122,24 +123,25 @@ (defun piem-notmuch-extract-patch-am-ready-mbox ()
              (= (notmuch-count-attachments
                  (piem-notmuch--with-current-message
                   (mm-dissect-buffer))) 0))
-    (let ((thread-id notmuch-show-thread-id))
+    (let* ((thread-id
+            (or notmuch-show-thread-id
+                (error "bug: notmuch-show-thread-id unexpectedly nil")))
+           (tid
+            ;; Copied from mailscripts.el
+            ;;
+            ;; If `notmuch-show' was called with a notmuch query rather
+            ;; than a thread ID, as `org-notmuch-follow-link' in
+            ;; org-notmuch.el does, then `notmuch-show-thread-id' might
+            ;; be an arbitrary notmuch query instead of a thread ID.  We
+            ;; need to wrap such a query in thread:{} before passing it
+            ;; to notmuch-extract-patch(1), or we might not get a whole
+            ;; thread extracted (e.g. if the query is just id:foo)
+            (if (string= (substring thread-id 0 7) "thread:")
+                thread-id
+              (concat "thread:{" thread-id "}"))))
       (lambda ()
-        (if-let ((tid
-                  ;; Copied from mailscripts.el
-                  ;;
-                  ;; If `notmuch-show' was called with a notmuch query rather
-                  ;; than a thread ID, as `org-notmuch-follow-link' in
-                  ;; org-notmuch.el does, then `notmuch-show-thread-id' might
-                  ;; be an arbitrary notmuch query instead of a thread ID.  We
-                  ;; need to wrap such a query in thread:{} before passing it
-                  ;; to notmuch-extract-patch(1), or we might not get a whole
-                  ;; thread extracted (e.g. if the query is just id:foo)
-                  (if (string= (substring thread-id 0 7) "thread:")
-                      thread-id
-                    (concat "thread:{" thread-id "}"))))
-            (call-process piem-notmuch-extract-patch-executable nil t nil
-                          tid)
-          (user-error "The executable notmuch-extract-patch was not found"))))))
+        (call-process piem-notmuch-extract-patch-executable nil t nil
+                      tid)))))
 
 (defun piem-notmuch-show-get-public-inbox-link (mid)
   "Given the message-id MID, return the public-inbox url.