From 044cbd920c6c68e741f1fb261c2710a0afb1ebad Mon Sep 17 00:00:00 2001 From: David Bremner Date: Tue, 20 Mar 2018 18:22:34 -0300 Subject: [PATCH 1/6] test: two new messages for the 'broken' corpus These have an 'In-Reply-To' loop, which currently confuses "notmuch new". --- test/corpora/broken/loop/loop-12 | 8 ++++++++ test/corpora/broken/loop/loop-21 | 8 ++++++++ 2 files changed, 16 insertions(+) create mode 100644 test/corpora/broken/loop/loop-12 create mode 100644 test/corpora/broken/loop/loop-21 diff --git a/test/corpora/broken/loop/loop-12 b/test/corpora/broken/loop/loop-12 new file mode 100644 index 00000000..b5c3af7e --- /dev/null +++ b/test/corpora/broken/loop/loop-12 @@ -0,0 +1,8 @@ +From: Alice +To: Daniel +Subject: referencing in-reply-to-loop-21 +Message-ID: +In-Reply-To: +Date: Thu, 16 Jun 2016 22:14:41 -0400 + +Note Message-ID and In-Reply-To: in file in-reply-to-loop-21 diff --git a/test/corpora/broken/loop/loop-21 b/test/corpora/broken/loop/loop-21 new file mode 100644 index 00000000..234f0323 --- /dev/null +++ b/test/corpora/broken/loop/loop-21 @@ -0,0 +1,8 @@ +From: Alice +To: Daniel +Subject: referencing in-reply-to-loop-12 +Message-ID: +In-Reply-To: +Date: Fri, 17 Jun 2016 22:14:41 -0400 + +Note Message-ID and In-Reply-To: in file in-reply-to-loop-12 From ab55ca8e0a84b8e00e42860fa0025c1ae86b4478 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Tue, 20 Mar 2018 18:22:35 -0300 Subject: [PATCH 2/6] test: add known broken test for indexing an In-Reply-To loop. This documents the bug discussed in id:87d10042pu.fsf@curie.anarc.at --- test/T050-new.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/T050-new.sh b/test/T050-new.sh index cd522364..b9854978 100755 --- a/test/T050-new.sh +++ b/test/T050-new.sh @@ -354,4 +354,9 @@ exit status: 75 EOF test_expect_equal_file EXPECTED OUTPUT +add_email_corpus broken +test_begin_subtest "reference loop does not crash" +test_subtest_known_broken +test_expect_code 0 "notmuch show --format=json id:mid-loop-12@example.org id:mid-loop-21@example.org > OUTPUT" + test_done From 9293d6da27494d7b607c945c6678bc890749b94f Mon Sep 17 00:00:00 2001 From: David Bremner Date: Fri, 13 Apr 2018 22:08:05 -0300 Subject: [PATCH 3/6] lib: break reference loop by choosing arbitrary top level msg Other parts of notmuch (e.g. notmuch show) expect each thread to contain at least one top level message, and crash if this expectation is not met. --- lib/thread.cc | 8 +++++++- test/T050-new.sh | 1 - 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/thread.cc b/lib/thread.cc index 3561b27f..dbac002f 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -397,7 +397,13 @@ _resolve_thread_relationships (notmuch_thread_t *thread) for (node = thread->message_list->head; node; node = node->next) { message = node->message; in_reply_to = _notmuch_message_get_in_reply_to (message); - if (in_reply_to && strlen (in_reply_to) && + /* + * if we reach the end of the list without finding a top-level + * message, that means the thread is a cycle (or set of + * cycles) and any message can be considered top-level + */ + if ((thread->toplevel_list->head || node->next) && + in_reply_to && strlen (in_reply_to) && g_hash_table_lookup_extended (thread->message_hash, in_reply_to, NULL, (void **) &parent)) diff --git a/test/T050-new.sh b/test/T050-new.sh index b9854978..f3bfe7b1 100755 --- a/test/T050-new.sh +++ b/test/T050-new.sh @@ -356,7 +356,6 @@ test_expect_equal_file EXPECTED OUTPUT add_email_corpus broken test_begin_subtest "reference loop does not crash" -test_subtest_known_broken test_expect_code 0 "notmuch show --format=json id:mid-loop-12@example.org id:mid-loop-21@example.org > OUTPUT" test_done From 4e085b6d92909b8f6d4a4b18ccd97fac8b7df620 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Fri, 13 Apr 2018 22:46:09 -0300 Subject: [PATCH 4/6] test: add known broken test for thread ordering from a loop The previous loop handling code chooses the last message in the message list, which turns out to be the last in date order. See the comment in _notmuch_thread_create. --- test/T050-new.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/T050-new.sh b/test/T050-new.sh index f3bfe7b1..320a7646 100755 --- a/test/T050-new.sh +++ b/test/T050-new.sh @@ -358,4 +358,14 @@ add_email_corpus broken test_begin_subtest "reference loop does not crash" test_expect_code 0 "notmuch show --format=json id:mid-loop-12@example.org id:mid-loop-21@example.org > OUTPUT" +test_begin_subtest "reference loop ordered by date" +test_subtest_known_broken +threadid=$(notmuch search --output=threads id:mid-loop-12@example.org) +notmuch show --format=mbox $threadid | grep '^Date' > OUTPUT +cat < EXPECTED +Date: Thu, 16 Jun 2016 22:14:41 -0400 +Date: Fri, 17 Jun 2016 22:14:41 -0400 +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done From 491b1f4b4082bee18418942846ec6508856be7b4 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Fri, 20 Apr 2018 11:59:48 -0300 Subject: [PATCH 5/6] lib: choose oldest message when breaking reference loops This preserves a sensible thread order --- lib/thread.cc | 35 ++++++++++++++++++++++++++--------- test/T050-new.sh | 1 - 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lib/thread.cc b/lib/thread.cc index dbac002f..e961c76b 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -390,20 +390,37 @@ _thread_add_matched_message (notmuch_thread_t *thread, static void _resolve_thread_relationships (notmuch_thread_t *thread) { - notmuch_message_node_t *node; + notmuch_message_node_t *node, *first_node; notmuch_message_t *message, *parent; const char *in_reply_to; - for (node = thread->message_list->head; node; node = node->next) { + first_node = thread->message_list->head; + if (! first_node) + return; + + for (node = first_node->next; node; node = node->next) { message = node->message; in_reply_to = _notmuch_message_get_in_reply_to (message); - /* - * if we reach the end of the list without finding a top-level - * message, that means the thread is a cycle (or set of - * cycles) and any message can be considered top-level - */ - if ((thread->toplevel_list->head || node->next) && - in_reply_to && strlen (in_reply_to) && + if (in_reply_to && strlen (in_reply_to) && + g_hash_table_lookup_extended (thread->message_hash, + in_reply_to, NULL, + (void **) &parent)) + _notmuch_message_add_reply (parent, message); + else + _notmuch_message_list_add_message (thread->toplevel_list, message); + } + + /* + * if we reach the end of the list without finding a top-level + * message, that means the thread is a cycle (or set of cycles) + * and any message can be considered top-level. Choose the oldest + * message, which happens to be first in our list. + */ + if (first_node) { + message = first_node->message; + in_reply_to = _notmuch_message_get_in_reply_to (message); + if (thread->toplevel_list->head && + in_reply_to && strlen (in_reply_to) && g_hash_table_lookup_extended (thread->message_hash, in_reply_to, NULL, (void **) &parent)) diff --git a/test/T050-new.sh b/test/T050-new.sh index 320a7646..9025fa7a 100755 --- a/test/T050-new.sh +++ b/test/T050-new.sh @@ -359,7 +359,6 @@ test_begin_subtest "reference loop does not crash" test_expect_code 0 "notmuch show --format=json id:mid-loop-12@example.org id:mid-loop-21@example.org > OUTPUT" test_begin_subtest "reference loop ordered by date" -test_subtest_known_broken threadid=$(notmuch search --output=threads id:mid-loop-12@example.org) notmuch show --format=mbox $threadid | grep '^Date' > OUTPUT cat < EXPECTED From 15aaa41ce2b9667d86a81b3928a71f542ed448a1 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Fri, 13 Apr 2018 22:37:45 -0300 Subject: [PATCH 6/6] NEWS: add item for reference loop fix. --- NEWS | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/NEWS b/NEWS index 39ce7707..3e431658 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,14 @@ +Notmuch 0.26.2 (UNRELEASED) +=========================== + +Library Changes +--------------- + +Make thread indexing more robust against reference loops + + Choose a thread root by date in case of reference loops. Fix a + related abort in `notmuch show`. + Notmuch 0.26.1 (2018-04-02) ===========================