Re: New Python bindings (notmuch2 module) fail to exclude tags

[Floris Bruynooghe <flub@devork.be>]

Hi All,

On Sat 21 Nov 2020 at 18:51 -0300, Jorge P. de Morais Neto wrote:

> Hi Floris.
>
> Em [2020-11-20 sex 19:17:56+0100], Floris Bruynooghe escreveu:
>
>> Looking at the implementation I don't seem much that could have gone
>> wrong.  However I did notice the bindings fail to check the return code
>> in one call where it probably should, you could try with this patch?
>>
>> diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
>> index 5ab0f20a..5dbfe68e 100644
>> --- a/bindings/python-cffi/notmuch2/_database.py
>> +++ b/bindings/python-cffi/notmuch2/_database.py
>> @@ -579,7 +579,10 @@ class Database(base.NotmuchObject):
>>              for tag in exclude_tags:
>>                  if isinstance(tag, str):
>>                      tag = str.encode('utf-8')
>> -                capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
>> +                ret = capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
>> +                if ret not in [capi.lib.NOTMUCH_STATUS_SUCCESS,
>> +                               capi.lib.NOTMUCH_STATUS_IGNORED]:
>> +                    raise errors.NotmuchError(ret)
>>          return querymod.Query(self, query_p)
>>  
>>      def messages(self, query, *,
>>
> After applying your patch, the call to nm_db.count_messages() fails:
>     AttributeError: cffi library 'notmuch2._capi' has no function, constant or global variable named 'NOTMUCH_STATUS_IGNORED'
>
> However, I then found the bug.  The patch below fixes it.
>
> $ diff -u notmuch2-orig/_database.py notmuch2/_database.py
> --- notmuch2-orig/_database.py	2020-11-21 18:02:17.560240619 -0300
> +++ notmuch2/_database.py	2020-11-21 18:43:44.827879141 -0300
> @@ -578,7 +578,7 @@
>          if exclude_tags is not None:
>              for tag in exclude_tags:
>                  if isinstance(tag, str):
> -                    tag = str.encode('utf-8')
> +                    tag = tag.encode('utf-8')
>                  capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
>          return querymod.Query(self, query_p)
>
>
> However, I think you should *also* add the error checking.  The only
> reason my patch omits error checking is that I don't know how to define
> capi.lib.NOTMUCH_STATUS_IGNORED.

Yup, that was missing from _build.py so wouldn't have been available.

Before fixing this I tried to write a test, but I don't know if this
behaves correctly:

I have a database with 3 messages organised in 2 threads and the
following tags:

msg1: all
  +- msg3: all, spam
msg2: all

I query '*', so all messages with exclude_tags=['spam'].  Querying this
with various flags gives me:

NOTMUCH_EXCLUDE_TRUE: 2 messages, what I expect
NOTMUCH_EXCLUDE_ALL: 2 messages, what I expect
NOTMUCH_EXCLUDE_FLAG: 2 messages, I expected 3
NOTMUCH_EXCLUDE_FLASE: 2 messages, I expected 3

Did I misunderstand the docs?  Are these results correct?


Cheers,
Floris