"id buttonization" test failure

"id buttonization" test failure

[Marius Bakke]

Hi,

I'm trying to run the test suite on GNU Guix and get a mysterious test
failure in T450-emacs-show.sh:

T450-emacs-show: Testing emacs notmuch-show view
 [...]
 PASS   [11] notmuch-show: disable indentation of thread content (w/ notmuch-show-toggle-thread-indentation)
 FAIL   [12] id buttonization
        --- T450-emacs-show.13.OUTPUT   2016-10-06 01:42:21.329950576 +0000
        +++ T450-emacs-show.13.EXPECTED 2016-10-06 01:42:21.329950576 +0000
        @@ -23,4 +23,4 @@
         cid:xxx
         <<mid:abc>> <<mid:abc/def>>
         <<mid:abc%20def>>
        -<<mid:a>>bc. <<mid:abc>>, <<mid:abc>>;
        +<<mid:abc>>. <<mid:abc>>, <<mid:abc>>;
nil
 PASS   [13] Show handles subprocess errors

Any idea what's going on here? It's using emacs-25.1.

Thanks!

Re: "id buttonization" test failure

[David Bremner]

Marius Bakke <m.bakke@fastmail.com> writes:

> Hi,
>
> I'm trying to run the test suite on GNU Guix and get a mysterious test
> failure in T450-emacs-show.sh:
>
> T450-emacs-show: Testing emacs notmuch-show view
>  [...]
>  PASS   [11] notmuch-show: disable indentation of thread content (w/ notmuch-show-toggle-thread-indentation)
>  FAIL   [12] id buttonization
>         --- T450-emacs-show.13.OUTPUT   2016-10-06 01:42:21.329950576 +0000
>         +++ T450-emacs-show.13.EXPECTED 2016-10-06 01:42:21.329950576 +0000
>         @@ -23,4 +23,4 @@
>          cid:xxx
>          <<mid:abc>> <<mid:abc/def>>
>          <<mid:abc%20def>>
>         -<<mid:a>>bc. <<mid:abc>>, <<mid:abc>>;
>         +<<mid:abc>>. <<mid:abc>>, <<mid:abc>>;
> nil
>  PASS   [13] Show handles subprocess errors
>
> Any idea what's going on here? It's using emacs-25.1.
>

Unfortunately I think the test suite (and
notmuch-emacs generally) is not very well tested against emacs 25.

About the only thing I can contribute at this point is that the diff is
reversed (minor bug in the test suite) so the wrong looking deleted line
actually is the output from emacs 25.

d

Re: "id buttonization" test failure

[David Bremner]

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

Marius Bakke <m.bakke@fastmail.com> writes:

> Hi,
>
> I'm trying to run the test suite on GNU Guix and get a mysterious test
> failure in T450-emacs-show.sh:
>
> T450-emacs-show: Testing emacs notmuch-show view
>  [...]
>  PASS   [11] notmuch-show: disable indentation of thread content (w/ notmuch-show-toggle-thread-indentation)
>  FAIL   [12] id buttonization
>         --- T450-emacs-show.13.OUTPUT   2016-10-06 01:42:21.329950576 +0000
>         +++ T450-emacs-show.13.EXPECTED 2016-10-06 01:42:21.329950576 +0000
>         @@ -23,4 +23,4 @@
>          cid:xxx
>          <<mid:abc>> <<mid:abc/def>>
>          <<mid:abc%20def>>
>         -<<mid:a>>bc. <<mid:abc>>, <<mid:abc>>;
>         +<<mid:abc>>. <<mid:abc>>, <<mid:abc>>;
> nil
>  PASS   [13] Show handles subprocess errors
>
> Any idea what's going on here? It's using emacs-25.1.
>

Some small progress.

1) I can duplicate the test failure with emacs 25.1 built from source on
   debian testing.

