bug in emacs reply code for References headers

bug in emacs reply code for References headers

[Jameson Graef Rollins]

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

Hi, folks.  I just wanted to make people aware of a bug in the new reply
code (new JSON reply format) on the current master.

It appears that newlines are not placed after the References header is
inserted, which means that the User-Agent header gets concatenated at
the end of the References line, causing malformed References.  I have
been able to get around this by adding the newline in manually in the
reply buffer.  But it is easy to not notice, and the bug may destroy
subsequent threading.

Adam Wolfe Gordon, the original author of the new JSON reply code, was
made aware of this on IRC and I believe he is looking into the issue.
In any event, those using master and the emacs UI should aware of this
issue.

jamie.

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

[BUG/PATCH] 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 |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 24918d3..b5d91b7 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 (header references)
+  (message-shorten-references 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,16 @@ 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-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
+	       (append '((References . notmuch-mua-insert-references))
+		       (remove-if (lambda (x) (eq (car x) 'References))
+				  message-header-format-alist))))
+	  (notmuch-mua-mail (plist-get reply-headers :To)
+			    (plist-get reply-headers :Subject)
+			    (notmuch-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

[BUG/PATCH v2] 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.
---

This version adds the local variables to suppress 'cl warings, per
id:"1332995623-9055-1-git-send-email-amdragon@mit.edu".

 emacs/notmuch-mua.el |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 24918d3..0d3fcd3 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 (header references)
+  (message-shorten-references 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,16 @@ 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-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
+	       (append '((References . notmuch-mua-insert-references))
+		       (remove-if (lambda (x) (eq (car x) 'References))
+				  message-header-format-alist))))
+	  (notmuch-mua-mail (plist-get reply-headers :To)
+			    (plist-get reply-headers :Subject)
+			    (notmuch-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))
@@ -301,3 +317,7 @@ simply runs the corresponding `message-mode' hook functions."
 ;;
 
 (provide 'notmuch-mua)
+
+;; Local Variables:
+;; byte-compile-warnings: (not cl-functions)
+;; End:
-- 
1.7.5.4

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

[Austin Clements]

Quoth Adam Wolfe Gordon on Mar 28 at 10:53 pm:
> 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.
> ---

Ugh.  message-mode is such a rat's nest.  I dug through this and it
looks like message-shorten-references really is at fault here.

I'm sure you already tracked this down, but for others who may be
interested, ultimately, the headers are inserted by
mail-header-format, which calls formatter functions or, if there is no
formatter, mail-header-format-function.  mail-header-format-function
inserts a newline after the header and, indeed, mail-header-format
does not insert anything between headers, so this is clearly up to the
formatter.  message-shorten-references, however, inserts its header by
calling message-insert-header, which looks remarkably like
mail-header-format-function, minus the newline.  Ironically,
message-shorten-references appears to be the only formatter configured
by default.

> This version adds the local variables to suppress 'cl warings, per
> id:"1332995623-9055-1-git-send-email-amdragon@mit.edu".
> 
>  emacs/notmuch-mua.el |   26 +++++++++++++++++++++++---
>  1 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 24918d3..0d3fcd3 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 (header references)
> +  (message-shorten-references header references)
> +  (unless (bolp) (insert "\n")))
> +

Would it be safer to call whatever was associated with References in
message-header-format-alist, rather than hard-coding
message-shorten-references?

>  (defun notmuch-mua-reply (query-string &optional sender reply-all)
>    (let ((args '("reply" "--format=json"))
>  	reply
> @@ -125,9 +134,16 @@ 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-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
> +	       (append '((References . notmuch-mua-insert-references))

(cons '(References . notmuch-mua-insert-references) ...)

> +		       (remove-if (lambda (x) (eq (car x) 'References))
> +				  message-header-format-alist))))

(assq-delete-all 'References (copy-alist message-header-format-alist))?

Hmm.  That's less shorter than I would have expected, but I think it's
less opaque.

Actually, if I'm reading mail-header-format correctly, the order of
this alist controls the order of the headers, so maybe what you
actually want is

(mapcar (lambda (x) (if (eq (car x) 'References)
                        '(References . notmuch-mua-insert-references)
                       x))
        message-header-format-alist)

> +	  (notmuch-mua-mail (plist-get reply-headers :To)
> +			    (plist-get reply-headers :Subject)
> +			    (notmuch-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))
> @@ -301,3 +317,7 @@ simply runs the corresponding `message-mode' hook functions."
>  ;;
>  
>  (provide 'notmuch-mua)
> +
> +;; Local Variables:
> +;; byte-compile-warnings: (not cl-functions)
> +;; End:

This won't be necessary if you use assq-delete-all or mapcar, but if
you stick with the remove-if, you should also change the
  (eval-when-compile (require 'cl))
to
  (require 'cl)

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

[Jameson Graef Rollins]

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

On Wed, Mar 28 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> 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.

Hey, Adam.  Thanks so much for working on this.

I just tested this patch and it does seem to fix the issue.  However, a
side effect seems to be that the References header is now appearing as
the first header in the reply buffer, rather than the last, as it used
to.  I suppose this is merely aesthetic, but I did prefer the ordering
as it was before.  Is there a way to tweak the
message-header-format-alist so that the References header appears last
again?

Given the various things that are being affected by this, it would
probably be good to add a test for this as well.

jamie.

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

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

[Adam Wolfe Gordon]

Hi Jamie,

On Thu, Mar 29, 2012 at 11:14, Jameson Graef Rollins
<jrollins@finestructure.net> wrote:
> I just tested this patch and it does seem to fix the issue.  However, a
> side effect seems to be that the References header is now appearing as
> the first header in the reply buffer, rather than the last, as it used
> to.  I suppose this is merely aesthetic, but I did prefer the ordering
> as it was before.  Is there a way to tweak the
> message-header-format-alist so that the References header appears last
> again?

That's actually fixed by the new version [1]. As Austin mentioned in
his review, the order of message-header-format-alist determines the
order of the headers.

[1] id:"1333038410-17927-3-git-send-email-awg+notmuch@xvx.ca"

> Given the various things that are being affected by this, it would
> probably be good to add a test for this as well.

Yeah, I think a test would be good. This isn't caught by the existing
emacs reply tests because the References and User-Agent headers are
hidden by default. Is there a function in emacs to tell message-mode
to show all the headers, so we can verify that they're correct?

-- 
Adam

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

[Jameson Graef Rollins]

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

On Thu, Mar 29 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> That's actually fixed by the new version [1]. As Austin mentioned in
> his review, the order of message-header-format-alist determines the
> order of the headers.
>
> [1] id:"1333038410-17927-3-git-send-email-awg+notmuch@xvx.ca"

Awesome!  Thanks so much Adam!  I just tested it and it works great:
solves the issue and the headers are in a nice order.  Thanks again for
the quick fix.

I see one tiny whitespace error, but overall the rest of the patches
looks great, so I say it's not worth resubmitting.  LGTM.

>> Given the various things that are being affected by this, it would
>> probably be good to add a test for this as well.
>
> Yeah, I think a test would be good. This isn't caught by the existing
> emacs reply tests because the References and User-Agent headers are
> hidden by default. Is there a function in emacs to tell message-mode
> to show all the headers, so we can verify that they're correct?

There must be, since I am seeing the In-Reply-To and References headers,
but I can't find what customization variable it is at the moment.  It
looks like I am using message-required-headers:

(message-required-headers ((optional . References) From)

But looking through the plethora of message-mode options, I think there
is probably multiple ways to set this.

jamie.

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