[PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload
Daniel Kahn Gillmor
dkg at fifthhorseman.net
Wed Jul 31 20:15:55 PDT 2019
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_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.
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?
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 227 bytes
Desc: not available
More information about the notmuch