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

[Daniel Kahn Gillmor]

Hi David--

Thanks for the review!

On Wed 2019-07-31 08:57:56 -0300, David Bremner wrote:
> 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.

hm, we ignore it only for the moment -- in patches 6 and 7 we take
action on the basis of the return value.  I'll cast them to void here in
this patch and update the commit message to explain the situation.

>> -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.

Sure, INTERNAL_ERROR makes sense, i think.

>> -	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?

The function is _notmuch_message_crypto_potential_payload, and it is
testing whether a given mime part (a GMimeObject) *is* the payload or
not.  the GMimeObject argument used to be named "payload", which is a
bit weird -- if we know it's the payload already, why call the function?
rather, we should call it "part".  i'll split that out into a separate
patch, though, since it's clearly intended as a non-functional change.

Do you have more review pending on this series or should i send the
updated series to the list with these changes?

      --dkg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20190731/70c135d2/attachment.sig>