From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 8EF4D429E25 for ; Wed, 8 Jun 2011 15:05:23 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: 0.01 X-Spam-Level: X-Spam-Status: No, score=0.01 tagged_above=-999 required=5 tests=[T_MIME_NO_TEXT=0.01] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id BoF5IYc0PBjr for ; Wed, 8 Jun 2011 15:05:22 -0700 (PDT) Received: from arlo.cworth.org (arlo.cworth.org [50.43.72.2]) by olra.theworths.org (Postfix) with ESMTP id A2015431FB6 for ; Wed, 8 Jun 2011 15:05:22 -0700 (PDT) Received: from yoom.home.cworth.org (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id 5578029A51D; Wed, 8 Jun 2011 15:05:21 -0700 (PDT) Received: by yoom.home.cworth.org (Postfix, from userid 1000) id 43D41254149; Wed, 8 Jun 2011 15:05:21 -0700 (PDT) From: Carl Worth To: Austin Clements , notmuch@notmuchmail.org Subject: Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues In-Reply-To: References: <1298015940-31986-1-git-send-email-amdragon@mit.edu> User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1 (i486-pc-linux-gnu) Date: Wed, 08 Jun 2011 15:05:14 -0700 Message-ID: <87ei34rnc5.fsf@yoom.home.cworth.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 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: Wed, 08 Jun 2011 22:05:23 -0000 --=-=-= Content-Transfer-Encoding: quoted-printable On Sat, 28 May 2011 22:51:10 -0400, Austin Clements wrot= e: > Rebased to current master (cb8418) as atomic-new-v4 (aka > for-review/atomic-new-v4). Hi Austin, Thanks so much for sending this series (and 4 times, even!). I *really* like the new robustness provided by this series, and I especially like the exhaustive testing here. Thanks so much! Having just gone through the for-review/atomic-new-v4 series, I have a few comments. Some are very minor and I'll be glad to implement them myself: 1. Two commits have "lose" misspelled as "loose". These are "ew: don't loose messages on SIGINT" and "new: Wrap adding a message in an atomic section". 2. The commit with summary of "lib: Make _notmuch_message_sync capable of deleting a message." is missing the rest of its commit message with a complete explanation. For example, this commit message should describe that a message document is deleted from the database (if the deleted field is set when _sync is called). And the commit message should also mention that this functionality is not currently used, but prepares for a subsequent use. 3. While reviewing the commit "lib: Indicate if there are more filenames after removal" the "if (status =3D=3D NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed, if status is any other value at this point in the code, then the function should have returned earlier. I intend to follow up with a commit that adds the missing early return and removes this condition. 4. I really don't like that the final state of the code has two different functions named notmuch_message_remove_filename and _notmuch_message_remove_filename. If the semantics of these functions are identical, then there should be only one function. If the semantics are different, then they need to have noticeably distinct names, (and a single underscore doesn't count). 5. The final code has a function inside of notmuch-new.c named "remove_file", but this function isn't removing a file---instead it's removing a message document from the database. So it needs a more accurate name. Like I said, those are all pretty minor and I would just implement all of those and push the series myself, but for one remaining issue that is a bit more significant. The last issue has to do with the addition of the notmuch_database_find_message_by_filename and notmuch_message_remove_filename functions. In the series as it stands, notmuch-new.c is updated to call these two functions instead of calling the existing notmuch_database_remove_message function (which itself also calls the same functions). That sets off a red flag in my mind. If our program is avoiding a library function and substituting its own implementation, how are other users of the library going to get things right? Should we deprecate notmuch_database_remove_message? Should we add more documentation to it describing the situation in which a user might prefer not to call it? It seems the library is harder to use than it should be in this area. Meanwhile, I'm not very satisfied by the existence of notmuch_message_remove_filename in the public API. It would have a natural pairing with notmuch_message_add_filename, but the series isn't exporting that functionality. So things feel more asymmetric than they should be as well. Now, why is notmuch-new going through all this effort to reimplement an existing library function (and requiring two new library functions in the process)? What it wants to do is to wrap the functionality of database_remove_message in freeze/thaw and while the message is frozen call notmuch_message_maildir_flags_to_tags. So, how to fix my complaints above? * Do we want to allow database_remove_message to optionally call maildir_flags_to_tags? This seems a little messy in requiring some additional information to the library so it can know whether to do the maildir synchronization here. And it's also asymmetric unless we would also support similar synchronization support in the library for simlar operations. * Do we want to expose notmuch_message_add_filename as well as remove_filename for better symmetry? I'm not sure I like that. It still feels like we're exposing too many internals and not making it obvious to the user how to do things. Having just the existing add_message/remove_message functions definitely makes the interface easier. * Can we fix the remove case without this new library API by simply adding calls to begin_atomic and end_atomic? I think this is probably the solution I would prefer to see. What do you think, Austin? =2DCarl =2D-=20 carl.d.worth@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEARECAAYFAk3v8hoACgkQ6JDdNq8qSWh7lQCfa5lAnUui+EsioKxW0vd4hF6n e5oAn2GiW8CXQUSLhpIcLJYuCWiwarQj =tG4z -----END PGP SIGNATURE----- --=-=-=--