[notmuch] [PATCH] notmuch-new: Eliminate tallocs whilst construct filenames.

[Carl Worth]

On Sun, 22 Nov 2009 00:57:10 +0000, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> The majority of filenames will fit within PATH_MAX [4096] (because
> that's a hard limit imposed by the filesystems) so we can avoid an
> allocation per lookup and thereby eliminate a large proportion of the
> overhead of scanning a maildir.

Hi Chris,

I *know* I composed a reply to this message earlier, but apparently
you're right that it never went out. (*sigh*---if only I had a reliable
mail client[*]).

  [*] I tried and tried to figure out how to get gnus to save an Fcc (a
      file copy of all outgoing messages), and failed to configure the
      various "fake newsgroup things" that gnus wanted for me to be able
      to do this. I also failed to get message-mode to do an Fcc. I
      settled instead for a Bcc to myself on all messages, deciding that
      it would be nice to see that mail actually went *out* and not just
      that I thought I sent it. Maybe that failed here, I don't know.

      The other piece I want is for unsent mail drafts to automatically
      be saved, and for notmuch to prompt me to continue with a draft if
      I start composing a new message while a draft is
      around. Currently, I can save a draft while composing in
      message-mode with "C-x C-s" but the big bug here is that the
      drafts never get deleted when I send the message, so I can't tell
      unfinished drafts apart from completed-and-sent messages.

Anyway, on to the promised review of the patch:

The basic idea of this I really like, of course. Thanks for helping to
improve the efficiency of notmuch. But this part I don't:

> -	    continue;
> -	}
> -
> -	if (S_ISREG (st->st_mode)) {
> -	    /* If the file hasn't been modified since the last
> -	     * add_files, then we need not look at it. */
> -	    if (path_dbtime == 0 || st->st_mtime > path_dbtime) {
> -		state->processed_files++;
> -
> -		status = notmuch_database_add_message (notmuch, next, &message);
> -		switch (status) {
> -		    /* success */
> +	} else {

It's true that in a former life, one of your primary jobs was to clean
up after me, especially cleaning up things like memory leaks on error
paths. But in my new talloc-enabled world, I'm quite happy to keep
cleaner, easier-to-understand code for the price of just having a
talloced object live slightly longer on an error path.

We do still have to be careful that we don't let such object accumulate
without bounds in some error case, and that they don't lock up important
resources. But when it's just a matter of a dozen bytes or so, talloced
into the parent's context which is going to be freed in the error path
above, then I'm much happier to allow for this transient "leak" rather
than convoluting the code with more cleanup gotos.

One might argue that the error-cleanup goto is a common and
well-understood idiom, so that it's not bad to have. The problem I have
with it is how much work it is to verify. If I'm reading one line of
code in the middle of a function that's testing for an error case and
handling it with goto, then I need to:

	1. Verify this condition, and that a return value variable gets
	   set.

	2. Check down at the end of the function to ensure the correct
	   objects are freed and the correct return value is returned.

	3. Check back up at the beginning of the function to ensure the
	   relevant objects are initialized to NULL.

And beyond verification, one has to code in these three places
simultaneously as well.

Meanwhile, by taking advantage of talloc like I did in the original
version of this code, an error return becomes a much more local decision
and is much simpler to code.

Chris, I'd be interested to hear any thoughts you have on this pattern.

-Carl