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 49CDC431FBC;
\r
6 Fri, 27 Nov 2009 05:23:22 -0800 (PST)
\r
7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
\r
8 Received: from olra.theworths.org ([127.0.0.1])
\r
9 by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)
\r
10 with ESMTP id saw5K6q+Y0+H; Fri, 27 Nov 2009 05:23:20 -0800 (PST)
\r
11 Received: from cworth.org (localhost [127.0.0.1])
\r
12 by olra.theworths.org (Postfix) with ESMTP id 6ED04431FAE;
\r
13 Fri, 27 Nov 2009 05:23:20 -0800 (PST)
\r
14 From: Carl Worth <cworth@cworth.org>
\r
15 To: Chris Wilson <chris@chris-wilson.co.uk>, notmuch@notmuchmail.org
\r
16 In-Reply-To: <1258851430-28732-1-git-send-email-chris@chris-wilson.co.uk>
\r
17 References: <1258851430-28732-1-git-send-email-chris@chris-wilson.co.uk>
\r
18 Date: Fri, 27 Nov 2009 05:23:06 -0800
\r
19 Message-ID: <87einkqeyt.fsf@yoom.home.cworth.org>
\r
21 Content-Type: text/plain; charset=us-ascii
\r
22 Subject: Re: [notmuch] [PATCH] notmuch-new: Eliminate tallocs whilst
\r
23 construct filenames.
\r
24 X-BeenThere: notmuch@notmuchmail.org
\r
25 X-Mailman-Version: 2.1.12
\r
27 List-Id: "Use and development of the notmuch mail system."
\r
28 <notmuch.notmuchmail.org>
\r
29 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
\r
30 <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
\r
31 List-Archive: <http://notmuchmail.org/pipermail/notmuch>
\r
32 List-Post: <mailto:notmuch@notmuchmail.org>
\r
33 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
\r
34 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
\r
35 <mailto:notmuch-request@notmuchmail.org?subject=subscribe>
\r
36 X-List-Received-Date: Fri, 27 Nov 2009 13:23:22 -0000
\r
38 On Sun, 22 Nov 2009 00:57:10 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
\r
39 > The majority of filenames will fit within PATH_MAX [4096] (because
\r
40 > that's a hard limit imposed by the filesystems) so we can avoid an
\r
41 > allocation per lookup and thereby eliminate a large proportion of the
\r
42 > overhead of scanning a maildir.
\r
46 I *know* I composed a reply to this message earlier, but apparently
\r
47 you're right that it never went out. (*sigh*---if only I had a reliable
\r
50 [*] I tried and tried to figure out how to get gnus to save an Fcc (a
\r
51 file copy of all outgoing messages), and failed to configure the
\r
52 various "fake newsgroup things" that gnus wanted for me to be able
\r
53 to do this. I also failed to get message-mode to do an Fcc. I
\r
54 settled instead for a Bcc to myself on all messages, deciding that
\r
55 it would be nice to see that mail actually went *out* and not just
\r
56 that I thought I sent it. Maybe that failed here, I don't know.
\r
58 The other piece I want is for unsent mail drafts to automatically
\r
59 be saved, and for notmuch to prompt me to continue with a draft if
\r
60 I start composing a new message while a draft is
\r
61 around. Currently, I can save a draft while composing in
\r
62 message-mode with "C-x C-s" but the big bug here is that the
\r
63 drafts never get deleted when I send the message, so I can't tell
\r
64 unfinished drafts apart from completed-and-sent messages.
\r
66 Anyway, on to the promised review of the patch:
\r
68 The basic idea of this I really like, of course. Thanks for helping to
\r
69 improve the efficiency of notmuch. But this part I don't:
\r
74 > - if (S_ISREG (st->st_mode)) {
\r
75 > - /* If the file hasn't been modified since the last
\r
76 > - * add_files, then we need not look at it. */
\r
77 > - if (path_dbtime == 0 || st->st_mtime > path_dbtime) {
\r
78 > - state->processed_files++;
\r
80 > - status = notmuch_database_add_message (notmuch, next, &message);
\r
81 > - switch (status) {
\r
85 It's true that in a former life, one of your primary jobs was to clean
\r
86 up after me, especially cleaning up things like memory leaks on error
\r
87 paths. But in my new talloc-enabled world, I'm quite happy to keep
\r
88 cleaner, easier-to-understand code for the price of just having a
\r
89 talloced object live slightly longer on an error path.
\r
91 We do still have to be careful that we don't let such object accumulate
\r
92 without bounds in some error case, and that they don't lock up important
\r
93 resources. But when it's just a matter of a dozen bytes or so, talloced
\r
94 into the parent's context which is going to be freed in the error path
\r
95 above, then I'm much happier to allow for this transient "leak" rather
\r
96 than convoluting the code with more cleanup gotos.
\r
98 One might argue that the error-cleanup goto is a common and
\r
99 well-understood idiom, so that it's not bad to have. The problem I have
\r
100 with it is how much work it is to verify. If I'm reading one line of
\r
101 code in the middle of a function that's testing for an error case and
\r
102 handling it with goto, then I need to:
\r
104 1. Verify this condition, and that a return value variable gets
\r
107 2. Check down at the end of the function to ensure the correct
\r
108 objects are freed and the correct return value is returned.
\r
110 3. Check back up at the beginning of the function to ensure the
\r
111 relevant objects are initialized to NULL.
\r
113 And beyond verification, one has to code in these three places
\r
114 simultaneously as well.
\r
116 Meanwhile, by taking advantage of talloc like I did in the original
\r
117 version of this code, an error return becomes a much more local decision
\r
118 and is much simpler to code.
\r
120 Chris, I'd be interested to hear any thoughts you have on this pattern.
\r
projects / notmuch-archives.git / blob