Daniel Kahn Gillmor <dkg@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_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)
> -	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?
_______________________________________________
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch