Re: [PATCH v6 06/14] reindex: drop all properties named with prefix "index."

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

On Thu 2017-10-19 08:52:46 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +    ret = notmuch_message_remove_all_properties_with_prefix (message, "index.");
>> +    if (ret) {
>> +	INTERNAL_ERROR ("failed to remove index.* properties");
>> +	goto DONE;
>> +    }
>
> 1) INTERNAL_ERROR is fatal, so the goto is unneeded / confusing.
>
> 2) Is there no sensible return value here? The function already has an
> error return path. I see there is one INTERNAL_ERROR there so maybe
> similar reasoning applies and a TODO is enough.

I suppose the error returned by
notmuch_message_remove_all_properties_with_prefix() is the value that we
should return.  I've switched this to a simple goto DONE in my gitlab
repo.

However, i note that an error here will be (from the outside)
indistinguishable from an error in _notmuch_database_ensure_writable or
_notmuch_message_remove_indexed_terms() immediately above, so i've added
a small TODO anyway.

thanks for the reviews, bremner.  please keep them coming!  (or tell me
when you're done so i can send v7 to the list with resolutions to all
these nitpicks)

  --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]