Re: [PATCH 6/6] lib: choose oldest message when breaking reference loops

2018-04-25 Thread Tomi Ollila
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

2018-04-24 Thread David Bremner
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

2018-04-21 Thread Tomi Ollila
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

2018-04-20 Thread David Bremner
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