Re: [PATCH 3/3] python/notmuch2: provide binding for collect_tags()

[Michael J Gruber <git@grubix.eu>]

Floris Bruynooghe venit, vidit, dixit 2021-01-11 21:33:16:
> Hi Micahel,
> 
> Thanks for adding this feature!
> 
> On Wed 06 Jan 2021 at 10:08 +0100, Michael J. Gruber wrote:
> > diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
> > index 1db6ec96..a1310944 100644
> > --- a/bindings/python-cffi/notmuch2/_query.py
> > +++ b/bindings/python-cffi/notmuch2/_query.py
> > @@ -2,6 +2,7 @@ from notmuch2 import _base as base
> >  from notmuch2 import _capi as capi
> >  from notmuch2 import _errors as errors
> >  from notmuch2 import _message as message
> > +from notmuch2 import _tags as tags
> >  from notmuch2 import _thread as thread
> >  
> >  
> > @@ -66,6 +67,17 @@ class Query(base.NotmuchObject):
> >              raise errors.NotmuchError(ret)
> >          return count_p[0]
> >  
> > +    def collect_tags(self):
> > +        """Return the tags of messages matching this query."""
> > +        msgs_pp = capi.ffi.new('notmuch_messages_t**')
> > +        ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
> > +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> > +            raise errors.NotmuchError(ret)
> > +        self._msgs_p = msgs_pp[0]
> > +        tagset = tags.ImmutableTagSet(
> > +            self, '_msgs_p', capi.lib.notmuch_messages_collect_tags)
> > +        return tags.ImmutableTagSet._from_iterable(tagset)
> > +
> >      def threads(self):
> >          """Return an iterator over all the threads found by the query."""
> >          threads_pp = capi.ffi.new('notmuch_threads_t **')
> 
> How this this look on the C level for the memory allocator?  If the
> query gets freed the messages also get freed?  In that case indeed the
> messages do need to be kept alive together with the query indeed.
> Keeping them as an attribute on the query object seems reasonable I
> think, they'd be freed at the same time and I don't think this creates a
> circular reference.
 
I'm afraid I don't have the full picture of the memory model and object
lifetimes in the bindings.

> There's only a small detail though: to get the correct ObjectDestroyed
> exceptions you need Query._msgs_p to be a _base.MemoryPoiniter
> descriptor, like Query._query_p is currently.  And then you also need to
> set it to None in Query._destroy.

I was trying to mimick what Query.messages() does: it creates msgs_pp
locally. The only reason for self._msgs_p in Thread.collect_tags() is
the way in which tags.ImmutableTagSet() expects the pointer: as an
attribute of self. But maybe there is a better way to call into
capi.lib.notmuch_messages_collect_tags()?

If not, and if I understand you correctly, then I should apply

diff --git i/bindings/python-cffi/notmuch2/_query.py w/bindings/python-cffi/notmuch2/_query.py
index a1310944..fa58825d 100644
--- i/bindings/python-cffi/notmuch2/_query.py
+++ w/bindings/python-cffi/notmuch2/_query.py
@@ -17,6 +17,7 @@ class Query(base.NotmuchObject):
     match libnotmuch's memory management.
     """
     _query_p = base.MemoryPointer()
+    _msgs_p = base.MemoryPointer()

     def __init__(self, db, query_p):
         self._db = db
@@ -40,6 +41,7 @@ class Query(base.NotmuchObject):
         if self.alive:
             capi.lib.notmuch_query_destroy(self._query_p)
         self._query_p = None
+        self._msgs_p = None

on top of this patch, right?


> It would also be really good to add at least one happy-path test for
> this, ensuring that you get the tags in the query.  You could add some
> to TestQuery in test_database.py.  The database is created with unread
> and inbox tags i think (see conftest.py:75) so the messages should
> already have some tags which is sufficient as you're not testing
> notmuch's behaviour itself but only that the bindings give you a right
> looking tagset (the tagset itself also has tests already, so no need to
> test this extensively, just see if you have some tags).

Thanks for the pointer. Will do for v2, comparing:
set(db.collect_tags('*')) == set(db.tags)

Michael