--- /dev/null
+Return-Path: <jani@nikula.org>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 4316B431FBC\r
+ for <notmuch@notmuchmail.org>; Sun, 30 Mar 2014 14:10:38 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+ tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id X6JfBu+sVYdc for <notmuch@notmuchmail.org>;\r
+ Sun, 30 Mar 2014 14:10:30 -0700 (PDT)\r
+Received: from mail-we0-f175.google.com (mail-we0-f175.google.com\r
+ [74.125.82.175]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 35A3B431FB6\r
+ for <notmuch@notmuchmail.org>; Sun, 30 Mar 2014 14:10:30 -0700 (PDT)\r
+Received: by mail-we0-f175.google.com with SMTP id q58so3924177wes.20\r
+ for <notmuch@notmuchmail.org>; Sun, 30 Mar 2014 14:10:26 -0700 (PDT)\r
+X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
+ d=1e100.net; s=20130820;\r
+ h=x-gm-message-state:from:to:cc:subject:in-reply-to:references\r
+ :user-agent:date:message-id:mime-version:content-type;\r
+ bh=Kllr2l+SxkOCXuc9h87xVTzIjR//GKogXIFNhgA110I=;\r
+ b=HeJn0dTK7twgwaH3SaNRHZEauMYGUM50wNWuVa2uQu6l87mc55mFheVL6rrsfkIHVo\r
+ uIHJBHeNJPvZJFgV/iDIDS1cORFBMUQM/SDhYL/od4Pj05iF5231bYVmFiscqX3gWb79\r
+ tmDQV+sYBJqePYjwWX3OrcByi04Wm+KF9zcFmTggM/8oY6M8B+B6ONrRp7NhRGcteX98\r
+ wvnD58gzyf5x94qEUIjXm/uuumqX4CXVxcr3J6tsJtMMaC8udAYALpiDfSZ+bBaSzf4f\r
+ ccabJ0AgOaJimWdAxcjqPONp2bvjB4cp79pCJI3OctzD/8HgxmFP212zcmfT1xG7lVEf\r
+ Nd/A==\r
+X-Gm-Message-State:\r
+ ALoCoQlV/gE6PtA+uBQdqTuVFLM3c2j2vbKJbv+0QHLFjiv0yCoOYlHdXC+pOdYC11LfhXW/YPZU\r
+X-Received: by 10.194.60.37 with SMTP id e5mr9233199wjr.32.1396213825059;\r
+ Sun, 30 Mar 2014 14:10:25 -0700 (PDT)\r
+Received: from localhost (dsl-hkibrasgw2-58c36f-91.dhcp.inet.fi.\r
+ [88.195.111.91])\r
+ by mx.google.com with ESMTPSA id 44sm28262047eek.30.2014.03.30.14.10.22\r
+ for <multiple recipients>\r
+ (version=TLSv1.2 cipher=RC4-SHA bits=128/128);\r
+ Sun, 30 Mar 2014 14:10:23 -0700 (PDT)\r
+From: Jani Nikula <jani@nikula.org>\r
+To: Austin Clements <amdragon@MIT.EDU>\r
+Subject: Re: [PATCH v4] lib: replace the header parser with gmime\r
+In-Reply-To: <20140329221127.GH31187@mit.edu>\r
+References: <1395604866-19188-1-git-send-email-jani@nikula.org>\r
+ <20140329221127.GH31187@mit.edu>\r
+User-Agent: Notmuch/0.17+164~gcd5fd5a4837e (http://notmuchmail.org)\r
+ Emacs/24.3.1 (x86_64-pc-linux-gnu)\r
+Date: Mon, 31 Mar 2014 00:10:20 +0300\r
+Message-ID: <87y4zrl7er.fsf@nikula.org>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Sun, 30 Mar 2014 21:10:38 -0000\r
+\r
+On Sun, 30 Mar 2014, Austin Clements <amdragon@MIT.EDU> wrote:\r
+> Various little comments below. Overall this is looking really good.\r
+>\r
+> Quoth Jani Nikula on Mar 23 at 10:01 pm:\r
+>> The notmuch library includes a full blown message header parser. Yet\r
+>> the same message headers are parsed by gmime during indexing. Switch\r
+>> to gmime parsing completely.\r
+>> \r
+>> These are the main changes:\r
+>> \r
+>> * Gmime stops header parsing at the first invalid header, and presumes\r
+>> the message body starts from there. The current parser is quite\r
+>> liberal in accepting broken headers. The change means we will be\r
+>> much pickier about accepting invalid messages.\r
+>> \r
+>> * The current parser converts tabs used in header folding to\r
+>> spaces. Gmime preserve the tabs. Due to a broken python library used\r
+>> in mailman, there are plenty of mailing lists that produce headers\r
+>> with tabs in header folding, and we'll see plenty of tabs. (This\r
+>> change has been mitigated in preparatory patches.)\r
+>> \r
+>> * For pure header parsing, the current parser is likely faster than\r
+>> gmime, which parses the whole message rather than just the\r
+>> headers. Since we parse the message and its headers using gmime for\r
+>> indexing anyway, this avoids and extra header parsing round when\r
+>> adding new messages. In case of duplicate messages, we'll end up\r
+>> parsing the full message although just headers would be\r
+>> sufficient. All in all this should still speed up 'notmuch new'.\r
+>> \r
+>> * Calls to notmuch_message_get_header() may be slightly slower than\r
+>> previously for headers that are not indexed in the database, due to\r
+>> parsing of the whole message. Within the notmuch code base, notmuch\r
+>> reply is the only such user.\r
+>> \r
+>> ---\r
+>> \r
+>> This is v4 of patches [1] and [2], combining them into one. Patch [3]\r
+>> is required to make all the tests pass.\r
+>> \r
+>> The implementation has been changed considerably. Notably we only\r
+>> cache the decoded headers to a hash table if and when they are\r
+>> needed. The main reasons for doing caching at all is that gmime\r
+>> provides raw headers that need decoding, and to minimize changes\r
+>> outside of message-file.c we want to own and track the allocated\r
+>> strings instead of pushing the responsibility to callers.\r
+>> \r
+>> This addresses most comments from Austin.\r
+>> \r
+>> The diff in message-file.c is rather confusing due to a mix of\r
+>> added/removed lines. Looking at the patched file is much more\r
+>> pleasant.\r
+>> \r
+>> BR,\r
+>> Jani.\r
+>> \r
+>> [1] id:bdef45dc21c777130a7ae1fa650f172d8c6eaaf4.1391456555.git.jani@nikula.org\r
+>> [2] id:31d785c4a3e4b90862a0fdc545d4e900a4c898e2.1391456555.git.jani@nikula.org\r
+>> [3] id:1395247490-19892-1-git-send-email-jani@nikula.org\r
+>> ---\r
+>> lib/database.cc | 15 +-\r
+>> lib/index.cc | 70 +--------\r
+>> lib/message-file.c | 420 ++++++++++++++++++++------------------------------\r
+>> lib/notmuch-private.h | 54 +++----\r
+>> 4 files changed, 208 insertions(+), 351 deletions(-)\r
+>> \r
+>> diff --git a/lib/database.cc b/lib/database.cc\r
+>> index aef748f75b5d..4750f40cf0fb 100644\r
+>> --- a/lib/database.cc\r
+>> +++ b/lib/database.cc\r
+>> @@ -1971,15 +1971,10 @@ notmuch_database_add_message (notmuch_database_t *notmuch,\r
+>> if (ret)\r
+>> goto DONE;\r
+>> \r
+>> - notmuch_message_file_restrict_headers (message_file,\r
+>> - "date",\r
+>> - "from",\r
+>> - "in-reply-to",\r
+>> - "message-id",\r
+>> - "references",\r
+>> - "subject",\r
+>> - "to",\r
+>> - (char *) NULL);\r
+>> + /* Parse message up front to get better error status. */\r
+>> + ret = notmuch_message_file_parse (message_file);\r
+>> + if (ret)\r
+>> + goto DONE;\r
+>> \r
+>> try {\r
+>> /* Before we do any real work, (especially before doing a\r
+>> @@ -2066,7 +2061,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,\r
+>> date = notmuch_message_file_get_header (message_file, "date");\r
+>> _notmuch_message_set_header_values (message, date, from, subject);\r
+>> \r
+>> - ret = _notmuch_message_index_file (message, filename);\r
+>> + ret = _notmuch_message_index_file (message, message_file);\r
+>> if (ret)\r
+>> goto DONE;\r
+>> } else {\r
+>> diff --git a/lib/index.cc b/lib/index.cc\r
+>> index 78c18cf36d10..46a019325454 100644\r
+>> --- a/lib/index.cc\r
+>> +++ b/lib/index.cc\r
+>> @@ -425,63 +425,15 @@ _index_mime_part (notmuch_message_t *message,\r
+>> \r
+>> notmuch_status_t\r
+>> _notmuch_message_index_file (notmuch_message_t *message,\r
+>> - const char *filename)\r
+>> + notmuch_message_file_t *message_file)\r
+>> {\r
+>> - GMimeStream *stream = NULL;\r
+>> - GMimeParser *parser = NULL;\r
+>> - GMimeMessage *mime_message = NULL;\r
+>> + GMimeMessage *mime_message;\r
+>> InternetAddressList *addresses;\r
+>> - FILE *file = NULL;\r
+>> const char *from, *subject;\r
+>> - notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;\r
+>> - static int initialized = 0;\r
+>> - char from_buf[5];\r
+>> - bool is_mbox = false;\r
+>> - static bool mbox_warning = false;\r
+>> -\r
+>> - if (! initialized) {\r
+>> - g_mime_init (GMIME_ENABLE_RFC2047_WORKAROUNDS);\r
+>> - initialized = 1;\r
+>> - }\r
+>> -\r
+>> - file = fopen (filename, "r");\r
+>> - if (! file) {\r
+>> - fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));\r
+>> - ret = NOTMUCH_STATUS_FILE_ERROR;\r
+>> - goto DONE;\r
+>> - }\r
+>> -\r
+>> - /* Is this mbox? */\r
+>> - if (fread (from_buf, sizeof (from_buf), 1, file) == 1 &&\r
+>> - strncmp (from_buf, "From ", 5) == 0)\r
+>> - is_mbox = true;\r
+>> - rewind (file);\r
+>> \r
+>> - /* Evil GMime steals my FILE* here so I won't fclose it. */\r
+>> - stream = g_mime_stream_file_new (file);\r
+>> -\r
+>> - parser = g_mime_parser_new_with_stream (stream);\r
+>> - g_mime_parser_set_scan_from (parser, is_mbox);\r
+>> -\r
+>> - mime_message = g_mime_parser_construct_message (parser);\r
+>> -\r
+>> - if (is_mbox) {\r
+>> - if (!g_mime_parser_eos (parser)) {\r
+>> - /* This is a multi-message mbox. */\r
+>> - ret = NOTMUCH_STATUS_FILE_NOT_EMAIL;\r
+>> - goto DONE;\r
+>> - }\r
+>> - /* For historical reasons, we support single-message mboxes,\r
+>> - * but this behavior is likely to change in the future, so\r
+>> - * warn. */\r
+>> - if (!mbox_warning) {\r
+>> - mbox_warning = true;\r
+>> - fprintf (stderr, "\\r
+>> -Warning: %s is an mbox containing a single message,\n\\r
+>> -likely caused by misconfigured mail delivery. Support for single-message\n\\r
+>> -mboxes is deprecated and may be removed in the future.\n", filename);\r
+>> - }\r
+>> - }\r
+>> + mime_message = notmuch_message_file_get_mime_message (message_file);\r
+>> + if (! mime_message)\r
+>> + return NOTMUCH_STATUS_FILE_NOT_EMAIL;\r
+>\r
+> This could also be because of out-of-memory.\r
+> notmuch_message_file_get_mime_message (and notmuch_message_file_parse,\r
+> in turn) should probably take a notmuch_status_t *status_ret argument\r
+> like other internal functions.\r
+\r
+Agreed (though I opted for returning notmuch_status_t and passing\r
+GMimeMessage** as parameter).\r
+\r
+>\r
+>> \r
+>> from = g_mime_message_get_sender (mime_message);\r
+>> \r
+>> @@ -502,15 +454,5 @@ mboxes is deprecated and may be removed in the future.\n", filename);\r
+>> \r
+>> _index_mime_part (message, g_mime_message_get_mime_part (mime_message));\r
+>> \r
+>> - DONE:\r
+>> - if (mime_message)\r
+>> - g_object_unref (mime_message);\r
+>> -\r
+>> - if (parser)\r
+>> - g_object_unref (parser);\r
+>> -\r
+>> - if (stream)\r
+>> - g_object_unref (stream);\r
+>> -\r
+>> - return ret;\r
+>> + return NOTMUCH_STATUS_SUCCESS;\r
+>> }\r
+>> diff --git a/lib/message-file.c b/lib/message-file.c\r
+>> index a2850c278b5a..88662608d319 100644\r
+>> --- a/lib/message-file.c\r
+>> +++ b/lib/message-file.c\r
+>> @@ -26,30 +26,15 @@\r
+>> \r
+>> #include <glib.h> /* GHashTable */\r
+>> \r
+>> -typedef struct {\r
+>> - char *str;\r
+>> - size_t size;\r
+>> - size_t len;\r
+>> -} header_value_closure_t;\r
+>> -\r
+>> struct _notmuch_message_file {\r
+>> /* File object */\r
+>> FILE *file;\r
+>> + char *filename;\r
+>> \r
+>> - /* Header storage */\r
+>> - int restrict_headers;\r
+>> + /* Cache for decoded headers */\r
+>> GHashTable *headers;\r
+>> - int broken_headers;\r
+>> - int good_headers;\r
+>> - size_t header_size; /* Length of full message header in bytes. */\r
+>> -\r
+>> - /* Parsing state */\r
+>> - char *line;\r
+>> - size_t line_size;\r
+>> - header_value_closure_t value;\r
+>> \r
+>> - int parsing_started;\r
+>> - int parsing_finished;\r
+>> + GMimeMessage *message;\r
+>> };\r
+>> \r
+>> static int\r
+>> @@ -76,15 +61,12 @@ strcase_hash (const void *ptr)\r
+>> static int\r
+>> _notmuch_message_file_destructor (notmuch_message_file_t *message)\r
+>> {\r
+>> - if (message->line)\r
+>> - free (message->line);\r
+>> -\r
+>> - if (message->value.size)\r
+>> - free (message->value.str);\r
+>> -\r
+>> if (message->headers)\r
+>> g_hash_table_destroy (message->headers);\r
+>> \r
+>> + if (message->message)\r
+>> + g_object_unref (message->message);\r
+>> +\r
+>> if (message->file)\r
+>> fclose (message->file);\r
+>> \r
+>> @@ -102,20 +84,17 @@ _notmuch_message_file_open_ctx (void *ctx, const char *filename)\r
+>> if (unlikely (message == NULL))\r
+>> return NULL;\r
+>> \r
+>> + /* Only needed for error messages during parsing. */\r
+>> + message->filename = talloc_strdup (message, filename);\r
+>> + if (message->filename == NULL)\r
+>> + goto FAIL;\r
+>> +\r
+>> talloc_set_destructor (message, _notmuch_message_file_destructor);\r
+>> \r
+>> message->file = fopen (filename, "r");\r
+>> if (message->file == NULL)\r
+>> goto FAIL;\r
+>> \r
+>> - message->headers = g_hash_table_new_full (strcase_hash,\r
+>> - strcase_equal,\r
+>> - free,\r
+>> - g_free);\r
+>> -\r
+>> - message->parsing_started = 0;\r
+>> - message->parsing_finished = 0;\r
+>> -\r
+>> return message;\r
+>> \r
+>> FAIL:\r
+>> @@ -137,264 +116,205 @@ notmuch_message_file_close (notmuch_message_file_t *message)\r
+>> talloc_free (message);\r
+>> }\r
+>> \r
+>> -void\r
+>> -notmuch_message_file_restrict_headersv (notmuch_message_file_t *message,\r
+>> - va_list va_headers)\r
+>> +notmuch_status_t\r
+>> +notmuch_message_file_parse (notmuch_message_file_t *message)\r
+>> {\r
+>> - char *header;\r
+>> + GMimeStream *stream;\r
+>> + GMimeParser *parser;\r
+>> + notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;\r
+>> + static int initialized = 0;\r
+>> + char from_buf[5];\r
+>> + notmuch_bool_t is_mbox = FALSE;\r
+>> + static notmuch_bool_t mbox_warning = FALSE;\r
+>> \r
+>> - if (message->parsing_started)\r
+>> - INTERNAL_ERROR ("notmuch_message_file_restrict_headers called after parsing has started");\r
+>> + if (message->message)\r
+>> + return NOTMUCH_STATUS_SUCCESS;\r
+>> \r
+>> - while (1) {\r
+>> - header = va_arg (va_headers, char*);\r
+>> - if (header == NULL)\r
+>> - break;\r
+>> - g_hash_table_insert (message->headers,\r
+>> - xstrdup (header), NULL);\r
+>> + if (! initialized) {\r
+>> + g_mime_init (GMIME_ENABLE_RFC2047_WORKAROUNDS);\r
+>> + initialized = 1;\r
+>> }\r
+>> \r
+>> - message->restrict_headers = 1;\r
+>> -}\r
+>> -\r
+>> -void\r
+>> -notmuch_message_file_restrict_headers (notmuch_message_file_t *message, ...)\r
+>> -{\r
+>> - va_list va_headers;\r
+>> -\r
+>> - va_start (va_headers, message);\r
+>> + message->headers = g_hash_table_new_full (strcase_hash, strcase_equal,\r
+>> + free, g_free);\r
+>> + if (! message->headers)\r
+>> + return NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>> \r
+>> - notmuch_message_file_restrict_headersv (message, va_headers);\r
+>> -}\r
+>> + /* Is this mbox? */\r
+>> + if (fread (from_buf, sizeof (from_buf), 1, message->file) == 1 &&\r
+>> + strncmp (from_buf, "From ", 5) == 0)\r
+>> + is_mbox = TRUE;\r
+>> + rewind (message->file);\r
+>> \r
+>> -static void\r
+>> -copy_header_unfolding (header_value_closure_t *value,\r
+>> - const char *chunk)\r
+>> -{\r
+>> - char *last;\r
+>> + stream = g_mime_stream_file_new (message->file);\r
+>> \r
+>> - if (chunk == NULL)\r
+>> - return;\r
+>> + /* We'll own and fclose the FILE* ourselves. */\r
+>> + g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);\r
+>> \r
+>> - while (*chunk == ' ' || *chunk == '\t')\r
+>> - chunk++;\r
+>> + parser = g_mime_parser_new_with_stream (stream);\r
+>> + g_mime_parser_set_scan_from (parser, is_mbox);\r
+>> \r
+>> - if (value->len + 1 + strlen (chunk) + 1 > value->size) {\r
+>> - unsigned int new_size = value->size;\r
+>> - if (value->size == 0)\r
+>> - new_size = strlen (chunk) + 1;\r
+>> - else\r
+>> - while (value->len + 1 + strlen (chunk) + 1 > new_size)\r
+>> - new_size *= 2;\r
+>> - value->str = xrealloc (value->str, new_size);\r
+>> - value->size = new_size;\r
+>> + message->message = g_mime_parser_construct_message (parser);\r
+>> + if (! message->message) {\r
+>> + status = NOTMUCH_STATUS_FILE_NOT_EMAIL;\r
+>> + goto DONE;\r
+>> }\r
+>> \r
+>> - last = value->str + value->len;\r
+>> - if (value->len) {\r
+>> - *last = ' ';\r
+>> - last++;\r
+>> - value->len++;\r
+>> + if (is_mbox) {\r
+>> + if (! g_mime_parser_eos (parser)) {\r
+>> + /* This is a multi-message mbox. */\r
+>> + status = NOTMUCH_STATUS_FILE_NOT_EMAIL;\r
+>> + goto DONE;\r
+>> + }\r
+>> + /*\r
+>> + * For historical reasons, we support single-message mboxes,\r
+>> + * but this behavior is likely to change in the future, so\r
+>> + * warn.\r
+>> + */\r
+>> + if (! mbox_warning) {\r
+>> + mbox_warning = TRUE;\r
+>> + fprintf (stderr, "\\r
+>> +Warning: %s is an mbox containing a single message,\n\\r
+>> +likely caused by misconfigured mail delivery. Support for single-message\n\\r
+>> +mboxes is deprecated and may be removed in the future.\n", message->filename);\r
+>\r
+> I still wonder if this is the right place to issue this warning. For\r
+> example, doesn't this mean that, when constructing a reply to a single\r
+> message mbox, we'll issue this warning?\r
+>\r
+> Or maybe it's time to drop single message mbox support. We deprecated\r
+> it in 0.15.1, which was over a year ago. OTOH, wheezy still has\r
+> 0.13.2, so there could be people still using mboxes without the\r
+> warning. OTOOH, we only kept this around to appear David anyway. So\r
+> maybe the answer is to leave this in an not think too hard about it.\r
+\r
+I'm adding a prep patch to drop mbox support altogether.\r
+\r
+>\r
+>> + }\r
+>> }\r
+>> \r
+>> - strcpy (last, chunk);\r
+>> - value->len += strlen (chunk);\r
+>> + DONE:\r
+>> + g_object_unref (stream);\r
+>> + g_object_unref (parser);\r
+>> +\r
+>> + if (status) {\r
+>> + g_hash_table_destroy (message->headers);\r
+>> + message->headers = NULL;\r
+>> \r
+>> - last = value->str + value->len - 1;\r
+>> - if (*last == '\n') {\r
+>> - *last = '\0';\r
+>> - value->len--;\r
+>> + if (message->message) {\r
+>> + g_object_unref (message->message);\r
+>> + message->message = NULL;\r
+>> + }\r
+>> +\r
+>> + rewind (message->file);\r
+>> }\r
+>> +\r
+>> + return status;\r
+>> }\r
+>> \r
+>> -/* As a special-case, a value of NULL for header_desired will force\r
+>> - * the entire header to be parsed if it is not parsed already. This is\r
+>> - * used by the _notmuch_message_file_get_headers_end function.\r
+>> - * Another special case is the Received: header. For this header we\r
+>> - * want to concatenate all instances of the header instead of just\r
+>> - * hashing the first instance as we use this when analyzing the path\r
+>> - * the mail has taken from sender to recipient.\r
+>> - */\r
+>> -const char *\r
+>> -notmuch_message_file_get_header (notmuch_message_file_t *message,\r
+>> - const char *header_desired)\r
+>> +GMimeMessage *\r
+>> +notmuch_message_file_get_mime_message (notmuch_message_file_t *message)\r
+>> {\r
+>> - int contains;\r
+>> - char *header, *decoded_value, *header_sofar, *combined_header;\r
+>> - const char *s, *colon;\r
+>> - int match, newhdr, hdrsofar, is_received;\r
+>> - static int initialized = 0;\r
+>> + if (notmuch_message_file_parse (message))\r
+>> + return NULL;\r
+>> \r
+>> - is_received = (strcmp(header_desired,"received") == 0);\r
+>> + return message->message;\r
+>> +}\r
+>> \r
+>> - if (! initialized) {\r
+>> - g_mime_init (GMIME_ENABLE_RFC2047_WORKAROUNDS);\r
+>> - initialized = 1;\r
+>> - }\r
+>> +/*\r
+>> + * Get all instances of a header decoded and concatenated.\r
+>> + *\r
+>> + * The result must be freed using g_free().\r
+>> + *\r
+>> + * Return NULL on errors, empty string for non-existing headers.\r
+>> + */\r
+>> +static char *\r
+>> +_notmuch_message_file_get_combined_header (notmuch_message_file_t *message,\r
+>> + const char *header)\r
+>> +{\r
+>> + GMimeHeaderList *headers;\r
+>> + GMimeHeaderIter *iter;\r
+>> + char *combined = NULL;\r
+>> \r
+>> - message->parsing_started = 1;\r
+>> -\r
+>> - if (header_desired == NULL)\r
+>> - contains = 0;\r
+>> - else\r
+>> - contains = g_hash_table_lookup_extended (message->headers,\r
+>> - header_desired, NULL,\r
+>> - (gpointer *) &decoded_value);\r
+>> -\r
+>> - if (contains && decoded_value)\r
+>> - return decoded_value;\r
+>> -\r
+>> - if (message->parsing_finished)\r
+>> - return "";\r
+>> -\r
+>> -#define NEXT_HEADER_LINE(closure) \\r
+>> - while (1) { \\r
+>> - ssize_t bytes_read = getline (&message->line, \\r
+>> - &message->line_size, \\r
+>> - message->file); \\r
+>> - if (bytes_read == -1) { \\r
+>> - message->parsing_finished = 1; \\r
+>> - break; \\r
+>> - } \\r
+>> - if (*message->line == '\n') { \\r
+>> - message->parsing_finished = 1; \\r
+>> - break; \\r
+>> - } \\r
+>> - if (closure && \\r
+>> - (*message->line == ' ' || *message->line == '\t')) \\r
+>> - { \\r
+>> - copy_header_unfolding ((closure), message->line); \\r
+>> - } \\r
+>> - if (*message->line == ' ' || *message->line == '\t') \\r
+>> - message->header_size += strlen (message->line); \\r
+>> - else \\r
+>> - break; \\r
+>> - }\r
+>> + headers = g_mime_object_get_header_list (GMIME_OBJECT (message->message));\r
+>> + if (! headers)\r
+>> + return NULL;\r
+>> \r
+>> - if (message->line == NULL)\r
+>> - NEXT_HEADER_LINE (NULL);\r
+>> + iter = g_mime_header_iter_new ();\r
+>> + if (! iter)\r
+>> + return NULL;\r
+>> \r
+>> - while (1) {\r
+>> + if (! g_mime_header_list_get_iter (headers, iter))\r
+>> + goto DONE;\r
+>> \r
+>> - if (message->parsing_finished)\r
+>> - break;\r
+>> + do {\r
+>> + const char *value;\r
+>\r
+> Incorrect indentation.\r
+\r
+I wonder what happened there!\r
+\r
+>\r
+>> + char *decoded;\r
+>> \r
+>> - colon = strchr (message->line, ':');\r
+>> + if (strcasecmp (g_mime_header_iter_get_name (iter), header) != 0)\r
+>> + continue;\r
+>> \r
+>> - if (colon == NULL) {\r
+>> - message->broken_headers++;\r
+>> - /* A simple heuristic for giving up on things that just\r
+>> - * don't look like mail messages. */\r
+>> - if (message->broken_headers >= 10 &&\r
+>> - message->good_headers < 5)\r
+>> - {\r
+>> - message->parsing_finished = 1;\r
+>> - break;\r
+>> + value = g_mime_header_iter_get_value (iter);\r
+>> + decoded = g_mime_utils_header_decode_text (value);\r
+>\r
+> Maybe a comment saying that GMime retains ownership of value, but\r
+> decoded must be freed with g_free? GMime is confusing about\r
+> ownership, and the fact that value *doesn't* have to be freed (which\r
+> is not documented by GMime!) just adds to the confusion. I could see\r
+> this being easy to screw up during later code changes.\r
+\r
+Agreed.\r
+\r
+>\r
+>> + if (! decoded) {\r
+>> + if (combined) {\r
+>> + g_free (combined);\r
+>> + combined = NULL;\r
+>> }\r
+>> - NEXT_HEADER_LINE (NULL);\r
+>> - continue;\r
+>> + goto DONE;\r
+>> }\r
+>> \r
+>> - message->header_size += strlen (message->line);\r
+>> + if (combined) {\r
+>> + char *tmp = g_strdup_printf ("%s %s", combined, decoded);\r
+>> + g_free (decoded);\r
+>> + g_free (combined);\r
+>> + if (! tmp) {\r
+>> + combined = NULL;\r
+>> + goto DONE;\r
+>> + }\r
+>> \r
+>> - message->good_headers++;\r
+>> + combined = tmp;\r
+>> + } else {\r
+>> + combined = decoded;\r
+>> + }\r
+>> + } while (g_mime_header_iter_next (iter));\r
+>\r
+> Yet another funny GMime API... Did you consider\r
+>\r
+> for (; g_mime_header_iter_is_valid (iter); g_mime_header_iter_next (iter))\r
+>\r
+> ? I realize that the g_mime_header_iter_is_valid isn't actually\r
+> necessary because g_mime_header_list_get_iter returns NULL if the\r
+> iterator would be empty, but using a for-loop would make it more\r
+> obvious that this code is correct.\r
+\r
+As I mentioned on IRC, that would lead to an infinite loop. Ugh.\r
+\r
+>\r
+>> \r
+>> - header = xstrndup (message->line, colon - message->line);\r
+>> + /* Return empty string for non-existing headers. */\r
+>> + if (! combined)\r
+>> + combined = g_strdup ("");\r
+>> \r
+>> - if (message->restrict_headers &&\r
+>> - ! g_hash_table_lookup_extended (message->headers,\r
+>> - header, NULL, NULL))\r
+>> - {\r
+>> - free (header);\r
+>> - NEXT_HEADER_LINE (NULL);\r
+>> - continue;\r
+>> - }\r
+>> + DONE:\r
+>> + g_mime_header_iter_free (iter);\r
+>> \r
+>> - s = colon + 1;\r
+>> - while (*s == ' ' || *s == '\t')\r
+>> - s++;\r
+>> + return combined;\r
+>> +}\r
+>\r
+> Oof. That received header had better be worth it!\r
+\r
+I'm not sure it is, but we've got users for it, so...\r
+\r
+>\r
+>> \r
+>> - message->value.len = 0;\r
+>> - copy_header_unfolding (&message->value, s);\r
+>> +/* Return NULL on errors, empty string for non-existing headers. */\r
+>\r
+> This may sound strange, but I'd be inclined to omit this comment,\r
+> since it made me not realize I should go looking for the much more\r
+> thorough comment in notmuch-private.h.\r
+\r
+Agreed.\r
+\r
+>\r
+>> +const char *\r
+>> +notmuch_message_file_get_header (notmuch_message_file_t *message,\r
+>> + const char *header)\r
+>> +{\r
+>> + const char *value = NULL;\r
+>> + char *decoded;\r
+>> + notmuch_bool_t found;\r
+>> \r
+>> - NEXT_HEADER_LINE (&message->value);\r
+>> + if (notmuch_message_file_parse (message))\r
+>> + return NULL;\r
+>> \r
+>> - if (header_desired == NULL)\r
+>> - match = 0;\r
+>> + /* If we have a cached decoded value, use it. */\r
+>> + found = g_hash_table_lookup_extended (message->headers, header,\r
+>> + NULL, (gpointer *) &value);\r
+>> + if (found)\r
+>> + return value ? value : "";\r
+>\r
+> When would header be found, but value be NULL? (If this can't in fact\r
+> happen, how about using g_hash_table_lookup?)\r
+\r
+Right. This was a remnant of header restricting which has now been\r
+removed. Switching to g_hash_table_lookup.\r
+\r
+>\r
+>> +\r
+>> + if (strcasecmp (header, "received") == 0) {\r
+>> + /*\r
+>> + * The Received: header is special. We concatenate all\r
+>> + * instances of the header as we use this when analyzing the\r
+>> + * path the mail has taken from sender to recipient.\r
+>> + */\r
+>> + decoded = _notmuch_message_file_get_combined_header (message, header);\r
+>> + } else {\r
+>> + value = g_mime_object_get_header (GMIME_OBJECT (message->message),\r
+>> + header);\r
+>> + if (value)\r
+>> + decoded = g_mime_utils_header_decode_text (value);\r
+>> else\r
+>> - match = (strcasecmp (header, header_desired) == 0);\r
+>> -\r
+>> - decoded_value = g_mime_utils_header_decode_text (message->value.str);\r
+>> - header_sofar = (char *)g_hash_table_lookup (message->headers, header);\r
+>> - /* we treat the Received: header special - we want to concat ALL of \r
+>> - * the Received: headers we encounter.\r
+>> - * for everything else we return the first instance of a header */\r
+>> - if (strcasecmp(header, "received") == 0) {\r
+>> - if (header_sofar == NULL) {\r
+>> - /* first Received: header we encountered; just add it */\r
+>> - g_hash_table_insert (message->headers, header, decoded_value);\r
+>> - } else {\r
+>> - /* we need to add the header to those we already collected */\r
+>> - newhdr = strlen(decoded_value);\r
+>> - hdrsofar = strlen(header_sofar);\r
+>> - combined_header = g_malloc(hdrsofar + newhdr + 2);\r
+>> - strncpy(combined_header,header_sofar,hdrsofar);\r
+>> - *(combined_header+hdrsofar) = ' ';\r
+>> - strncpy(combined_header+hdrsofar+1,decoded_value,newhdr+1);\r
+>> - g_free (decoded_value);\r
+>> - g_hash_table_insert (message->headers, header, combined_header);\r
+>> - }\r
+>> - } else {\r
+>> - if (header_sofar == NULL) {\r
+>> - /* Only insert if we don't have a value for this header, yet. */\r
+>> - g_hash_table_insert (message->headers, header, decoded_value);\r
+>> - } else {\r
+>> - free (header);\r
+>> - g_free (decoded_value);\r
+>> - decoded_value = header_sofar;\r
+>> - }\r
+>> - }\r
+>> - /* if we found a match we can bail - unless of course we are\r
+>> - * collecting all the Received: headers */\r
+>> - if (match && !is_received)\r
+>> - return decoded_value;\r
+>> - }\r
+>> -\r
+>> - if (message->parsing_finished) {\r
+>> - fclose (message->file);\r
+>> - message->file = NULL;\r
+>> + decoded = g_strdup ("");\r
+>> }\r
+>> \r
+>> - if (message->line)\r
+>> - free (message->line);\r
+>> - message->line = NULL;\r
+>> -\r
+>> - if (message->value.size) {\r
+>> - free (message->value.str);\r
+>> - message->value.str = NULL;\r
+>> - message->value.size = 0;\r
+>> - message->value.len = 0;\r
+>> - }\r
+>> + if (! decoded)\r
+>> + return NULL;\r
+>> \r
+>> - /* For the Received: header we actually might end up here even\r
+>> - * though we found the header (as we force continued parsing\r
+>> - * in that case). So let's check if that's the header we were\r
+>> - * looking for and return the value that we found (if any)\r
+>> - */\r
+>> - if (is_received)\r
+>> - return (char *)g_hash_table_lookup (message->headers, "received");\r
+>> -\r
+>> - /* We've parsed all headers and never found the one we're looking\r
+>> - * for. It's probably just not there, but let's check that we\r
+>> - * didn't make a mistake preventing us from seeing it. */\r
+>> - if (message->restrict_headers && header_desired &&\r
+>> - ! g_hash_table_lookup_extended (message->headers,\r
+>> - header_desired, NULL, NULL))\r
+>> - {\r
+>> - INTERNAL_ERROR ("Attempt to get header \"%s\" which was not\n"\r
+>> - "included in call to notmuch_message_file_restrict_headers\n",\r
+>> - header_desired);\r
+>> - }\r
+>> + /* Cache the decoded value. We also own the strings. */\r
+>> + g_hash_table_insert (message->headers, xstrdup (header), decoded);\r
+>> \r
+>> - return "";\r
+>> + return decoded;\r
+>> }\r
+>> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h\r
+>> index 59eb2bc285a5..734a4e338554 100644\r
+>> --- a/lib/notmuch-private.h\r
+>> +++ b/lib/notmuch-private.h\r
+>> @@ -46,6 +46,8 @@ NOTMUCH_BEGIN_DECLS\r
+>> \r
+>> #include <talloc.h>\r
+>> \r
+>> +#include <gmime/gmime.h>\r
+>> +\r
+>> #include "xutil.h"\r
+>> #include "error_util.h"\r
+>> \r
+>> @@ -320,13 +322,6 @@ notmuch_message_set_author (notmuch_message_t *message, const char *author);\r
+>> const char *\r
+>> notmuch_message_get_author (notmuch_message_t *message);\r
+>> \r
+>> -\r
+>> -/* index.cc */\r
+>> -\r
+>> -notmuch_status_t\r
+>> -_notmuch_message_index_file (notmuch_message_t *message,\r
+>> - const char *filename);\r
+>> -\r
+>> /* message-file.c */\r
+>> \r
+>> /* XXX: I haven't decided yet whether these will actually get exported\r
+>> @@ -352,32 +347,31 @@ _notmuch_message_file_open_ctx (void *ctx, const char *filename);\r
+>> void\r
+>> notmuch_message_file_close (notmuch_message_file_t *message);\r
+>> \r
+>> -/* Restrict 'message' to only save the named headers.\r
+>> +/* Parse the message.\r
+>> *\r
+>> - * When the caller is only interested in a short list of headers,\r
+>> - * known in advance, calling this function can avoid wasted time and\r
+>> - * memory parsing/saving header values that will never be needed.\r
+>> + * This will be done automatically as necessary on other calls\r
+>> + * depending on it, but an explicit call allows for better error\r
+>> + * status reporting.\r
+>> + */\r
+>> +notmuch_status_t\r
+>> +notmuch_message_file_parse (notmuch_message_file_t *message);\r
+>\r
+> Most internal functions have a leading _. (We've diverged from that\r
+> in a few places, especially in notmuch_message_file_*, but it would be\r
+> nice to not diverge further from that convention.)\r
+\r
+Fixed.\r
+\r
+>\r
+>> +\r
+>> +/* Get the gmime message of a message file.\r
+>> *\r
+>> - * The variable arguments should be a list of const char * with a\r
+>> - * final '(const char *) NULL' to terminate the list.\r
+>> + * The message file is parsed as necessary.\r
+>> *\r
+>> - * If this function is called, it must be called before any calls to\r
+>> - * notmuch_message_get_header for this message.\r
+>> + * Returns GMimeMessage* on success (which the caller must not unref),\r
+>> + * NULL if the message file parsing fails.\r
+>> *\r
+>> - * After calling this function, if notmuch_message_get_header is\r
+>> - * called with a header name not in this list, then NULL will be\r
+>> - * returned even if that header exists in the actual message.\r
+>> + * XXX: Would be nice to not have to expose GMimeMessage here.\r
+>> */\r
+>> -void\r
+>> -notmuch_message_file_restrict_headers (notmuch_message_file_t *message, ...);\r
+>> -\r
+>> -/* Identical to notmuch_message_restrict_headers but accepting a va_list. */\r
+>> -void\r
+>> -notmuch_message_file_restrict_headersv (notmuch_message_file_t *message,\r
+>> - va_list va_headers);\r
+>> +GMimeMessage *\r
+>> +notmuch_message_file_get_mime_message (notmuch_message_file_t *message);\r
+>\r
+> Same comment about _.\r
+\r
+Ditto.\r
+\r
+>\r
+>> \r
+>> /* Get the value of the specified header from the message as a UTF-8 string.\r
+>> *\r
+>> + * The message file is parsed as necessary.\r
+>> + *\r
+>> * The header name is case insensitive.\r
+>> *\r
+>> * The Received: header is special - for it all Received: headers in\r
+>> @@ -387,13 +381,19 @@ notmuch_message_file_restrict_headersv (notmuch_message_file_t *message,\r
+>> * only until the message is closed. The caller should copy it if\r
+>> * needing to modify the value or to hold onto it for longer.\r
+>> *\r
+>> - * Returns NULL if the message does not contain a header line matching\r
+>> - * 'header'.\r
+>> + * Returns NULL on errors, empty string if the message does not\r
+>> + * contain a header line matching 'header'.\r
+>> */\r
+>> const char *\r
+>> notmuch_message_file_get_header (notmuch_message_file_t *message,\r
+>> const char *header);\r
+>> \r
+>> +/* index.cc */\r
+>> +\r
+>> +notmuch_status_t\r
+>> +_notmuch_message_index_file (notmuch_message_t *message,\r
+>> + notmuch_message_file_t *message_file);\r
+>> +\r
+>\r
+> Any particular reason this floated down? (Obviously it's not an\r
+> actual problem.)\r
+\r
+It needs the typedef for notmuch_message_file_t. Previous versions of\r
+the patch moved the typedef, but this keeps the typedef next to other\r
+stuff from message-file.c.\r
+\r
+\r
+Thanks for the review.\r
+\r
+BR,\r
+Jani.\r
+\r
+\r
+\r
+>\r
+>> /* messages.c */\r
+>> \r
+>> typedef struct _notmuch_message_node {\r
projects / notmuch-archives.git / commitdiff