2) I can interactively duplicate a similar, but not identical, bug as
   follows.

   - add the attached message (extracted from the test suite) to my mail
     store
   - open it notmuch-show (emacs 25.1, gtk/GUI)
   - go to the last line of the body. By using C-u C-x =, we can observe
     that the first and third copies of mid:abc are buttonized, but not
     the second.
   - this can also be seen by using the tab button.

3) The bug in 2) seems to be sensitive to the body text. If I delete the
   first 8 body lines, then all of the mid's on the last line are
   buttonized.


[-- Attachment #2: msg-002:2,S --]
[-- Type: application/octet-stream, Size: 542 bytes --]

From: Notmuch Test Suite <test_suite@notmuchmail.org>
To: Notmuch Test Suite <test_suite@notmuchmail.org>
Message-Id: <msg-002@notmuch-test-suite>
Subject: id buttonization
Date: Fri, 05 Jan 2001 15:43:55 +0000


id:abc
id:abc.def. id:abc,def, id:abc;def; id:abc:def:
id:foo@bar.?baz? id:foo@bar!.baz!
(id:foo@bar.baz) [id:foo@bar.baz]
id:foo@bar.baz...
id:2+2=5
id:=_-:/.[]@$%+
id:abc)def
id:ab"c def
id:"abc"def
id:"ab""c"def
id:"ab c"def
id:"abc".def
id:"abc
"
id:)
id:
cid:xxx
mid:abc mid:abc/def
mid:abc%20def
mid:abc. mid:abc, mid:abc;

Re: "id buttonization" test failure

[David Bremner]

David Bremner <david@tethera.net> writes:


> 2) I can interactively duplicate a similar, but not identical, bug as
>    follows.
>
>    - add the attached message (extracted from the test suite) to my mail
>      store
>    - open it notmuch-show (emacs 25.1, gtk/GUI)
>    - go to the last line of the body. By using C-u C-x =, we can observe
>      that the first and third copies of mid:abc are buttonized, but not
>      the second.
>    - this can also be seen by using the tab button.


The following "fixes" this test failure. This suggests to me something
that only fails when notmuch-show-buttonise-links is called from the C
redisplay code, and not when it's called from lisp.

diff --git a/test/T450-emacs-show.sh b/test/T450-emacs-show.sh
index 0342a87..afa75bd 100755
--- a/test/T450-emacs-show.sh
+++ b/test/T450-emacs-show.sh
@@ -129,7 +129,8 @@ cid:xxx
 mid:abc mid:abc/def
 mid:abc%20def
 mid:abc. mid:abc, mid:abc;"'
-test_emacs '(notmuch-show "id:'$gen_msg_id'")
+test_emacs '(jit-lock-debug-mode)
+	(notmuch-show "id:'$gen_msg_id'")
 	(notmuch-test-mark-links)
 	(test-visible-output "OUTPUT.raw")'
 cat <<EOF >EXPECTED

Re: "id buttonization" test failure

[David Bremner]

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

David Bremner <david@tethera.net> writes:

>
>
> The following "fixes" this test failure. This suggests to me something
> that only fails when notmuch-show-buttonise-links is called from the C
> redisplay code, and not when it's called from lisp.

The last paragraph of

    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20146#47

made a lightbulb turn on over my head. I _think_ the problem is that
that fontification expects to be called with the whole message, but jit
lock really doesn't guarantee that.

If you run the attached test with emacs25, you see the list of calls (in tmp.foo/MESSAGES)

fontifying from 1 to 501
fontifying from 1 to 1
fontifying from 62 to 69
fontifying from 494 to 516
fontifying from 187 to 189
fontifying from 195 to 197
fontifying from 198 to 200
fontifying from 210 to 212
fontifying from 214 to 216
fontifying from 226 to 228
fontifying from 230 to 232
fontifying from 586 to 588

and lots more, but the point is many of them are not the whole messages,
which would allow buttons to be created out of partial matches




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: foo.sh --]
[-- Type: text/x-sh, Size: 1455 bytes --]

