[PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload
david at tethera.net
Wed Jul 31 04:57:56 PDT 2019
Daniel Kahn Gillmor <dkg at fifthhorseman.net> 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_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum)
> +_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)
> - return NOTMUCH_STATUS_NULL_POINTER;
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?
More information about the notmuch