Adding support for editing patches before applying them

Adding support for editing patches before applying them

[sourcehut]

Hello,

Sometimes it is necessary to edit patches before applying them. These changes
allows you to do that for notmuch only by grabbing the mbox string from an
*notmuch-mbox-<thread-id>* buffer if it exists, where <thread-id> is the thread
ID of the current notmuch message.

I've additionally included `piem-notmuch-show-view-mbox-message' which is a fork
of `notmuch-show-view-raw-message' that uses "--format=mbox" instead. It also
supports opening the entire thread via a prefix argument so that you can apply
the entire thread.

[PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[sourcehut]

From: <sourcehut@relevant-information.com>

---
 piem-notmuch.el | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/piem-notmuch.el b/piem-notmuch.el
index 8b2a353..2ad4ace 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -77,11 +77,35 @@ (defun piem-notmuch-mid-to-thread (mid)
                     "show" "--format=mbox" "--entire-thread=true"
                     (concat "id:" mid)))))
 
+(defun piem-notmuch-show-view-mbox-message (arg)
+  "View the mbox formatted version of the current message.
+
+Used for editing patches before applying them.
+
+With one universal prefix argument, view the entire thread instead. "
+  (interactive "P")
+  (let* ((id (notmuch-show-get-message-id))
+	 (buf (get-buffer-create (concat "*notmuch-mbox-" id "*")))
+	 (inhibit-read-only t))
+    (pop-to-buffer-same-window buf)
+    (erase-buffer)
+    (let ((coding-system-for-read 'no-conversion))
+      (if (equal '(4) arg)
+          (call-process notmuch-command nil t nil "show" "--entire-thread=true" "--format=mbox" id)
+        (call-process notmuch-command nil t nil "show" "--format=mbox" id)))
+    (goto-char (point-min))
+    (set-buffer-modified-p nil)
+    (setq buffer-read-only nil)
+    (view-buffer buf 'kill-buffer-if-not-modified)))
+
+
 (defun piem-notmuch-am-ready-mbox ()
   "Return a function that inserts an am-ready mbox.
 If the buffer has any MIME parts that look like a patch, use
 those parts' contents (in order) as the mbox.  Otherwise, use the
-message itself if it looks like a patch."
+message itself if it looks like a patch. If a corresponding
+*notmuch-mbox- buffer exists, use that instead of fetching from
+the notmuch database."
   (when (derived-mode-p 'notmuch-show-mode)
     (let* ((handle (piem-notmuch--with-current-message
                      (mm-dissect-buffer)))
@@ -90,10 +114,16 @@ (defun piem-notmuch-am-ready-mbox ()
       (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))))
+            (if-let ((buf (get-buffer (concat "*notmuch-mbox-" (notmuch-show-get-message-id) "*"))))
+                (let ((str (with-current-buffer buf
+                             (buffer-substring-no-properties (point-min)
+                                                             (point-max)))))
+                  (lambda ()
+                    (insert str)))
+              (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)
            (when-let ((patch (piem-am-extract-attached-patch p)))
-- 
2.31.0

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Kyle Meyer]

[ I left inline comments as I read, but there's a higher-level comment
  about an alternative design at the end, so you might prefer to look at
  that first. ]

sourcehut@relevant-information.com writes:

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

The above will lead to Git repeating the email as the author name:

  sourcehut@relevant-information.com <sourcehut@relevant-information.com>

Are you okay with filling in a name here?  (It of course doesn't have to
be a real one.)

Also, I think it'd be good to have some of the motivation you gave in
your initial message here in the commit message.

> ---
>  piem-notmuch.el | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/piem-notmuch.el b/piem-notmuch.el
> index 8b2a353..2ad4ace 100644
> --- a/piem-notmuch.el
> +++ b/piem-notmuch.el
> @@ -77,11 +77,35 @@ (defun piem-notmuch-mid-to-thread (mid)
>                      "show" "--format=mbox" "--entire-thread=true"
>                      (concat "id:" mid)))))
>  
> +(defun piem-notmuch-show-view-mbox-message (arg)
> +  "View the mbox formatted version of the current message.
> +
> +Used for editing patches before applying them.
> +
> +With one universal prefix argument, view the entire thread instead. "

Please drop the trailing space at the end of the docstring.

> +  (interactive "P")
> +  (let* ((id (notmuch-show-get-message-id))
> +	 (buf (get-buffer-create (concat "*notmuch-mbox-" id "*")))

I know this buffer name is following notmuch-show-view-raw-message
(s/raw/mbox/), but, given the buffer's created by piem, I'd prefer to
tack on a "piem-" in front of "notmuch-mbox".

> +	 (inhibit-read-only t))

Even if this is a copy from the notmuch codebase, let's convert these
tabs to spaces for consistency.

> +    (pop-to-buffer-same-window buf)
> +    (erase-buffer)
> +    (let ((coding-system-for-read 'no-conversion))
> +      (if (equal '(4) arg)
> +          (call-process notmuch-command nil t nil "show" "--entire-thread=true" "--format=mbox" id)

Please reflow this line to be under 80 chars.  (Same comment applies to
a line in piem-notmuch-am-ready-mbox.)

> +        (call-process notmuch-command nil t nil "show" "--format=mbox" id)))
> +    (goto-char (point-min))
> +    (set-buffer-modified-p nil)
> +    (setq buffer-read-only nil)
> +    (view-buffer buf 'kill-buffer-if-not-modified)))

Hmm, the goal is to tweak the patch, so I understand setting
buffer-read-only to nil, but does that play well with View mode?  I
think using View pretty much assumes read-only operation, and, testing
out a call to piem-notmuch-show-view-mbox-message, it looks like
view-buffer will end up resetting buffer-read-only to t anyway.

> +
> +

Please reduce this to a single space...

>  (defun piem-notmuch-am-ready-mbox ()
>    "Return a function that inserts an am-ready mbox.
>  If the buffer has any MIME parts that look like a patch, use
>  those parts' contents (in order) as the mbox.  Otherwise, use the
> -message itself if it looks like a patch."
> +message itself if it looks like a patch. If a corresponding

... and add two spaces after periods to match the Emacs project
convention.

> +*notmuch-mbox- buffer exists, use that instead of fetching from
> +the notmuch database."
>    (when (derived-mode-p 'notmuch-show-mode)
>      (let* ((handle (piem-notmuch--with-current-message
>                       (mm-dissect-buffer)))
> @@ -90,10 +114,16 @@ (defun piem-notmuch-am-ready-mbox ()
>        (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))))
> +            (if-let ((buf (get-buffer (concat "*notmuch-mbox-" (notmuch-show-get-message-id) "*"))))
> +                (let ((str (with-current-buffer buf
> +                             (buffer-substring-no-properties (point-min)
> +                                                             (point-max)))))
> +                  (lambda ()
> +                    (insert str)))
> +              (let ((id (notmuch-show-get-message-id)))
> +                (lambda ()
> +                  (call-process notmuch-command nil t nil
> +                                "show" "--format=mbox" id)))))

Okay, so stepping back, I believe the proposed workflow is

  * from a notmuch message, invoke piem-notmuch-show-view-mbox-message

  * make changes to that new buffer

  * switch back to the notmuch message buffer

  * call piem-am, and underneath piem-notmuch-am-ready-mbox will grab
    the intended mbox based on the message-ID-matched buffer name

    (Note: If the piem-notmuch-show-view-mbox-message caller used a
     prefix argument to dump the entire thread, they need to make sure
     to prune any non-patch messages in the second step above because
     piem-notmuch-am-ready-mbox will use the mbox as is, and it should
     return an am-ready mbox.)

I wonder if we can redesign this a bit to more closely and transparently
connect the "edit" and "am" parts.  Something like:

  * from a notmuch message, invoke piem-notmuch-edit-message [*] (or
    whatever name)

    This commands puts the message (or perhaps thread) into a dedicated
    piem mode for editing, setting up any buffer-local variables piem
    needs for record-keeping/linking (e.g., what piem-inbox-coderepo
    returns in the notmuch message) and then ...

  * once the user is done editing, they can hit, say, 'C-c C-c' to call
    a wrapper command that feeds the mbox to piem-am

    (Perhaps there could also be a b4 variant.)

What do you think?


[*] And it might make sense to write the command in a way that it could
    eventually work with integration libraries too (e.g., Gnus), but
    tailoring things to notmuch initially is fine by me.

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Kyle Meyer]

Kyle Meyer writes:

> I wonder if we can redesign this a bit to more closely and transparently
> connect the "edit" and "am" parts.  Something like:
>
>   * from a notmuch message, invoke piem-notmuch-edit-message [*] (or
>     whatever name)
>
>     This commands puts the message (or perhaps thread) into a dedicated
>     piem mode for editing, setting up any buffer-local variables piem
>     needs for record-keeping/linking (e.g., what piem-inbox-coderepo
>     returns in the notmuch message) and then ...
>
>   * once the user is done editing, they can hit, say, 'C-c C-c' to call
>     a wrapper command that feeds the mbox to piem-am
>
>     (Perhaps there could also be a b4 variant.)

Here's a sketch of what that could look like.

diff --git a/Makefile b/Makefile
index b8d9fe6f..83635de6 100644
--- a/Makefile
+++ b/Makefile
@@ -4,8 +4,8 @@ EMACS   = emacs
 # Rely on EMACSLOADPATH for everything but the current directory.
 BATCH   = $(EMACS) --batch -Q -L . -L tests
 
-EL = piem.el piem-b4.el piem-elfeed.el piem-eww.el piem-gnus.el \
-     piem-lei.el piem-maildir.el piem-notmuch.el piem-rmail.el \
+EL = piem.el piem-b4.el piem-edit-patch.el piem-elfeed.el piem-eww.el \
+     piem-gnus.el piem-lei.el piem-maildir.el piem-notmuch.el piem-rmail.el \
      tests/piem-lei-tests.el tests/piem-rmail-tests.el tests/piem-tests.el
 ELC = $(EL:.el=.elc)
 
@@ -32,6 +32,7 @@ docs: Documentation/piem.html Documentation/piem.info
 		-c TOP_NODE_UP_URL=/ Documentation/piem.texi
 
 piem-b4.elc: piem-b4.el piem.elc
+piem-edit-patch.elc: piem-edit-patch.el piem-b4.elc piem.elc
 piem-elfeed.elc: piem-elfeed.el piem.elc
 piem-eww.elc: piem-eww.el piem.elc
 piem-gnus.elc: piem-gnus.el piem.elc
diff --git a/piem-edit-patch.el b/piem-edit-patch.el
new file mode 100644
index 00000000..9b309dec
--- /dev/null
+++ b/piem-edit-patch.el
@@ -0,0 +1,66 @@
+;;; piem-edit-patch.el --- TODO  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2021  all contributors <piem@inbox.kyleam.com>
+
+;; TODO
+
+;;; Code:
+
+(require 'piem)
+(require 'piem-b4)                      ; not used yet
+
+(define-derived-mode piem-edit-patch-mode nil "piem-edit-patch"
+  "TODO"
+  :group 'piem)
+
+(defvar piem-edit-patch-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (kbd "C-c C-c") #'piem-edit-patch-am)
+    map)
+  "TODO")
+
+(defvar-local piem-edit-patch--coderepo nil)
+(defvar-local piem-edit-patch--format nil)
+(defvar-local piem-edit-patch--mid nil)
+
+(defun piem-edit-patch-open (&optional entire-thread)
+  (interactive "P")
+  (if-let ((mid (or (piem-mid)
+                     (user-error
+                      "No message ID found for the current buffer")))
+           (fn (if entire-thread
+                   ;; NEEDSWORK: B4 isn't wired up, so caller would
+                   ;; need to prune non-patch messages.
+                   (run-hook-with-args-until-success
+                    'piem-mid-to-thread-functions mid)
+                 (run-hook-with-args-until-success
+                  'piem-am-ready-mbox-functions))))
+      (let ((coderepo (piem-inbox-coderepo))
+            (format "mboxrd"))
+        (when (member (cdr-safe fn) '("mbox" "mboxrd"))
+          (setq format (cdr fn))
+          (setq fn (car fn)))
+        (with-current-buffer (get-buffer-create
+                              (format "*piem-edit-patch-%s*" mid))
+          (erase-buffer)
+          (funcall fn)
+          (unless (/= (point-max) 1)
+            (error "Failed to generated mbox"))
+          (goto-char (point-min))
+          (piem-edit-patch-mode)
+          (setq piem-edit-patch--coderepo coderepo)
+          (setq piem-edit-patch--format format)
+          (setq piem-edit-patch--mid mid)
+          (pop-to-buffer (current-buffer))))
+    (user-error "Do not know how to generate patch for current buffer")))
+
+(defun piem-edit-patch-am (&optional toggle-worktree)
+  (interactive "P")
+  (piem-am (current-buffer)
+           piem-edit-patch--format
+           (piem-extract-mbox-info (current-buffer))
+           piem-edit-patch--coderepo
+           toggle-worktree))
+
+;;; piem-edit-patch.el ends here
+(provide 'piem-edit-patch)

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Leo]

Kyle Meyer <kyle@kyleam.com> writes:

> Are you okay with filling in a name here?  (It of course doesn't have to
> be a real one.)
>

Sure, let's go with Leo.

> Okay, so stepping back, I believe the proposed workflow is ...
>
>

Yes that is correct. Although I realize that it is pretty clunky, see below.

> I wonder if we can redesign this a bit to more closely and transparently
> connect the "edit" and "am" parts.  Something like:
>
>   * from a notmuch message, invoke piem-notmuch-edit-message [*] (or
>     whatever name)
>
>     This commands puts the message (or perhaps thread) into a dedicated
>     piem mode for editing, setting up any buffer-local variables piem
>     needs for record-keeping/linking (e.g., what piem-inbox-coderepo
>     returns in the notmuch message) and then ...
>
>   * once the user is done editing, they can hit, say, 'C-c C-c' to call
>     a wrapper command that feeds the mbox to piem-am
>
>     (Perhaps there could also be a b4 variant.)
>
> What do you think?
>

I actually thought something similar a while after I sent the patch.
Instead of editing the buffer generated by notmuch we could use the
buffer returned by `piem-am-ready-mbox'. That way we get all the
libraries for free. Perhaps a new command `piem-am-edit' could be added
that calls `piem-am-ready-mbox' with your suggested dedicated piem mode
for editing. Would it be a good idea to filter out email headers that
`git am' doesn't care about here? Otherwise it can be a bit overwhelming.



--
---Leo

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Leo]

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

> I actually thought something similar a while after I sent the patch.
> Instead of editing the buffer generated by notmuch we could use the
> buffer returned by `piem-am-ready-mbox'. That way we get all the
> libraries for free. Perhaps a new command `piem-am-edit' could be added
> that calls `piem-am-ready-mbox' with your suggested dedicated piem mode
> for editing. Would it be a good idea to filter out email headers that
> `git am' doesn't care about here? Otherwise it can be a bit overwhelming.
>
>
>
Here is a sketch based on the sketch you sent. The workflow being: 1)
call `piem-edit' 2) edit the mbox buffer 3) call `piem-edit-patch-am'.


---
 piem.el | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/piem.el b/piem.el
index afe6b7a..807de65 100644
--- a/piem.el
+++ b/piem.el
@@ -900,6 +900,35 @@ (defun piem-am-read-worktree (coderepo branch)
           (concat (file-name-nondirectory fname) "-"
                   (replace-regexp-in-string "/" "-" branch))))))
 
+(defvar-local piem-edit-patch--coderepo nil)
+(defvar-local piem-edit-patch--format nil)
+(defvar-local piem-edit-patch--mid nil)
+
+(defun piem-edit-patch-am ()
+  (interactive)
+  (piem-am (current-buffer)
+       "mbox"
+       (piem-extract-mbox-info (current-buffer))
+       piem-edit-patch--coderepo))
+
+(define-derived-mode piem-edit-patch-mode nil "piem-edit-patch"
+  "TODO"
+  :group 'piem)
+
+(defvar piem-edit-patch-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (kbd "C-c C-c") #'piem-edit-patch-am)
+    map)
+  "TODO")
+
+(defun piem-edit ()
+  (interactive)
+  (piem-am-ready-mbox)
+  (let ((coderepo (piem-inbox-coderepo)))
+    (switch-to-buffer " *piem am-ready mbox*")
+    (piem-edit-patch-mode)
+    (setq piem-edit-patch--coderepo coderepo)))
+
 ;;;###autoload
 (defun piem-am (mbox &optional format info coderepo toggle-worktree)
   "Feed an am-ready mbox to `git am'.
-- 
2.31.0

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Kyle Meyer]

