MIME-Version: 1.0
Sender: amdragon@gmail.com
In-Reply-To: <BANLkTimeWnn5YtKjarFB8f8Fowvz0PxK2Q@mail.gmail.com>
References: <87ei34rnc5.fsf@yoom.home.cworth.org>
	<1307822683-848-1-git-send-email-amdragon@mit.edu>
	<BANLkTimeWnn5YtKjarFB8f8Fowvz0PxK2Q@mail.gmail.com>
Date: Wed, 29 Jun 2011 02:37:00 -0400
Message-ID:
 <CAH-f9Wt9ZH0rMsaCK1bMfsccTGhYvcBBAAH=dSL5ps+KhyHtwA@mail.gmail.com>
Subject: Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
From: Austin Clements <amdragon@mit.edu>
To: Carl Worth <cworth@cworth.org>
Content-Type: text/plain; charset=ISO-8859-1
Cc: notmuch@notmuchmail.org
Precedence: list

Just found one more atomicity bug in notmuch-new.  I would prefer to
not update this patch series yet again, but rather to follow up with a
separate fix for this.  The full bug is described below, but the gist
is that how add_files_recursive computes new_directory from the mtime
isn't sound.  The simplest fix is to remove the new_directory
optimization, but then we wouldn't scan files in inode order.  The
best fix is probably to add an out-argument to
notmuch_database_get_directory that indicates if the directory really
was just created in the database (and hence has no files or subdirs).
Unfortunately, that requires an API change.  If that's undesirable, an
alternate would be to record that bit in the notmuch_directory_t and
add some notmuch_directory_is_new API that returns it.  Thoughts?


Bug description:

add_files_recursive determines whether a directory is "new" by
inspecting the database mtime returned by notmuch_directory_get_mtime.
 However, if there's an interruption after adding
messages/subdirectories from a new directory but before updating the
directory's mtime, it will be considered a new directory again on the
next run.  As a result, on the next run, db_files and db_subdirs will
not be loaded from the database (even though they *wouldn't* be NULL).
 As a result, we'll miss removing messages or entire subdirectories
that have been deleted from the "new" directory.  (We'll also re-add
the messages in this directory to the database rather than skipping
them, which will throw off the notmuch new statistics, but that's
fine.)

This didn't show up in the atomicity test because throwing off
anything besides the statistics requires *two* interruptions.  In
fact, I don't even know how I would write an automated test for this.