[BUG/PATCH 0/2] Replying from other addresses in emacs

[BUG/PATCH 0/2] Replying from other addresses in emacs

[Adam Wolfe Gordon]

Hi everyone,

This patch series adds a test for and fixes a bug that Dmitry pointed
out on IRC yesterday:

When replying to mail sent to an other_address, the CLI correctly sets
the From header to the address that received the message. This works
correctly in the default format and the JSON format.  However, emacs was
ignoring the provided From header and inserting the primary address, due
to an incompatibility between how we construct the header list and how
the `mail-header` function from the message library looks up header
values in the list.

People with other_addresses who are using the latest reply changes
should probably apply this patch right away. Without it, you might end
up replying to messages from the wrong address.

Adam Wolfe Gordon (2):
  test: Tests for reply from alternate addresses in emacs
  emacs: Fix replying from alternate addresses

 emacs/notmuch-mua.el |    4 +-
 test/emacs           |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

-- 
1.7.5.4

[BUG/PATCH 1/2] test: Tests for reply from alternate addresses in emacs

[Adam Wolfe Gordon]

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 |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 8a28705..fa5d706 100755
--- a/test/emacs
+++ b/test/emacs
@@ -274,6 +274,58 @@ 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 \
+	     [subject]=notmuch-reply-test \
+	    '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
+	    '[body]="reply from alternate address"'
+
+test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+	    (notmuch-test-wait)
+	    (notmuch-search-reply-to-thread)
+	    (test-output)"
+sed -i -e 's/^In-Reply-To: <.*>$/In-Reply-To: <XXX>/' OUTPUT
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite_other@notmuchmail.org>
+To: Sender <sender@example.com>
+Subject: Re: notmuch-reply-test
+In-Reply-To: <XXX>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Sender <sender@example.com> writes:
+
+> reply from alternate address
+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 \
+             [subject]=notmuch-reply-test \
+            '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
+            '[body]="Reply from address in named group list"'
+
+test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
+	    (notmuch-test-wait)
+	    (notmuch-search-reply-to-thread)
+	    (test-output)"
+sed -i -e 's/^In-Reply-To: <.*>$/In-Reply-To: <XXX>/' OUTPUT
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Sender <sender@example.com>, someone@example.com
+Subject: Re: notmuch-reply-test
+In-Reply-To: <XXX>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Sender <sender@example.com> writes:
+
+> Reply from address in named group list
+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

[BUG/PATCH 2/2] emacs: Fix replying from alternate addresses

[Adam Wolfe Gordon]

The bug was that notmuch-mua-mail used `mail-header` to check whether
it was passed a "From" header. The implementation of `mail-header`
must try to compare symbols instead of strings when looking for
headers, as it was returning nil when a From header was present. This
is probably because the mail functions construct headers as alists
with symbols for the header names, while our code uses strings for the
header names.

