ad/b5e66b0cc9756646b90d4e3eaf120a5de5f15b

   1 Return-Path: <dkg@fifthhorseman.net>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5  by arlo.cworth.org (Postfix) with ESMTP id 86AFB6DE0243\r
   6  for <notmuch@notmuchmail.org>; Tue,  5 Apr 2016 13:05:31 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at cworth.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.029\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.029 tagged_above=-999 required=5\r
  12  tests=[AWL=-0.029] autolearn=disabled\r
  13 Received: from arlo.cworth.org ([127.0.0.1])\r
  14  by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15  with ESMTP id b0atHe6C1KdY for <notmuch@notmuchmail.org>;\r
  16  Tue,  5 Apr 2016 13:05:22 -0700 (PDT)\r
  17 Received: from che.mayfirst.org (che.mayfirst.org [209.234.253.108])\r
  18  by arlo.cworth.org (Postfix) with ESMTP id 6DDBE6DE01F7\r
  19  for <notmuch@notmuchmail.org>; Tue,  5 Apr 2016 13:05:22 -0700 (PDT)\r
  20 Received: from fifthhorseman.net (dhcp-a215.meeting.ietf.org [31.133.162.21])\r
  21  by che.mayfirst.org (Postfix) with ESMTPSA id D31E8F991;\r
  22  Tue,  5 Apr 2016 16:05:17 -0400 (EDT)\r
  23 Received: by fifthhorseman.net (Postfix, from userid 1000)\r
  24  id 75104200DD; Tue,  5 Apr 2016 17:05:09 -0300 (BRT)\r
  25 From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>\r
  26 To: Tomi Ollila <tomi.ollila@iki.fi>, Notmuch Mail <notmuch@notmuchmail.org>\r
  27 Subject: Re: [PATCH v2 3/7] fix thread breakage via ghost-on-removal\r
  28 In-Reply-To: <m2lh4sczrl.fsf@guru.guru-group.fi>\r
  29 References: <1459445693-3900-1-git-send-email-dkg@fifthhorseman.net>\r
  30  <1459606541-23889-1-git-send-email-dkg@fifthhorseman.net>\r
  31  <1459606541-23889-3-git-send-email-dkg@fifthhorseman.net>\r
  32  <m2lh4sczrl.fsf@guru.guru-group.fi>\r
  33 User-Agent: Notmuch/0.21+124~gbf604e9 (http://notmuchmail.org) Emacs/24.5.1\r
  34  (x86_64-pc-linux-gnu)\r
  35 Date: Tue, 05 Apr 2016 17:05:06 -0300\r
  36 Message-ID: <874mbfvn2l.fsf@alice.fifthhorseman.net>\r
  37 MIME-Version: 1.0\r
  38 Content-Type: multipart/signed; boundary="=-=-=";\r
  39  micalg=pgp-sha512; protocol="application/pgp-signature"\r
  40 X-BeenThere: notmuch@notmuchmail.org\r
  41 X-Mailman-Version: 2.1.20\r
  42 Precedence: list\r
  43 List-Id: "Use and development of the notmuch mail system."\r
  44  <notmuch.notmuchmail.org>\r
  45 List-Unsubscribe: <https://notmuchmail.org/mailman/options/notmuch>,\r
  46  <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  47 List-Archive: <http://notmuchmail.org/pipermail/notmuch/>\r
  48 List-Post: <mailto:notmuch@notmuchmail.org>\r
  49 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  50 List-Subscribe: <https://notmuchmail.org/mailman/listinfo/notmuch>,\r
  51  <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  52 X-List-Received-Date: Tue, 05 Apr 2016 20:05:31 -0000\r
  53 \r
  54 --=-=-=\r
  55 Content-Type: text/plain\r
  56 \r
  57 Hi Tomi--\r
  58 \r
  59 On Tue 2016-04-05 03:53:34 -0300, Tomi Ollila wrote:\r
  60 > I fetched your changes from lair.fifthhorseman.net yesterday and diffed\r
  61 > against master; looks pretty good, some quick comments (this email has\r
  62 > most relevant changes related to those changes):\r
  63 \r
  64 thanks for the review!\r
  65 \r
  66 >> -/* Delete a message document from the database. */\r
  67 >> +/* Delete a message document from the database, leaving a ghost\r
  68 >> + * message in its place */\r
  69 >\r
  70 > This comment could tell the condition when ghost message is left --\r
  71 > versus the case all ghost messages are dropped (thread becomes empty of\r
  72 > mail messages).\r
  73 \r
  74 right, i can improve these comments.\r
  75 \r
  76 > In next lines the expected case NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND\r
  77 > could be first. Although the performance difference is negligible to me\r
  78 > it looks silly do this first check and basically always fail there and\r
  79 > then do 'else if' which is likely to succeeed...\r
  80 > (your latest version in lair does not return in this first case but sets\r
  81 > status to NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID. Perhaps later messages in this\r
  82 > thread does the same but those are somewhat out-of-context for this reply).\r
  83 \r
  84 sure, i can do the positive check first :)\r
  85 \r
  86 >> +    if (private_status == NOTMUCH_PRIVATE_STATUS_SUCCESS) {\r
  87 >> +    /* this is deeply weird, and we should not have gotten into\r
  88 >> +       this state.  is there a better error message to return\r
  89 >> +       here? */\r
  90 >> +    return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;\r
  91 >> +    } else if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {\r
  92 >> +    private_status = _notmuch_message_initialize_ghost (ghost, tid);\r
  93 >> +    if (! private_status)\r
  94 >> +        _notmuch_message_sync (ghost);\r
  95 >> +    }\r
  96 >> +    notmuch_message_destroy (ghost);\r
  97 >> +    return COERCE_STATUS (private_status, "Error converting to ghost message");\r
  98 >>  }\r
  99 >\r
 100 > Outside of this patch, but in some of the next messages, adds functions\r
 101 > _notmuch_message_has_term() and _notmuch_message_has_term_st(). Perhaps\r
 102 > the _notmuch_message_has_term() could be left unimplemented?\r
 103 \r
 104 yeah, i can do that, though i have to say it's programmatically\r
 105 convenient to have a simple boolean test that defaults to some value if\r
 106 there was an error.\r
 107 \r
 108 I'll post one more round of patches to the mailing list to address these\r
 109 cleanup bits in the next day or two.\r
 110 \r
 111 I'm happy to read more reviews if anyone has them,\r
 112 \r
 113       --dkg\r
 114 \r
 115 --=-=-=\r
 116 Content-Type: application/pgp-signature; name="signature.asc"\r
 117 \r
 118 -----BEGIN PGP SIGNATURE-----\r
 119 Version: GnuPG v2\r
 120 \r
 121 iQJ8BAEBCgBmBQJXBBpyXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w\r
 122 ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRFREIyRTc0RjU2RkNGMkI2NzI5N0I3MzUy\r
 123 NEVDRkY1QUZGNjgzNzBBAAoJECTs/1r/aDcKna8QAJP3qDgHG/NXoLnrPZfnTTII\r
 124 pPvNxW+EfeV5QT8z2LOzOCV3qAhty7cg/ga0AyJsB2RDcSa4biWiGzh3MbvJz/ZJ\r
 125 uML2vuW/qOKENkTbfOYNgDUVK9+7tOEjLw85Py1U6EsHGeUAT0aazA9trdR6naC/\r
 126 I0udxxaNMKqE8JLC1aWNw629AcwkKklvqP1bX4/q2DASdVMnOJwAPZTwkOxDRkpP\r
 127 gMkOrBZA+MSkzd55OWFbDCstlIF27PmUx/6dqBaYHp4l+Qk3ikbw1hAquyPNgK5v\r
 128 68XtR8yabu8ULktTbYR0xin3NuPv/osYEH01PHWzFn/6B7BlimlFhd4JqmYR5ZGT\r
 129 QHTLLFI8tb8wPK7VTj0gt7WnnpARDTBpGOFbV7UW6asi18IiEdfUrdVe58W9wpCG\r
 130 72o6aa3aYPEB13Jbz2MHMo6UrdVvB6aaYIaPaK1ngAbnXeatGko/v2zQq9Gi4142\r
 131 4yHqcDYZmh1xE3yl6mI/zb38l1ywYfh7k4lBjbtggoIzcXk0FuBDqZjVltvNlKjC\r
 132 J33E7QAxEhP9tB9boVjUbhHrfqNY4nzMpF6FESkjQoFbZqpucBVDexqwkjWJkxBu\r
 133 ovsmsH4hfdfdAVc0pbKF8SlCiF3ptG7vqsTuJz1fRRaNEQ4tNB3tdXPD+FeTZAzL\r
 134 9gTwM9LAtdqO1YtRDW4R\r
 135 =nBqb\r
 136 -----END PGP SIGNATURE-----\r
 137 --=-=-=--\r