Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

Subject: Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

Date: Wed, 31 Jul 2019 08:57:56 -0300

To: Daniel Kahn Gillmor, Notmuch Mail


From: David Bremner

Daniel Kahn Gillmor <> writes:

> Our _notmuch_message_crypto_potential_payload implementation could
> only return a failure if bad arguments were passed to it.  It is an
> internal function, so if that happens it's an entirely internal bug
> for notmuch.
> It will be more useful for this function to return whether or not the
> part is in fact a cryptographic payload, so we dispense with the
> status return.

Is a bit confusing that this patch introduces a new return value, and
then ignores. Perhaps cast the calls to (void) to make it clear you are
ignoring it on purpose.
> -notmuch_status_t
> -_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum)
> +bool
> +_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *part, GMimeObject *parent, int childnum)
>  {
>      const char *protected_headers = NULL;
>      const char *forwarded = NULL;
>      const char *subject = NULL;
> -    if (! msg_crypto || ! payload)
> -

what about leaving an assert or call to INTERNAL_ERROR here? I'm a bit
concerned this change is making the code less robust. I guess we'll see
a segfault, if either is NULL, but that seems bit icky to rely on.

> -	GMimeContentType *ct = g_mime_object_get_content_type (payload);
> +	GMimeContentType *ct = g_mime_object_get_content_type (part);

The purpose of this change is unclear to me from the diff. Can you explain?
It seems like there is a related s/payload/part/ in several places
below. Maybe this deserves a note in the commit message?
notmuch mailing list