unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series
@ 2012-04-01  0:33 Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01  0:33 UTC (permalink / raw)
  To: notmuch

Hi everyone,

This series is a combination of the three bugfixes I sent this week [1]
[2]. As Mark pointed out, the fix for the emacs 23.2 bug actually fixes the
alternate address bug as well, so there's no real reason to have a
separate patch for it, especially one that implements a different fix.

I've also adjusted the reply tests so that they show all the headers in the
reply message. This would catch the References/User-Agent bug.

I'm hoping these can be pushed quickly, as they do fix bugs, and the
individual patches were reviewed and pretty much ready to go.

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

Adam Wolfe Gordon (4):
  test: Tests for reply from alternate addresses in emacs
  emacs: Fix two bugs in reply
  test: Show all headers in emacs reply tests
  emacs: Fix the References header in reply

 emacs/notmuch-lib.el |    7 +++-
 emacs/notmuch-mua.el |   36 +++++++++++++++++----
 test/emacs           |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 110 insertions(+), 17 deletions(-)

-- 
1.7.5.4

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

* [BUG/PATCH v3 1/4] test: Tests for reply from alternate addresses in emacs
  2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
@ 2012-04-01  0:33 ` Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 2/4] emacs: Fix two bugs in reply Adam Wolfe Gordon
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01  0:33 UTC (permalink / raw)
  To: notmuch

Since the recent reply changes were pushed, there has been a bug that
causes emacs to always reply from the primary address, even if the
JSON or default CLI reply output uses an alternate address.

This adds two tests to the emacs test library based on the two "Reply
form..." tests in the reply test library. One is currently marked
broken.
---
 test/emacs |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 8a28705..3402efb 100755
--- a/test/emacs
+++ b/test/emacs
@@ -274,6 +274,50 @@ Notmuch Test Suite <test_suite@notmuchmail.org> writes:
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Reply from alternate address within emacs"
+test_subtest_known_broken
+add_message '[from]="Sender <sender@example.com>"' \
+	     [to]=test_suite_other@notmuchmail.org
+
+test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+	    (notmuch-test-wait)
+	    (notmuch-search-reply-to-thread)
+	    (test-output)"
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite_other@notmuchmail.org>
+To: Sender <sender@example.com>
+Subject: Re: ${test_subtest_name}
+In-Reply-To: <${gen_msg_id}>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Sender <sender@example.com> writes:
+
+> This is just a test message (#${gen_msg_cnt})
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+test_begin_subtest "Reply from address in named group list within emacs"
+add_message '[from]="Sender <sender@example.com>"' \
+            '[to]=group:test_suite@notmuchmail.org,someone@example.com\;' \
+             [cc]=test_suite_other@notmuchmail.org
+
+test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+	    (notmuch-test-wait)
+	    (notmuch-search-reply-to-thread)
+	    (test-output)"
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Sender <sender@example.com>, someone@example.com
+Subject: Re: ${test_subtest_name}
+In-Reply-To: <${gen_msg_id}>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Sender <sender@example.com> writes:
+
+> This is just a test message (#${gen_msg_cnt})
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
 test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
-- 
1.7.5.4

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

* [BUG/PATCH v3 2/4] emacs: Fix two bugs in reply
  2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
@ 2012-04-01  0:33 ` Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01  0:33 UTC (permalink / raw)
  To: notmuch

Bug 1: Replying from alternate addresses
----------------------------------------

The reply code was inconsistent in its use of symbols and strings for
header names being passed to message.el functions. This caused the
From header to be lookup up incorrectly, causing an additional From
header to be added with the user's primary address instead of the
correct alternate address.

This is fixed by using symbols everywhere, i.e. never using strings
for header names when interacting with message.el.

This change also removes our use of `mail-header`, since we don't use
it anywhere else, and using assq makes it clear how the header lists
are expected to work.

Bug 2: Duplicate headers in emacs 23.2
--------------------------------------

The message.el code in emacs 23.2 assumes that header names will
always be passed as symbols, so our use of strings caused
problems. The symptom was that on 23.2 (and presumably on earlier
versions) the reply message would end up with two of some headers.

Converting everything to symbols also fixes this issue.
---
 emacs/notmuch-lib.el |    7 +++++--
 emacs/notmuch-mua.el |   10 +++++-----
 test/emacs           |    1 -
 3 files changed, 10 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 6aae3a0..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 (mail-header '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)
diff --git a/test/emacs b/test/emacs
index 3402efb..06291d3 100755
--- a/test/emacs
+++ b/test/emacs
@@ -275,7 +275,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from alternate address within emacs"
-test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
 	     [to]=test_suite_other@notmuchmail.org
 
-- 
1.7.5.4

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

* [BUG/PATCH v3 3/4] test: Show all headers in emacs reply tests
  2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 2/4] emacs: Fix two bugs in reply Adam Wolfe Gordon
@ 2012-04-01  0:33 ` Adam Wolfe Gordon
  2012-04-01  0:33 ` [BUG/PATCH v3 4/4] emacs: Fix the References header in reply Adam Wolfe Gordon
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01  0:33 UTC (permalink / raw)
  To: notmuch

By default, emacs hides the User-Agent and References headers when
composing mail. This is a good thing for users, but a bad thing for
testing, since we can create ugly or invalid headers and not have it
show up in the tests.

By setting message-hidden-headers to an empty list, we force emacs to
show all the headers, so we can check that they're correct. Users
won't see this, but it will let us catch future bugs.

As a side-effect, this breaks all the reply tests, since there is a
bug with the References and User-Agent headers, fixed in the next commit.
---
 test/emacs |   55 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/test/emacs b/test/emacs
index 06291d3..7d00cae 100755
--- a/test/emacs
+++ b/test/emacs
@@ -256,17 +256,23 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs"
-test_emacs '(notmuch-search "subject:\"testing message sent via SMTP\"")
+test_subtest_known_broken
+test_emacs '(let ((message-hidden-headers ''()))
+	    (notmuch-search "subject:\"testing message sent via SMTP\"")
 	    (notmuch-test-wait)
 	    (notmuch-search-reply-to-thread)
-	    (test-output)'
+	    (test-output))'
 sed -i -e 's/^In-Reply-To: <.*>$/In-Reply-To: <XXX>/' OUTPUT
+sed -i -e 's/^References: <.*>$/References: <XXX>/' OUTPUT
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: user@example.com
 Subject: Re: Testing message sent via SMTP
 In-Reply-To: <XXX>
 Fcc: ${MAIL_DIR}/sent
+References: <XXX>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Notmuch Test Suite <test_suite@notmuchmail.org> writes:
 
@@ -275,19 +281,24 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from alternate address within emacs"
+test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
 	     [to]=test_suite_other@notmuchmail.org
 
