[PATCH 0/2] improve `notmuch-show-clean-address'

[PATCH 0/2] improve `notmuch-show-clean-address'

[David Edmondson]

Avoids warnings due to unicode characters and improves display.

David Edmondson (2):
  emacs: Avoid `mail-header-parse-address' in
    `notmuch-show-clean-address'.
  emacs: Another special case for `notmuch-show-clean-address'.

 emacs/notmuch-show.el          |   54 +++++++++++++++++++++++++++++----------
 test/emacs-address-cleaning.el |    6 +++-
 test/emacs-address-cleaning.sh |    1 -
 3 files changed, 44 insertions(+), 17 deletions(-)

-- 
1.7.8.3

[PATCH 1/2] emacs: Avoid `mail-header-parse-address' in `notmuch-show-clean-address'.

[David Edmondson]

`mail-header-parse-address' expects un-decoded mailbox parts, which is
not what we have at this point. Replace it with simple string
deconstruction.

Mark the corresponding test as no longer broken.

Minor whitespace cleanup.
---
 emacs/notmuch-show.el          |   50 ++++++++++++++++++++++++++++-----------
 test/emacs-address-cleaning.sh |    1 -
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e6a5b31..1fd2524 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -289,21 +289,43 @@ operation on the contents of the current buffer."
   "Try to clean a single email ADDRESS for display.  Return
 unchanged ADDRESS if parsing fails."
   (condition-case nil
-    (let* ((parsed (mail-header-parse-address address))
-	   (address (car parsed))
-	   (name (cdr parsed)))
-      ;; Remove double quotes. They might be required during transport,
-      ;; but we don't need to see them.
-      (when name
-        (setq name (replace-regexp-in-string "\"" "" name)))
+    (let (p-name p-address)
+      ;; It would be convenient to use `mail-header-parse-address',
+      ;; but that expects un-decoded mailbox parts, whereas our
+      ;; mailbox parts are already decoded (and hence may contain
+      ;; UTF-8). Given that notmuch should handle most of the awkward
+      ;; cases, some simple string deconstruction should be sufficient
+      ;; here.
+      (cond
+       ;; "User <user@dom.ain>" style.
+       ((string-match "\\(.*\\) <\\(.*\\)>" address)
+	(setq p-name (match-string 1 address)
+	      p-address (match-string 2 address)))
+
+       ;; "<user@dom.ain>" style.
+       ((string-match "<\\(.*\\)>" address)
+	(setq p-address (match-string 1 address)))
+
+       ;; Everything else.
+       (t
+	(setq p-address address)))
+
+      ;; Remove outer double quotes. They might be required during
+      ;; transport, but we don't need to see them.
+      (when (and p-name
+		 (string-match "^\"\\(.*\\)\"$" p-name))
+	(setq p-name (match-string 1 p-name)))
+
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
-      (when (string= name address)
-        (setq name nil))
-
-      (if (not name)
-        address
-        (concat name " <" address ">")))
+      (when (string= p-name p-address)
+	(setq p-name nil))
+
+      ;; If no name results, return just the address.
+      (if (not p-name)
+	  p-address
+	;; Otherwise format the name and address together.
+	(concat p-name " <" p-address ">")))
     (error address)))
 
 (defun notmuch-show-insert-headerline (headers date tags depth)
@@ -1417,7 +1439,7 @@ than only the current message."
   (interactive "P\nsPipe message to command: ")
   (let (shell-command)
     (if entire-thread
-	(setq shell-command 
+	(setq shell-command
 	      (concat notmuch-command " show --format=mbox "
 		      (shell-quote-argument
 		       (mapconcat 'identity (notmuch-show-get-message-ids-for-open-messages) " OR "))
diff --git a/test/emacs-address-cleaning.sh b/test/emacs-address-cleaning.sh
index 0d85bdc..51018fe 100755
--- a/test/emacs-address-cleaning.sh
+++ b/test/emacs-address-cleaning.sh
@@ -12,7 +12,6 @@ test_emacs_expect_t \
     '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-2)'
 
 test_begin_subtest "notmuch-test-address-clean part 3"
-test_subtest_known_broken
 test_emacs_expect_t \
     '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-3)'
 
-- 
1.7.8.3

[PATCH 2/2] emacs: Another special case for `notmuch-show-clean-address'.

[David Edmondson]

Remove backslashes.
---
 emacs/notmuch-show.el          |   14 +++++++++-----
 test/emacs-address-cleaning.el |    6 ++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1fd2524..c849e84 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -310,11 +310,15 @@ unchanged ADDRESS if parsing fails."
        (t
 	(setq p-address address)))
 
-      ;; Remove outer double quotes. They might be required during
-      ;; transport, but we don't need to see them.
-      (when (and p-name
-		 (string-match "^\"\\(.*\\)\"$" p-name))
-	(setq p-name (match-string 1 p-name)))
+      ;; Remove elements of the mailbox part that are not relevant for
+      ;; display, even if they are required during transport.
+      (when p-name
+	;; Outer double quotes.
+	(when (string-match "^\"\\(.*\\)\"$" p-name)
+	  (setq p-name (match-string 1 p-name)))
+
+	;; Backslashes.
+	(setq p-name (replace-regexp-in-string "\\\\" "" p-name)))
 
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
diff --git a/test/emacs-address-cleaning.el b/test/emacs-address-cleaning.el
index 19e9e05..3b0b109 100644
--- a/test/emacs-address-cleaning.el
+++ b/test/emacs-address-cleaning.el
@@ -20,10 +20,12 @@
   (let* ((input '("ДБ <db-uknot@stop.me.uk>"
 		  "foo (at home) <foo@bar.com>"
 		  "foo [at home] <foo@bar.com>"
-		  "Foo Bar"))
+		  "Foo Bar"
+		  "Fred Dibna \\[extraordinaire\\] <fred@dibna.com>"))
 	 (expected '("ДБ <db-uknot@stop.me.uk>"
 		     "foo (at home) <foo@bar.com>"
 		     "foo [at home] <foo@bar.com>"
-		     "Foo Bar"))
+		     "Foo Bar"
+		     "Fred Dibna [extraordinaire] <fred@dibna.com>"))
 	 (output (mapcar #'notmuch-show-clean-address input)))
     (notmuch-test-expect-equal output expected)))
-- 
1.7.8.3

Re: [PATCH 1/2] emacs: Avoid `mail-header-parse-address' in `notmuch-show-clean-address'.

[Thomas Jost]

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

On Wed, 25 Jan 2012 11:45:03 +0000, David Edmondson <dme@dme.org> wrote:
> `mail-header-parse-address' expects un-decoded mailbox parts, which is
> not what we have at this point. Replace it with simple string
> deconstruction.
> 
> Mark the corresponding test as no longer broken.
> 
> Minor whitespace cleanup.
> ---
>  emacs/notmuch-show.el          |   50 ++++++++++++++++++++++++++++-----------
>  test/emacs-address-cleaning.sh |    1 -
>  2 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index e6a5b31..1fd2524 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -289,21 +289,43 @@ operation on the contents of the current buffer."
>    "Try to clean a single email ADDRESS for display.  Return
>  unchanged ADDRESS if parsing fails."
>    (condition-case nil
> -    (let* ((parsed (mail-header-parse-address address))
> -	   (address (car parsed))
> -	   (name (cdr parsed)))
> -      ;; Remove double quotes. They might be required during transport,
> -      ;; but we don't need to see them.
> -      (when name
> -        (setq name (replace-regexp-in-string "\"" "" name)))
> +    (let (p-name p-address)
> +      ;; It would be convenient to use `mail-header-parse-address',
> +      ;; but that expects un-decoded mailbox parts, whereas our
> +      ;; mailbox parts are already decoded (and hence may contain
> +      ;; UTF-8). Given that notmuch should handle most of the awkward
> +      ;; cases, some simple string deconstruction should be sufficient
> +      ;; here.
> +      (cond
> +       ;; "User <user@dom.ain>" style.
> +       ((string-match "\\(.*\\) <\\(.*\\)>" address)
> +	(setq p-name (match-string 1 address)
> +	      p-address (match-string 2 address)))
> +
> +       ;; "<user@dom.ain>" style.
> +       ((string-match "<\\(.*\\)>" address)
> +	(setq p-address (match-string 1 address)))
> +
> +       ;; Everything else.
> +       (t
> +	(setq p-address address)))
> +
> +      ;; Remove outer double quotes. They might be required during
> +      ;; transport, but we don't need to see them.
> +      (when (and p-name
> +		 (string-match "^\"\\(.*\\)\"$" p-name))
> +	(setq p-name (match-string 1 p-name)))
> +
>        ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
>        ;; 'foo@bar.com'.
> -      (when (string= name address)
> -        (setq name nil))
> -
> -      (if (not name)
> -        address
> -        (concat name " <" address ">")))
> +      (when (string= p-name p-address)
> +	(setq p-name nil))
> +
> +      ;; If no name results, return just the address.
> +      (if (not p-name)
> +	  p-address
> +	;; Otherwise format the name and address together.
> +	(concat p-name " <" p-address ">")))
>      (error address)))
>  
>  (defun notmuch-show-insert-headerline (headers date tags depth)
> @@ -1417,7 +1439,7 @@ than only the current message."
>    (interactive "P\nsPipe message to command: ")
>    (let (shell-command)
>      (if entire-thread
> -	(setq shell-command 
> +	(setq shell-command
>  	      (concat notmuch-command " show --format=mbox "
>  		      (shell-quote-argument
>  		       (mapconcat 'identity (notmuch-show-get-message-ids-for-open-messages) " OR "))
> diff --git a/test/emacs-address-cleaning.sh b/test/emacs-address-cleaning.sh
> index 0d85bdc..51018fe 100755
> --- a/test/emacs-address-cleaning.sh
> +++ b/test/emacs-address-cleaning.sh
> @@ -12,7 +12,6 @@ test_emacs_expect_t \
>      '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-2)'
>  
>  test_begin_subtest "notmuch-test-address-clean part 3"
> -test_subtest_known_broken
>  test_emacs_expect_t \
>      '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-3)'
>  
> -- 
> 1.7.8.3

Hello David,

Works fine for me, but this also breaks en Emacs test:

 FAIL   notmuch-show for message with invalid From
	--- emacs.10.expected	2012-01-25 12:50:00.310786410 +0000
	+++ emacs.10.output	2012-01-25 12:50:00.310786410 +0000
	@@ -1,4 +1,4 @@
	-"Invalid " From" <test_suite@notmuchmail.org> (2001-01-05) (inbox)
	+Invalid " From <test_suite@notmuchmail.org> (2001-01-05) (inbox)
	 Subject: message-with-invalid-from
	 To: Notmuch Test Suite <test_suite@notmuchmail.org>
	 Date: Fri, 05 Jan 2001 15:43:57 +0000
nil

Thanks for this patch anyway :)

Regards,

-- 
Thomas/Schnouki

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

Re: [PATCH 1/2] emacs: Avoid `mail-header-parse-address' in `notmuch-show-clean-address'.

[David Edmondson]

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

On Wed, 25 Jan 2012 13:51:46 +0100, Thomas Jost <schnouki@schnouki.net> wrote:
> Works fine for me, but this also breaks en Emacs test:
> 
>  FAIL   notmuch-show for message with invalid From
> 	--- emacs.10.expected	2012-01-25 12:50:00.310786410 +0000
> 	+++ emacs.10.output	2012-01-25 12:50:00.310786410 +0000
> 	@@ -1,4 +1,4 @@
> 	-"Invalid " From" <test_suite@notmuchmail.org> (2001-01-05) (inbox)
> 	+Invalid " From <test_suite@notmuchmail.org> (2001-01-05) (inbox)
> 	 Subject: message-with-invalid-from
> 	 To: Notmuch Test Suite <test_suite@notmuchmail.org>
> 	 Date: Fri, 05 Jan 2001 15:43:57 +0000
> nil

Bah, I forgot to cherry pick that part of the fix. Will re-submit.

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

[PATCH 0/3 v2 ] improve `notmuch-show-clean-address'

[David Edmondson]

Avoids warnings due to unicode characters and improves display.

v2: Update the existing test output accordingly.

David Edmondson (3):
  emacs: Avoid `mail-header-parse-address' in
    `notmuch-show-clean-address'.
  test: Updated expected output for new `notmuch-show-clean-address'.
  emacs: Another special case for `notmuch-show-clean-address'.

 emacs/notmuch-show.el          |   54 +++++++++++++++++++++++++++++----------
 test/emacs                     |    2 +-
 test/emacs-address-cleaning.el |    6 +++-
 test/emacs-address-cleaning.sh |    1 -
 4 files changed, 45 insertions(+), 18 deletions(-)

-- 
1.7.8.3

[PATCH 1/3] emacs: Avoid `mail-header-parse-address' in `notmuch-show-clean-address'.

[David Edmondson]

`mail-header-parse-address' expects un-decoded mailbox parts, which is
not what we have at this point. Replace it with simple string
deconstruction.

Mark the corresponding test as no longer broken.

Minor whitespace cleanup.
---
 emacs/notmuch-show.el          |   50 ++++++++++++++++++++++++++++-----------
 test/emacs-address-cleaning.sh |    1 -
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e6a5b31..1fd2524 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -289,21 +289,43 @@ operation on the contents of the current buffer."
   "Try to clean a single email ADDRESS for display.  Return
 unchanged ADDRESS if parsing fails."
   (condition-case nil
-    (let* ((parsed (mail-header-parse-address address))
-	   (address (car parsed))
-	   (name (cdr parsed)))
-      ;; Remove double quotes. They might be required during transport,
-      ;; but we don't need to see them.
-      (when name
-        (setq name (replace-regexp-in-string "\"" "" name)))
+    (let (p-name p-address)
+      ;; It would be convenient to use `mail-header-parse-address',
+      ;; but that expects un-decoded mailbox parts, whereas our
+      ;; mailbox parts are already decoded (and hence may contain
+      ;; UTF-8). Given that notmuch should handle most of the awkward
+      ;; cases, some simple string deconstruction should be sufficient
+      ;; here.
+      (cond
+       ;; "User <user@dom.ain>" style.
+       ((string-match "\\(.*\\) <\\(.*\\)>" address)
+	(setq p-name (match-string 1 address)
+	      p-address (match-string 2 address)))
+
+       ;; "<user@dom.ain>" style.
+       ((string-match "<\\(.*\\)>" address)
+	(setq p-address (match-string 1 address)))
+
+       ;; Everything else.
+       (t
+	(setq p-address address)))
+
+      ;; Remove outer double quotes. They might be required during
+      ;; transport, but we don't need to see them.
+      (when (and p-name
+		 (string-match "^\"\\(.*\\)\"$" p-name))
+	(setq p-name (match-string 1 p-name)))
+
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
-      (when (string= name address)
-        (setq name nil))
-
-      (if (not name)
-        address
-        (concat name " <" address ">")))
+      (when (string= p-name p-address)
+	(setq p-name nil))
+
+      ;; If no name results, return just the address.
+      (if (not p-name)
+	  p-address
+	;; Otherwise format the name and address together.
+	(concat p-name " <" p-address ">")))
     (error address)))
 
 (defun notmuch-show-insert-headerline (headers date tags depth)
@@ -1417,7 +1439,7 @@ than only the current message."
   (interactive "P\nsPipe message to command: ")
   (let (shell-command)
     (if entire-thread
-	(setq shell-command 
+	(setq shell-command
 	      (concat notmuch-command " show --format=mbox "
 		      (shell-quote-argument
 		       (mapconcat 'identity (notmuch-show-get-message-ids-for-open-messages) " OR "))
diff --git a/test/emacs-address-cleaning.sh b/test/emacs-address-cleaning.sh
index 0d85bdc..51018fe 100755
--- a/test/emacs-address-cleaning.sh
+++ b/test/emacs-address-cleaning.sh
@@ -12,7 +12,6 @@ test_emacs_expect_t \
     '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-2)'
 
 test_begin_subtest "notmuch-test-address-clean part 3"
-test_subtest_known_broken
 test_emacs_expect_t \
     '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-3)'
 
-- 
1.7.8.3

[PATCH 2/3] test: Updated expected output for new `notmuch-show-clean-address'.

[David Edmondson]

---
 test/emacs |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/emacs b/test/emacs
index f150d95..8ca4c8a 100755
--- a/test/emacs
+++ b/test/emacs
@@ -78,7 +78,7 @@ thread=$(notmuch search --output=threads subject:message-with-invalid-from)
 test_emacs "(notmuch-show \"$thread\")
 	    (test-output)"
 cat <<EOF >EXPECTED
-"Invalid " From" <test_suite@notmuchmail.org> (2001-01-05) (inbox)
+Invalid " From <test_suite@notmuchmail.org> (2001-01-05) (inbox)
 Subject: message-with-invalid-from
 To: Notmuch Test Suite <test_suite@notmuchmail.org>
 Date: Fri, 05 Jan 2001 15:43:57 +0000
-- 
1.7.8.3

[PATCH 3/3] emacs: Another special case for `notmuch-show-clean-address'.

[David Edmondson]

Remove backslashes.
---
 emacs/notmuch-show.el          |   14 +++++++++-----
 test/emacs-address-cleaning.el |    6 ++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1fd2524..c849e84 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -310,11 +310,15 @@ unchanged ADDRESS if parsing fails."
        (t
 	(setq p-address address)))
 
-      ;; Remove outer double quotes. They might be required during
-      ;; transport, but we don't need to see them.
-      (when (and p-name
-		 (string-match "^\"\\(.*\\)\"$" p-name))
-	(setq p-name (match-string 1 p-name)))
+      ;; Remove elements of the mailbox part that are not relevant for
+      ;; display, even if they are required during transport.
+      (when p-name
+	;; Outer double quotes.
+	(when (string-match "^\"\\(.*\\)\"$" p-name)
+	  (setq p-name (match-string 1 p-name)))
+
+	;; Backslashes.
+	(setq p-name (replace-regexp-in-string "\\\\" "" p-name)))
 
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
diff --git a/test/emacs-address-cleaning.el b/test/emacs-address-cleaning.el
index 19e9e05..3b0b109 100644
--- a/test/emacs-address-cleaning.el
+++ b/test/emacs-address-cleaning.el
@@ -20,10 +20,12 @@
   (let* ((input '("ДБ <db-uknot@stop.me.uk>"
 		  "foo (at home) <foo@bar.com>"
 		  "foo [at home] <foo@bar.com>"
-		  "Foo Bar"))
+		  "Foo Bar"
+		  "Fred Dibna \\[extraordinaire\\] <fred@dibna.com>"))
 	 (expected '("ДБ <db-uknot@stop.me.uk>"
 		     "foo (at home) <foo@bar.com>"
 		     "foo [at home] <foo@bar.com>"
-		     "Foo Bar"))
+		     "Foo Bar"
+		     "Fred Dibna [extraordinaire] <fred@dibna.com>"))
 	 (output (mapcar #'notmuch-show-clean-address input)))
     (notmuch-test-expect-equal output expected)))
-- 
1.7.8.3

Re: [PATCH 0/3 v2 ] improve `notmuch-show-clean-address'

[Tomi Ollila]

On Wed, 25 Jan 2012 13:53:57 +0000, David Edmondson <dme@dme.org> wrote:
> Avoids warnings due to unicode characters and improves display.
> 
> v2: Update the existing test output accordingly.
> 
> David Edmondson (3):
>   emacs: Avoid `mail-header-parse-address' in
>     `notmuch-show-clean-address'.
>   test: Updated expected output for new `notmuch-show-clean-address'.
>   emacs: Another special case for `notmuch-show-clean-address'.

+1

Tomi

Re: [PATCH 0/3 v2 ] improve `notmuch-show-clean-address'

[David Bremner]

On Wed, 25 Jan 2012 13:53:57 +0000, David Edmondson <dme@dme.org> wrote:
> Avoids warnings due to unicode characters and improves display.
> 
> v2: Update the existing test output accordingly.

pushed,

d