Possible bug in notmuch_thread_get_authors ?

Possible bug in notmuch_thread_get_authors ?

[Róman Joost]

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

Hi,

we're currently working on Haskell bindings for notmuch[1] and stumbled
over an oddity in relation to the function notmuch_thread_get_authors in
that it can be NULL.

Checking other bindings it seems there is an explicit check for NULL and
we're wondering if that is really necessary.  Perhaps a patch could
initialize `*authors` in the _resolve_thread_authors_string function to an
empty string OR at least mention it in the API documentation that the
return value can be NULL?

Any thoughts?

[1] - https://github.com/purebred-mua/hs-notmuch

Kind Regards,
-- 
Róman Joost
email: roman@bromeco.de
GPG key: 3A765B52

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Possible bug in notmuch_thread_get_authors ?

[David Bremner]

Róman Joost <roman@bromeco.de> writes:

> Checking other bindings it seems there is an explicit check for NULL and
> we're wondering if that is really necessary.  Perhaps a patch could
> initialize `*authors` in the _resolve_thread_authors_string function to an
> empty string OR at least mention it in the API documentation that the
> return value can be NULL?

Certainly the documentation update is fine. It's not clear to me without
diving into the code whether returning NULL is signalling an error, or
just a bug.

d

[PATCH] lib: return "" rather than NULL from notmuch_thread_get_authors

[David Bremner]

The current beheviour is at best underdocumented. The modified test in
T470-missing-headers.sh previously relied on printf doing the right
thing with NULL, which seems ick.

The use of talloc_strdup here is probably overkill, but it avoids
having to enforce that thread->authors is never mutated outside
_resolve_thread_authors_string.
---
 lib/thread.cc                | 3 +++
 test/T470-missing-headers.sh | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index 1632da4c..3561b27f 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -160,6 +160,9 @@ _resolve_thread_authors_string (notmuch_thread_t *thread)
     thread->authors_array = NULL;
     g_ptr_array_free (thread->matched_authors_array, true);
     thread->matched_authors_array = NULL;
+
+    if (!thread->authors)
+	thread->authors = talloc_strdup(thread, "");
 }
 
 /* clean up the ugly "Lastname, Firstname" format that some mail systems
diff --git a/test/T470-missing-headers.sh b/test/T470-missing-headers.sh
index 4bf5d285..555fd4e9 100755
--- a/test/T470-missing-headers.sh
+++ b/test/T470-missing-headers.sh
@@ -25,7 +25,7 @@ NOTMUCH_NEW >/dev/null
 test_begin_subtest "Search: text"
 output=$(notmuch search '*' | notmuch_search_sanitize)
 test_expect_equal "$output" "\
-thread:XXX   2001-01-05 [1/1] (null);  (inbox unread)
+thread:XXX   2001-01-05 [1/1] ;  (inbox unread)
 thread:XXX   1970-01-01 [1/1] Notmuch Test Suite;  (inbox unread)"
 
 test_begin_subtest "Search: json"
-- 
2.15.1

Re: [PATCH] lib: return "" rather than NULL from notmuch_thread_get_authors

[Róman Joost]

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

Dear David,

On Thu, Dec 14, 2017 at 10:29:57PM -0400, David Bremner wrote:
> The current beheviour is at best underdocumented. The modified test in
> T470-missing-headers.sh previously relied on printf doing the right
> thing with NULL, which seems ick.
> 
> The use of talloc_strdup here is probably overkill, but it avoids
> having to enforce that thread->authors is never mutated outside
> _resolve_thread_authors_string.
> [...]

Thanks a lot for this patch. I think it'll help future authors to be
less surprised :))

Kind Regards,
-- 
Róman Joost
email: roman@bromeco.de
GPG key: 3A765B52

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] lib: return "" rather than NULL from notmuch_thread_get_authors

[Tomi Ollila]

On Thu, Dec 14 2017, David Bremner wrote:

> The current beheviour is at best underdocumented. The modified test in

Typo in commit message ;), otherwise looks reasonable to me

> T470-missing-headers.sh previously relied on printf doing the right
> thing with NULL, which seems ick.
>
> The use of talloc_strdup here is probably overkill, but it avoids
> having to enforce that thread->authors is never mutated outside
> _resolve_thread_authors_string.
> ---
>  lib/thread.cc                | 3 +++
>  test/T470-missing-headers.sh | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index 1632da4c..3561b27f 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -160,6 +160,9 @@ _resolve_thread_authors_string (notmuch_thread_t *thread)
>      thread->authors_array = NULL;
>      g_ptr_array_free (thread->matched_authors_array, true);
>      thread->matched_authors_array = NULL;
> +
> +    if (!thread->authors)
> +	thread->authors = talloc_strdup(thread, "");
>  }
>  
>  /* clean up the ugly "Lastname, Firstname" format that some mail systems
> diff --git a/test/T470-missing-headers.sh b/test/T470-missing-headers.sh
> index 4bf5d285..555fd4e9 100755
> --- a/test/T470-missing-headers.sh
> +++ b/test/T470-missing-headers.sh
> @@ -25,7 +25,7 @@ NOTMUCH_NEW >/dev/null
>  test_begin_subtest "Search: text"
>  output=$(notmuch search '*' | notmuch_search_sanitize)
>  test_expect_equal "$output" "\
> -thread:XXX   2001-01-05 [1/1] (null);  (inbox unread)
> +thread:XXX   2001-01-05 [1/1] ;  (inbox unread)
>  thread:XXX   1970-01-01 [1/1] Notmuch Test Suite;  (inbox unread)"
>  
>  test_begin_subtest "Search: json"
> -- 
> 2.15.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] lib: return "" rather than NULL from notmuch_thread_get_authors

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Thu, Dec 14 2017, David Bremner wrote:
>
>> The current beheviour is at best underdocumented. The modified test in
>
> Typo in commit message ;), otherwise looks reasonable to me
>

spill chucked and pished,

d