From: David Bremner <david@tethera.net>
To: "Alexey I. Froloff" <raorn@raorn.name>, notmuch@notmuchmail.org
Subject: Re: [PATCH] lib: Add a new prefix "list" to the search-terms syntax
In-Reply-To: <1365549369-12776-1-git-send-email-raorn@raorn.name>
References: <20130409083010.GA27675@raorn.name>
	<1365549369-12776-1-git-send-email-raorn@raorn.name>
User-Agent: Notmuch/0.15.2+84~g12d5e4e (http://notmuchmail.org) Emacs/24.2.1
	(x86_64-pc-linux-gnu)
Date: Mon, 29 Apr 2013 22:12:16 -0300
Message-ID: <87vc74vn2n.fsf@zancas.localnet>
MIME-Version: 1.0
Content-Type: text/plain
X-Spam_bar: -
X-BeenThere: notmuch@notmuchmail.org
X-Mailman-Version: 2.1.13
Precedence: list
List-Id: "Use and development of the notmuch mail system."
	<notmuch.notmuchmail.org>
List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
List-Archive: <http://notmuchmail.org/pipermail/notmuch>
List-Post: <mailto:notmuch@notmuchmail.org>
List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=subscribe>
X-List-Received-Date: Tue, 30 Apr 2013 01:12:33 -0000


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.