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 23:15:55 -0400

To: David Bremner, Notmuch Mail


From: 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)
>> -
> 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?

signature.asc (application/pgp-signature)
notmuch mailing list