Re: [PATCH v2 12/12] lib: Remove unnecessary thread linking steps when using ghost messages

From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>
Cc: notmuch@notmuchmail.org
Subject: Re: [PATCH v2 12/12] lib: Remove unnecessary thread linking steps when	using ghost messages
Date: Tue, 21 Oct 2014 21:51:01 -0400	[thread overview]
Message-ID: <20141022015101.GF7970@csail.mit.edu> (raw)
In-Reply-To: <87wq7thv27.fsf@qmul.ac.uk>

Quoth Mark Walters on Oct 22 at 12:17 am:
> On Tue, 07 Oct 2014, Austin Clements <aclements@csail.mit.edu> wrote:
> > From: Austin Clements <amdragon@mit.edu>
> >
> > Previously, it was necessary to link new messages to children to work
> > around some (though not all) problems with the old metadata-based
> > approach to stored thread IDs.  With ghost messages, this is no longer
> > necessary, so don't bother with child linking when ghost messages are
> > in use.
> > ---
> >  lib/database.cc | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/database.cc b/lib/database.cc
> > index 1316529..6e51a72 100644
> > --- a/lib/database.cc
> > +++ b/lib/database.cc
> > @@ -2169,10 +2169,23 @@ _notmuch_database_link_message (notmuch_database_t *notmuch,
> >      if (status)
> >  	goto DONE;
> >  
> > -    status = _notmuch_database_link_message_to_children (notmuch, message,
> > -							 &thread_id);
> > -    if (status)
> > -	goto DONE;
> > +    if (! (notmuch->features & NOTMUCH_FEATURE_GHOSTS)) {
> > +	/* In general, it shouldn't be necessary to link children,
> > +	 * since the earlier indexing of those children will have
> > +	 * stored a thread ID for the missing parent.  However, prior
> > +	 * to ghost messages, these stored thread IDs were NOT
> > +	 * rewritten during thread merging (and there was no
> > +	 * performant way to do so), so if indexed children were
> > +	 * pulled into a different thread ID by a merge, it was
> > +	 * necessary to pull them *back* into the stored thread ID of
> > +	 * the parent.  With ghost messages, we just rewrite the
> > +	 * stored thread IDs during merging, so this workaround isn't
> > +	 * necessary. */
> > +	status = _notmuch_database_link_message_to_children (notmuch, message,
> > +							     &thread_id);
> > +	if (status)
> > +	    goto DONE;
> > +    }
> 
> Ok so this basically answers my earlier comment. It might be worth
> updating the big comment at the start of the function to match the new
> code though.

Like so?

diff --git a/lib/database.cc b/lib/database.cc
index d063ec8..3601f9d 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -2136,11 +2136,11 @@ _consume_metadata_thread_id (void *ctx, notmuch_database_t *notmuch,
  * reference 'message'.
  *
  * In all cases, we assign to the current message the first thread ID
- * found (through either parent or child). We will also merge any
- * existing, distinct threads where this message belongs to both,
- * (which is not uncommon when messages are processed out of order).
+ * found. We will also merge any existing, distinct threads where this
+ * message belongs to both, (which is not uncommon when messages are
+ * processed out of order).
  *
- * Finally, if no thread ID has been found through parent or child, we
+ * Finally, if no thread ID has been found through referenced messages, we
  * call _notmuch_message_generate_thread_id to generate a new thread
  * ID. This should only happen for new, top-level messages, (no
  * References or In-Reply-To header in this message, and no previously


> Best wishes
> 
> Mark
> 
> >  
> >      /* If not part of any existing thread, generate a new thread ID. */
> >      if (thread_id == NULL) {
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
