discussion and development of piem
 help / color / mirror / code / Atom feed
From: Kyle Meyer <kyle@kyleam.com>
To: piem@inbox.kyleam.com
Subject: [PATCH] Fix handling of -am-ready-mbox values
Date: Mon, 10 Aug 2020 23:16:32 -0400	[thread overview]
Message-ID: <877du5c1nz.fsf@kyleam.com> (raw)
In-Reply-To: <20200810020704.30150-1-kyle@kyleam.com>

Kyle Meyer writes:

>  (defun piem-am-ready-mbox ()
> -  "Return buffer containing an am-ready mbox.
> -Callers are responsible for killing the buffer."
> -  (when-let ((fn (run-hook-with-args-until-success
> -                  'piem-am-ready-mbox-functions)))
> -    (let ((buffer (generate-new-buffer " *piem am-ready mbox*"))
> -          (mid (and piem-add-message-id-header (piem-mid)))
> -          has-content)
> +  "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."
> +  (when-let ((res (run-hook-with-args-until-success
> +                   'piem-am-ready-mbox-functions)))
> +    (pcase-let ((buffer (generate-new-buffer " *piem am-ready mbox*"))
> +                (`(,fn ,format) (if (listp res) res (list res "mboxrd")))
> +                (mid (and piem-add-message-id-header (piem-mid)))
> +                (has-content nil))

Bah, caught only after applying, but this logic is flawed.  res is a
list even if it doesn't include the format.  The issue shows up in the
text/plain code path of piem-notmuch-am-ready-mbox because it doesn't
explicitly specify a format, relying on mboxrd being the default.

-- >8 --
Subject: [PATCH] Fix handling of -am-ready-mbox values

0ee97e9 (Explicitly specify --patch-format in git-am calls,
2020-08-09) made it possible for a piem-am-ready-mbox-functions member
to specify the format of the mbox by returning (FUNCTION FORMAT).  If
FUNCTION is returned, then mboxrd is supposed to be taken as the
default format.  The handling is broken, though, because
piem-am-ready-mbox tries to detect the (FUNCTION FORMAT) form with
listp, but that of course also returns true when the return value is
simply a function.

Instead, check to see whether the element matches a valid format
value.  Switch from (FUNCTION FORMAT) to (FUNCTION . FORMAT) to make
it more convenient to pull out FORMAT with cdr-safe.
---
 piem-gnus.el    |  4 ++--
 piem-notmuch.el |  2 +-
 piem.el         | 13 ++++++++-----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/piem-gnus.el b/piem-gnus.el
index 9e42185..244c389 100644
--- a/piem-gnus.el
+++ b/piem-gnus.el
@@ -77,7 +77,7 @@ (defun piem-gnus-am-ready-mbox ()
                                      (point-min) (point-max)))))
                            gnus-article-mime-handles))))
         (when patches
-          (list (lambda ()
+          (cons (lambda ()
                   (dolist (patch patches)
                     (insert patch)))
                 "mbox"))))
@@ -90,7 +90,7 @@ (defun piem-gnus-am-ready-mbox ()
                             (buffer-substring-no-properties
                              (point-min) (point-max)))))))
         (when patch
-          (list (lambda () (insert patch))
+          (cons (lambda () (insert patch))
                 "mbox")))))))
 
 ;;;###autoload
diff --git a/piem-notmuch.el b/piem-notmuch.el
index c9c3bed..29f4d27 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -88,7 +88,7 @@ (defun piem-notmuch-am-ready-mbox ()
                                      (plist-get part :content)))
                               (plist-get body :content)))))
            (when patches
-             (list (lambda ()
+             (cons (lambda ()
                      (dolist (patch patches)
                        (insert patch)))
                    "mbox"))))))))
diff --git a/piem.el b/piem.el
index e70c259..ece30bd 100644
--- a/piem.el
+++ b/piem.el
@@ -99,7 +99,7 @@ (defcustom piem-am-ready-mbox-functions nil
 The functions are called with no arguments.  If a function knows
 how to create an mbox, it should return a function that takes no
 arguments and inserts the mbox's contents in the current buffer.
-The return value can also be (FUNCTION FORMAT), where FORMAT is
+The return value can also be (FUNCTION . FORMAT), where FORMAT is
 either \"mbox\" or \"mboxrd\" and maps to the --patch-format
 value passed to `git am'.  If unspecified, \"mboxrd\" is used."
   :type 'hook)
@@ -337,13 +337,16 @@ (defun piem--insert-message-id-header (mid)
 
 (defun piem-am-ready-mbox ()
   "Generate a buffer containing an am-ready mbox.
-The return value is (BUFFER FORMAT), where FORMAT is either
+The return value is (BUFFER . FORMAT), where FORMAT is either
 \"mbox\" or \"mboxrd\".  Callers are responsible for killing the
 buffer."
   (when-let ((res (run-hook-with-args-until-success
                    'piem-am-ready-mbox-functions)))
     (pcase-let ((buffer (generate-new-buffer " *piem am-ready mbox*"))
-                (`(,fn ,format) (if (listp res) res (list res "mboxrd")))
+                (`(,fn . ,format)
+                 (if (member (cdr-safe res) '("mbox" "mboxrd"))
+                     res
+                   (cons res "mboxrd")))
                 (mid (and piem-add-message-id-header (piem-mid)))
                 (has-content nil))
       (with-current-buffer buffer
@@ -353,7 +356,7 @@ (defun piem-am-ready-mbox ()
           (goto-char (point-min))
           (piem--insert-message-id-header mid)))
       (if has-content
-          (list buffer format)
+          (cons buffer format)
         (kill-buffer buffer)
         nil))))
 
@@ -546,7 +549,7 @@ (defun piem-am (mbox &optional format info coderepo)
 If CODEREPO is given, switch to this directory before calling
 `git am'."
   (interactive
-   (pcase-let ((`(,mbox ,format)
+   (pcase-let ((`(,mbox . ,format)
                 (or (piem-am-ready-mbox)
                     (user-error
                      "Could not find am-ready mbox for current buffer"))))

base-commit: 0ee97e900f7daef3995284d0b4312c371b3427fa
-- 
2.28.0


      reply	other threads:[~2020-08-11  3:16 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-10  2:07 [PATCH] Explicitly specify --patch-format in git-am calls Kyle Meyer
2020-08-11  3:16 ` Kyle Meyer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://git.kyleam.com/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877du5c1nz.fsf@kyleam.com \
    --to=kyle@kyleam.com \
    --cc=piem@inbox.kyleam.com \
    --subject='Re: [PATCH] Fix handling of -am-ready-mbox values' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this 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 NNTP newsgroup(s).