Re: [PATCH 6/6] lib: choose oldest message when breaking reference loops
On Tue, Apr 24 2018, David Bremner wrote: > Tomi Ollila writes: > >> Otherwise the series _looks_ good do me. The thing that disturbs me are >> these `strlen (in_reply_to)` contents. Perhaps SomeOne(TM) changes these >> to e.g. in_reply_to[0] in the future... >> > > That same file defines and uses an EMPTY_STRING macro. We should > probably be consistent, either use that macro everywehre or > nowhere. Perhaps move it to lib/notmuch-private.h? I looked macro replacement for in_reply_to[0] through internet, but weren't smart enough to git grep notmuch source tree. I'd say use it everwhere now that it is there (and move it...) SMOP (or not so) for someone(tm) > > d Tomi ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 6/6] lib: choose oldest message when breaking reference loops
Tomi Ollila writes: > Otherwise the series _looks_ good do me. The thing that disturbs me are > these `strlen (in_reply_to)` contents. Perhaps SomeOne(TM) changes these > to e.g. in_reply_to[0] in the future... > That same file defines and uses an EMPTY_STRING macro. We should probably be consistent, either use that macro everywehre or nowhere. Perhaps move it to lib/notmuch-private.h? d ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 6/6] lib: choose oldest message when breaking reference loops
On Fri, Apr 20 2018, David Bremner wrote:
> This preserves a sensible thread order
> ---
> lib/thread.cc| 33 -
> test/T050-new.sh | 1 -
> 2 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index dbac002f..a6dc4e5a 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -390,20 +390,18 @@ _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)
Our style *dictates* space after '!'.
> + 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))
> @@ -412,6 +410,23 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
> _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.
> + */
> +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) &&
Otherwise the series _looks_ good do me. The thing that disturbs me are
these `strlen (in_reply_to)` contents. Perhaps SomeOne(TM) changes these
to e.g. in_reply_to[0] in the future...
Tomi
> + 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);
> +
> /* XXX: After scanning through the entire list looking for parents
> * via "In-Reply-To", we should do a second pass that looks at the
> * list of messages IDs in the "References" header instead. (And
> 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...@example.org
> id:mid-loop...@example.org > OUTPUT"
>
> test_begin_subtest "reference loop ordered by date"
> -test_subtest_known_broken
> threadid=$(notmuch search --output=threads id:mid-loop...@example.org)
> notmuch show --format=mbox $threadid | grep '^Date' > OUTPUT
> cat < EXPECTED
> --
> 2.17.0
>
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 6/6] lib: choose oldest message when breaking reference loops
This preserves a sensible thread order
---
lib/thread.cc| 33 -
test/T050-new.sh | 1 -
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/lib/thread.cc b/lib/thread.cc
index dbac002f..a6dc4e5a 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -390,20 +390,18 @@ _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))
@@ -412,6 +410,23 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
_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.
+ */
+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))
+ _notmuch_message_add_reply (parent, message);
+else
+ _notmuch_message_list_add_message (thread->toplevel_list, message);
+
/* XXX: After scanning through the entire list looking for parents
* via "In-Reply-To", we should do a second pass that looks at the
* list of messages IDs in the "References" header instead. (And
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...@example.org
id:mid-loop...@example.org > OUTPUT"
test_begin_subtest "reference loop ordered by date"
-test_subtest_known_broken
threadid=$(notmuch search --output=threads id:mid-loop...@example.org)
notmuch show --format=mbox $threadid | grep '^Date' > OUTPUT
cat < EXPECTED
--
2.17.0
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
