[BUG/PATCH v2 0/2] emacs: Yesterday's bugfixes

[BUG/PATCH v2 0/2] emacs: Yesterday's bugfixes

[Adam Wolfe Gordon]

This is last night's two bugfixes [1] [2], combined into one series since they
won't both apply cleanly separately.

The only change to the fix for emacs 23.2 is the name of the function
notmuch-plist-to-alist is now notmuch-header-plist-to-alist, which more
accurately reflects what it does.

The References fix has been changed to use the existing formatter for
References instead of hard-coding message-shorten-references, and to keep the
headers in the same order they're in by default.

[1] id:"1332943338-9708-1-git-send-email-awg+notmuch@xvx.ca"
[2] id:"1332996818-15700-1-git-send-email-awg+notmuch@xvx.ca"

Adam Wolfe Gordon (2):
  emacs: Fix header problem in reply for emacs 23.2
  emacs: Fix the References header in reply

 emacs/notmuch-lib.el |    7 +++++--
 emacs/notmuch-mua.el |   38 ++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 10 deletions(-)

-- 
1.7.5.4

[BUG/PATCH v2 1/2] emacs: Fix header problem in reply for emacs 23.2

[Adam Wolfe Gordon]

The new reply code used strings instead of symbols for header names,
which message-mail is OK with on emacs 23.3, but not 23.2. The symptom
is that on 23.2 (and presumably on earlier versions) the reply message
would end up with two of some headers.

This fixes the problem by converting the header names to symbols of
the type message.el usually expects before passing the headers to
message-mail.
---
 emacs/notmuch-lib.el |    7 +++++--
 emacs/notmuch-mua.el |   12 ++++++------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index c146748..0effe24 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -232,9 +232,12 @@ the given type."
   (or (plist-get part :content)
       (notmuch-get-bodypart-internal (concat "id:" (plist-get msg :id)) nth process-crypto)))
 
-(defun notmuch-plist-to-alist (plist)
+;; Converts a plist of headers to an alist of headers. The input plist should
+;; have symbols of the form :Header as keys, and the resulting alist will have
+;; symbols of the form 'Header as keys.
+(defun notmuch-headers-plist-to-alist (plist)
   (loop for (key value . rest) on plist by #'cddr
-	collect (cons (substring (symbol-name key) 1) value)))
+	collect (cons (intern (substring (symbol-name key) 1)) value)))
 
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 9805d79..cfa3d61 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -127,7 +127,7 @@ list."
 	  ((same-window-regexps '("\\*mail .*")))
 	(notmuch-mua-mail (plist-get reply-headers :To)
 			  (plist-get reply-headers :Subject)
-			  (notmuch-plist-to-alist reply-headers)))
+			  (notmuch-headers-plist-to-alist reply-headers)))
       ;; Insert the message body - but put it in front of the signature
       ;; if one is present
       (goto-char (point-max))
@@ -185,11 +185,11 @@ OTHER-ARGS are passed through to `message-mail'."
   (when notmuch-mua-user-agent-function
     (let ((user-agent (funcall notmuch-mua-user-agent-function)))
       (when (not (string= "" user-agent))
-	(push (cons "User-Agent" user-agent) other-headers))))
+	(push (cons 'User-Agent user-agent) other-headers))))
 
-  (unless (assoc "From" other-headers)
-    (push (cons "From" (concat
-			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-headers))
+  (unless (assq 'From other-headers)
+    (push (cons 'From (concat
+		       (notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-headers))
 
   (apply #'message-mail to subject other-headers other-args)
   (message-sort-headers)
@@ -250,7 +250,7 @@ the From: address first."
   (interactive "P")
   (let ((other-headers
 	 (when (or prompt-for-sender notmuch-always-prompt-for-sender)
-	   (list (cons "From" (notmuch-mua-prompt-for-sender))))))
+	   (list (cons 'From (notmuch-mua-prompt-for-sender))))))
     (notmuch-mua-mail nil nil other-headers)))
 
 (defun notmuch-mua-new-forward-message (&optional prompt-for-sender)
-- 
1.7.5.4

Re: [BUG/PATCH v2 1/2] emacs: Fix header problem in reply for emacs 23.2

[Mark Walters]

Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:

> The new reply code used strings instead of symbols for header names,
> which message-mail is OK with on emacs 23.3, but not 23.2. The symptom
> is that on 23.2 (and presumably on earlier versions) the reply message
> would end up with two of some headers.
>
> This fixes the problem by converting the header names to symbols of
> the type message.el usually expects before passing the headers to
> message-mail.

A few minor comments:

I think this code applies on top of   
  id:"1332941635-21019-2-git-send-email-awg+notmuch@xvx.ca" and 
  id:"1332941635-21019-3-git-send-email-awg+notmuch@xvx.ca"

Secondly it seems a little odd that the second patch above changes the
header stuff from 'From to "From" and then this patch changes it back
again. This is probably only a matter of a tidy history but I have to
admit I am confused about why the reply from alternate address still
works (but it does seem to)

Finally, there is a trailing white-space in the second patch in this
series.

Best wishes

Mark

> ---
>  emacs/notmuch-lib.el |    7 +++++--
>  emacs/notmuch-mua.el |   12 ++++++------
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index c146748..0effe24 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -232,9 +232,12 @@ the given type."
>    (or (plist-get part :content)
>        (notmuch-get-bodypart-internal (concat "id:" (plist-get msg :id)) nth process-crypto)))
>  
> -(defun notmuch-plist-to-alist (plist)
> +;; Converts a plist of headers to an alist of headers. The input plist should
> +;; have symbols of the form :Header as keys, and the resulting alist will have
> +;; symbols of the form 'Header as keys.
> +(defun notmuch-headers-plist-to-alist (plist)
>    (loop for (key value . rest) on plist by #'cddr
> -	collect (cons (substring (symbol-name key) 1) value)))
> +	collect (cons (intern (substring (symbol-name key) 1)) value)))
>  
>  ;; Compatibility functions for versions of emacs before emacs 23.
>  ;;
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 9805d79..cfa3d61 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -127,7 +127,7 @@ list."
>  	  ((same-window-regexps '("\\*mail .*")))
>  	(notmuch-mua-mail (plist-get reply-headers :To)
>  			  (plist-get reply-headers :Subject)
> -			  (notmuch-plist-to-alist reply-headers)))
> +			  (notmuch-headers-plist-to-alist reply-headers)))
>        ;; Insert the message body - but put it in front of the signature
>        ;; if one is present
>        (goto-char (point-max))
> @@ -185,11 +185,11 @@ OTHER-ARGS are passed through to `message-mail'."
>    (when notmuch-mua-user-agent-function
>      (let ((user-agent (funcall notmuch-mua-user-agent-function)))
>        (when (not (string= "" user-agent))
> -	(push (cons "User-Agent" user-agent) other-headers))))
> +	(push (cons 'User-Agent user-agent) other-headers))))
>  
> -  (unless (assoc "From" other-headers)
> -    (push (cons "From" (concat
> -			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-headers))
> +  (unless (assq 'From other-headers)
> +    (push (cons 'From (concat
> +		       (notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-headers))
>  
>    (apply #'message-mail to subject other-headers other-args)
>    (message-sort-headers)
> @@ -250,7 +250,7 @@ the From: address first."
>    (interactive "P")
>    (let ((other-headers
>  	 (when (or prompt-for-sender notmuch-always-prompt-for-sender)
> -	   (list (cons "From" (notmuch-mua-prompt-for-sender))))))
> +	   (list (cons 'From (notmuch-mua-prompt-for-sender))))))
>      (notmuch-mua-mail nil nil other-headers)))
>  
>  (defun notmuch-mua-new-forward-message (&optional prompt-for-sender)
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [BUG/PATCH v2 1/2] emacs: Fix header problem in reply for emacs 23.2

[Adam Wolfe Gordon]

Hi Mark,

On Sat, Mar 31, 2012 at 03:13, Mark Walters <markwalters1009@gmail.com> wrote:
> Secondly it seems a little odd that the second patch above changes the
> header stuff from 'From to "From" and then this patch changes it back
> again. This is probably only a matter of a tidy history but I have to
> admit I am confused about why the reply from alternate address still
> works (but it does seem to)

Good question. These three bugs (alternate address, References header,
double headers in 23.2) basically stem from two problems: poor
interactions between notmuch and message.el, and inconsistency in
using strings and symbols for header names.

All the message.el functions usually assume symbols for header names,
but when I was writing the new reply stuff I found it easier to use
strings. In emacs 23.3, message.el is happy to accept strings. The
alternate address bug was caused by the fact that we were using
strings in one place and symbols in another, and 'From isn't equal to
"From". Rather than changing everything to symbols, I just changed the
one place we were using symbols to use strings.

But, it turns out that emacs 23.2 isn't happy with strings, which
caused the double headers bug. So I ended up converting everything to
use symbols. Because we're now consistent in using symbols, and never
pass strings to message.el, the alternate address bug is still fixed.

All that's a long way of saying: the fix for emacs 23.2 should
actually fix the alternate reply bug as well. This probably means I
could drop the alternate reply bug patch, and rebase the other two
onto master, and we'd still have all the bugs fixed. Is that
preferable, to reduce the number of code changes in the history?

-- Adam

[BUG/PATCH v2 2/2] emacs: Fix the References header in reply

[Adam Wolfe Gordon]

In the new reply code, the References header gets inserted by
message.el using a function called message-shorten-references. Unlike
all the other header-inserting functions, it doesn't put a newline
after the header, causing the next header to end up on the same
line. In our case, this header happened to be User-Agent, so it's hard
to notice. This is probably a bug in message.el, but we need to work
around it.

This fixes the problem by wrapping message-shorten-references in a
function that inserts a newline after if necessary. This should
protect against the message.el bug being fixed in the future.
---
 emacs/notmuch-mua.el |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index cfa3d61..9f279d9 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -90,6 +90,15 @@ list."
 	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
 	  collect part))
 
+;; There is a bug in emacs 23's message.el that results in a newline
+;; not being inserted after the References header, so the next header
+;; is concatenated to the end of it. This function fixes the problem,
+;; while guarding against the possibility that some current or future
+;; version of emacs has the bug fixed.
+(defun notmuch-mua-insert-references (original-func header references)
+  (funcall original-func header references)
+  (unless (bolp) (insert "\n")))
+
 (defun notmuch-mua-reply (query-string &optional sender reply-all)
   (let ((args '("reply" "--format=json"))
 	reply
@@ -125,9 +134,22 @@ list."
 	  ;; Overlay the composition window on that being used to read
 	  ;; the original message.
 	  ((same-window-regexps '("\\*mail .*")))
-	(notmuch-mua-mail (plist-get reply-headers :To)
-			  (plist-get reply-headers :Subject)
-			  (notmuch-headers-plist-to-alist reply-headers)))
+
+	;; We modify message-header-format-alist to get around a bug in message.el.
+	;; See the comment above on notmuch-mua-insert-references.
+	(let ((message-header-format-alist
+	       (loop for pair in message-header-format-alist
+		     if (eq (car pair) 'References)
+		     collect (cons 'References 
+				   (apply-partially
+				    'notmuch-mua-insert-references
+				    (cdr pair)))
+		     else
+		     collect pair)))
+	  (notmuch-mua-mail (plist-get reply-headers :To)
+			    (plist-get reply-headers :Subject)
+			    (notmuch-headers-plist-to-alist reply-headers))))
+
       ;; Insert the message body - but put it in front of the signature
       ;; if one is present
       (goto-char (point-max))
-- 
1.7.5.4

Re: [BUG/PATCH v2 0/2] emacs: Yesterday's bugfixes

[Austin Clements]

On Thu, 29 Mar 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> This is last night's two bugfixes [1] [2], combined into one series since they
> won't both apply cleanly separately.
>
> The only change to the fix for emacs 23.2 is the name of the function
> notmuch-plist-to-alist is now notmuch-header-plist-to-alist, which more
> accurately reflects what it does.
>
> The References fix has been changed to use the existing formatter for
> References instead of hard-coding message-shorten-references, and to keep the
> headers in the same order they're in by default.
>
> [1] id:"1332943338-9708-1-git-send-email-awg+notmuch@xvx.ca"
> [2] id:"1332996818-15700-1-git-send-email-awg+notmuch@xvx.ca"
>
> Adam Wolfe Gordon (2):
>   emacs: Fix header problem in reply for emacs 23.2
>   emacs: Fix the References header in reply
>
>  emacs/notmuch-lib.el |    7 +++++--
>  emacs/notmuch-mua.el |   38 ++++++++++++++++++++++++++++++--------
>  2 files changed, 35 insertions(+), 10 deletions(-)

LGTM.

To keep everything in one thread, Jamie also signed off this patch in
id:"87obrf48ci.fsf@servo.finestructure.net".

Re: [BUG/PATCH v2 0/2] emacs: Yesterday's bugfixes

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On Thu, Mar 29 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> To keep everything in one thread, Jamie also signed off this patch in
> id:"87obrf48ci.fsf@servo.finestructure.net".

Doh.  Sorry about that.  Yes, I've tested this new version and it works
and LGTM.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]