-test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+test_emacs "(let ((message-hidden-headers '()))
+	    (notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
 	    (notmuch-test-wait)
 	    (notmuch-search-reply-to-thread)
-	    (test-output)"
+	    (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite_other@notmuchmail.org>
 To: Sender <sender@example.com>
 Subject: Re: ${test_subtest_name}
 In-Reply-To: <${gen_msg_id}>
 Fcc: ${MAIL_DIR}/sent
+References: <${gen_msg_id}>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Sender <sender@example.com> writes:
 
@@ -296,20 +307,25 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from address in named group list within emacs"
+test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
             '[to]=group:test_suite@notmuchmail.org,someone@example.com\;' \
              [cc]=test_suite_other@notmuchmail.org
 
-test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+test_emacs "(let ((message-hidden-headers '()))
+	    (notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
 	    (notmuch-test-wait)
 	    (notmuch-search-reply-to-thread)
-	    (test-output)"
+	    (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: Sender <sender@example.com>, someone@example.com
 Subject: Re: ${test_subtest_name}
 In-Reply-To: <${gen_msg_id}>
 Fcc: ${MAIL_DIR}/sent
+References: <${gen_msg_id}>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Sender <sender@example.com> writes:
 
@@ -318,15 +334,20 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
-test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
+test_subtest_known_broken
+test_emacs '(let ((message-hidden-headers ''()))
+	    (notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
-		(test-output)'
+		(test-output))'
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: Adrian Perez de Castro <aperez@igalia.com>, notmuch@notmuchmail.org
 Subject: Re: [notmuch] Introducing myself
 In-Reply-To: <20091118002059.067214ed@hikari>
 Fcc: ${MAIL_DIR}/sent
+References: <20091118002059.067214ed@hikari>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Adrian Perez de Castro <aperez@igalia.com> writes:
 
@@ -377,15 +398,20 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/alternative message"
-test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
+test_subtest_known_broken
+test_emacs '(let ((message-hidden-headers ''()))
+	    (notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
 		(notmuch-show-reply)
-		(test-output)'
+		(test-output))'
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: Alex Botero-Lowry <alex.boterolowry@gmail.com>, notmuch@notmuchmail.org
 Subject: Re: [notmuch] preliminary FreeBSD support
 In-Reply-To: <cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com>
 Fcc: ${MAIL_DIR}/sent
+References: <cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 
@@ -413,19 +439,24 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Quote MML tags in reply"
+test_subtest_known_broken
 message_id='test-emacs-mml-quoting@message.id'
 add_message [id]="$message_id" \
 	    "[subject]='$test_subtest_name'" \
 	    '[body]="<#part disposition=inline>"'
-test_emacs "(notmuch-show \"id:$message_id\")
+test_emacs "(let ((message-hidden-headers '()))
+	      (notmuch-show \"id:$message_id\")
 	      (notmuch-show-reply)
-	      (test-output)"
+	      (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: 
 Subject: Re: Quote MML tags in reply
 In-Reply-To: <test-emacs-mml-quoting@message.id>
 Fcc: ${MAIL_DIR}/sent
+References: <test-emacs-mml-quoting@message.id>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Notmuch Test Suite <test_suite@notmuchmail.org> writes:
 
-- 
1.7.5.4

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

* [BUG/PATCH v3 4/4] emacs: Fix the References header in reply
  2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
                   ` (2 preceding siblings ...)
  2012-04-01  0:33 ` [BUG/PATCH v3 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
@ 2012-04-01  0:33 ` Adam Wolfe Gordon
  2012-04-01 10:26 ` [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Mark Walters
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
  5 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01  0:33 UTC (permalink / raw)
  To: notmuch

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 +++++++++++++++++++++++++---
 test/emacs           |    6 ------
 2 files changed, 25 insertions(+), 9 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))
diff --git a/test/emacs b/test/emacs
index 7d00cae..e319f83 100755
--- a/test/emacs
+++ b/test/emacs
@@ -256,7 +256,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs"
-test_subtest_known_broken
 test_emacs '(let ((message-hidden-headers ''()))
 	    (notmuch-search "subject:\"testing message sent via SMTP\"")
 	    (notmuch-test-wait)
@@ -281,7 +280,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from alternate address within emacs"
-test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
 	     [to]=test_suite_other@notmuchmail.org
 
@@ -307,7 +305,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from address in named group list within emacs"
-test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
             '[to]=group:test_suite@notmuchmail.org,someone@example.com\;' \
              [cc]=test_suite_other@notmuchmail.org
@@ -334,7 +331,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
-test_subtest_known_broken
 test_emacs '(let ((message-hidden-headers ''()))
 	    (notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
@@ -398,7 +394,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/alternative message"
-test_subtest_known_broken
 test_emacs '(let ((message-hidden-headers ''()))
 	    (notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
 		(notmuch-show-reply)
@@ -439,7 +434,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Quote MML tags in reply"
-test_subtest_known_broken
 message_id='test-emacs-mml-quoting@message.id'
 add_message [id]="$message_id" \
 	    "[subject]='$test_subtest_name'" \
-- 
1.7.5.4

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

* Re: [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series
  2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
                   ` (3 preceding siblings ...)
  2012-04-01  0:33 ` [BUG/PATCH v3 4/4] emacs: Fix the References header in reply Adam Wolfe Gordon
@ 2012-04-01 10:26 ` Mark Walters
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
  5 siblings, 0 replies; 16+ messages in thread
From: Mark Walters @ 2012-04-01 10:26 UTC (permalink / raw)
  To: Adam Wolfe Gordon, notmuch


On Sun, 01 Apr 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Hi everyone,
>
> This series is a combination of the three bugfixes I sent this week [1]
> [2]. As Mark pointed out, the fix for the emacs 23.2 bug actually fixes the
> alternate address bug as well, so there's no real reason to have a
> separate patch for it, especially one that implements a different fix.
>
> I've also adjusted the reply tests so that they show all the headers in the
> reply message. This would catch the References/User-Agent bug.
>
> I'm hoping these can be pushed quickly, as they do fix bugs, and the
> individual patches were reviewed and pretty much ready to go.
>
> [1] id:"1333038410-17927-1-git-send-email-awg+notmuch@xvx.ca"
> [2] id:"1332941635-21019-1-git-send-email-awg+notmuch@xvx.ca"

I have tested this on emacs 23.2 and emacs 23.3 and it seems to work on
both. As mentioned on irc there is a minor conflict with Austin's commit
ee118001 (quote message ids) in the lib part of patch 2/4 but it's only
in the first line of the context.

My lisp is not good enough to review the actual patches.

Best wishes

Mark

> Adam Wolfe Gordon (4):
>   test: Tests for reply from alternate addresses in emacs
>   emacs: Fix two bugs in reply
>   test: Show all headers in emacs reply tests
>   emacs: Fix the References header in reply
>
>  emacs/notmuch-lib.el |    7 +++-
>  emacs/notmuch-mua.el |   36 +++++++++++++++++----
>  test/emacs           |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 110 insertions(+), 17 deletions(-)
>
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

* [BUG/PATCH v4 0/4] Bug fixes for reply, rebased
  2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
                   ` (4 preceding siblings ...)
  2012-04-01 10:26 ` [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Mark Walters
@ 2012-04-01 15:24 ` Adam Wolfe Gordon
  2012-04-01 15:24   ` [BUG/PATCH v4 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
                     ` (6 more replies)
  5 siblings, 7 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01 15:24 UTC (permalink / raw)
  To: notmuch

This series is the same as the one from yesterday [1], rebased onto the
current master to resolve some conflicts with Austin's chnages.

[1] id:"1333240404-13076-1-git-send-email-awg+notmuch@xvx.ca"

Adam Wolfe Gordon (4):
  test: Tests for reply from alternate addresses in emacs
  emacs: Fix two bugs in reply
  test: Show all headers in emacs reply tests
  emacs: Fix the References header in reply

 emacs/notmuch-lib.el |    7 +++-
 emacs/notmuch-mua.el |   36 +++++++++++++++++----
 test/emacs           |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 110 insertions(+), 17 deletions(-)

-- 
1.7.5.4

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

* [BUG/PATCH v4 1/4] test: Tests for reply from alternate addresses in emacs
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
@ 2012-04-01 15:24   ` Adam Wolfe Gordon
  2012-04-01 15:24   ` [BUG/PATCH v4 2/4] emacs: Fix two bugs in reply Adam Wolfe Gordon
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01 15:24 UTC (permalink / raw)
  To: notmuch

Since the recent reply changes were pushed, there has been a bug that
causes emacs to always reply from the primary address, even if the
JSON or default CLI reply output uses an alternate address.

This adds two tests to the emacs test library based on the two "Reply
form..." tests in the reply test library. One is currently marked
broken.
---
 test/emacs |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 8b92d0a..576bc1f 100755
--- a/test/emacs
+++ b/test/emacs
@@ -285,6 +285,50 @@ Notmuch Test Suite <test_suite@notmuchmail.org> writes:
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Reply from alternate address within emacs"
+test_subtest_known_broken
+add_message '[from]="Sender <sender@example.com>"' \
+	     [to]=test_suite_other@notmuchmail.org
+
+test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+	    (notmuch-test-wait)
+	    (notmuch-search-reply-to-thread)
+	    (test-output)"
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite_other@notmuchmail.org>
+To: Sender <sender@example.com>
+Subject: Re: ${test_subtest_name}
+In-Reply-To: <${gen_msg_id}>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Sender <sender@example.com> writes:
+
+> This is just a test message (#${gen_msg_cnt})
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+test_begin_subtest "Reply from address in named group list within emacs"
+add_message '[from]="Sender <sender@example.com>"' \
+            '[to]=group:test_suite@notmuchmail.org,someone@example.com\;' \
+             [cc]=test_suite_other@notmuchmail.org
+
+test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+	    (notmuch-test-wait)
+	    (notmuch-search-reply-to-thread)
+	    (test-output)"
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Sender <sender@example.com>, someone@example.com
+Subject: Re: ${test_subtest_name}
+In-Reply-To: <${gen_msg_id}>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Sender <sender@example.com> writes:
+
+> This is just a test message (#${gen_msg_cnt})
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
 test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
-- 
1.7.5.4

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

* [BUG/PATCH v4 2/4] emacs: Fix two bugs in reply
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
  2012-04-01 15:24   ` [BUG/PATCH v4 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
@ 2012-04-01 15:24   ` Adam Wolfe Gordon
  2012-04-01 15:24   ` [BUG/PATCH v4 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01 15:24 UTC (permalink / raw)
  To: notmuch

Bug 1: Replying from alternate addresses
----------------------------------------

The reply code was inconsistent in its use of symbols and strings for
header names being passed to message.el functions. This caused the
From header to be lookup up incorrectly, causing an additional From
header to be added with the user's primary address instead of the
correct alternate address.

This is fixed by using symbols everywhere, i.e. never using strings
for header names when interacting with message.el.

This change also removes our use of `mail-header`, since we don't use
it anywhere else, and using assq makes it clear how the header lists
are expected to work.

Bug 2: Duplicate headers in emacs 23.2
--------------------------------------

The message.el code in emacs 23.2 assumes that header names will
always be passed as symbols, so our use of strings caused
problems. The symptom was that on 23.2 (and presumably on earlier
versions) the reply message would end up with two of some headers.

Converting everything to symbols also fixes this issue.
---
 emacs/notmuch-lib.el |    7 +++++--
 emacs/notmuch-mua.el |   10 +++++-----
 test/emacs           |    1 -
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index c159dda..6907a5f 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -237,9 +237,12 @@ the given type."
   (or (plist-get part :content)
       (notmuch-get-bodypart-internal (notmuch-id-to-query (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 6aae3a0..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 (mail-header '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)
diff --git a/test/emacs b/test/emacs
index 576bc1f..30654bb 100755
--- a/test/emacs
+++ b/test/emacs
@@ -286,7 +286,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from alternate address within emacs"
-test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
 	     [to]=test_suite_other@notmuchmail.org
 
-- 
1.7.5.4

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

* [BUG/PATCH v4 3/4] test: Show all headers in emacs reply tests
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
  2012-04-01 15:24   ` [BUG/PATCH v4 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
  2012-04-01 15:24   ` [BUG/PATCH v4 2/4] emacs: Fix two bugs in reply Adam Wolfe Gordon
@ 2012-04-01 15:24   ` Adam Wolfe Gordon
  2012-04-02 17:54     ` Jameson Graef Rollins
  2012-04-01 15:24   ` [BUG/PATCH v4 4/4] emacs: Fix the References header in reply Adam Wolfe Gordon
                     ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01 15:24 UTC (permalink / raw)
  To: notmuch

By default, emacs hides the User-Agent and References headers when
composing mail. This is a good thing for users, but a bad thing for
testing, since we can create ugly or invalid headers and not have it
show up in the tests.

By setting message-hidden-headers to an empty list, we force emacs to
show all the headers, so we can check that they're correct. Users
won't see this, but it will let us catch future bugs.

As a side-effect, this breaks all the reply tests, since there is a
bug with the References and User-Agent headers, fixed in the next commit.
---
 test/emacs |   55 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/test/emacs b/test/emacs
index 30654bb..15cc778 100755
--- a/test/emacs
+++ b/test/emacs
@@ -267,17 +267,23 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs"
-test_emacs '(notmuch-search "subject:\"testing message sent via SMTP\"")
+test_subtest_known_broken
+test_emacs '(let ((message-hidden-headers ''()))
+	    (notmuch-search "subject:\"testing message sent via SMTP\"")
 	    (notmuch-test-wait)
 	    (notmuch-search-reply-to-thread)
-	    (test-output)'
+	    (test-output))'
 sed -i -e 's/^In-Reply-To: <.*>$/In-Reply-To: <XXX>/' OUTPUT
+sed -i -e 's/^References: <.*>$/References: <XXX>/' OUTPUT
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: user@example.com
 Subject: Re: Testing message sent via SMTP
 In-Reply-To: <XXX>
 Fcc: ${MAIL_DIR}/sent
+References: <XXX>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Notmuch Test Suite <test_suite@notmuchmail.org> writes:
 
@@ -286,19 +292,24 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from alternate address within emacs"
+test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
 	     [to]=test_suite_other@notmuchmail.org
 
-test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+test_emacs "(let ((message-hidden-headers '()))
+	    (notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
 	    (notmuch-test-wait)
 	    (notmuch-search-reply-to-thread)
-	    (test-output)"
+	    (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite_other@notmuchmail.org>
 To: Sender <sender@example.com>
 Subject: Re: ${test_subtest_name}
 In-Reply-To: <${gen_msg_id}>
 Fcc: ${MAIL_DIR}/sent
+References: <${gen_msg_id}>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Sender <sender@example.com> writes:
 
@@ -307,20 +318,25 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from address in named group list within emacs"
+test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
             '[to]=group:test_suite@notmuchmail.org,someone@example.com\;' \
              [cc]=test_suite_other@notmuchmail.org
 
-test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+test_emacs "(let ((message-hidden-headers '()))
+	    (notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
 	    (notmuch-test-wait)
 	    (notmuch-search-reply-to-thread)
-	    (test-output)"
+	    (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: Sender <sender@example.com>, someone@example.com
 Subject: Re: ${test_subtest_name}
 In-Reply-To: <${gen_msg_id}>
 Fcc: ${MAIL_DIR}/sent
+References: <${gen_msg_id}>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Sender <sender@example.com> writes:
 
@@ -329,15 +345,20 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
-test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
+test_subtest_known_broken
+test_emacs '(let ((message-hidden-headers ''()))
+	    (notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
-		(test-output)'
+		(test-output))'
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: Adrian Perez de Castro <aperez@igalia.com>, notmuch@notmuchmail.org
 Subject: Re: [notmuch] Introducing myself
 In-Reply-To: <20091118002059.067214ed@hikari>
 Fcc: ${MAIL_DIR}/sent
+References: <20091118002059.067214ed@hikari>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Adrian Perez de Castro <aperez@igalia.com> writes:
 
@@ -388,15 +409,20 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/alternative message"
-test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
+test_subtest_known_broken
+test_emacs '(let ((message-hidden-headers ''()))
+	    (notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
 		(notmuch-show-reply)
-		(test-output)'
+		(test-output))'
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: Alex Botero-Lowry <alex.boterolowry@gmail.com>, notmuch@notmuchmail.org
 Subject: Re: [notmuch] preliminary FreeBSD support
 In-Reply-To: <cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com>
 Fcc: ${MAIL_DIR}/sent
+References: <cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 
@@ -424,19 +450,24 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Quote MML tags in reply"
+test_subtest_known_broken
 message_id='test-emacs-mml-quoting@message.id'
 add_message [id]="$message_id" \
 	    "[subject]='$test_subtest_name'" \
 	    '[body]="<#part disposition=inline>"'
-test_emacs "(notmuch-show \"id:$message_id\")
+test_emacs "(let ((message-hidden-headers '()))
+	      (notmuch-show \"id:$message_id\")
 	      (notmuch-show-reply)
-	      (test-output)"
+	      (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
 cat <<EOF >EXPECTED
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
 To: 
 Subject: Re: Quote MML tags in reply
 In-Reply-To: <test-emacs-mml-quoting@message.id>
 Fcc: ${MAIL_DIR}/sent
+References: <test-emacs-mml-quoting@message.id>
+User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Notmuch Test Suite <test_suite@notmuchmail.org> writes:
 
-- 
1.7.5.4

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

* [BUG/PATCH v4 4/4] emacs: Fix the References header in reply
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
                     ` (2 preceding siblings ...)
  2012-04-01 15:24   ` [BUG/PATCH v4 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
@ 2012-04-01 15:24   ` Adam Wolfe Gordon
  2012-04-01 22:17   ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Jameson Graef Rollins
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-01 15:24 UTC (permalink / raw)
  To: notmuch

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 +++++++++++++++++++++++++---
 test/emacs           |    6 ------
 2 files changed, 25 insertions(+), 9 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))
diff --git a/test/emacs b/test/emacs
index 15cc778..c7510e9 100755
--- a/test/emacs
+++ b/test/emacs
@@ -267,7 +267,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs"
-test_subtest_known_broken
 test_emacs '(let ((message-hidden-headers ''()))
 	    (notmuch-search "subject:\"testing message sent via SMTP\"")
 	    (notmuch-test-wait)
@@ -292,7 +291,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from alternate address within emacs"
-test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
 	     [to]=test_suite_other@notmuchmail.org
 
@@ -318,7 +316,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply from address in named group list within emacs"
-test_subtest_known_broken
 add_message '[from]="Sender <sender@example.com>"' \
             '[to]=group:test_suite@notmuchmail.org,someone@example.com\;' \
              [cc]=test_suite_other@notmuchmail.org
@@ -345,7 +342,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
-test_subtest_known_broken
 test_emacs '(let ((message-hidden-headers ''()))
 	    (notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
@@ -409,7 +405,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/alternative message"
-test_subtest_known_broken
 test_emacs '(let ((message-hidden-headers ''()))
 	    (notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
 		(notmuch-show-reply)
@@ -450,7 +445,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Quote MML tags in reply"
-test_subtest_known_broken
 message_id='test-emacs-mml-quoting@message.id'
 add_message [id]="$message_id" \
 	    "[subject]='$test_subtest_name'" \
-- 
1.7.5.4

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

* Re: [BUG/PATCH v4 0/4] Bug fixes for reply, rebased
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
                     ` (3 preceding siblings ...)
  2012-04-01 15:24   ` [BUG/PATCH v4 4/4] emacs: Fix the References header in reply Adam Wolfe Gordon
@ 2012-04-01 22:17   ` Jameson Graef Rollins
  2012-04-02  8:10   ` Jani Nikula
  2012-04-02 20:51   ` David Bremner
  6 siblings, 0 replies; 16+ messages in thread
From: Jameson Graef Rollins @ 2012-04-01 22:17 UTC (permalink / raw)
  To: Adam Wolfe Gordon, notmuch

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

On Sun, Apr 01 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> This series is the same as the one from yesterday [1], rebased onto the
> current master to resolve some conflicts with Austin's chnages.
>
> [1] id:"1333240404-13076-1-git-send-email-awg+notmuch@xvx.ca"

This series applies cleanly against the current master and all tests
pass and it seems to fix all the issues it's meant to address.  The code
all looks good to me as well (given my pidgin lisp).

> Adam Wolfe Gordon (4):
>   test: Tests for reply from alternate addresses in emacs
>   emacs: Fix two bugs in reply
>   test: Show all headers in emacs reply tests

I really like your justification for the changes to the tests here.
Smart move.

>   emacs: Fix the References header in reply
>
>  emacs/notmuch-lib.el |    7 +++-
>  emacs/notmuch-mua.el |   36 +++++++++++++++++----
>  test/emacs           |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 110 insertions(+), 17 deletions(-)

LGTM.

Thanks so much for fixing the remaining issues here, Adam.  Good work.

jamie.

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

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

* Re: [BUG/PATCH v4 0/4] Bug fixes for reply, rebased
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
                     ` (4 preceding siblings ...)
  2012-04-01 22:17   ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Jameson Graef Rollins
@ 2012-04-02  8:10   ` Jani Nikula
  2012-04-02  8:40     ` Jani Nikula
  2012-04-02 20:51   ` David Bremner
  6 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2012-04-02  8:10 UTC (permalink / raw)
  To: Adam Wolfe Gordon, notmuch

On Sun,  1 Apr 2012 09:24:19 -0600, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> This series is the same as the one from yesterday [1], rebased onto the
> current master to resolve some conflicts with Austin's chnages.

Hi Adam -

All tests pass but I still get the "notmuch-mua-reply: Symbol's function
definition is void: remove-if" error in the GUI. I don't think I should
have to manually (require 'cl) before using notmuch.

Related or not, the other cl related build warning is still there too:

In notmuch-parts-filter-by-type:
notmuch-lib.el:219:4:Warning: Function `remove-if-not' from cl package
called at runtime


BR,
Jani.

> 
> [1] id:"1333240404-13076-1-git-send-email-awg+notmuch@xvx.ca"
> 
> Adam Wolfe Gordon (4):
>   test: Tests for reply from alternate addresses in emacs
>   emacs: Fix two bugs in reply
>   test: Show all headers in emacs reply tests
>   emacs: Fix the References header in reply
> 
>  emacs/notmuch-lib.el |    7 +++-
>  emacs/notmuch-mua.el |   36 +++++++++++++++++----
>  test/emacs           |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 110 insertions(+), 17 deletions(-)
> 
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

* Re: [BUG/PATCH v4 0/4] Bug fixes for reply, rebased
  2012-04-02  8:10   ` Jani Nikula
@ 2012-04-02  8:40     ` Jani Nikula
  0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2012-04-02  8:40 UTC (permalink / raw)
  To: Adam Wolfe Gordon, notmuch

On Mon, 02 Apr 2012 08:10:16 +0000, Jani Nikula <jani@nikula.org> wrote:
> On Sun,  1 Apr 2012 09:24:19 -0600, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> > This series is the same as the one from yesterday [1], rebased onto the
> > current master to resolve some conflicts with Austin's chnages.
> 
> Hi Adam -
> 
> All tests pass but I still get the "notmuch-mua-reply: Symbol's function
> definition is void: remove-if" error in the GUI. I don't think I should
> have to manually (require 'cl) before using notmuch.

Strike that. PEBKAC. Sorry for the noise.

Jani.

> 
> Related or not, the other cl related build warning is still there too:
> 
> In notmuch-parts-filter-by-type:
> notmuch-lib.el:219:4:Warning: Function `remove-if-not' from cl package
> called at runtime
> 
> 
> BR,
> Jani.
> 
> > 
> > [1] id:"1333240404-13076-1-git-send-email-awg+notmuch@xvx.ca"
> > 
> > Adam Wolfe Gordon (4):
> >   test: Tests for reply from alternate addresses in emacs
> >   emacs: Fix two bugs in reply
> >   test: Show all headers in emacs reply tests
> >   emacs: Fix the References header in reply
> > 
> >  emacs/notmuch-lib.el |    7 +++-
> >  emacs/notmuch-mua.el |   36 +++++++++++++++++----
> >  test/emacs           |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
> >  3 files changed, 110 insertions(+), 17 deletions(-)
> > 
> > -- 
> > 1.7.5.4
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

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

* Re: [BUG/PATCH v4 3/4] test: Show all headers in emacs reply tests
  2012-04-01 15:24   ` [BUG/PATCH v4 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
@ 2012-04-02 17:54     ` Jameson Graef Rollins
  0 siblings, 0 replies; 16+ messages in thread
From: Jameson Graef Rollins @ 2012-04-02 17:54 UTC (permalink / raw)
  To: Adam Wolfe Gordon, notmuch

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

On Sun, Apr 01 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> +test_emacs '(let ((message-hidden-headers ''()))

The more I think about it, the more I think that we should make sure
we're not hiding any headers in tests where notmuch and/or emacs creates
message templates.  We should probably do this for *all* the emacs
message composing/replying/sending tests.  Hidden headers is definitely
something that could bite us in the rear.

jamie.

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

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

* Re: [BUG/PATCH v4 0/4] Bug fixes for reply, rebased
  2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
                     ` (5 preceding siblings ...)
  2012-04-02  8:10   ` Jani Nikula
@ 2012-04-02 20:51   ` David Bremner
  6 siblings, 0 replies; 16+ messages in thread
From: David Bremner @ 2012-04-02 20:51 UTC (permalink / raw)
  To: Adam Wolfe Gordon, notmuch

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

> This series is the same as the one from yesterday [1], rebased onto the
> current master to resolve some conflicts with Austin's chnages.
>
> [1] id:"1333240404-13076-1-git-send-email-awg+notmuch@xvx.ca"

Pushed, 

d

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

end of thread, other threads:[~2012-04-02 20:51 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-01  0:33 [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Adam Wolfe Gordon
2012-04-01  0:33 ` [BUG/PATCH v3 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
2012-04-01  0:33 ` [BUG/PATCH v3 2/4] emacs: Fix two bugs in reply Adam Wolfe Gordon
2012-04-01  0:33 ` [BUG/PATCH v3 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
2012-04-01  0:33 ` [BUG/PATCH v3 4/4] emacs: Fix the References header in reply Adam Wolfe Gordon
2012-04-01 10:26 ` [BUG/PATCH v3 0/4] Bug fixes for reply: three bugs, one series Mark Walters
2012-04-01 15:24 ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Adam Wolfe Gordon
2012-04-01 15:24   ` [BUG/PATCH v4 1/4] test: Tests for reply from alternate addresses in emacs Adam Wolfe Gordon
2012-04-01 15:24   ` [BUG/PATCH v4 2/4] emacs: Fix two bugs in reply Adam Wolfe Gordon
2012-04-01 15:24   ` [BUG/PATCH v4 3/4] test: Show all headers in emacs reply tests Adam Wolfe Gordon
2012-04-02 17:54     ` Jameson Graef Rollins
2012-04-01 15:24   ` [BUG/PATCH v4 4/4] emacs: Fix the References header in reply Adam Wolfe Gordon
2012-04-01 22:17   ` [BUG/PATCH v4 0/4] Bug fixes for reply, rebased Jameson Graef Rollins
2012-04-02  8:10   ` Jani Nikula
2012-04-02  8:40     ` Jani Nikula
2012-04-02 20:51   ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

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 read-only IMAP folder(s) and NNTP newsgroup(s).