Leo writes:

> Leo <sourcehut@relevant-information.com> writes:
>
>> I actually thought something similar a while after I sent the patch.
>> Instead of editing the buffer generated by notmuch we could use the
>> buffer returned by `piem-am-ready-mbox'. That way we get all the
>> libraries for free. Perhaps a new command `piem-am-edit' could be added
>> that calls `piem-am-ready-mbox' with your suggested dedicated piem mode
>> for editing. Would it be a good idea to filter out email headers that
>> `git am' doesn't care about here? Otherwise it can be a bit overwhelming.

Yeah, filtering headers (and perhaps positioning point) sounds like a
good idea.  Note sure if any built-in Emacs libraries have something to
use or at least follow here.

> Here is a sketch based on the sketch you sent. The workflow being: 1)
> call `piem-edit' 2) edit the mbox buffer 3) call `piem-edit-patch-am'.

Nice, seems like we're looking in the same general direction.  (I
unfortunately don't have time tonight to give a more substantial reply
to this or your other thread.)

Is this topic something you'd like to flesh out and polish?  (No worries
if not.)

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Leo]

Kyle Meyer <kyle@kyleam.com> writes:

> Yeah, filtering headers (and perhaps positioning point) sounds like a
> good idea.  Note sure if any built-in Emacs libraries have something to
> use or at least follow here.
>
> Nice, seems like we're looking in the same general direction.  (I
> unfortunately don't have time tonight to give a more substantial reply
> to this or your other thread.)
>
> Is this topic something you'd like to flesh out and polish?  (No worries
> if not.)

I think the patch I sent should work well, at least it did with my
limited testing. I don't have much time to work on this either, but I
also don't think it's much work left to do:

1) The naming of identifiers could probably be improved
2) The keybinding is currently not bound, but executing the command via
  M-x works
3) Filtering is not implemented. This doesn't seem urgent to me and could
  be implemented later.


Cheers,

--
---Leo

Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them

[Kyle Meyer]

Leo writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
[...]
>> Is this topic something you'd like to flesh out and polish?  (No worries
>> if not.)
>
> I think the patch I sent should work well, at least it did with my
> limited testing. I don't have much time to work on this either, but I
> also don't think it's much work left to do:

All right, then, I'll look forward to your patch.  Thanks.

[PATCH] Add ability to edit patches before applying them

[sourcehut]

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

Sometimes it is necessary to edit patches before applying them.  These changes
allows you to do that by providing a new command `piem-edit` that
shows the buffer that is prepared by `piem-am-ready-mbox` to the user.
---
Alright, here is the patch!

 piem.el | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/piem.el b/piem.el
index afe6b7a..668f282 100644
--- a/piem.el
+++ b/piem.el
@@ -900,6 +900,40 @@ (defun piem-am-read-worktree (coderepo branch)
           (concat (file-name-nondirectory fname) "-"
                   (replace-regexp-in-string "/" "-" branch))))))
 
+(defvar-local piem-edit-patch--coderepo nil)
+
+(defun piem-edit ()
+  "Edit an am-ready mbox before feeding it to `git am'."
+  (interactive)
+  (pcase-let ((`(,mbox . ,_)
+               (or (piem-am-ready-mbox)
+                   (user-error
+                    "Could not find am-ready mbox for current buffer")))
+              (coderepo (piem-inbox-coderepo)))
+    (progn
+      (switch-to-buffer mbox)
+      (piem-edit-patch-mode)
+      (setq piem-edit-patch--coderepo coderepo))))
+
+(defun piem-edit-patch-am ()
+  "Apply the patch that is currently edited."
+  (interactive)
+  (piem-am (current-buffer)
+       "mbox"
+       (piem-extract-mbox-info (current-buffer))
+       piem-edit-patch--coderepo))
+
+(defvar piem-edit-patch-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (kbd "C-c C-c") #'piem-edit-patch-am)
+    map)
+  "Keymap for editing patches with piem.")
+
+(define-derived-mode piem-edit-patch-mode text-mode  "piem-edit-patch"
+  "Major mode for editing patches with piem."
+  :group 'piem
+  :after-hook (buffer-enable-undo))
+
 ;;;###autoload
 (defun piem-am (mbox &optional format info coderepo toggle-worktree)
   "Feed an am-ready mbox to `git am'.

base-commit: e519aa44d148d5b8f22d7fe8844dc566046b04c2
-- 
2.34.1

Re: [PATCH] Add ability to edit patches before applying them

[Kyle Meyer]

sourcehut@relevant-information.com writes:

> From: Leo <sourcehut@relevant-information.com>
>
> Sometimes it is necessary to edit patches before applying them.  These changes
> allows you to do that by providing a new command `piem-edit` that
> shows the buffer that is prepared by `piem-am-ready-mbox` to the user.
> ---
> Alright, here is the patch!

Thanks, I don't have time tonight to play around with this, but this is
looking good.  Here are some comments from a quick read.

>  piem.el | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/piem.el b/piem.el
> index afe6b7a..668f282 100644
> --- a/piem.el
> +++ b/piem.el
> @@ -900,6 +900,40 @@ (defun piem-am-read-worktree (coderepo branch)
>            (concat (file-name-nondirectory fname) "-"
>                    (replace-regexp-in-string "/" "-" branch))))))
>  
> +(defvar-local piem-edit-patch--coderepo nil)
> +
> +(defun piem-edit ()
> +  "Edit an am-ready mbox before feeding it to `git am'."
> +  (interactive)
> +  (pcase-let ((`(,mbox . ,_)

The ignored return value above should be passed as the format argument
to piem-am.

> +               (or (piem-am-ready-mbox)

I like the idea of using piem-am-ready-mbox, but it generates a hidden
buffer name (due to the leading space), which is inconvenient/confusing
for a user-facing buffer.  Perhaps piem-am-ready-mbox should gain an
optional argument to allow the buffer name to be specified and a
dedicated name should be used for this buffer ("*piem-edit-patch*"?).

> +                   (user-error
> +                    "Could not find am-ready mbox for current buffer")))
> +              (coderepo (piem-inbox-coderepo)))
> +    (progn

superfluous progn

> +      (switch-to-buffer mbox)

I'd prefer pop-to-buffer unless there's a specific reason to go with
switch-to-buffer.

> +      (piem-edit-patch-mode)
> +      (setq piem-edit-patch--coderepo coderepo))))
> +
> +(defun piem-edit-patch-am ()
> +  "Apply the patch that is currently edited."
> +  (interactive)
> +  (piem-am (current-buffer)
> +       "mbox"
> +       (piem-extract-mbox-info (current-buffer))
> +       piem-edit-patch--coderepo))

piem-edit-patch-am should probably take care of killing the buffer
(piem-am only does that for interactive calls).

> +(defvar piem-edit-patch-mode-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map (kbd "C-c C-c") #'piem-edit-patch-am)
> +    map)
> +  "Keymap for editing patches with piem.")
> +
> +(define-derived-mode piem-edit-patch-mode text-mode  "piem-edit-patch"

extra space after text-mode

> +  "Major mode for editing patches with piem."
> +  :group 'piem
> +  :after-hook (buffer-enable-undo))

Hmm, I don't see a reason for after-hook here versus the more standard
call in BODY.

And very much a nitpick, but I'd like if this code came after piem-am.

[PATCH v2] Add ability to edit patches before applying them

[sourcehut]

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

Sometimes it is necessary to edit patches before applying them.  These changes
allows you to do that by providing a new command `piem-edit` that
shows the buffer that is prepared by `piem-am-ready-mbox` to the user.
---
I wasn't sure if there were cases when `(kill-current-buffer)` could kill
the wrong buffer so I let-bound it to be safe.

I chose `generate-new-buffer` for the buffer name in
`piem-am-ready-mbox` because I thought sometimes the user could want to
open two edited patches side by side.  That is one of my annoyances with
mu4e anyways, not sure if it would actually be an issue in piem though.


 piem.el | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/piem.el b/piem.el
index afe6b7a..eef0649 100644
--- a/piem.el
+++ b/piem.el
@@ -597,14 +597,18 @@ (defun piem--insert-message-id-header (mid)
             ;; blank line.  Assume we're in a header.
             (insert (format "Message-Id: <%s>\n" mid))))))))
 
-(defun piem-am-ready-mbox ()
+(defun piem-am-ready-mbox (&optional buffer-name)
   "Generate a buffer containing an am-ready mbox.
 The return value is (BUFFER . FORMAT), where FORMAT is either
 \"mbox\" or \"mboxrd\".  Callers are responsible for killing the
-buffer."
+buffer.
+
+By default the buffer name is hidden, but when BUFFER-NAME is
+non-nil, use that name instead."
   (when-let ((res (run-hook-with-args-until-success
                    'piem-am-ready-mbox-functions)))
-    (pcase-let ((buffer (generate-new-buffer " *piem am-ready mbox*"))
+    (pcase-let ((buffer (generate-new-buffer
+                         (or buffer-name " *piem am-ready mbox*")))
                 (`(,fn . ,format)
                  (if (member (cdr-safe res) '("mbox" "mboxrd"))
                      res
@@ -981,6 +985,43 @@ (defun piem-am (mbox &optional format info coderepo toggle-worktree)
         (magit-status-setup-buffer am-directory)
       (dired am-directory))))
 
+(defvar-local piem-edit-patch--coderepo nil)
+(defvar-local piem-edit-patch--format nil)
+
+(defun piem-edit ()
+  "Edit an am-ready mbox before feeding it to `git am'."
+  (interactive)
+  (pcase-let ((`(,mbox . ,format)
+               (or (piem-am-ready-mbox "*piem-edit-patch*")
+                   (user-error
+                    "Could not find am-ready mbox for current buffer")))
+              (coderepo (piem-inbox-coderepo)))
+    (pop-to-buffer mbox)
+    (piem-edit-patch-mode)
+    (setq piem-edit-patch--coderepo coderepo)
+    (setq piem-edit-patch--format format)))
+
+(defun piem-edit-patch-am ()
+  "Apply the patch that is currently edited."
+  (interactive)
+  (let ((buf (current-buffer)))
+    (piem-am  buf
+          "mbox"
+          (piem-extract-mbox-info (current-buffer))
+          piem-edit-patch--coderepo))
+  (kill-buffer buf))
+
+(defvar piem-edit-patch-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (kbd "C-c C-c") #'piem-edit-patch-am)
+    map)
+  "Keymap for editing patches with piem.")
+
+(define-derived-mode piem-edit-patch-mode text-mode "piem-edit-patch"
+  "Major mode for editing patches with piem."
+  :group 'piem
+  (buffer-enable-undo))
+
 \f
 ;;;; Dispatch
 

base-commit: e519aa44d148d5b8f22d7fe8844dc566046b04c2
-- 
2.34.1

Re: [PATCH v2] Add ability to edit patches before applying them

[Kyle Meyer]

sourcehut@relevant-information.com writes:

> +(defun piem-edit-patch-am ()
> +  "Apply the patch that is currently edited."
> +  (interactive)
> +  (let ((buf (current-buffer)))
> +    (piem-am  buf
> +          "mbox"
> +          (piem-extract-mbox-info (current-buffer))
> +          piem-edit-patch--coderepo))
> +  (kill-buffer buf))

This last 'buf' is outside of the let-binding:

  $ make compile
  emacs --batch -Q -L . -L tests -f batch-byte-compile piem-maildir.el
  emacs --batch -Q -L . -L tests -f batch-byte-compile piem.el
  
  In piem-edit-patch-am:
  piem.el:1012:16:Warning: reference to free variable ‘buf’
  [...]

I've moved it inside (diff below).

Tested this out a bit, and it seems to work as advertised.  Pushed
(a996e8d4).  Thanks for proposing and implementing this feature.

diff --git a/piem.el b/piem.el
index eef06492..b06719b2 100644
--- a/piem.el
+++ b/piem.el
@@ -1005,11 +1005,11 @@ (defun piem-edit-patch-am ()
   "Apply the patch that is currently edited."
   (interactive)
   (let ((buf (current-buffer)))
-    (piem-am  buf
-          "mbox"
-          (piem-extract-mbox-info (current-buffer))
-          piem-edit-patch--coderepo))
-  (kill-buffer buf))
+    (piem-am buf
+             "mbox"
+             (piem-extract-mbox-info (current-buffer))
+             piem-edit-patch--coderepo)
+    (kill-buffer buf)))
 
 (defvar piem-edit-patch-mode-map
   (let ((map (make-sparse-keymap)))

Re: [PATCH v2] Add ability to edit patches before applying them

[Leo]

I must thank you for your patience with my sloppy elisp mistakes.  I
feel like I've learned a lot from submitting these patches thanks to
you.

By the way, is there any reason why this package hasn't been submitted
to elpa or melpa?  It's currently pretty difficult to find this package
despite its usefulness which seems like a shame to me.

distributing piem via *ELPA

[Kyle Meyer]

Leo writes:

> I must thank you for your patience with my sloppy elisp mistakes.  I
> feel like I've learned a lot from submitting these patches thanks to
> you.

Thanks for the kind words, and no worries about mistakes.  I appreciate
the interest and contributions.

> By the way, is there any reason why this package hasn't been submitted
> to elpa or melpa?  It's currently pretty difficult to find this package
> despite its usefulness which seems like a shame to me.

Yeah, that's a valid complaint.

I've come around to the view that in general software maintainers
shouldn't distribute their source code [1].  That feels natural to
someone like me that pulls in most of their packages from Guix (and
nearly everything else from Debian), but it's of course not the norm for
a lot of repositories, particularly language-specific ones.

So, while I don't plan to distribute piem myself, I'd be glad to see
others do so at some point.  Of course that depends on some critical
mass of users gaining interest even though, as you say, piem is pretty
difficult to discover.  I suppose there are other ways I could try to
increase its visibility (e.g., posting demos or info in places that get
more traffic), but that's not something I've done much of---just a few
mentions on Guix lists [2] and a pointer in public-inbox's
Documentation/clients.txt [3].

If you or someone else is interested in submitting piem to an *ELPA
repository, NonGNU ELPA [4] or MELPA would be the primary options, I
think.  ELPA proper is off the table because I don't want to require
copyright assignments from contributors.


[1] I think this post from Drew Default lays out the position well,
    though I can't say whether it's likely to be convincing to someone
    that doesn't already share a similar view:

    https://drewdevault.com/2019/12/09/Developers-shouldnt-distribute.html

[2] https://yhetil.org/guix/?q=nq%3Apiem

[3] https://80x24.org/public-inbox.git/commit/?id=c62ab5b7500bd9e7c584f5972e3c115842598957

[4] https://elpa.nongnu.org/

Re: distributing piem via *ELPA

[Kyle Meyer]

Kyle Meyer writes:

> [1] I think this post from Drew Default lays out the position well,

Bah, sorry for the typo: DeVault

Re: Adding support for editing patches before applying them

[Kyle Meyer]

sourcehut@relevant-information.com writes:

> Hello,
>
> Sometimes it is necessary to edit patches before applying them. These changes
> allows you to do that for notmuch only by grabbing the mbox string from an
> *notmuch-mbox-<thread-id>* buffer if it exists, where <thread-id> is the thread
> ID of the current notmuch message.
>
> I've additionally included `piem-notmuch-show-view-mbox-message' which is a fork
> of `notmuch-show-view-raw-message' that uses "--format=mbox" instead. It also
> supports opening the entire thread via a prefix argument so that you can apply
> the entire thread.

Interesting, thanks for the patch.

There are definitely times when I dump a message (or sometimes the
b4-am-processed thread) to a file to edit before applying, so I could
see myself using something like this.  (I tend to do that if something
like a filename in the patch needs to be tweaked.  For most edits, I
just touch up with git commands _after_ applying.)