discussion and development of piem
 help / color / mirror / Atom feed
* [PATCH] Explicitly specify --patch-format in git-am calls
@ 2020-08-10  2:07 Kyle Meyer
  2020-08-11  3:16 ` [PATCH] Fix handling of -am-ready-mbox values Kyle Meyer
  0 siblings, 1 reply; 2+ messages in thread
From: Kyle Meyer @ 2020-08-10  2:07 UTC (permalink / raw)
  To: piem

The sources of mbox patches fed to git-am are 1) threads downloaded
from a public-inbox HTTP instance, 2) mboxes generated via
piem-mid-to-thread-functions, and 3) those generated via
piem-am-ready-mbox-functions.  The first source should always be
mboxrd.  For the second, piem-notmuch-mid-to-thread is currently the
only function suitable for piem-mid-to-thread-functions, and it uses
mboxrd.  The third source is a mix between mbox and mboxrd.

By default, git-am tries to auto-detect the patch format, but let's
explicitly specify --patch-format to avoid any incorrect guesses.
---
 piem-b4.el      |  1 +
 piem-gnus.el    | 10 ++++----
 piem-notmuch.el |  7 +++---
 piem.el         | 61 ++++++++++++++++++++++++++++++-------------------
 4 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/piem-b4.el b/piem-b4.el
index d1ec157..3381b01 100644
--- a/piem-b4.el
+++ b/piem-b4.el
@@ -126,6 +126,7 @@ (defun piem-b4-am-from-mid (mid &optional args)
                 (piem-b4--get-am-files mid coderepo args))
                (default-directory coderepo))
     (piem-am mbox-file
+             nil
              (with-temp-buffer
                (insert-file-contents (or cover mbox-file))
                (piem-extract-mbox-info))
diff --git a/piem-gnus.el b/piem-gnus.el
index fcfb08e..9e42185 100644
--- a/piem-gnus.el
+++ b/piem-gnus.el
@@ -77,9 +77,10 @@ (defun piem-gnus-am-ready-mbox ()
                                      (point-min) (point-max)))))
                            gnus-article-mime-handles))))
         (when patches
-          (lambda ()
-            (dolist (patch patches)
-              (insert patch))))))
+          (list (lambda ()
+                  (dolist (patch patches)
+                    (insert patch)))
+                "mbox"))))
      (gnus-article-buffer
       (let ((patch (with-current-buffer gnus-article-buffer
                      (save-restriction
@@ -89,7 +90,8 @@ (defun piem-gnus-am-ready-mbox ()
                             (buffer-substring-no-properties
                              (point-min) (point-max)))))))
         (when patch
-          (lambda () (insert patch))))))))
+          (list (lambda () (insert patch))
+                "mbox")))))))
 
 ;;;###autoload
 (define-minor-mode piem-gnus-mode
diff --git a/piem-notmuch.el b/piem-notmuch.el
index 1e4dbb2..c9c3bed 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -88,9 +88,10 @@ (defun piem-notmuch-am-ready-mbox ()
                                      (plist-get part :content)))
                               (plist-get body :content)))))
            (when patches
-             (lambda ()
-               (dolist (patch patches)
-                 (insert patch))))))))))
+             (list (lambda ()
+                     (dolist (patch patches)
+                       (insert patch)))
+                   "mbox"))))))))
 
 ;;;###autoload
 (define-minor-mode piem-notmuch-mode
diff --git a/piem.el b/piem.el
index 6dc531d..e70c259 100644
--- a/piem.el
+++ b/piem.el
@@ -91,14 +91,17 @@ (defcustom piem-mid-to-thread-functions nil
 Each function should accept one argument, the message ID.  If the
 function knows how to create an mbox for the message ID, it
 should return a function that takes no arguments and inserts the
-mbox's contents in the current buffer."
+mbox's contents (in mboxrd format) in the current buffer."
   :type 'hook)
 
 (defcustom piem-am-ready-mbox-functions nil
   "Functions tried to get an mbox to be fed to `git am'.
 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."
+arguments and inserts the mbox's contents in the current buffer.
+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)
 
 (defcustom piem-add-message-id-header nil
@@ -333,13 +336,16 @@ (defun piem--insert-message-id-header (mid)
             (insert (format "Message-Id: <%s>\n" mid))))))))
 
 (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))
       (with-current-buffer buffer
         (funcall fn)
         (setq has-content (< 1 (point-max)))
@@ -347,7 +353,7 @@ (defun piem-am-ready-mbox ()
           (goto-char (point-min))
           (piem--insert-message-id-header mid)))
       (if has-content
-          buffer
+          (list buffer format)
         (kill-buffer buffer)
         nil))))
 
@@ -524,24 +530,31 @@ (defun piem-name-branch-who-what-v (info)
 (defvar piem-am-args (list "--scissors" "--3way"))
 
 ;;;###autoload
-(defun piem-am (mbox &optional info coderepo)
+(defun piem-am (mbox &optional format info coderepo)
   "Feed an am-ready mbox to `git am'.
 
 MBOX is a buffer whose contents are an am-ready mbox (obtained
-via `piem-am-ready-mbox' when called interactively).  INFO is a
-plist that with information to help choose a default branch name
-or starting point (see `piem-default-branch-function' for a list
-of possible properties).
+via `piem-am-ready-mbox' when called interactively).  FORMAT
+controls the value passed as the --patch-format option of `git
+am'.  \"mbox\" and \"mboxrd\" are valid values, and \"mboxrd\" is
+the default.
+
+INFO is a plist that with information to help choose a default
+branch name or starting point (see `piem-default-branch-function'
+for a list of possible properties).
 
 If CODEREPO is given, switch to this directory before calling
 `git am'."
   (interactive
-   (let ((mbox (or (piem-am-ready-mbox)
-                   (user-error
-                    "Could not find am-ready mbox for current buffer"))))
+   (pcase-let ((`(,mbox ,format)
+                (or (piem-am-ready-mbox)
+                    (user-error
+                     "Could not find am-ready mbox for current buffer"))))
      (list mbox
+           format
            (piem-extract-mbox-info mbox)
            (piem-inbox-coderepo-maybe-read))))
+  (setq format (or format "mboxrd"))
   (let ((default-directory (or coderepo default-directory)))
     (let ((new-branch (read-string
                        "New branch (empty for detached): "
@@ -559,11 +572,13 @@ (defun piem-am (mbox &optional info coderepo)
                        (list "-b" new-branch))
                      (and (not (string-blank-p base))
                           (list base)))))
-    (if (bufferp mbox)
-        (apply #'piem-process-call-with-buffer-input
-               nil mbox piem-git-executable "am" piem-am-args)
-      (apply #'piem-process-call nil piem-git-executable "am"
-             (append piem-am-args (list mbox))))
+    (let ((args (cons (concat "--patch-format=" format)
+                      piem-am-args)))
+      (if (bufferp mbox)
+          (apply #'piem-process-call-with-buffer-input
+                 nil mbox piem-git-executable "am" args)
+        (apply #'piem-process-call nil piem-git-executable "am"
+               (append args (list mbox)))))
     (if (and piem-use-magit
              (fboundp 'magit-status-setup-buffer))
         (magit-status-setup-buffer)

base-commit: 118643c2985c0113c179d734356fc1b9e082b4c5
-- 
2.27.0


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

* [PATCH] Fix handling of -am-ready-mbox values
  2020-08-10  2:07 [PATCH] Explicitly specify --patch-format in git-am calls Kyle Meyer
@ 2020-08-11  3:16 ` Kyle Meyer
  0 siblings, 0 replies; 2+ messages in thread
From: Kyle Meyer @ 2020-08-11  3:16 UTC (permalink / raw)
  To: piem

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


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

end of thread, other threads:[~2020-08-11  3:16 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-10  2:07 [PATCH] Explicitly specify --patch-format in git-am calls Kyle Meyer
2020-08-11  3:16 ` [PATCH] Fix handling of -am-ready-mbox values Kyle Meyer

discussion and development of piem

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.kyleam.com/piem/0 piem/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 piem piem/ https://inbox.kyleam.com/piem \
		piem@inbox.kyleam.com
	public-inbox-index piem

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.piem


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git