From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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----- --=-=-=--