From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
To: Notmuch Mail <notmuch@notmuchmail.org>
Subject: [PATCH v3 3/7] fix thread breakage via ghost-on-removal
Date: Fri,  8 Apr 2016 22:02:30 -0300
Message-Id: <1460163754-22994-3-git-send-email-dkg@fifthhorseman.net>
In-Reply-To: <1460163754-22994-1-git-send-email-dkg@fifthhorseman.net>
References: <1459445693-3900-1-git-send-email-dkg@fifthhorseman.net>
 <1460163754-22994-1-git-send-email-dkg@fifthhorseman.net>
Precedence: list

implement 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 already introduces new
message_ids to the database, so i think just searching for a given
message ID may introduce the same metadata leakage.
---
 lib/message.cc               | 30 +++++++++++++++++++++++++++---
 test/T590-thread-breakage.sh | 25 ++++++++++++-------------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 8d72ea2..435b78a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1037,20 +1037,44 @@ _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 */
 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);
+    if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
+	private_status = _notmuch_message_initialize_ghost (ghost, tid);
+	if (! private_status)
+	    _notmuch_message_sync (ghost);
+    } else 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;
+    }
+
+    notmuch_message_destroy (ghost);
+    return COERCE_STATUS (private_status, "Error converting to ghost message");
 }
 
 /* Transform a blank message into a ghost message.  The caller must
diff --git a/test/T590-thread-breakage.sh b/test/T590-thread-breakage.sh
index 24ffb42..22e5cbc 100755
--- a/test/T590-thread-breakage.sh
+++ b/test/T590-thread-breakage.sh
@@ -96,20 +96,11 @@ notmuch new >/dev/null
 test_thread_count 1 'First message removed: still only one thread'
 test_subject_count apple 0
 test_subject_count banana 1
-test_begin_subtest 'should be one ghost after first message removed'
-test_subtest_known_broken
-ghosts=$(../ghost-report ${MAIL_DIR}/.notmuch/xapian)
-test_expect_equal "$ghosts" "1"
+test_ghost_count 1 'should be one ghost after first message removed'
 
 message_a
 notmuch new >/dev/null
-# this is known to fail (it shows 2 threads) because no "ghost
-# message" was created for message A when it was removed from the
-# index, despite message B still pointing to it.
-test_begin_subtest 'First message reappears: should return to the same thread'
-test_subtest_known_broken
-count=$(notmuch count --output=threads)
-test_expect_equal "$count" "1"
+test_thread_count 1 'First message reappears: should return to the same thread'
 test_subject_count apple 1
 test_subject_count banana 1
 test_ghost_count 0
@@ -119,13 +110,21 @@ notmuch new >/dev/null
 test_thread_count 1 'Removing second message: still only one thread'
 test_subject_count apple 1
 test_subject_count banana 0
-test_ghost_count 0 'No ghosts should remain after deletion of second message'
+test_begin_subtest 'No ghosts should remain after deletion of second message'
+# this is known to fail; we are leaking ghost messages deliberately
+test_subtest_known_broken
+ghosts=$(../ghost-report ${MAIL_DIR}/.notmuch/xapian)
+test_expect_equal "$ghosts" "0"
 
 rm -f ${MAIL_DIR}/cur/a
 notmuch new >/dev/null
 test_thread_count 0 'All messages gone: no threads'
 test_subject_count apple 0
 test_subject_count banana 0
-test_ghost_count 0
+test_begin_subtest 'No ghosts should remain after full thread deletion'
+# this is known to fail; we are leaking ghost messages deliberately
+test_subtest_known_broken
+ghosts=$(../ghost-report ${MAIL_DIR}/.notmuch/xapian)
+test_expect_equal "$ghosts" "0"
 
 test_done
-- 
2.8.0.rc3