86/349e3dfd548db171bdcda3a40d23a43f99ef73

   1 Return-Path: <cworth@cworth.org>\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 olra.theworths.org (Postfix) with ESMTP id 8EF4D429E25\r
   6         for <notmuch@notmuchmail.org>; Wed,  8 Jun 2011 15:05:23 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 0.01\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0.01 tagged_above=-999 required=5\r
  12         tests=[T_MIME_NO_TEXT=0.01] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id BoF5IYc0PBjr for <notmuch@notmuchmail.org>;\r
  16         Wed,  8 Jun 2011 15:05:22 -0700 (PDT)\r
  17 Received: from arlo.cworth.org (arlo.cworth.org [50.43.72.2])\r
  18         by olra.theworths.org (Postfix) with ESMTP id A2015431FB6\r
  19         for <notmuch@notmuchmail.org>; Wed,  8 Jun 2011 15:05:22 -0700 (PDT)\r
  20 Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
  21         by arlo.cworth.org (Postfix) with ESMTP id 5578029A51D;\r
  22         Wed,  8 Jun 2011 15:05:21 -0700 (PDT)\r
  23 Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
  24         id 43D41254149; Wed,  8 Jun 2011 15:05:21 -0700 (PDT)\r
  25 From: Carl Worth <cworth@cworth.org>\r
  26 To: Austin Clements <amdragon@mit.edu>, notmuch@notmuchmail.org\r
  27 Subject: Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues\r
  28 In-Reply-To: <BANLkTimcVvQjSF7E65gO1KSY4KQSXsVQNw@mail.gmail.com>\r
  29 References: <1298015940-31986-1-git-send-email-amdragon@mit.edu>\r
  30         <BANLkTikOVqK7p1G2SmgJoac+n8p19_XsdQ@mail.gmail.com>\r
  31         <BANLkTiktPcf9Y4fF8nP1+66GpDP6eGY2Vw@mail.gmail.com>\r
  32         <BANLkTimcVvQjSF7E65gO1KSY4KQSXsVQNw@mail.gmail.com>\r
  33 User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1\r
  34         (i486-pc-linux-gnu)\r
  35 Date: Wed, 08 Jun 2011 15:05:14 -0700\r
  36 Message-ID: <87ei34rnc5.fsf@yoom.home.cworth.org>\r
  37 MIME-Version: 1.0\r
  38 Content-Type: multipart/signed; boundary="=-=-=";\r
  39         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  40 X-BeenThere: notmuch@notmuchmail.org\r
  41 X-Mailman-Version: 2.1.13\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: <http://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: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  51         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  52 X-List-Received-Date: Wed, 08 Jun 2011 22:05:23 -0000\r
  53 \r
  54 --=-=-=\r
  55 Content-Transfer-Encoding: quoted-printable\r
  56 \r
  57 On Sat, 28 May 2011 22:51:10 -0400, Austin Clements <amdragon@mit.edu> wrot=\r
  58 e:\r
  59 > Rebased to current master (cb8418) as atomic-new-v4 (aka\r
  60 > for-review/atomic-new-v4).\r
  61 \r
  62 Hi Austin,\r
  63 \r
  64 Thanks so much for sending this series (and 4 times, even!).\r
  65 \r
  66 I *really* like the new robustness provided by this series, and I\r
  67 especially like the exhaustive testing here. Thanks so much!\r
  68 \r
  69 Having just gone through the for-review/atomic-new-v4 series, I have a\r
  70 few comments. Some are very minor and I'll be glad to implement them\r
  71 myself:\r
  72 \r
  73         1. Two commits have "lose" misspelled as "loose". These are "ew:\r
  74            don't loose messages on SIGINT" and "new: Wrap adding a\r
  75            message in an atomic section".\r
  76 \r
  77         2. The commit with summary of "lib: Make _notmuch_message_sync\r
  78            capable of deleting a message." is missing the rest of its\r
  79            commit message with a complete explanation. For example, this\r
  80            commit message should describe that a message document is\r
  81            deleted from the database (if the deleted field is set when\r
  82            _sync is called). And the commit message should also mention\r
  83            that this functionality is not currently used, but prepares\r
  84            for a subsequent use.\r
  85 \r
  86         3. While reviewing the commit "lib: Indicate if there are more\r
  87            filenames after removal" the "if (status =3D=3D\r
  88            NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed,\r
  89            if status is any other value at this point in the code, then\r
  90            the function should have returned earlier. I intend to follow\r
  91            up with a commit that adds the missing early return and\r
  92            removes this condition.\r
  93 \r
  94         4. I really don't like that the final state of the code has two\r
  95            different functions named notmuch_message_remove_filename and\r
  96            _notmuch_message_remove_filename. If the semantics of these\r
  97            functions are identical, then there should be only one\r
  98            function. If the semantics are different, then they need to\r
  99            have noticeably distinct names, (and a single underscore\r
 100            doesn't count).\r
 101 \r
 102         5. The final code has a function inside of notmuch-new.c named\r
 103            "remove_file", but this function isn't removing a\r
 104            file---instead it's removing a message document from the\r
 105            database. So it needs a more accurate name.\r
 106 \r
 107 Like I said, those are all pretty minor and I would just implement all\r
 108 of those and push the series myself, but for one remaining issue that is\r
 109 a bit more significant.\r
 110 \r
 111 The last issue has to do with the addition of the\r
 112 notmuch_database_find_message_by_filename and\r
 113 notmuch_message_remove_filename functions. In the series as it stands,\r
 114 notmuch-new.c is updated to call these two functions instead of calling\r
 115 the existing notmuch_database_remove_message function (which itself also\r
 116 calls the same functions).\r
 117 \r
 118 That sets off a red flag in my mind. If our program is avoiding a\r
 119 library function and substituting its own implementation, how are other\r
 120 users of the library going to get things right? Should we deprecate\r
 121 notmuch_database_remove_message? Should we add more documentation to it\r
 122 describing the situation in which a user might prefer not to call it? It\r
 123 seems the library is harder to use than it should be in this area.\r
 124 \r
 125 Meanwhile, I'm not very satisfied by the existence of\r
 126 notmuch_message_remove_filename in the public API. It would have a\r
 127 natural pairing with notmuch_message_add_filename, but the series isn't\r
 128 exporting that functionality. So things feel more asymmetric than they\r
 129 should be as well.\r
 130 \r
 131 Now, why is notmuch-new going through all this effort to reimplement an\r
 132 existing library function (and requiring two new library functions in\r
 133 the process)? What it wants to do is to wrap the functionality of\r
 134 database_remove_message in freeze/thaw and while the message is frozen\r
 135 call notmuch_message_maildir_flags_to_tags.\r
 136 \r
 137 So, how to fix my complaints above?\r
 138 \r
 139   * Do we want to allow database_remove_message to optionally call\r
 140     maildir_flags_to_tags?\r
 141 \r
 142     This seems a little messy in requiring some additional information\r
 143     to the library so it can know whether to do the maildir\r
 144     synchronization here. And it's also asymmetric unless we would also\r
 145     support similar synchronization support in the library for simlar\r
 146     operations.\r
 147 \r
 148   * Do we want to expose notmuch_message_add_filename as well as\r
 149     remove_filename for better symmetry?\r
 150 \r
 151     I'm not sure I like that. It still feels like we're exposing too\r
 152     many internals and not making it obvious to the user how to do\r
 153     things. Having just the existing add_message/remove_message\r
 154     functions definitely makes the interface easier.\r
 155 \r
 156   * Can we fix the remove case without this new library API by simply\r
 157     adding calls to begin_atomic and end_atomic?\r
 158 \r
 159     I think this is probably the solution I would prefer to see.\r
 160 \r
 161 What do you think, Austin?\r
 162 \r
 163 =2DCarl\r
 164 \r
 165 =2D-=20\r
 166 carl.d.worth@intel.com\r
 167 \r
 168 --=-=-=\r
 169 Content-Type: application/pgp-signature\r
 170 \r
 171 -----BEGIN PGP SIGNATURE-----\r
 172 Version: GnuPG v1.4.11 (GNU/Linux)\r
 173 \r
 174 iEYEARECAAYFAk3v8hoACgkQ6JDdNq8qSWh7lQCfa5lAnUui+EsioKxW0vd4hF6n\r
 175 e5oAn2GiW8CXQUSLhpIcLJYuCWiwarQj\r
 176 =tG4z\r
 177 -----END PGP SIGNATURE-----\r
 178 --=-=-=--\r