[PATCH 0/6] API for iterating over all messages in a thread

[PATCH 0/6] API for iterating over all messages in a thread

[Austin Clements]

This series adds a library API for iterating over all messages in a
thread in sorted order.  This is easy for the library to provide and
difficult to obtain from the current API.  Plus, if you don't count
the code added to the bindings, this series is actually a net
decrease of 4 lines of code because of simplifications it enables.

Do we want the API to do more?  Currently it's very minimal, but I can
imagine two ways it could be generalized.  It could take an argument
to indicate which message list to return, which could be all messages,
matched messages, top-level messages, or maybe even unmatched messages
(possibly all in terms of message flags).  It could also take an
argument indicating the desired sort order.  Currently, the caller can
use existing message flag APIs to distinguish matched and unmatched
messages and there's a separate function for the top-level messages.
However, if the API could do all of these things, it would subsume
various other API functions, such as notmuch_thread_get_*_date.

Also, is this the right name for the new API?  In particular, if we do
later want to add a function that returns, say, the list of matched
messages, we'll have a convention collision with
notmuch_thread_get_matched_messages, which returns only a count.

[PATCH 1/6] lib: Clean up error handling in _notmuch_thread_create

[Austin Clements]

Previously, there were various opportunities for memory leaks in the
error-handling paths of this function.  Use a local talloc context and
some reparenting to make eliminate these leaks, while keeping the
control flow simple.
---
 lib/thread.cc |   33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index e976d64..aed87b1 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -406,7 +406,8 @@ _notmuch_thread_create (void *ctx,
 			notmuch_string_list_t *exclude_terms,
 			notmuch_sort_t sort)
 {
-    notmuch_thread_t *thread;
+    void *local = talloc_new (ctx);
+    notmuch_thread_t *thread = NULL;
     notmuch_message_t *seed_message;
     const char *thread_id;
     char *thread_id_query_string;
@@ -415,24 +416,23 @@ _notmuch_thread_create (void *ctx,
     notmuch_messages_t *messages;
     notmuch_message_t *message;
 
-    seed_message = _notmuch_message_create (ctx, notmuch, seed_doc_id, NULL);
+    seed_message = _notmuch_message_create (local, notmuch, seed_doc_id, NULL);
     if (! seed_message)
 	INTERNAL_ERROR ("Thread seed message %u does not exist", seed_doc_id);
 
     thread_id = notmuch_message_get_thread_id (seed_message);
-    thread_id_query_string = talloc_asprintf (ctx, "thread:%s", thread_id);
+    thread_id_query_string = talloc_asprintf (local, "thread:%s", thread_id);
     if (unlikely (thread_id_query_string == NULL))
-	return NULL;
+	goto DONE;
 
-    thread_id_query = notmuch_query_create (notmuch, thread_id_query_string);
+    thread_id_query = talloc_steal (
+	local, notmuch_query_create (notmuch, thread_id_query_string));
     if (unlikely (thread_id_query == NULL))
-	return NULL;
+	goto DONE;
 
-    talloc_free (thread_id_query_string);
-
-    thread = talloc (ctx, notmuch_thread_t);
+    thread = talloc (local, notmuch_thread_t);
     if (unlikely (thread == NULL))
-	return NULL;
+	goto DONE;
 
     talloc_set_destructor (thread, _notmuch_thread_destructor);
 
@@ -451,8 +451,10 @@ _notmuch_thread_create (void *ctx,
 					  free, NULL);
 
     thread->message_list = _notmuch_message_list_create (thread);
-    if (unlikely (thread->message_list == NULL))
-	return NULL;
+    if (unlikely (thread->message_list == NULL)) {
+	thread = NULL;
+	goto DONE;
+    }
 
     thread->message_hash = g_hash_table_new_full (g_str_hash, g_str_equal,
 						  free, NULL);
@@ -489,12 +491,15 @@ _notmuch_thread_create (void *ctx,
 	_notmuch_message_close (message);
     }
 
-    notmuch_query_destroy (thread_id_query);
-
     _resolve_thread_authors_string (thread);
 
     _resolve_thread_relationships (thread);
 
+    /* Commit to returning thread. */
+    talloc_steal (ctx, thread);
+
+  DONE:
+    talloc_free (local);
     return thread;
 }
 
-- 
1.7.10.4

Re: [PATCH 1/6] lib: Clean up error handling in _notmuch_thread_create

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> Previously, there were various opportunities for memory leaks in the
> error-handling paths of this function.  Use a local talloc context and
> some reparenting to make eliminate these leaks, while keeping the
> control flow simple.

pushed all but the python bindings patch.

Re: [PATCH 1/6] lib: Clean up error handling in _notmuch_thread_create

[David Bremner]

David Bremner <david@tethera.net> writes:

> Austin Clements <amdragon@MIT.EDU> writes:
>
>> Previously, there were various opportunities for memory leaks in the
>> error-handling paths of this function.  Use a local talloc context and
>> some reparenting to make eliminate these leaks, while keeping the
>> control flow simple.
>
> pushed all but the python bindings patch.

any comments on that patch, Justus?

For reference, id:1353819427-13182-6-git-send-email-amdragon@mit.edu

d

[PATCH 2/6] lib: Separate list of all messages from top-level messages

[Austin Clements]

Previously, thread.cc built up a list of all messages, then
proceeded to tear it apart to transform it into a list of
top-level messages.  Now we simply build a new list of top-level
messages.

This simplifies the interface to _notmuch_message_add_reply,
eliminates the pointer acrobatics from
_resolve_thread_relationships, and will enable us to do things
with the list of all messages in the following patches.
---
 lib/message.cc        |    4 ++--
 lib/notmuch-private.h |    2 +-
 lib/thread.cc         |   29 ++++++++++++++---------------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 978de06..171c580 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -462,9 +462,9 @@ notmuch_message_get_thread_id (notmuch_message_t *message)
 
 void
 _notmuch_message_add_reply (notmuch_message_t *message,
-			    notmuch_message_node_t *reply)
+			    notmuch_message_t *reply)
 {
-    _notmuch_message_list_append (message->replies, reply);
+    _notmuch_message_list_add_message (message->replies, reply);
 }
 
 notmuch_messages_t *
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 7a409f5..c054a0e 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -462,7 +462,7 @@ _notmuch_doc_id_set_remove (notmuch_doc_id_set_t *doc_ids,
 
 void
 _notmuch_message_add_reply (notmuch_message_t *message,
-			    notmuch_message_node_t *reply);
+			    notmuch_message_t *reply);
 
 /* sha1.c */
 
