Re: [PATCH] lib: add 'body:' field, stop indexing headers twice.

[Matt Armstrong <marmstrong@google.com>]

David, interesting idea.  I'm not very familiar with this code or its
conventions so my feedback should be taken with that in mind.  More
below.


David Bremner <david@tethera.net> writes:

> diff --git a/lib/database.cc b/lib/database.cc
> index 9cf8062c..27c2d042 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -259,6 +259,8 @@ prefix_t prefix_table[] = {
>      { "directory",		"XDIRECTORY",	NOTMUCH_FIELD_NO_FLAGS },
>      { "file-direntry",		"XFDIRENTRY",	NOTMUCH_FIELD_NO_FLAGS },
>      { "directory-direntry",	"XDDIRENTRY",	NOTMUCH_FIELD_NO_FLAGS },
> +    { "body",			"",		NOTMUCH_FIELD_EXTERNAL |
> +						NOTMUCH_FIELD_PROBABILISTIC},
>      { "thread",			"G",		NOTMUCH_FIELD_EXTERNAL |
>  						NOTMUCH_FIELD_PROCESSOR },
>      { "tag",			"K",		NOTMUCH_FIELD_EXTERNAL |

Above this new code in database.cc there is a comment describing the
schema.  E.g. "Mail document" describes id:, thread:, etc.  Add a
description of body: there?

Also, near those comments there is a double-space in the phrase
'uniquely identified by its "id" field' that you might fix while you're
nearby.


> diff --git a/lib/message.cc b/lib/message.cc
> index 6f2f6345..64349f83 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -1443,13 +1443,13 @@ _notmuch_message_gen_terms (notmuch_message_t *message,
>  	message->termpos = term_gen->get_termpos () + 100;
>  
>  	_notmuch_message_invalidate_metadata (message, prefix_name);
> +    } else {
> +	term_gen->set_termpos (message->termpos);
> +	term_gen->index_text (text);
> +	/* Create a term gap, as above. */
> +	message->termpos = term_gen->get_termpos () + 100;
>      }
>  
> -    term_gen->set_termpos (message->termpos);
> -    term_gen->index_text (text);
> -    /* Create a term gap, as above. */
> -    message->termpos = term_gen->get_termpos () + 100;
> -
>      return NOTMUCH_PRIVATE_STATUS_SUCCESS;
>  }

Instead of the above I think I find what follows more clear.  This makes
it obvious which logic depends on the presence of a prefix and which
logic does not, which was a question I immediately had reading the code.

    term_gen->set_termpos(message->termpos);
    if (prefix_name) {
      term_gen->index_text (text, 1, _find_prefix (prefix_name));
    } else {
      term_gen->index_text (text);
    }
    /* Create a gap between this an the next terms so they don't appear to be a
     * phrase. */
    message->termpos = term_gen->get_termpos () + 100;
    if (prefix_name) {
      _notmuch_message_invalidate_metadata (message, prefix_name);
    }