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
signature.asc
Description: PGP signature
_______________________________________________ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch