--- /dev/null
+Return-Path: <amdragon@mit.edu>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 41171431FD0\r
+ for <notmuch@notmuchmail.org>; Fri, 10 Jun 2011 14:11:15 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+ tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id LLGF2Iysr5n8 for <notmuch@notmuchmail.org>;\r
+ Fri, 10 Jun 2011 14:11:13 -0700 (PDT)\r
+Received: from dmz-mailsec-scanner-7.mit.edu (DMZ-MAILSEC-SCANNER-7.MIT.EDU\r
+ [18.7.68.36])\r
+ by olra.theworths.org (Postfix) with ESMTP id 5ED71431FB6\r
+ for <notmuch@notmuchmail.org>; Fri, 10 Jun 2011 14:11:13 -0700 (PDT)\r
+X-AuditID: 12074424-b7bc6ae000005a77-23-4df288700099\r
+Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
+ by dmz-mailsec-scanner-7.mit.edu (Symantec Messaging Gateway) with SMTP\r
+ id C6.6F.23159.07882FD4; Fri, 10 Jun 2011 17:11:12 -0400 (EDT)\r
+Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
+ by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id p5ALBClq014402; \r
+ Fri, 10 Jun 2011 17:11:12 -0400\r
+Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
+ (authenticated bits=0)\r
+ (User authenticated as amdragon@ATHENA.MIT.EDU)\r
+ by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id p5ALBARn021129\r
+ (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
+ Fri, 10 Jun 2011 17:11:11 -0400 (EDT)\r
+Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.72)\r
+ (envelope-from <amdragon@mit.edu>)\r
+ id 1QV8yt-0005nv-Ji; Fri, 10 Jun 2011 17:11:03 -0400\r
+Date: Fri, 10 Jun 2011 17:11:03 -0400\r
+From: Austin Clements <amdragon@MIT.EDU>\r
+To: Carl Worth <cworth@cworth.org>\r
+Subject: Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues\r
+Message-ID: <20110610211103.GC16025@mit.edu>\r
+References: <1298015940-31986-1-git-send-email-amdragon@mit.edu>\r
+ <BANLkTikOVqK7p1G2SmgJoac+n8p19_XsdQ@mail.gmail.com>\r
+ <BANLkTiktPcf9Y4fF8nP1+66GpDP6eGY2Vw@mail.gmail.com>\r
+ <BANLkTimcVvQjSF7E65gO1KSY4KQSXsVQNw@mail.gmail.com>\r
+ <87ei34rnc5.fsf@yoom.home.cworth.org>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+Content-Disposition: inline\r
+In-Reply-To: <87ei34rnc5.fsf@yoom.home.cworth.org>\r
+User-Agent: Mutt/1.5.20 (2009-06-14)\r
+X-Brightmail-Tracker:\r
+ H4sIAAAAAAAAA+NgFmphleLIzCtJLcpLzFFi42IR4hTV1i3o+ORrsL3FyOLmzzlsFtdvzmR2\r
+ YPLYvfkBi8ezVbeYA5iiuGxSUnMyy1KL9O0SuDKudL5mKljgUPFtcyNzA+M3oy5GTg4JAROJ\r
+ Dz93sEPYYhIX7q1n62Lk4hAS2McosWfqaXYIZwOjxMGzK1khnJNMEhfnXmSEcJYwSpy89IsR\r
+ pJ9FQFXi4sPZrCA2m4CGxLb9y8HiIgJKEk+PrGICsZkFpCW+/W4Gs4UFbCV2f7oKtIKDg1dA\r
+ R2JiuyjEzF4miX2NvWA1vAKCEidnPmGB6NWSuPHvJRNIPcic5f84QMKcAkYSJ9qWgpWLCqhI\r
+ XNvfzjaBUWgWku5ZSLpnIXQvYGRexSibklulm5uYmVOcmqxbnJyYl5dapGuul5tZopeaUrqJ\r
+ ERTY7C4qOxibDykdYhTgYFTi4RXR+OQrxJpYVlyZe4hRkoNJSZT3fjtQiC8pP6UyI7E4I76o\r
+ NCe1+BCjBAezkgjvepAcb0piZVVqUT5MSpqDRUmcd56kuq+QQHpiSWp2ampBahFMVoaDQ0mC\r
+ 9w5Io2BRanpqRVpmTglCmomDE2Q4D9DwFrDhxQWJucWZ6RD5U4yKUuK8j0ESAiCJjNI8uF5Y\r
+ 4nnFKA70ijDvWZAqHmDSgut+BTSYCWhw26GPIINLEhFSUg2MMxj61rfk/u5kvWt106fHUMj9\r
+ GzebrrCUoVn8c7kf8ycZhLW8ftLQcnn/HrmuzJsWXOqpJ58xcMz+Gv3hyYqy1Uy6UxI+T8xy\r
+ vPEyN/y2aFgz98l7wdvOPjfzz5yi2WPb86n2oPcBL/XrMbqbd/z/XhdfZR525sD+SR+9T5tu\r
+ Oa8x3avil/o/JZbijERDLeai4kQAvT08QxcDAAA=\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Fri, 10 Jun 2011 21:11:15 -0000\r
+\r
+Quoth Carl Worth on Jun 08 at 3:05 pm:\r
+> On Sat, 28 May 2011 22:51:10 -0400, Austin Clements <amdragon@mit.edu> wrote:\r
+> > Rebased to current master (cb8418) as atomic-new-v4 (aka\r
+> > for-review/atomic-new-v4).\r
+> \r
+> Hi Austin,\r
+> \r
+> Thanks so much for sending this series (and 4 times, even!).\r
+> \r
+> I *really* like the new robustness provided by this series, and I\r
+> especially like the exhaustive testing here. Thanks so much!\r
+> \r
+> Having just gone through the for-review/atomic-new-v4 series, I have a\r
+> few comments. Some are very minor and I'll be glad to implement them\r
+> myself:\r
+> \r
+> 1. Two commits have "lose" misspelled as "loose". These are "ew:\r
+> don't loose messages on SIGINT" and "new: Wrap adding a\r
+> message in an atomic section".\r
+\r
+Ooops.\r
+\r
+> 2. The commit with summary of "lib: Make _notmuch_message_sync\r
+> capable of deleting a message." is missing the rest of its\r
+> commit message with a complete explanation. For example, this\r
+> commit message should describe that a message document is\r
+> deleted from the database (if the deleted field is set when\r
+> _sync is called). And the commit message should also mention\r
+> that this functionality is not currently used, but prepares\r
+> for a subsequent use.\r
+\r
+Fixed.\r
+\r
+> 3. While reviewing the commit "lib: Indicate if there are more\r
+> filenames after removal" the "if (status ==\r
+> NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed,\r
+> if status is any other value at this point in the code, then\r
+> the function should have returned earlier. I intend to follow\r
+> up with a commit that adds the missing early return and\r
+> removes this condition.\r
+\r
+Okay. I suspect I was just retaining the error semantics in this\r
+function (which were probably a hold-out from before the folder search\r
+patch). I've slipped a patch in at the beginning that adds the\r
+missing check and removed the condition.\r
+\r
+> 4. I really don't like that the final state of the code has two\r
+> different functions named notmuch_message_remove_filename and\r
+> _notmuch_message_remove_filename. If the semantics of these\r
+> functions are identical, then there should be only one\r
+> function. If the semantics are different, then they need to\r
+> have noticeably distinct names, (and a single underscore\r
+> doesn't count).\r
+\r
+Too much Linux kernel hacking, I suppose. I've left this alone for\r
+the moment because it's likely to change with the below discussion.\r
+\r
+(Two solutions are either to rename _notmuch_message_remove_filename\r
+something even more ridiculously long like\r
+_notmuch_message_remove_filename_no_delete, or to make\r
+notmuch_message_tags_to_maildir_flags first add the new file name and\r
+then remove the old one, so a message can't transiently have no file\r
+names and the merge the two filename removal functions into one.)\r
+\r
+> 5. The final code has a function inside of notmuch-new.c named\r
+> "remove_file", but this function isn't removing a\r
+> file---instead it's removing a message document from the\r
+> database. So it needs a more accurate name.\r
+\r
+Mm. It's now remove_filename (could be remove_message_filename?)\r
+It's *might* remove a message document from the database, but its\r
+primary purpose is to remove a filename from a message.\r
+\r
+I've pushed the easy changes as atomic-new-v5, mostly to get them in\r
+the record.\r
+\r
+> Like I said, those are all pretty minor and I would just implement all\r
+> of those and push the series myself, but for one remaining issue that is\r
+> a bit more significant.\r
+> \r
+> The last issue has to do with the addition of the\r
+> notmuch_database_find_message_by_filename and\r
+> notmuch_message_remove_filename functions. In the series as it stands,\r
+> notmuch-new.c is updated to call these two functions instead of calling\r
+> the existing notmuch_database_remove_message function (which itself also\r
+> calls the same functions).\r
+> \r
+> That sets off a red flag in my mind. If our program is avoiding a\r
+> library function and substituting its own implementation, how are other\r
+> users of the library going to get things right? Should we deprecate\r
+> notmuch_database_remove_message? Should we add more documentation to it\r
+> describing the situation in which a user might prefer not to call it? It\r
+> seems the library is harder to use than it should be in this area.\r
+\r
+The intent was to deprecate notmuch_database_remove_message, yes.\r
+\r
+> Meanwhile, I'm not very satisfied by the existence of\r
+> notmuch_message_remove_filename in the public API. It would have a\r
+> natural pairing with notmuch_message_add_filename, but the series isn't\r
+> exporting that functionality. So things feel more asymmetric than they\r
+> should be as well.\r
+\r
+Part of why atomicity was a mess was because the API blurs the\r
+distinction between a message as a concrete, single file and a message\r
+as a message-id that may have many file names.\r
+find_message_by_filename and remove_filename were attempting to\r
+sharpen this distinction.\r
+\r
+But, maybe they sharpen it in the wrong direction. An alternate way\r
+to look at this is that a message is a single file that can also tell\r
+you file names that contain equivalent messages. This might be more\r
+of a mindset (or documentation change) than an actual API change; I'm\r
+not sure. It certainly fits better with the existing\r
+{add,remove}_message, but it's not clear if that's intentional or\r
+historical. Thoughts?\r
+\r
+> Now, why is notmuch-new going through all this effort to reimplement an\r
+> existing library function (and requiring two new library functions in\r
+> the process)? What it wants to do is to wrap the functionality of\r
+> database_remove_message in freeze/thaw and while the message is frozen\r
+> call notmuch_message_maildir_flags_to_tags.\r
+> \r
+> So, how to fix my complaints above?\r
+> \r
+> * Do we want to allow database_remove_message to optionally call\r
+> maildir_flags_to_tags?\r
+> \r
+> This seems a little messy in requiring some additional information\r
+> to the library so it can know whether to do the maildir\r
+> synchronization here. And it's also asymmetric unless we would also\r
+> support similar synchronization support in the library for simlar\r
+> operations.\r
+> \r
+> * Do we want to expose notmuch_message_add_filename as well as\r
+> remove_filename for better symmetry?\r
+> \r
+> I'm not sure I like that. It still feels like we're exposing too\r
+> many internals and not making it obvious to the user how to do\r
+> things. Having just the existing add_message/remove_message\r
+> functions definitely makes the interface easier.\r
+> \r
+> * Can we fix the remove case without this new library API by simply\r
+> adding calls to begin_atomic and end_atomic?\r
+> \r
+> I think this is probably the solution I would prefer to see.\r
+> \r
+> What do you think, Austin?\r
+\r
+Of these three, I would definitely go with the last. In fact, I tried\r
+the first two when I was originally designing this patch and can\r
+assure you they'll get us into trouble. ]:--8)\r
+\r
+I recall a few reasons for why I designed this the way I did. One was\r
+what I mentioned above about sharpening the distinction between\r
+messages and filenames. Another was that I wanted to reuse the\r
+freeze/thaw mechanism; in fact, I introduced atomic sections only when\r
+I realized that using freeze/thaw was going to be impossible for add.\r
+Perhaps it makes more sense to lean the other direction. Finally, I\r
+felt that it was important that the API be easy to use correctly, from\r
+an atomicity standpoint; hence the introduction of more operations\r
+that were meaningful on frozen messages (I also tried to make it so\r
+you couldn't overlook atomicity issues when using the library, but I\r
+don't think I succeeded).\r
+\r
+That last reason is also compatible with your last suggestion. If we\r
+move to atomic sections, I think we have to make sure the library\r
+never internally violates atomicity and that the library user only\r
+needs to use atomic sections directly if they need atomicity across\r
+multiple library calls. This shouldn't be hard, especially with\r
+nested atomics.\r
+\r
+I'll give this a try and see where it leads.\r
+\r
+> -Carl\r
projects / notmuch-archives.git / commitdiff