Re: [PATCH v2 3/7] fix thread breakage via ghost-on-removal

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

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

Hi Tomi--

On Tue 2016-04-05 03:53:34 -0300, Tomi Ollila wrote:
> I fetched your changes from lair.fifthhorseman.net yesterday and diffed
> against master; looks pretty good, some quick comments (this email has
> most relevant changes related to those changes):

thanks for the review!

>> -/* Delete a message document from the database. */
>> +/* Delete a message document from the database, leaving a ghost
>> + * message in its place */
>
> This comment could tell the condition when ghost message is left --
> versus the case all ghost messages are dropped (thread becomes empty of
> mail messages).

right, i can improve these comments.

> In next lines the expected case NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND
> could be first. Although the performance difference is negligible to me
> it looks silly do this first check and basically always fail there and
> then do 'else if' which is likely to succeeed...
> (your latest version in lair does not return in this first case but sets
> status to NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID. Perhaps later messages in this
> thread does the same but those are somewhat out-of-context for this reply).

sure, i can do the positive check first :)

>> +    if (private_status == NOTMUCH_PRIVATE_STATUS_SUCCESS) {
>> +	/* this is deeply weird, and we should not have gotten into
>> +	   this state.  is there a better error message to return
>> +	   here? */
>> +	return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
>> +    } else if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
>> +	private_status = _notmuch_message_initialize_ghost (ghost, tid);
>> +	if (! private_status)
>> +	    _notmuch_message_sync (ghost);
>> +    }
>> +    notmuch_message_destroy (ghost);
>> +    return COERCE_STATUS (private_status, "Error converting to ghost message");
>>  }
>
> Outside of this patch, but in some of the next messages, adds functions
> _notmuch_message_has_term() and _notmuch_message_has_term_st(). Perhaps
> the _notmuch_message_has_term() could be left unimplemented?

yeah, i can do that, though i have to say it's programmatically
convenient to have a simple boolean test that defaults to some value if
there was an error.

I'll post one more round of patches to the mailing list to address these
cleanup bits in the next day or two.

I'm happy to read more reviews if anyone has them,

      --dkg

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