1 Return-Path: <amdragon@mit.edu>
\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 41171431FD0
\r
6 for <notmuch@notmuchmail.org>; Fri, 10 Jun 2011 14:11:15 -0700 (PDT)
\r
7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
\r
11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5
\r
12 tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 LLGF2Iysr5n8 for <notmuch@notmuchmail.org>;
\r
16 Fri, 10 Jun 2011 14:11:13 -0700 (PDT)
\r
17 Received: from dmz-mailsec-scanner-7.mit.edu (DMZ-MAILSEC-SCANNER-7.MIT.EDU
\r
19 by olra.theworths.org (Postfix) with ESMTP id 5ED71431FB6
\r
20 for <notmuch@notmuchmail.org>; Fri, 10 Jun 2011 14:11:13 -0700 (PDT)
\r
21 X-AuditID: 12074424-b7bc6ae000005a77-23-4df288700099
\r
22 Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])
\r
23 by dmz-mailsec-scanner-7.mit.edu (Symantec Messaging Gateway) with SMTP
\r
24 id C6.6F.23159.07882FD4; Fri, 10 Jun 2011 17:11:12 -0400 (EDT)
\r
25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])
\r
26 by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id p5ALBClq014402;
\r
27 Fri, 10 Jun 2011 17:11:12 -0400
\r
28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])
\r
29 (authenticated bits=0)
\r
30 (User authenticated as amdragon@ATHENA.MIT.EDU)
\r
31 by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id p5ALBARn021129
\r
32 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);
\r
33 Fri, 10 Jun 2011 17:11:11 -0400 (EDT)
\r
34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.72)
\r
35 (envelope-from <amdragon@mit.edu>)
\r
36 id 1QV8yt-0005nv-Ji; Fri, 10 Jun 2011 17:11:03 -0400
\r
37 Date: Fri, 10 Jun 2011 17:11:03 -0400
\r
38 From: Austin Clements <amdragon@MIT.EDU>
\r
39 To: Carl Worth <cworth@cworth.org>
\r
40 Subject: Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues
\r
41 Message-ID: <20110610211103.GC16025@mit.edu>
\r
42 References: <1298015940-31986-1-git-send-email-amdragon@mit.edu>
\r
43 <BANLkTikOVqK7p1G2SmgJoac+n8p19_XsdQ@mail.gmail.com>
\r
44 <BANLkTiktPcf9Y4fF8nP1+66GpDP6eGY2Vw@mail.gmail.com>
\r
45 <BANLkTimcVvQjSF7E65gO1KSY4KQSXsVQNw@mail.gmail.com>
\r
46 <87ei34rnc5.fsf@yoom.home.cworth.org>
\r
48 Content-Type: text/plain; charset=us-ascii
\r
49 Content-Disposition: inline
\r
50 In-Reply-To: <87ei34rnc5.fsf@yoom.home.cworth.org>
\r
51 User-Agent: Mutt/1.5.20 (2009-06-14)
\r
52 X-Brightmail-Tracker:
\r
53 H4sIAAAAAAAAA+NgFmphleLIzCtJLcpLzFFi42IR4hTV1i3o+ORrsL3FyOLmzzlsFtdvzmR2
\r
54 YPLYvfkBi8ezVbeYA5iiuGxSUnMyy1KL9O0SuDKudL5mKljgUPFtcyNzA+M3oy5GTg4JAROJ
\r
55 Dz93sEPYYhIX7q1n62Lk4hAS2McosWfqaXYIZwOjxMGzK1khnJNMEhfnXmSEcJYwSpy89IsR
\r
56 pJ9FQFXi4sPZrCA2m4CGxLb9y8HiIgJKEk+PrGICsZkFpCW+/W4Gs4UFbCV2f7oKtIKDg1dA
\r
57 R2JiuyjEzF4miX2NvWA1vAKCEidnPmGB6NWSuPHvJRNIPcic5f84QMKcAkYSJ9qWgpWLCqhI
\r
58 XNvfzjaBUWgWku5ZSLpnIXQvYGRexSibklulm5uYmVOcmqxbnJyYl5dapGuul5tZopeaUrqJ
\r
59 ERTY7C4qOxibDykdYhTgYFTi4RXR+OQrxJpYVlyZe4hRkoNJSZT3fjtQiC8pP6UyI7E4I76o
\r
60 NCe1+BCjBAezkgjvepAcb0piZVVqUT5MSpqDRUmcd56kuq+QQHpiSWp2ampBahFMVoaDQ0mC
\r
61 9w5Io2BRanpqRVpmTglCmomDE2Q4D9DwFrDhxQWJucWZ6RD5U4yKUuK8j0ESAiCJjNI8uF5Y
\r
62 4nnFKA70ijDvWZAqHmDSgut+BTSYCWhw26GPIINLEhFSUg2MMxj61rfk/u5kvWt106fHUMj9
\r
63 GzebrrCUoVn8c7kf8ycZhLW8ftLQcnn/HrmuzJsWXOqpJ58xcMz+Gv3hyYqy1Uy6UxI+T8xy
\r
64 vPEyN/y2aFgz98l7wdvOPjfzz5yi2WPb86n2oPcBL/XrMbqbd/z/XhdfZR525sD+SR+9T5tu
\r
65 Oa8x3avil/o/JZbijERDLeai4kQAvT08QxcDAAA=
\r
66 Cc: notmuch@notmuchmail.org
\r
67 X-BeenThere: notmuch@notmuchmail.org
\r
68 X-Mailman-Version: 2.1.13
\r
70 List-Id: "Use and development of the notmuch mail system."
\r
71 <notmuch.notmuchmail.org>
\r
72 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
\r
73 <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
\r
74 List-Archive: <http://notmuchmail.org/pipermail/notmuch>
\r
75 List-Post: <mailto:notmuch@notmuchmail.org>
\r
76 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
\r
77 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
\r
78 <mailto:notmuch-request@notmuchmail.org?subject=subscribe>
\r
79 X-List-Received-Date: Fri, 10 Jun 2011 21:11:15 -0000
\r
81 Quoth Carl Worth on Jun 08 at 3:05 pm:
\r
82 > On Sat, 28 May 2011 22:51:10 -0400, Austin Clements <amdragon@mit.edu> wrote:
\r
83 > > Rebased to current master (cb8418) as atomic-new-v4 (aka
\r
84 > > for-review/atomic-new-v4).
\r
88 > Thanks so much for sending this series (and 4 times, even!).
\r
90 > I *really* like the new robustness provided by this series, and I
\r
91 > especially like the exhaustive testing here. Thanks so much!
\r
93 > Having just gone through the for-review/atomic-new-v4 series, I have a
\r
94 > few comments. Some are very minor and I'll be glad to implement them
\r
97 > 1. Two commits have "lose" misspelled as "loose". These are "ew:
\r
98 > don't loose messages on SIGINT" and "new: Wrap adding a
\r
99 > message in an atomic section".
\r
103 > 2. The commit with summary of "lib: Make _notmuch_message_sync
\r
104 > capable of deleting a message." is missing the rest of its
\r
105 > commit message with a complete explanation. For example, this
\r
106 > commit message should describe that a message document is
\r
107 > deleted from the database (if the deleted field is set when
\r
108 > _sync is called). And the commit message should also mention
\r
109 > that this functionality is not currently used, but prepares
\r
110 > for a subsequent use.
\r
114 > 3. While reviewing the commit "lib: Indicate if there are more
\r
115 > filenames after removal" the "if (status ==
\r
116 > NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed,
\r
117 > if status is any other value at this point in the code, then
\r
118 > the function should have returned earlier. I intend to follow
\r
119 > up with a commit that adds the missing early return and
\r
120 > removes this condition.
\r
122 Okay. I suspect I was just retaining the error semantics in this
\r
123 function (which were probably a hold-out from before the folder search
\r
124 patch). I've slipped a patch in at the beginning that adds the
\r
125 missing check and removed the condition.
\r
127 > 4. I really don't like that the final state of the code has two
\r
128 > different functions named notmuch_message_remove_filename and
\r
129 > _notmuch_message_remove_filename. If the semantics of these
\r
130 > functions are identical, then there should be only one
\r
131 > function. If the semantics are different, then they need to
\r
132 > have noticeably distinct names, (and a single underscore
\r
135 Too much Linux kernel hacking, I suppose. I've left this alone for
\r
136 the moment because it's likely to change with the below discussion.
\r
138 (Two solutions are either to rename _notmuch_message_remove_filename
\r
139 something even more ridiculously long like
\r
140 _notmuch_message_remove_filename_no_delete, or to make
\r
141 notmuch_message_tags_to_maildir_flags first add the new file name and
\r
142 then remove the old one, so a message can't transiently have no file
\r
143 names and the merge the two filename removal functions into one.)
\r
145 > 5. The final code has a function inside of notmuch-new.c named
\r
146 > "remove_file", but this function isn't removing a
\r
147 > file---instead it's removing a message document from the
\r
148 > database. So it needs a more accurate name.
\r
150 Mm. It's now remove_filename (could be remove_message_filename?)
\r
151 It's *might* remove a message document from the database, but its
\r
152 primary purpose is to remove a filename from a message.
\r
154 I've pushed the easy changes as atomic-new-v5, mostly to get them in
\r
157 > Like I said, those are all pretty minor and I would just implement all
\r
158 > of those and push the series myself, but for one remaining issue that is
\r
159 > a bit more significant.
\r
161 > The last issue has to do with the addition of the
\r
162 > notmuch_database_find_message_by_filename and
\r
163 > notmuch_message_remove_filename functions. In the series as it stands,
\r
164 > notmuch-new.c is updated to call these two functions instead of calling
\r
165 > the existing notmuch_database_remove_message function (which itself also
\r
166 > calls the same functions).
\r
168 > That sets off a red flag in my mind. If our program is avoiding a
\r
169 > library function and substituting its own implementation, how are other
\r
170 > users of the library going to get things right? Should we deprecate
\r
171 > notmuch_database_remove_message? Should we add more documentation to it
\r
172 > describing the situation in which a user might prefer not to call it? It
\r
173 > seems the library is harder to use than it should be in this area.
\r
175 The intent was to deprecate notmuch_database_remove_message, yes.
\r
177 > Meanwhile, I'm not very satisfied by the existence of
\r
178 > notmuch_message_remove_filename in the public API. It would have a
\r
179 > natural pairing with notmuch_message_add_filename, but the series isn't
\r
180 > exporting that functionality. So things feel more asymmetric than they
\r
181 > should be as well.
\r
183 Part of why atomicity was a mess was because the API blurs the
\r
184 distinction between a message as a concrete, single file and a message
\r
185 as a message-id that may have many file names.
\r
186 find_message_by_filename and remove_filename were attempting to
\r
187 sharpen this distinction.
\r
189 But, maybe they sharpen it in the wrong direction. An alternate way
\r
190 to look at this is that a message is a single file that can also tell
\r
191 you file names that contain equivalent messages. This might be more
\r
192 of a mindset (or documentation change) than an actual API change; I'm
\r
193 not sure. It certainly fits better with the existing
\r
194 {add,remove}_message, but it's not clear if that's intentional or
\r
195 historical. Thoughts?
\r
197 > Now, why is notmuch-new going through all this effort to reimplement an
\r
198 > existing library function (and requiring two new library functions in
\r
199 > the process)? What it wants to do is to wrap the functionality of
\r
200 > database_remove_message in freeze/thaw and while the message is frozen
\r
201 > call notmuch_message_maildir_flags_to_tags.
\r
203 > So, how to fix my complaints above?
\r
205 > * Do we want to allow database_remove_message to optionally call
\r
206 > maildir_flags_to_tags?
\r
208 > This seems a little messy in requiring some additional information
\r
209 > to the library so it can know whether to do the maildir
\r
210 > synchronization here. And it's also asymmetric unless we would also
\r
211 > support similar synchronization support in the library for simlar
\r
214 > * Do we want to expose notmuch_message_add_filename as well as
\r
215 > remove_filename for better symmetry?
\r
217 > I'm not sure I like that. It still feels like we're exposing too
\r
218 > many internals and not making it obvious to the user how to do
\r
219 > things. Having just the existing add_message/remove_message
\r
220 > functions definitely makes the interface easier.
\r
222 > * Can we fix the remove case without this new library API by simply
\r
223 > adding calls to begin_atomic and end_atomic?
\r
225 > I think this is probably the solution I would prefer to see.
\r
227 > What do you think, Austin?
\r
229 Of these three, I would definitely go with the last. In fact, I tried
\r
230 the first two when I was originally designing this patch and can
\r
231 assure you they'll get us into trouble. ]:--8)
\r
233 I recall a few reasons for why I designed this the way I did. One was
\r
234 what I mentioned above about sharpening the distinction between
\r
235 messages and filenames. Another was that I wanted to reuse the
\r
236 freeze/thaw mechanism; in fact, I introduced atomic sections only when
\r
237 I realized that using freeze/thaw was going to be impossible for add.
\r
238 Perhaps it makes more sense to lean the other direction. Finally, I
\r
239 felt that it was important that the API be easy to use correctly, from
\r
240 an atomicity standpoint; hence the introduction of more operations
\r
241 that were meaningful on frozen messages (I also tried to make it so
\r
242 you couldn't overlook atomicity issues when using the library, but I
\r
243 don't think I succeeded).
\r
245 That last reason is also compatible with your last suggestion. If we
\r
246 move to atomic sections, I think we have to make sure the library
\r
247 never internally violates atomicity and that the library user only
\r
248 needs to use atomic sections directly if they need atomicity across
\r
249 multiple library calls. This shouldn't be hard, especially with
\r
252 I'll give this a try and see where it leads.
\r
projects / notmuch-archives.git / blob