Re: [RFC] [PATCH] lib/database.cc: change how the parent of a message is calculated

diff --git a/a4/8857eac6ebfea2007dc38b73848f29813135d8 b/a4/8857eac6ebfea2007dc38b73848f29813135d8
new file mode 100644 (file)
index 0000000..0320ca5
--- /dev/null
+++ b/a4/8857eac6ebfea2007dc38b73848f29813135d8
@@ -0,0 +1,162 @@
+Return-Path: <aaronecay@gmail.com>\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 2FD0D431FAF\r
+       for <notmuch@notmuchmail.org>; Sun,  3 Mar 2013 15:46:22 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: 1.7\r
+X-Spam-Level: *\r
+X-Spam-Status: No, score=1.7 tagged_above=-999 required=5\r
+       tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+       FREEMAIL_FROM=0.001, FREEMAIL_REPLY=2.499, RCVD_IN_DNSWL_LOW=-0.7]\r
+       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 itfX0F7TynOB for <notmuch@notmuchmail.org>;\r
+       Sun,  3 Mar 2013 15:46:21 -0800 (PST)\r
+Received: from mail-qe0-f54.google.com (mail-qe0-f54.google.com\r
+       [209.85.128.54]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+       (No client certificate requested)\r
+       by olra.theworths.org (Postfix) with ESMTPS id 4CCB0431FAE\r
+       for <notmuch@notmuchmail.org>; Sun,  3 Mar 2013 15:46:21 -0800 (PST)\r
+Received: by mail-qe0-f54.google.com with SMTP id i11so2674234qej.27\r
+       for <notmuch@notmuchmail.org>; Sun, 03 Mar 2013 15:46:20 -0800 (PST)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
+       h=x-received:from:to:cc:subject:in-reply-to:references:user-agent\r
+       :date:message-id:mime-version:content-type:content-transfer-encoding;\r
+       bh=XeVIO0io3ByeZxaPSIdKKwTeTcZKfzZwQRrpoPdQ2uA=;\r
+       b=xr6wNjvgKUQL0IGZBCoLzpXgfkFys+fmchU0uI75Y75Zt1X4uDNw7PT8VN4XU+tIv+\r
+       i8skIPlntYNVTxO9eRDySeA5ZR+YeJhHsE485pczbndalRt6hTt4IdiGAWYGj9l20HLy\r
+       9NbqS4ipgMlWcPCXLbuFpi4+SGMG9aDJhkcZroMg64sP0Sg/6Ad99DovFKiS9Er8KL98\r
+       7u5wzYIoRjK08Nsm52iA1s3kf3YmH5NGPOEVSlYlMpwbe/NeP8DcogFWwweCCFMAOUL7\r
+       8qmTPmVI8ymg6ku6TlQmLzjcqryKZEwskQpD04BQJgK9RhyI8Z2kRMJBt5S+JtnKncc5\r
+       qiYA==\r
+X-Received: by 10.229.175.25 with SMTP id v25mr6301859qcz.36.1362354379700;\r
+       Sun, 03 Mar 2013 15:46:19 -0800 (PST)\r
+Received: from localhost (c-68-80-94-73.hsd1.pa.comcast.net. [68.80.94.73])\r
+       by mx.google.com with ESMTPS id g6sm34092051qav.6.2013.03.03.15.46.18\r
+       (version=TLSv1.2 cipher=RC4-SHA bits=128/128);\r
+       Sun, 03 Mar 2013 15:46:19 -0800 (PST)\r
+From: Aaron Ecay <aaronecay@gmail.com>\r
+To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
+Subject: Re: [RFC] [PATCH] lib/database.cc: change how the parent of a message\r
+       is calculated\r
+In-Reply-To: <871ubzt5gr.fsf@nikula.org>\r
+References: <1361836225-17279-1-git-send-email-aaronecay@gmail.com>\r
+       <87621cteeb.fsf@nikula.org> <871ubzt5gr.fsf@nikula.org>\r
+User-Agent: Notmuch/0.15.2+33~g0c0a530 (http://notmuchmail.org)\r
+       Emacs/24.3.50.2 (x86_64-unknown-linux-gnu)\r
+Date: Sun, 03 Mar 2013 18:46:18 -0500\r
+Message-ID: <87wqtovygl.fsf@gmail.com>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=utf-8\r
+Content-Transfer-Encoding: quoted-printable\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, 03 Mar 2013 23:46:22 -0000\r
+\r
+Hi Jani,\r
+\r
+Thanks to you and Austin for the comments.\r
+\r
+2013ko martxoak 1an, Jani Nikula-ek idatzi zuen:\r
+>> I think the background is that RFC 822 defines In-Reply-To (and\r
+>> References too for that matter) as *(phrase / msg-id), while RFC 2822\r
+>> defines them as 1*msg-id. I'd like something about RFC 822 being\r
+>> mentioned in the commit message.\r
+>>=20\r
+>> The problem in the gmane message you link to in\r
+>> id:87liaa3luc.fsf@gmail.com is likely related to the FAQ item 05.26\r
+>> "How do I fix a bogus In-Reply-To or missing References field?" in\r
+>> the MH FAQ http://www.newt.com/faq/mh.html.\r
+\r
+Likely yes.  But I think notmuch should handle these messages, since\r
+they are seen in the wild (and I don=E2=80=99t think you disagree with me on\r
+this point?)\r
+\r
+\r
+>>=20\r
+>> As the comment for the function says, we explicitly avoid including\r
+>> self-references. I think I'd err on the safe side and return NULL if\r
+>> the last ref equals message-id.\r
+\r
+Done.\r
+\r
+>>=20\r
+>> I don't know how you got this non-change hunk here, but please remove\r
+>> it. :)\r
+\r
+That=E2=80=99s what I get for setting my editor to delete trailing whitespa=\r
+ce on\r
+save (then not reading outgoing patches carefully).  Fixed.\r
+\r
+>> I wonder if you should reuse your parse_references() change here, so\r
+>> you'd set in_reply_to_message_id to the last message-id in\r
+>> In-Reply-To. This might tackle some of the problematic cases\r
+>> directly, but should still be all right per RFC 2822. I didn't verify\r
+>> how the parser handles an RFC 2822 violating free form header though.\r
+>=20\r
+> Strike that based on http://www.jwz.org/doc/threading.html:\r
+>=20\r
+> "If there are multiple things in In-Reply-To that look like\r
+> Message-IDs, only use the first one of them: odds are that the later\r
+> ones are actually email addresses, not IDs."\r
+\r
+Hmm.  I think it=E2=80=99s a toss-up which of multiple quasi-message-ids is=\r
+ the\r
+real one.  In the email message example I linked upthread, it was the\r
+last one that was real.  I decided to use the last one, because it\r
+allows the self-reference checking to be pushed entirely into\r
+parse_references.  If you feel strongly that we should use the first\r
+one, I can change it back.\r
+\r
+> I talked to Austin (CC) about the patch on IRC, and his comment was,\r
+> perceptive as always:\r
+>=20\r
+>  23:38 amdragon Is the logic in that patch equivalent to always using\r
+> the last message ID in references unless there is no references\r
+> header?  Seems like it is, but in a convoluted way.\r
+>=20\r
+> And that's actually the case, isn't it? To make the code reflect that,\r
+> you should use last_ref_message_id, and if that's NULL, fallback to\r
+> in_reply_to_message_id.\r
+\r
+Yes.  Fixed.\r
+\r
+>=20\r
+>> I suggest adding an else if branch (or revamp the above if condition)\r
+>> to tackle the missing In-Reply-To header:\r
+>>=20\r
+>> else if (!in_reply_to_message_id && last_ref_message_id) {\r
+>> in_reply_to_message_id =3D last_ref_message_id; }\r
+>=20\r
+> Strike that, it should be the other way round.\r
+\r
+Now that the self-reference check is in parse_references, the\r
+conditional is much simpler.\r
+\r
+One additional change I made in this version was to factor out 3 calls\r
+to =E2=80=9Cnotmuch_message_get_message_id (message)=E2=80=9D into a variab=\r
+le inside the\r
+_notmuch_database_link_message_to_parents function, for a small boost to\r
+readability (and perhaps speed, depending on how clever the compiler is\r
+I guess).\r
+\r
+I also added tests =E2=80=93 those are the first of two patches that will f=\r
+ollow\r
+this email, the second being the code to make them pass.\r
+\r
+--=20\r
+Aaron Ecay\r