From a42336bf2c18e93a5d702ba279662536680dd1a9 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Wed, 6 Apr 2016 17:05:06 +2100 Subject: [PATCH] Re: [PATCH v2 3/7] fix thread breakage via ghost-on-removal --- ad/b5e66b0cc9756646b90d4e3eaf120a5de5f15b | 137 ++++++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 ad/b5e66b0cc9756646b90d4e3eaf120a5de5f15b diff --git a/ad/b5e66b0cc9756646b90d4e3eaf120a5de5f15b b/ad/b5e66b0cc9756646b90d4e3eaf120a5de5f15b new file mode 100644 index 000000000..c37a11a41 --- /dev/null +++ b/ad/b5e66b0cc9756646b90d4e3eaf120a5de5f15b @@ -0,0 +1,137 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by arlo.cworth.org (Postfix) with ESMTP id 86AFB6DE0243 + for ; Tue, 5 Apr 2016 13:05:31 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at cworth.org +X-Spam-Flag: NO +X-Spam-Score: -0.029 +X-Spam-Level: +X-Spam-Status: No, score=-0.029 tagged_above=-999 required=5 + tests=[AWL=-0.029] autolearn=disabled +Received: from arlo.cworth.org ([127.0.0.1]) + by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id b0atHe6C1KdY for ; + Tue, 5 Apr 2016 13:05:22 -0700 (PDT) +Received: from che.mayfirst.org (che.mayfirst.org [209.234.253.108]) + by arlo.cworth.org (Postfix) with ESMTP id 6DDBE6DE01F7 + for ; Tue, 5 Apr 2016 13:05:22 -0700 (PDT) +Received: from fifthhorseman.net (dhcp-a215.meeting.ietf.org [31.133.162.21]) + by che.mayfirst.org (Postfix) with ESMTPSA id D31E8F991; + Tue, 5 Apr 2016 16:05:17 -0400 (EDT) +Received: by fifthhorseman.net (Postfix, from userid 1000) + id 75104200DD; Tue, 5 Apr 2016 17:05:09 -0300 (BRT) +From: Daniel Kahn Gillmor +To: Tomi Ollila , Notmuch Mail +Subject: Re: [PATCH v2 3/7] fix thread breakage via ghost-on-removal +In-Reply-To: +References: <1459445693-3900-1-git-send-email-dkg@fifthhorseman.net> + <1459606541-23889-1-git-send-email-dkg@fifthhorseman.net> + <1459606541-23889-3-git-send-email-dkg@fifthhorseman.net> + +User-Agent: Notmuch/0.21+124~gbf604e9 (http://notmuchmail.org) Emacs/24.5.1 + (x86_64-pc-linux-gnu) +Date: Tue, 05 Apr 2016 17:05:06 -0300 +Message-ID: <874mbfvn2l.fsf@alice.fifthhorseman.net> +MIME-Version: 1.0 +Content-Type: multipart/signed; boundary="=-=-="; + micalg=pgp-sha512; protocol="application/pgp-signature" +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.20 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Tue, 05 Apr 2016 20:05:31 -0000 + +--=-=-= +Content-Type: text/plain + +Hi Tomi-- + +On Tue 2016-04-05 03:53:34 -0300, Tomi Ollila wrote: +> I fetched your changes from lair.fifthhorseman.net yesterday and diffed +> against master; looks pretty good, some quick comments (this email has +> most relevant changes related to those changes): + +thanks for the review! + +>> -/* Delete a message document from the database. */ +>> +/* Delete a message document from the database, leaving a ghost +>> + * message in its place */ +> +> This comment could tell the condition when ghost message is left -- +> versus the case all ghost messages are dropped (thread becomes empty of +> mail messages). + +right, i can improve these comments. + +> In next lines the expected case NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND +> could be first. Although the performance difference is negligible to me +> it looks silly do this first check and basically always fail there and +> then do 'else if' which is likely to succeeed... +> (your latest version in lair does not return in this first case but sets +> status to NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID. Perhaps later messages in this +> thread does the same but those are somewhat out-of-context for this reply). + +sure, i can do the positive check first :) + +>> + if (private_status == NOTMUCH_PRIVATE_STATUS_SUCCESS) { +>> + /* this is deeply weird, and we should not have gotten into +>> + this state. is there a better error message to return +>> + here? */ +>> + return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID; +>> + } else if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) { +>> + private_status = _notmuch_message_initialize_ghost (ghost, tid); +>> + if (! private_status) +>> + _notmuch_message_sync (ghost); +>> + } +>> + notmuch_message_destroy (ghost); +>> + return COERCE_STATUS (private_status, "Error converting to ghost message"); +>> } +> +> Outside of this patch, but in some of the next messages, adds functions +> _notmuch_message_has_term() and _notmuch_message_has_term_st(). Perhaps +> the _notmuch_message_has_term() could be left unimplemented? + +yeah, i can do that, though i have to say it's programmatically +convenient to have a simple boolean test that defaults to some value if +there was an error. + +I'll post one more round of patches to the mailing list to address these +cleanup bits in the next day or two. + +I'm happy to read more reviews if anyone has them, + + --dkg + +--=-=-= +Content-Type: application/pgp-signature; name="signature.asc" + +-----BEGIN PGP SIGNATURE----- +Version: GnuPG v2 + +iQJ8BAEBCgBmBQJXBBpyXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w +ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRFREIyRTc0RjU2RkNGMkI2NzI5N0I3MzUy +NEVDRkY1QUZGNjgzNzBBAAoJECTs/1r/aDcKna8QAJP3qDgHG/NXoLnrPZfnTTII +pPvNxW+EfeV5QT8z2LOzOCV3qAhty7cg/ga0AyJsB2RDcSa4biWiGzh3MbvJz/ZJ +uML2vuW/qOKENkTbfOYNgDUVK9+7tOEjLw85Py1U6EsHGeUAT0aazA9trdR6naC/ +I0udxxaNMKqE8JLC1aWNw629AcwkKklvqP1bX4/q2DASdVMnOJwAPZTwkOxDRkpP +gMkOrBZA+MSkzd55OWFbDCstlIF27PmUx/6dqBaYHp4l+Qk3ikbw1hAquyPNgK5v +68XtR8yabu8ULktTbYR0xin3NuPv/osYEH01PHWzFn/6B7BlimlFhd4JqmYR5ZGT +QHTLLFI8tb8wPK7VTj0gt7WnnpARDTBpGOFbV7UW6asi18IiEdfUrdVe58W9wpCG +72o6aa3aYPEB13Jbz2MHMo6UrdVvB6aaYIaPaK1ngAbnXeatGko/v2zQq9Gi4142 +4yHqcDYZmh1xE3yl6mI/zb38l1ywYfh7k4lBjbtggoIzcXk0FuBDqZjVltvNlKjC +J33E7QAxEhP9tB9boVjUbhHrfqNY4nzMpF6FESkjQoFbZqpucBVDexqwkjWJkxBu +ovsmsH4hfdfdAVc0pbKF8SlCiF3ptG7vqsTuJz1fRRaNEQ4tNB3tdXPD+FeTZAzL +9gTwM9LAtdqO1YtRDW4R +=nBqb +-----END PGP SIGNATURE----- +--=-=-=-- -- 2.26.2