Re: [PATCH] lib: Add a new prefix "list" to the search-terms syntax

Subject: Re: [PATCH] lib: Add a new prefix "list" to the search-terms syntax

Date: Mon, 29 Apr 2013 22:12:16 -0300

To: Alexey I. Froloff, notmuch@notmuchmail.org

Cc:

From: David Bremner

Hi Alexey, 

Thanks for working on this. I think the boolean prefix version makes
more sense, and it seems to work OK. I have a few comments below

"Alexey I. Froloff" <raorn@raorn.name> writes:

> +    begin_list_id = strrchr (list_id_header, '<');
> +    if (!begin_list_id) {
> +	fprintf (stderr, "Warning: Not indexing mailformed List-Id tag.\n");
> +	return;
> +    }

- I guess this should say "malformed". 

- I got about 1800 lines of such messages when indexing 280k
  messages. That might strike some people as excessive. On the otherhand
  I guess we need to re-think error reporting overall.

  What do you think about printing filename or message-id here its
  easier to double check that it is not a bug?

> +    end_list_id = strrchr(begin_list_id, '>');
> +    if (!end_list_id || (end_list_id - begin_list_id < 2)) {
> +	fprintf (stderr, "Warning: Not indexing mailformed List-Id tag.\n");
> +	return;
> +    }
> +

Same comments here.

> +    void *local = talloc_new (message);
> +
> +    /* We extract the list id between the angle brackets */
> +    const char *list_id = talloc_strndup (local, begin_list_id + 1,
> +					  end_list_id - begin_list_id - 1);
> +
      we should handle ENOMEM here, I think.

> +    /* _notmuch_message_add_term() may return
> +     * NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG here.  We can't fix it, but
> +     * this is not a reason to exit with error... */
> +    if (_notmuch_message_add_term (message, "list", list_id))
> +	fprintf (stderr, "Warning: Not indexing List-Id: <%s>\n", list_id);

This should say why the indexing failed.

Other than that:

- We need a couple tests for this code; tests/search should give some
  hints how to proceed.

- We need a patch for NEWS, explaining what people need to do take
  advantage of the new functionality.  I think that adding new prefixes
  to an existing database is OK, but I'd welcome confirmation.

BTW, my not too scientific tests show no detectable bloat in the
database, at least after running xapian-compact. I'd be curious what
other people report.

Thread: