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

[Kyle Meyer <kyle@kyleam.com>]

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