diff --git a/lib/thread.cc b/lib/thread.cc
index aed87b1..45a7d1d 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -35,7 +35,11 @@ struct visible _notmuch_thread {
     char *authors;
     GHashTable *tags;
 
+    /* All messages, oldest first. */
     notmuch_message_list_t *message_list;
+    /* Top-level messages, oldest first. */
+    notmuch_message_list_t *toplevel_list;
+
     GHashTable *message_hash;
     int total_messages;
     int matched_messages;
@@ -345,29 +349,22 @@ _thread_add_matched_message (notmuch_thread_t *thread,
 }
 
 static void
-_resolve_thread_relationships (unused (notmuch_thread_t *thread))
+_resolve_thread_relationships (notmuch_thread_t *thread)
 {
-    notmuch_message_node_t **prev, *node;
+    notmuch_message_node_t *node;
     notmuch_message_t *message, *parent;
     const char *in_reply_to;
 
-    prev = &thread->message_list->head;
-    while ((node = *prev)) {
+    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) &&
 	    g_hash_table_lookup_extended (thread->message_hash,
 					  in_reply_to, NULL,
 					  (void **) &parent))
-	{
-	    *prev = node->next;
-	    if (thread->message_list->tail == &node->next)
-		thread->message_list->tail = prev;
-	    node->next = NULL;
-	    _notmuch_message_add_reply (parent, node);
-	} else {
-	    prev = &((*prev)->next);
-	}
+	    _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
@@ -451,7 +448,9 @@ _notmuch_thread_create (void *ctx,
 					  free, NULL);
 
     thread->message_list = _notmuch_message_list_create (thread);
-    if (unlikely (thread->message_list == NULL)) {
+    thread->toplevel_list = _notmuch_message_list_create (thread);
+    if (unlikely (thread->message_list == NULL ||
+		  thread->toplevel_list == NULL)) {
 	thread = NULL;
 	goto DONE;
     }
@@ -506,7 +505,7 @@ _notmuch_thread_create (void *ctx,
 notmuch_messages_t *
 notmuch_thread_get_toplevel_messages (notmuch_thread_t *thread)
 {
-    return _notmuch_messages_create (thread->message_list);
+    return _notmuch_messages_create (thread->toplevel_list);
 }
 
 const char *
-- 
1.7.10.4

[PATCH 3/6] lib: Eliminate _notmuch_message_list_append

[Austin Clements]

This API invited micro-optimized and complicated list pointer
manipulation and is no longer used.
---
 lib/messages.c        |   17 +++--------------
 lib/notmuch-private.h |    4 ----
 2 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/lib/messages.c b/lib/messages.c
index 1121864..0eee569 100644
--- a/lib/messages.c
+++ b/lib/messages.c
@@ -42,19 +42,7 @@ _notmuch_message_list_create (const void *ctx)
     return list;
 }
 
-/* Append a single 'node' to the end of 'list'.
- */
-void
-_notmuch_message_list_append (notmuch_message_list_t *list,
-			      notmuch_message_node_t *node)
-{
-    *(list->tail) = node;
-    list->tail = &node->next;
-}
-
-/* Allocate a new node for 'message' and append it to the end of
- * 'list'.
- */
+/* Append 'message' to the end of 'list'. */
 void
 _notmuch_message_list_add_message (notmuch_message_list_t *list,
 				   notmuch_message_t *message)
@@ -64,7 +52,8 @@ _notmuch_message_list_add_message (notmuch_message_list_t *list,
     node->message = message;
     node->next = NULL;
 
-    _notmuch_message_list_append (list, node);
+    *(list->tail) = node;
+    list->tail = &node->next;
 }
 
 notmuch_messages_t *
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index c054a0e..f38ccb3 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -429,10 +429,6 @@ notmuch_message_list_t *
 _notmuch_message_list_create (const void *ctx);
 
 void
-_notmuch_message_list_append (notmuch_message_list_t *list,
-			      notmuch_message_node_t *node);
-
-void
 _notmuch_message_list_add_message (notmuch_message_list_t *list,
 				   notmuch_message_t *message);
 
-- 
1.7.10.4

[PATCH 4/6] lib: Add an iterator over all messages in a thread

[Austin Clements]

Previously, getting the list of all messages in a thread required
recursively traversing the thread's message hierarchy, which was both
difficult and resulted in messages being out of order.  This adds a
public function to retrieve an iterator over all of the messages in a
thread in oldest-first order.
---
 lib/notmuch.h |   13 +++++++------
 lib/thread.cc |    6 ++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3633bed..3739336 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -719,20 +719,21 @@ int
 notmuch_thread_get_total_messages (notmuch_thread_t *thread);
 
 /* Get a notmuch_messages_t iterator for the top-level messages in
- * 'thread'.
+ * 'thread' in oldest-first order.
  *
  * This iterator will not necessarily iterate over all of the messages
  * in the thread. It will only iterate over the messages in the thread
  * which are not replies to other messages in the thread.
- *
- * To iterate over all messages in the thread, the caller will need to
- * iterate over the result of notmuch_message_get_replies for each
- * top-level message (and do that recursively for the resulting
- * messages, etc.).
  */
 notmuch_messages_t *
 notmuch_thread_get_toplevel_messages (notmuch_thread_t *thread);
 
+/* Get a notmuch_thread_t iterator for all messages in 'thread' in
+ * oldest-first order.
+ */
+notmuch_messages_t *
+notmuch_thread_get_messages (notmuch_thread_t *thread);
+
 /* Get the number of messages in 'thread' that matched the search.
  *
  * This count includes only the messages in this thread that were
diff --git a/lib/thread.cc b/lib/thread.cc
index 45a7d1d..c126aac 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -508,6 +508,12 @@ notmuch_thread_get_toplevel_messages (notmuch_thread_t *thread)
     return _notmuch_messages_create (thread->toplevel_list);
 }
 
+notmuch_messages_t *
+notmuch_thread_get_messages (notmuch_thread_t *thread)
+{
+    return _notmuch_messages_create (thread->message_list);
+}
+
 const char *
 notmuch_thread_get_thread_id (notmuch_thread_t *thread)
 {
-- 
1.7.10.4

[PATCH 5/6] python: Add bindings for notmuch_thread_get_messages

[Austin Clements]

---
 bindings/python/notmuch/thread.py |   27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/bindings/python/notmuch/thread.py b/bindings/python/notmuch/thread.py
index 009cb2b..0454dbd 100644
--- a/bindings/python/notmuch/thread.py
+++ b/bindings/python/notmuch/thread.py
@@ -128,11 +128,6 @@ class Thread(object):
            in the thread. It will only iterate over the messages in the thread
            which are not replies to other messages in the thread.
 
-           To iterate over all messages in the thread, the caller will need to
-           iterate over the result of :meth:`Message.get_replies` for each
-           top-level message (and do that recursively for the resulting
-           messages, etc.).
-
         :returns: :class:`Messages`
         :raises: :exc:`NotInitializedError` if query is not initialized
         :raises: :exc:`NullPointerError` if search_messages failed
@@ -147,6 +142,28 @@ class Thread(object):
 
         return Messages(msgs_p, self)
 
+    """notmuch_thread_get_messages"""
+    _get_messages = nmlib.notmuch_thread_get_messages
+    _get_messages.argtypes = [NotmuchThreadP]
+    _get_messages.restype = NotmuchMessagesP
+
+    def get_messages(self):
+        """Returns a :class:`Messages` iterator for all messages in 'thread'
+
+        :returns: :class:`Messages`
+        :raises: :exc:`NotInitializedError` if query is not initialized
+        :raises: :exc:`NullPointerError` if get_messages failed
+        """
+        if not self._thread:
+            raise NotInitializedError()
+
+        msgs_p = Thread._get_messages(self._thread)
+
+        if not msgs_p:
+            raise NullPointerError()
+
+        return Messages(msgs_p, self)
+
     _get_matched_messages = nmlib.notmuch_thread_get_matched_messages
     _get_matched_messages.argtypes = [NotmuchThreadP]
     _get_matched_messages.restype = c_int
-- 
1.7.10.4

Re: [PATCH 5/6] python: Add bindings for notmuch_thread_get_messages

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> ---
>  bindings/python/notmuch/thread.py |   27 ++++++++++++++++++++++-----

Justus OKed this privately.

d

Re: [PATCH 5/6] python: Add bindings for notmuch_thread_get_messages

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> ---
>  bindings/python/notmuch/thread.py |   27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)

Pushed, finally.

d

[PATCH 6/6] ruby: Add bindings for notmuch_thread_get_messages

[Austin Clements]

---
 bindings/ruby/defs.h   |    3 +++
 bindings/ruby/init.c   |    1 +
 bindings/ruby/thread.c |   20 ++++++++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index fe81b3f..5b44585 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -262,6 +262,9 @@ VALUE
 notmuch_rb_thread_get_toplevel_messages (VALUE self);
 
 VALUE
+notmuch_rb_thread_get_messages (VALUE self);
+
+VALUE
 notmuch_rb_thread_get_matched_messages (VALUE self);
 
 VALUE
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index f4931d3..663271d 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -306,6 +306,7 @@ Init_notmuch (void)
     rb_define_method (notmuch_rb_cThread, "thread_id", notmuch_rb_thread_get_thread_id, 0); /* in thread.c */
     rb_define_method (notmuch_rb_cThread, "total_messages", notmuch_rb_thread_get_total_messages, 0); /* in thread.c */
     rb_define_method (notmuch_rb_cThread, "toplevel_messages", notmuch_rb_thread_get_toplevel_messages, 0); /* in thread.c */
+    rb_define_method (notmuch_rb_cThread, "messages", notmuch_rb_thread_get_messages, 0); /* in thread.c */
     rb_define_method (notmuch_rb_cThread, "matched_messages", notmuch_rb_thread_get_matched_messages, 0); /* in thread.c */
     rb_define_method (notmuch_rb_cThread, "authors", notmuch_rb_thread_get_authors, 0); /* in thread.c */
     rb_define_method (notmuch_rb_cThread, "subject", notmuch_rb_thread_get_subject, 0); /* in thread.c */
diff --git a/bindings/ruby/thread.c b/bindings/ruby/thread.c
index efe5aaf..56616d9 100644
--- a/bindings/ruby/thread.c
+++ b/bindings/ruby/thread.c
@@ -92,6 +92,26 @@ notmuch_rb_thread_get_toplevel_messages (VALUE self)
 }
 
 /*
+ * call-seq: THREAD.messages => MESSAGES
+ *
+ * Get a Notmuch::Messages iterator for the all messages in thread.
+ */
+VALUE
+notmuch_rb_thread_get_messages (VALUE self)
+{
+    notmuch_messages_t *messages;
+    notmuch_thread_t *thread;
+
+    Data_Get_Notmuch_Thread (self, thread);
+
+    messages = notmuch_thread_get_messages (thread);
+    if (!messages)
+	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
+
+    return Data_Wrap_Struct (notmuch_rb_cMessages, NULL, NULL, messages);
+}
+
+/*
  * call-seq: THREAD.matched_messages => fixnum
  *
  * Get the number of messages in thread that matched the search
-- 
1.7.10.4

Re: [PATCH 6/6] ruby: Add bindings for notmuch_thread_get_messages

[Ali Polatel]

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

LGTM. Thanks for showing love to the ruby bindings as I was away.

On Sat, Nov 24, 2012 at 11:57:07PM -0500, Austin Clements wrote:
>---
> bindings/ruby/defs.h   |    3 +++
> bindings/ruby/init.c   |    1 +
> bindings/ruby/thread.c |   20 ++++++++++++++++++++
> 3 files changed, 24 insertions(+)
>
>diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
>index fe81b3f..5b44585 100644
>--- a/bindings/ruby/defs.h
>+++ b/bindings/ruby/defs.h
>@@ -262,6 +262,9 @@ VALUE
> notmuch_rb_thread_get_toplevel_messages (VALUE self);
>
> VALUE
>+notmuch_rb_thread_get_messages (VALUE self);
>+
>+VALUE
> notmuch_rb_thread_get_matched_messages (VALUE self);
>
> VALUE
>diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
>index f4931d3..663271d 100644
>--- a/bindings/ruby/init.c
>+++ b/bindings/ruby/init.c
>@@ -306,6 +306,7 @@ Init_notmuch (void)
>     rb_define_method (notmuch_rb_cThread, "thread_id", notmuch_rb_thread_get_thread_id, 0); /* in thread.c */
>     rb_define_method (notmuch_rb_cThread, "total_messages", notmuch_rb_thread_get_total_messages, 0); /* in thread.c */
>     rb_define_method (notmuch_rb_cThread, "toplevel_messages", notmuch_rb_thread_get_toplevel_messages, 0); /* in thread.c */
>+    rb_define_method (notmuch_rb_cThread, "messages", notmuch_rb_thread_get_messages, 0); /* in thread.c */
>     rb_define_method (notmuch_rb_cThread, "matched_messages", notmuch_rb_thread_get_matched_messages, 0); /* in thread.c */
>     rb_define_method (notmuch_rb_cThread, "authors", notmuch_rb_thread_get_authors, 0); /* in thread.c */
>     rb_define_method (notmuch_rb_cThread, "subject", notmuch_rb_thread_get_subject, 0); /* in thread.c */
>diff --git a/bindings/ruby/thread.c b/bindings/ruby/thread.c
>index efe5aaf..56616d9 100644
>--- a/bindings/ruby/thread.c
>+++ b/bindings/ruby/thread.c
>@@ -92,6 +92,26 @@ notmuch_rb_thread_get_toplevel_messages (VALUE self)
> }
>
> /*
>+ * call-seq: THREAD.messages => MESSAGES
>+ *
>+ * Get a Notmuch::Messages iterator for the all messages in thread.
>+ */
>+VALUE
>+notmuch_rb_thread_get_messages (VALUE self)
>+{
>+    notmuch_messages_t *messages;
>+    notmuch_thread_t *thread;
>+
>+    Data_Get_Notmuch_Thread (self, thread);
>+
>+    messages = notmuch_thread_get_messages (thread);
>+    if (!messages)
>+	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
>+
>+    return Data_Wrap_Struct (notmuch_rb_cMessages, NULL, NULL, messages);
>+}
>+
>+/*
>  * call-seq: THREAD.matched_messages => fixnum
>  *
>  * Get the number of messages in thread that matched the search
>-- 
>1.7.10.4
>
>_______________________________________________
>notmuch mailing list
>notmuch@notmuchmail.org
>http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/6] API for iterating over all messages in a thread

[Mark Walters]

Hi

This series looks good to me (I have not reviewed the two bindings
patches). Patch 2 looks like it makes things much easier to follow than
the current code (if I understood the current pointer stuff it
constructs the top-level list by doing pointer stuff to remove all
messages which are replies from the complete message list). Indeed, the
diff is more complicated than the new code!

On Sun, 25 Nov 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This series adds a library API for iterating over all messages in a
> thread in sorted order.  This is easy for the library to provide and
> difficult to obtain from the current API.  Plus, if you don't count
> the code added to the bindings, this series is actually a net
> decrease of 4 lines of code because of simplifications it enables.
>
> Do we want the API to do more?  Currently it's very minimal, but I can
> imagine two ways it could be generalized.  It could take an argument
> to indicate which message list to return, which could be all messages,
> matched messages, top-level messages, or maybe even unmatched messages
> (possibly all in terms of message flags).  It could also take an
> argument indicating the desired sort order.  Currently, the caller can
> use existing message flag APIs to distinguish matched and unmatched
> messages and there's a separate function for the top-level messages.
> However, if the API could do all of these things, it would subsume
> various other API functions, such as notmuch_thread_get_*_date.

I don't know if this is the right API. For the matched message etc I
think using the existing message flag APIs is simple enough. I am not
sure about sort orders though: that looks like it would be much easier
for the caller to have the correct sort by I am not sure what users
would need it.

Best wishes

Mark




>
> Also, is this the right name for the new API?  In particular, if we do
> later want to add a function that returns, say, the list of matched
> messages, we'll have a convention collision with
> notmuch_thread_get_matched_messages, which returns only a count.

Re: [PATCH 0/6] API for iterating over all messages in a thread

[Austin Clements]

Quoth Mark Walters on Nov 25 at  2:31 pm:
> 
> Hi
> 
> This series looks good to me (I have not reviewed the two bindings
> patches). Patch 2 looks like it makes things much easier to follow than
> the current code (if I understood the current pointer stuff it
> constructs the top-level list by doing pointer stuff to remove all
> messages which are replies from the complete message list). Indeed, the
> diff is more complicated than the new code!
> 
> On Sun, 25 Nov 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This series adds a library API for iterating over all messages in a
> > thread in sorted order.  This is easy for the library to provide and
> > difficult to obtain from the current API.  Plus, if you don't count
> > the code added to the bindings, this series is actually a net
> > decrease of 4 lines of code because of simplifications it enables.
> >
> > Do we want the API to do more?  Currently it's very minimal, but I can
> > imagine two ways it could be generalized.  It could take an argument
> > to indicate which message list to return, which could be all messages,
> > matched messages, top-level messages, or maybe even unmatched messages
> > (possibly all in terms of message flags).  It could also take an
> > argument indicating the desired sort order.  Currently, the caller can
> > use existing message flag APIs to distinguish matched and unmatched
> > messages and there's a separate function for the top-level messages.
> > However, if the API could do all of these things, it would subsume
> > various other API functions, such as notmuch_thread_get_*_date.
> 
> I don't know if this is the right API. For the matched message etc I
> think using the existing message flag APIs is simple enough. I am not
> sure about sort orders though: that looks like it would be much easier
> for the caller to have the correct sort by I am not sure what users
> would need it.

For sort order, I would be inclined to simply construct the reverse
list the first time a caller asks for it.  Theoretically the caller
could do this just as easily as the library, except that we don't
expose the list routines.

If I do add sort order, I would also want to add some control over
which list is returned, since it would be asymmetric to be able to
request all messages in either order, but top-level messages only in
oldest-first.  I think this would be pretty simple, and would give us
a reasonably general-purpose and extensible API.  (It would also solve
the naming conundrum I mentioned below in my original email.)

> Best wishes
> 
> Mark
> 
> 
> 
> 
> >
> > Also, is this the right name for the new API?  In particular, if we do
> > later want to add a function that returns, say, the list of matched
> > messages, we'll have a convention collision with
> > notmuch_thread_get_matched_messages, which returns only a count.

Re: [PATCH 0/6] API for iterating over all messages in a thread

[Tomi Ollila]

On Sun, Nov 25 2012, Austin Clements <amdragon@MIT.EDU> wrote:

> Quoth Mark Walters on Nov 25 at  2:31 pm:
>> 
>> Hi
>> 
>> This series looks good to me (I have not reviewed the two bindings
>> patches). Patch 2 looks like it makes things much easier to follow than
>> the current code (if I understood the current pointer stuff it
>> constructs the top-level list by doing pointer stuff to remove all
>> messages which are replies from the complete message list). Indeed, the
>> diff is more complicated than the new code!
>> 
>> On Sun, 25 Nov 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> > This series adds a library API for iterating over all messages in a
>> > thread in sorted order.  This is easy for the library to provide and
>> > difficult to obtain from the current API.  Plus, if you don't count
>> > the code added to the bindings, this series is actually a net
>> > decrease of 4 lines of code because of simplifications it enables.
>> >
>> > Do we want the API to do more?  Currently it's very minimal, but I can
>> > imagine two ways it could be generalized.  It could take an argument
>> > to indicate which message list to return, which could be all messages,
>> > matched messages, top-level messages, or maybe even unmatched messages
>> > (possibly all in terms of message flags).  It could also take an
>> > argument indicating the desired sort order.  Currently, the caller can
>> > use existing message flag APIs to distinguish matched and unmatched
>> > messages and there's a separate function for the top-level messages.
>> > However, if the API could do all of these things, it would subsume
>> > various other API functions, such as notmuch_thread_get_*_date.
>> 
>> I don't know if this is the right API. For the matched message etc I
>> think using the existing message flag APIs is simple enough. I am not
>> sure about sort orders though: that looks like it would be much easier
>> for the caller to have the correct sort by I am not sure what users
>> would need it.
>
> For sort order, I would be inclined to simply construct the reverse
> list the first time a caller asks for it.  Theoretically the caller
> could do this just as easily as the library, except that we don't
> expose the list routines.
>
> If I do add sort order, I would also want to add some control over
> which list is returned, since it would be asymmetric to be able to
> request all messages in either order, but top-level messages only in
> oldest-first.  I think this would be pretty simple, and would give us
> a reasonably general-purpose and extensible API.  (It would also solve
> the naming conundrum I mentioned below in my original email.)

The code looks good to me. 

I'm interested to see the extensible interface for returning desired
list in desired sort order :)

Tomi

>
>> Best wishes
>> 
>> Mark
>> 
>> 
>> >
>> > Also, is this the right name for the new API?  In particular, if we do
>> > later want to add a function that returns, say, the list of matched
>> > messages, we'll have a convention collision with
>> > notmuch_thread_get_matched_messages, which returns only a count.

Re: [PATCH 0/6] API for iterating over all messages in a thread

[Austin Clements]

Quoth Tomi Ollila on Nov 26 at  7:19 pm:
> On Sun, Nov 25 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> > Quoth Mark Walters on Nov 25 at  2:31 pm:
> >> 
> >> Hi
> >> 
> >> This series looks good to me (I have not reviewed the two bindings
> >> patches). Patch 2 looks like it makes things much easier to follow than
> >> the current code (if I understood the current pointer stuff it
> >> constructs the top-level list by doing pointer stuff to remove all
> >> messages which are replies from the complete message list). Indeed, the
> >> diff is more complicated than the new code!
> >> 
> >> On Sun, 25 Nov 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> >> > This series adds a library API for iterating over all messages in a
> >> > thread in sorted order.  This is easy for the library to provide and
> >> > difficult to obtain from the current API.  Plus, if you don't count
> >> > the code added to the bindings, this series is actually a net
> >> > decrease of 4 lines of code because of simplifications it enables.
> >> >
> >> > Do we want the API to do more?  Currently it's very minimal, but I can
> >> > imagine two ways it could be generalized.  It could take an argument
> >> > to indicate which message list to return, which could be all messages,
> >> > matched messages, top-level messages, or maybe even unmatched messages
> >> > (possibly all in terms of message flags).  It could also take an
> >> > argument indicating the desired sort order.  Currently, the caller can
> >> > use existing message flag APIs to distinguish matched and unmatched
> >> > messages and there's a separate function for the top-level messages.
> >> > However, if the API could do all of these things, it would subsume
> >> > various other API functions, such as notmuch_thread_get_*_date.
> >> 
> >> I don't know if this is the right API. For the matched message etc I
> >> think using the existing message flag APIs is simple enough. I am not
> >> sure about sort orders though: that looks like it would be much easier
> >> for the caller to have the correct sort by I am not sure what users
> >> would need it.
> >
> > For sort order, I would be inclined to simply construct the reverse
> > list the first time a caller asks for it.  Theoretically the caller
> > could do this just as easily as the library, except that we don't
> > expose the list routines.
> >
> > If I do add sort order, I would also want to add some control over
> > which list is returned, since it would be asymmetric to be able to
> > request all messages in either order, but top-level messages only in
> > oldest-first.  I think this would be pretty simple, and would give us
> > a reasonably general-purpose and extensible API.  (It would also solve
> > the naming conundrum I mentioned below in my original email.)
> 
> The code looks good to me. 
> 
> I'm interested to see the extensible interface for returning desired
> list in desired sort order :)

I'll give this a shot (probably later today) and people can see what
they think.

> Tomi
> 
> >
> >> Best wishes
> >> 
> >> Mark
> >> 
> >> 
> >> >
> >> > Also, is this the right name for the new API?  In particular, if we do
> >> > later want to add a function that returns, say, the list of matched
> >> > messages, we'll have a convention collision with
> >> > notmuch_thread_get_matched_messages, which returns only a count.