discussion and development of piem
 help / color / mirror / code / Atom feed
* [PATCH v2] Use notmuch-extract-patch if available
@ 2021-12-14 15:09 leo
  2021-12-15  5:16 ` Kyle Meyer
  0 siblings, 1 reply; 9+ messages in thread
From: leo @ 2021-12-14 15:09 UTC (permalink / raw)
  To: piem; +Cc: 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Use notmuch-extract-patch if available
  2021-12-14 15:09 [PATCH v2] Use notmuch-extract-patch if available leo
@ 2021-12-15  5:16 ` Kyle Meyer
  2021-12-15  5:22   ` Kyle Meyer
  2021-12-16 19:32   ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch sourcehut
  0 siblings, 2 replies; 9+ messages in thread
From: Kyle Meyer @ 2021-12-15  5:16 UTC (permalink / raw)
  To: leo; +Cc: piem

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Use notmuch-extract-patch if available
  2021-12-15  5:16 ` Kyle Meyer
@ 2021-12-15  5:22   ` Kyle Meyer
  2021-12-16 19:32   ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch sourcehut
  1 sibling, 0 replies; 9+ messages in thread
From: Kyle Meyer @ 2021-12-15  5:22 UTC (permalink / raw)
  To: leo; +Cc: piem

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch
  2021-12-15  5:16 ` Kyle Meyer
  2021-12-15  5:22   ` Kyle Meyer
@ 2021-12-16 19:32   ` sourcehut
  2021-12-16 19:32     ` [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch sourcehut
  2021-12-17  5:15     ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch Kyle Meyer
  1 sibling, 2 replies; 9+ messages in thread
From: sourcehut @ 2021-12-16 19:32 UTC (permalink / raw)
  To: piem; +Cc: 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.
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch
  2021-12-16 19:32   ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch sourcehut
@ 2021-12-16 19:32     ` sourcehut
  2021-12-17  5:22       ` Kyle Meyer
  2021-12-17  5:15     ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch Kyle Meyer
  1 sibling, 1 reply; 9+ messages in thread
From: sourcehut @ 2021-12-16 19:32 UTC (permalink / raw)
  To: piem; +Cc: Leo

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch
  2021-12-16 19:32   ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch sourcehut
  2021-12-16 19:32     ` [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch sourcehut
@ 2021-12-17  5:15     ` Kyle Meyer
  1 sibling, 0 replies; 9+ messages in thread
From: Kyle Meyer @ 2021-12-17  5:15 UTC (permalink / raw)
  To: sourcehut; +Cc: piem

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).

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch
  2021-12-16 19:32     ` [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch sourcehut
@ 2021-12-17  5:22       ` Kyle Meyer
  2021-12-21 19:15         ` [PATCH v4] " Leo
  0 siblings, 1 reply; 9+ messages in thread
From: Kyle Meyer @ 2021-12-17  5:22 UTC (permalink / raw)
  To: sourcehut; +Cc: piem

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4] Add user option for specifying path to notmuch-extract-patch
  2021-12-17  5:22       ` Kyle Meyer
@ 2021-12-21 19:15         ` Leo
  2021-12-24 18:31           ` Kyle Meyer
  0 siblings, 1 reply; 9+ messages in thread
From: Leo @ 2021-12-21 19:15 UTC (permalink / raw)
  To: piem; +Cc: 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] Add user option for specifying path to notmuch-extract-patch
  2021-12-21 19:15         ` [PATCH v4] " Leo
@ 2021-12-24 18:31           ` Kyle Meyer
  0 siblings, 0 replies; 9+ messages in thread
From: Kyle Meyer @ 2021-12-24 18:31 UTC (permalink / raw)
  To: Leo; +Cc: piem

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.

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-12-24 18:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-14 15:09 [PATCH v2] Use notmuch-extract-patch if available leo
2021-12-15  5:16 ` Kyle Meyer
2021-12-15  5:22   ` Kyle Meyer
2021-12-16 19:32   ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch sourcehut
2021-12-16 19:32     ` [PATCH v3 2/2] Add user option for specifying path to notmuch-extract-patch sourcehut
2021-12-17  5:22       ` Kyle Meyer
2021-12-21 19:15         ` [PATCH v4] " Leo
2021-12-24 18:31           ` Kyle Meyer
2021-12-17  5:15     ` [PATCH v3 1/2] Support preparing am-ready mbox via notmuch-extract-patch Kyle Meyer

Code repositories for project(s) associated with this public inbox

	https://git.kyleam.com/piem/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).