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

[Tomi Ollila <tomi.ollila@iki.fi>]

On Sat, Apr 02 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

> ghost-on-removal the solution to T590-thread-breakage.sh that just
> adds a ghost message after removing each message.
>
> It leaks information about whether we've ever seen a given message id,
> but it's a fairly simple implementation.
>
> Note that _resolve_message_id_to_thread_id also introduces new
> message_ids to the database, so i think just searching for a given
> message ID may introduce the same metadata leakage.
>
> This differs from v1 of this changeset in that we implement the change
> in _notmuch_message_delete, a more "internal" function.

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):

> ---
>  lib/database.cc |  2 +-
>  lib/message.cc  | 29 ++++++++++++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3b342f1..d5733c9 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -2557,7 +2557,7 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
>  
>      if (status == NOTMUCH_STATUS_SUCCESS && message) {
>  	    status = _notmuch_message_remove_filename (message, filename);
> -	    if (status == NOTMUCH_STATUS_SUCCESS)
> +	    if (status == NOTMUCH_STATUS_SUCCESS) 

It looks to be that this change is insignificant enough so it could be
dropped ;)

>  		_notmuch_message_delete (message);
>  	    else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
>  		_notmuch_message_sync (message);
> diff --git a/lib/message.cc b/lib/message.cc
> index 8d72ea2..e414e9c 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -1037,20 +1037,43 @@ _notmuch_message_sync (notmuch_message_t *message)
>      message->modified = FALSE;
>  }
>  
> -/* 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).

>  notmuch_status_t
>  _notmuch_message_delete (notmuch_message_t *message)
>  {
>      notmuch_status_t status;
>      Xapian::WritableDatabase *db;
> +    const char *mid, *tid;
> +    notmuch_message_t *ghost;
> +    notmuch_private_status_t private_status;
> +    notmuch_database_t *notmuch;
> +	    
> +    mid = notmuch_message_get_message_id (message);
> +    tid = notmuch_message_get_thread_id (message);
> +    notmuch = message->notmuch;
>  
>      status = _notmuch_database_ensure_writable (message->notmuch);
>      if (status)
>  	return status;
>  
> -    db = static_cast <Xapian::WritableDatabase *> (message->notmuch->xapian_db);
> +    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
>      db->delete_document (message->doc_id);
> -    return NOTMUCH_STATUS_SUCCESS;
> +	    
> +    /* and reintroduce a ghost in its place */
> +    ghost = _notmuch_message_create_for_message_id (notmuch, mid, &private_status);

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).

> +    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?

>  
>  /* Transform a blank message into a ghost message.  The caller must
> -- 
> 2.8.0.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch