Date: Thu, 29 Mar 2012 01:32:21 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>
Subject: Re: [BUG/PATCH v2] emacs: Fix the References header in reply
Message-ID: <20120329053221.GA2670@mit.edu>
References: <1332991226-510-1-git-send-email-awg+notmuch@xvx.ca>
	<1332996818-15700-1-git-send-email-awg+notmuch@xvx.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1332996818-15700-1-git-send-email-awg+notmuch@xvx.ca>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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)