On Mon 2019-05-27 07:24:41 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +    status = _notmuch_message_crypto_potential_payload (msg_crypto, clear, GMIME_OBJECT (encrypted_data), GMIME_MULTIPART_ENCRYPTED_CONTENT);
>> +    _index_mime_part (message, indexopts, clear, msg_crypto);
>>      g_object_unref (clear);
>
> If you're going to ignore the return value here (not sure if that's a
> good idea?)  please explicitly cast to void rather than putting in
> status to ignore.

Good catch, thanks.  I've logged the error with
_notmuch_database_log_append in v3 of this patch, i hope that makes
sense to you!

i note that _index_encrypted_mime_part() itself uses an odd mixture of
_notmuch_database_log_append and _notmuch_database_log, which maybe is a
sign that more cleanup is due there.

I confess i always get a bit confused about when to use one vs. the
other, though.  _log resets the database's status_string, while
_log_append just appends to it.

On IRC, you wrote "I'd say it makes sense to append for warnings", which
is a plausible rule of thumb, but seems like it might not map to the
intent of how we expect the status_string to be used -- is it for the
caller of the library?  or something else?

For example, should every call into the library reset the status string,
and then there would only be one _database_log() function?  (i don't
know whether that's feasible, or what it would mean for internal code
that already calls exported functions directly).

It'd probably be worthwhile for someone to do an audit of those uses and
come up with some normalized way of handling this that we can clearly
explain, because i think it's a bit unwieldy now.  I'm writing this
report with the intent of tagging this e-mail with notmuch::bug, in the
hopes that someone interested in doing maintenance work will take this
on as a future project :)

       --dkg