threading fixes v4

David Bremner david at tethera.net
Tue Sep 4 03:57:06 PDT 2018


This is mainly coding style issues pointed out by Tomi, along with
moving some of the low level list access into two new functions.  The
interdiff is a bit noisy since I dropped the creation of
debug_print.{c,h}.

I'm going to optimistically claim this version is ready to go, but
feel free to point out any issues.

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 90d69453..d64aa85b 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -1,7 +1,7 @@
 #include <assert.h>
 #include <string.h>
 #include <stdio.h>
-#include "debug_print.h"
+#include "error_util.h"
 #include "command-line-arguments.h"
 
 typedef enum {
diff --git a/lib/message-id.c b/lib/message-id.c
index a1dce9c8..e71ce9f4 100644
--- a/lib/message-id.c
+++ b/lib/message-id.c
@@ -100,7 +100,6 @@ char *
 _notmuch_message_id_parse_strict (void *ctx, const char *message_id)
 {
     const char *s, *end;
-    char *result;
 
     if (message_id == NULL || *message_id == '\0')
 	return NULL;
@@ -118,11 +117,10 @@ _notmuch_message_id_parse_strict (void *ctx, const char *message_id)
     if (*end != '>')
 	return NULL;
     else {
-	const char *last =  skip_space (end+1);
+	const char *last = skip_space (end + 1);
 	if (*last != '\0')
 	    return NULL;
     }
 
-    result = talloc_strndup (ctx, s, end - s);
-    return result;
+    return talloc_strndup (ctx, s, end - s);
 }
diff --git a/lib/message.cc b/lib/message.cc
index 5e17029a..6f2f6345 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -637,7 +637,12 @@ _cmpmsg (const void *pa, const void *pb)
     time_t time_a = notmuch_message_get_date (*a);
     time_t time_b = notmuch_message_get_date (*b);
 
-    return (int) difftime (time_a, time_b);
+    if (time_a == time_b)
+	return 0;
+    else if (time_a < time_b)
+	return -1;
+    else
+	return 1;
 }
 
 notmuch_message_list_t *
diff --git a/lib/messages.c b/lib/messages.c
index a88f974f..04fa19f8 100644
--- a/lib/messages.c
+++ b/lib/messages.c
@@ -56,6 +56,15 @@ _notmuch_message_list_add_message (notmuch_message_list_t *list,
     list->tail = &node->next;
 }
 
+bool
+_notmuch_message_list_empty (notmuch_message_list_t *list)
+{
+    if (list == NULL)
+	return TRUE;
+
+    return (list->head == NULL);
+}
+
 notmuch_messages_t *
 _notmuch_messages_create (notmuch_message_list_t *list)
 {
@@ -101,6 +110,18 @@ notmuch_messages_valid (notmuch_messages_t *messages)
     return (messages->iterator != NULL);
 }
 
+bool
+_notmuch_messages_has_next (notmuch_messages_t *messages)
+{
+    if (! notmuch_messages_valid (messages))
+	return false;
+
+    if (! messages->is_of_list_type)
+	INTERNAL_ERROR("_notmuch_messages_has_next not implimented for msets");
+
+    return (messages->iterator->next != NULL);
+}
+
 notmuch_message_t *
 notmuch_messages_get (notmuch_messages_t *messages)
 {
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 5bbaa292..df32d39c 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -50,12 +50,13 @@ NOTMUCH_BEGIN_DECLS
 #include "gmime-extra.h"
 
 #include "xutil.h"
-#include "debug_print.h"
+#include "error_util.h"
 #include "string-util.h"
 #include "crypto.h"
 
 #ifdef DEBUG
 # define DEBUG_DATABASE_SANITY 1
+# define DEBUG_THREADING 1
 # define DEBUG_QUERY 1
 #endif
 
@@ -476,6 +477,9 @@ struct _notmuch_messages {
 notmuch_message_list_t *
 _notmuch_message_list_create (const void *ctx);
 
+bool
+_notmuch_message_list_empty (notmuch_message_list_t *list);
+
 void
 _notmuch_message_list_add_message (notmuch_message_list_t *list,
 				   notmuch_message_t *message);
@@ -483,6 +487,9 @@ _notmuch_message_list_add_message (notmuch_message_list_t *list,
 notmuch_messages_t *
 _notmuch_messages_create (notmuch_message_list_t *list);
 
+bool
+_notmuch_messages_has_next (notmuch_messages_t *messages);
+
 /* query.cc */
 
 bool
diff --git a/lib/thread.cc b/lib/thread.cc
index e4ab4319..47c90664 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -24,6 +24,12 @@
 #include <gmime/gmime.h>
 #include <glib.h> /* GHashTable */
 
+#ifdef DEBUG_THREADING
+#define THREAD_DEBUG(format, ...) fprintf(stderr, format " (%s).\n", ##__VA_ARGS__, __location__)
+#else
+#define THREAD_DEBUG(format, ...) do {} while (0) /* ignored */
+#endif
+
 #define EMPTY_STRING(s) ((s)[0] == '\0')
 
 struct _notmuch_thread {
@@ -393,10 +399,10 @@ _parent_via_in_reply_to (notmuch_thread_t *thread, notmuch_message_t *message) {
     const char *in_reply_to;
 
     in_reply_to = _notmuch_message_get_in_reply_to (message);
-    DEBUG_PRINTF("checking message = %s in_reply_to=%s\n",
+    THREAD_DEBUG("checking message = %s in_reply_to=%s\n",
 		 notmuch_message_get_message_id (message), in_reply_to);
 
-    if (in_reply_to && strlen (in_reply_to) &&
+    if (in_reply_to && (! EMPTY_STRING(in_reply_to)) &&
 	g_hash_table_lookup_extended (thread->message_hash,
 				      in_reply_to, NULL,
 				      (void **) &parent)) {
@@ -416,31 +422,31 @@ _parent_or_toplevel (notmuch_thread_t *thread, notmuch_message_t *message)
     const notmuch_string_list_t *references =
 	_notmuch_message_get_references (message);
 
-    DEBUG_PRINTF("trying to reparent via references: %s\n",
+    THREAD_DEBUG("trying to reparent via references: %s\n",
 		     notmuch_message_get_message_id (message));
 
     for (notmuch_string_node_t *ref_node = references->head;
 	 ref_node; ref_node = ref_node->next) {
-	DEBUG_PRINTF("checking reference=%s\n", ref_node->string);
+	THREAD_DEBUG("checking reference=%s\n", ref_node->string);
 	if ((g_hash_table_lookup_extended (thread->message_hash,
 					   ref_node->string, NULL,
 					   (void **) &new_parent))) {
 	    size_t new_depth = _notmuch_message_get_thread_depth (new_parent);
-	    DEBUG_PRINTF("got depth %lu\n", new_depth);
+	    THREAD_DEBUG("got depth %lu\n", new_depth);
 	    if (new_depth > max_depth || !parent) {
-		DEBUG_PRINTF("adding at depth %lu parent=%s\n", new_depth, ref_node->string);
+		THREAD_DEBUG("adding at depth %lu parent=%s\n", new_depth, ref_node->string);
 		max_depth = new_depth;
 		parent = new_parent;
 	    }
 	}
     }
     if (parent) {
-	DEBUG_PRINTF("adding reply %s to parent=%s\n",
+	THREAD_DEBUG("adding reply %s to parent=%s\n",
 		 notmuch_message_get_message_id (message),
 		 notmuch_message_get_message_id (parent));
 	_notmuch_message_add_reply (parent, message);
     } else {
-	DEBUG_PRINTF("adding as toplevel %s\n",
+	THREAD_DEBUG("adding as toplevel %s\n",
 		 notmuch_message_get_message_id (message));
 	_notmuch_message_list_add_message (thread->toplevel_list, message);
     }
@@ -475,13 +481,14 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
      */
     if (first_node) {
 	message = first_node->message;
-	DEBUG_PRINTF("checking first message  %s\n",
+	THREAD_DEBUG("checking first message  %s\n",
 		     notmuch_message_get_message_id (message));
 
-	if (! maybe_toplevel_list->head ||
+        if (_notmuch_message_list_empty (maybe_toplevel_list) ||
 	    ! _parent_via_in_reply_to (thread, message)) {
-	    DEBUG_PRINTF("adding first message as toplevel = %s\n",
-		     notmuch_message_get_message_id (message));
+
+	    THREAD_DEBUG("adding first message as toplevel = %s\n",
+			 notmuch_message_get_message_id (message));
 	    _notmuch_message_list_add_message (maybe_toplevel_list, message);
 	}
     }
@@ -498,8 +505,7 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
 	 notmuch_messages_valid (roots);
 	 notmuch_messages_move_to_next (roots)) {
 	notmuch_message_t *message = notmuch_messages_get (roots);
-	/* XXX TODO: cleaner way to test iterator for last, list for emptiness  */
-	if (roots->iterator->next || thread->toplevel_list->head)
+	if (_notmuch_messages_has_next (roots) || ! _notmuch_message_list_empty (thread->toplevel_list))
 	    _parent_or_toplevel (thread, message);
 	else
 	    _notmuch_message_list_add_message (thread->toplevel_list, message);
diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 4688c0ab..5d6bea7e 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -222,5 +222,4 @@ End of search results.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-
 test_done


More information about the notmuch mailing list