Since we don't use `mail-header` anywhere else, and `message-mail` is
perfectly happy to accept string header names, the fix is just to use
`assoc` to look for the From header, so that the strings get compared
properly.
---
 emacs/notmuch-mua.el |    4 ++--
 test/emacs           |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 6aae3a0..9805d79 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -187,7 +187,7 @@ OTHER-ARGS are passed through to `message-mail'."
       (when (not (string= "" user-agent))
 	(push (cons "User-Agent" user-agent) other-headers))))
 
-  (unless (mail-header 'From other-headers)
+  (unless (assoc "From" other-headers)
     (push (cons "From" (concat
 			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-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)
diff --git a/test/emacs b/test/emacs
index fa5d706..08db1ee 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 \
 	     [subject]=notmuch-reply-test \
-- 
1.7.5.4

Re: [BUG/PATCH 2/2] emacs: Fix replying from alternate addresses

[Mark Walters]

Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:
> The bug was that notmuch-mua-mail used `mail-header` to check whether
> it was passed a "From" header. The implementation of `mail-header`
> must try to compare symbols instead of strings when looking for
> headers, as it was returning nil when a From header was present. This
> is probably because the mail functions construct headers as alists
> with symbols for the header names, while our code uses strings for the
> header names.
>
> Since we don't use `mail-header` anywhere else, and `message-mail` is
> perfectly happy to accept string header names, the fix is just to use
> `assoc` to look for the From header, so that the strings get compared
> properly.

I can confirm that everything works as expected with this fix. 

Best wishes

Mark
> ---
>  emacs/notmuch-mua.el |    4 ++--
>  test/emacs           |    1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 6aae3a0..9805d79 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -187,7 +187,7 @@ OTHER-ARGS are passed through to `message-mail'."
>        (when (not (string= "" user-agent))
>  	(push (cons "User-Agent" user-agent) other-headers))))
>  
> -  (unless (mail-header 'From other-headers)
> +  (unless (assoc "From" other-headers)
>      (push (cons "From" (concat
>  			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-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)
> diff --git a/test/emacs b/test/emacs
> index fa5d706..08db1ee 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 \
>  	     [subject]=notmuch-reply-test \
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [BUG/PATCH 1/2] test: Tests for reply from alternate addresses in emacs

[Dmitry Kurochkin]

Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:
> 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 |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/test/emacs b/test/emacs
> index 8a28705..fa5d706 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -274,6 +274,58 @@ 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 \
> +	     [subject]=notmuch-reply-test \
> +	    '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
> +	    '[body]="reply from alternate address"'

Please remove subject, data and body parameters.  add_message would
provide sane default values.

> +
> +test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
> +	    (notmuch-test-wait)
> +	    (notmuch-search-reply-to-thread)
> +	    (test-output)"
> +sed -i -e 's/^In-Reply-To: <.*>$/In-Reply-To: <XXX>/' OUTPUT
> +cat <<EOF >EXPECTED
> +From: Notmuch Test Suite <test_suite_other@notmuchmail.org>
> +To: Sender <sender@example.com>
> +Subject: Re: notmuch-reply-test
> +In-Reply-To: <XXX>
> +Fcc: ${MAIL_DIR}/sent
> +--text follows this line--
> +Sender <sender@example.com> writes:
> +
> +> reply from alternate address
> +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 \
> +             [subject]=notmuch-reply-test \
> +            '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
> +            '[body]="Reply from address in named group list"'

Same here.

Regards,
  Dmitry

> +
> +test_emacs "(notmuch-search \"id:\\\"${gen_msg_id}\\\"\")
> +	    (notmuch-test-wait)
> +	    (notmuch-search-reply-to-thread)
> +	    (test-output)"
> +sed -i -e 's/^In-Reply-To: <.*>$/In-Reply-To: <XXX>/' OUTPUT
> +cat <<EOF >EXPECTED
> +From: Notmuch Test Suite <test_suite@notmuchmail.org>
> +To: Sender <sender@example.com>, someone@example.com
> +Subject: Re: notmuch-reply-test
> +In-Reply-To: <XXX>
> +Fcc: ${MAIL_DIR}/sent
> +--text follows this line--
> +Sender <sender@example.com> writes:
> +
> +> Reply from address in named group list
> +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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [BUG/PATCH 2/2] emacs: Fix replying from alternate addresses

[Dmitry Kurochkin]

Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:
> The bug was that notmuch-mua-mail used `mail-header` to check whether
> it was passed a "From" header. The implementation of `mail-header`
> must try to compare symbols instead of strings when looking for
> headers, as it was returning nil when a From header was present. This
> is probably because the mail functions construct headers as alists
> with symbols for the header names, while our code uses strings for the
> header names.
>
> Since we don't use `mail-header` anywhere else, and `message-mail` is
> perfectly happy to accept string header names, the fix is just to use
> `assoc` to look for the From header, so that the strings get compared
> properly.
> ---

LGTM.  Thanks for fixing this.

Regards,
  Dmitry

>  emacs/notmuch-mua.el |    4 ++--
>  test/emacs           |    1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 6aae3a0..9805d79 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -187,7 +187,7 @@ OTHER-ARGS are passed through to `message-mail'."
>        (when (not (string= "" user-agent))
>  	(push (cons "User-Agent" user-agent) other-headers))))
>  
> -  (unless (mail-header 'From other-headers)
> +  (unless (assoc "From" other-headers)
>      (push (cons "From" (concat
>  			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-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)
> diff --git a/test/emacs b/test/emacs
> index fa5d706..08db1ee 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 \
>  	     [subject]=notmuch-reply-test \
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[BUG/PATCH v2 0/2] Replying from other addresses in emacs

[Adam Wolfe Gordon]

This is a new version of my previous series [1], fixing the bug Dmitry noticed
when replying from alternate addresses. This version fixes a couple of review
issues in the test patch [2], but is otherwise identical.

[1] id:"1332635232-15269-1-git-send-email-awg+notmuch@xvx.ca"
[2] id:"87bonjjny9.fsf@gmail.com"

Adam Wolfe Gordon (2):
  test: Tests for reply from alternate addresses in emacs
  emacs: Fix replying from alternate addresses

 emacs/notmuch-mua.el |    4 ++--
 test/emacs           |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

-- 
1.7.5.4

[BUG/PATCH v2 1/2] test: Tests for reply from alternate addresses in emacs

[Adam Wolfe Gordon]

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

[BUG/PATCH v2 2/2] emacs: Fix replying from alternate addresses

[Adam Wolfe Gordon]

The bug was that notmuch-mua-mail used `mail-header` to check whether
it was passed a "From" header. The implementation of `mail-header`
must try to compare symbols instead of strings when looking for
headers, as it was returning nil when a From header was present. This
is probably because the mail functions construct headers as alists
with symbols for the header names, while our code uses strings for the
header names.

Since we don't use `mail-header` anywhere else, and `message-mail` is
perfectly happy to accept string header names, the fix is just to use
`assoc` to look for the From header, so that the strings get compared
properly.
---
 emacs/notmuch-mua.el |    4 ++--
 test/emacs           |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 6aae3a0..9805d79 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -187,7 +187,7 @@ OTHER-ARGS are passed through to `message-mail'."
       (when (not (string= "" user-agent))
 	(push (cons "User-Agent" user-agent) other-headers))))
 
-  (unless (mail-header 'From other-headers)
+  (unless (assoc "From" other-headers)
     (push (cons "From" (concat
 			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-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)
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

Re: [BUG/PATCH v2 0/2] Replying from other addresses in emacs

[Jameson Graef Rollins]

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

On Wed, Mar 28 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> This is a new version of my previous series [1], fixing the bug Dmitry noticed
> when replying from alternate addresses. This version fixes a couple of review
> issues in the test patch [2], but is otherwise identical.

I have tested this patch against current master and can confirm that it
fixes the bug for me.  The patches also look nice clean, and include a
good test, so this definitely LGTM.  Since it fixes a bug I recommend
pushing as soon as possible.

jamie.

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