#!/usr/bin/env bash

test_description="emacs notmuch-show view"
. ./test-lib.sh || exit 1

EXPECTED=$TEST_DIRECTORY/emacs-show.expected-output

add_email_corpus

test_begin_subtest "id buttonization"
add_message '[body]="
id:abc
id:abc.def. id:abc,def, id:abc;def; id:abc:def:
id:foo@bar.?baz? id:foo@bar!.baz!
(id:foo@bar.baz) [id:foo@bar.baz]
id:foo@bar.baz...
id:2+2=5
id:=_-:/.[]@$%+
id:abc)def
id:ab\"c def
id:\"abc\"def
id:\"ab\"\"c\"def
id:\"ab c\"def
id:\"abc\".def
id:\"abc
\"
id:)
id:
cid:xxx
mid:abc mid:abc/def
mid:abc%20def
mid:abc. mid:abc, mid:abc;"'
test_emacs '(notmuch-show "id:'$gen_msg_id'")
	(notmuch-test-mark-links)
	(test-visible-output "OUTPUT.raw")
	(with-current-buffer "*Messages*"
		  (test-output "MESSAGES"))

'
cat <<EOF >EXPECTED
Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-05) (inbox)
Subject: id buttonization
To: Notmuch Test Suite <test_suite@notmuchmail.org>
Date: GENERATED_DATE

<<id:abc>>
<<id:abc.def>>. <<id:abc,def>>, <<id:abc;def>>; <<id:abc:def>>:
<<id:foo@bar.?baz>>? <<id:foo@bar!.baz>>!
(<<id:foo@bar.baz>>) [<<id:foo@bar.baz>>]
<<id:foo@bar.baz>>...
<<id:2+2=5>>
<<id:=_-:/.[]@$%+>>
<<id:abc>>)def
<<id:ab"c>> def
<<id:"abc">>def
<<id:"ab""c">>def
<<id:"ab c">>def
<<id:"abc">>.def
id:"abc
"
id:)
id:
cid:xxx
<<mid:abc>> <<mid:abc/def>>
<<mid:abc%20def>>
<<mid:abc>>. <<mid:abc>>, <<mid:abc>>;
EOF
notmuch_date_sanitize < OUTPUT.raw > OUTPUT
test_expect_equal_file OUTPUT EXPECTED

test_done

[PATCH] emacs/show: force notmuch-show-buttonise-links to act on lines

[David Bremner]

This seems to fix a problem with emacs 25 creating partial buttons by
calling n-s-b-l with a region that does not include the whole button.
I'm not 100% sure it's legit to act outside the region passed by
jit-lock, but goto-address-fontify-region (where I borrowed the code
from) already does this, so this patch to not make things worse.
---
 emacs/notmuch-show.el | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

This makes the test suite pass with emacs 25 for me. If people think
this is sane, I'd like to do a bug fix release with this change.

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f2487ab..643dee6 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1174,13 +1174,15 @@ This also turns id:\"<message id>\"-parts and mid: links into
 buttons for a corresponding notmuch search."
   (goto-address-fontify-region start end)
   (save-excursion
-    (let (links)
-      (goto-char start)
-      (while (re-search-forward notmuch-id-regexp end t)
+    (let (links
+	  (beg-line (progn (goto-char start) (line-beginning-position)))
+	  (end-line (progn (goto-char end) (line-end-position))))
+      (goto-char beg-line)
+      (while (re-search-forward notmuch-id-regexp end-line t)
 	(push (list (match-beginning 0) (match-end 0)
 		    (match-string-no-properties 0)) links))
-      (goto-char start)
-      (while (re-search-forward notmuch-mid-regexp end t)
+      (goto-char beg-line)
+      (while (re-search-forward notmuch-mid-regexp end-line t)
 	(let* ((mid-cid (match-string-no-properties 1))
 	       (mid (save-match-data
 		      (string-match "^[^/]*" mid-cid)
-- 
2.9.3

Re: [PATCH] emacs/show: force notmuch-show-buttonise-links to act on lines

[Tomi Ollila]

On Mon, Oct 10 2016, David Bremner <david@tethera.net> wrote:

> This seems to fix a problem with emacs 25 creating partial buttons by
> calling n-s-b-l with a region that does not include the whole button.
> I'm not 100% sure it's legit to act outside the region passed by
> jit-lock, but goto-address-fontify-region (where I borrowed the code
> from) already does this, so this patch to not make things worse.
> ---

Applies cleanly and fixes the test in question on fedora 24

>  emacs/notmuch-show.el | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> This makes the test suite pass with emacs 25 for me. If people think
> this is sane, I'd like to do a bug fix release with this change.
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index f2487ab..643dee6 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1174,13 +1174,15 @@ This also turns id:\"<message id>\"-parts and mid: links into
>  buttons for a corresponding notmuch search."
>    (goto-address-fontify-region start end)
>    (save-excursion
> -    (let (links)
> -      (goto-char start)
> -      (while (re-search-forward notmuch-id-regexp end t)
> +    (let (links
> +	  (beg-line (progn (goto-char start) (line-beginning-position)))
> +	  (end-line (progn (goto-char end) (line-end-position))))
> +      (goto-char beg-line)
> +      (while (re-search-forward notmuch-id-regexp end-line t)
>  	(push (list (match-beginning 0) (match-end 0)
>  		    (match-string-no-properties 0)) links))
> -      (goto-char start)
> -      (while (re-search-forward notmuch-mid-regexp end t)
> +      (goto-char beg-line)
> +      (while (re-search-forward notmuch-mid-regexp end-line t)
>  	(let* ((mid-cid (match-string-no-properties 1))
>  	       (mid (save-match-data
>  		      (string-match "^[^/]*" mid-cid)
> -- 
> 2.9.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs/show: force notmuch-show-buttonise-links to act on lines

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Mon, Oct 10 2016, David Bremner <david@tethera.net> wrote:
>
>> This seems to fix a problem with emacs 25 creating partial buttons by
>> calling n-s-b-l with a region that does not include the whole button.
>> I'm not 100% sure it's legit to act outside the region passed by
>> jit-lock, but goto-address-fontify-region (where I borrowed the code
>> from) already does this, so this patch to not make things worse.
>> ---
>
> Applies cleanly and fixes the test in question on fedora 24

I was a bit worried about the performance impact, but according to
profiling it doesn't really seem to be a hotspot, 0% in profiling of
rending a 600 message thread. So unless I hear some cogent objections,
I'm going ahead with this.

d

Re: [PATCH] emacs/show: force notmuch-show-buttonise-links to act on lines

[David Bremner]

David Bremner <david@tethera.net> writes:

> This seems to fix a problem with emacs 25 creating partial buttons by
> calling n-s-b-l with a region that does not include the whole button.
> I'm not 100% sure it's legit to act outside the region passed by
> jit-lock, but goto-address-fontify-region (where I borrowed the code
> from) already does this, so this patch to not make things worse.

pushed to master and release

Re: "id buttonization" test failure

[David Bremner]

David Bremner <david@tethera.net> writes:

> David Bremner <david@tethera.net> writes:
>
>>
>>
>> The following "fixes" this test failure. This suggests to me something
>> that only fails when notmuch-show-buttonise-links is called from the C
>> redisplay code, and not when it's called from lisp.
>
> The last paragraph of
>
>     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20146#47
>
> made a lightbulb turn on over my head. I _think_ the problem is that
> that fontification expects to be called with the whole message, but jit
> lock really doesn't guarantee that.
>
> If you run the attached test with emacs25, you see the list of calls (in tmp.foo/MESSAGES)

This should now be fixed in master.

d