On Mon, Sep 22, 2014 at 06:50:50PM +0000, Austin Clements wrote:
> Quoth W. Trevor King on Sep 22 at  9:59 am:
> > On Mon, Sep 22, 2014 at 11:43:35AM -0400, Austin Clements wrote:
> > > This patch simplifies notmuch_database_close to just call
> > > Database::close.  This works for both read-only and read/write
> > > databases, takes care of committing changes, unifies the
> > > exception handling path, and codifies aborting outstanding
> > > transactions.
> > 
> > If we're dropping the flush call here, where will it be triggered
> > instead?  We'll need to flush/commit our changes to the database
> > at some point before closing.  Do clients now need an explicit
> > flush/commit command (explicit, client-initiated flushes sound
> > like a good idea to me).
> 
> The call to Database::close implicitly flushes/commits, as mentioned
> in the comment in the patch, so there's no need for any new APIs or
> client changes.  The call to Database::flush in
> notmuch_database_close was entirely redundant with the call to
> Database::close.

Ah, I thought the implicit flush/commit was just in our code.  Since
it's also in the underlying Xapian close, then this patch looks pretty
good to me.  I'd mention Xapian's explicit close in the notmuch.h
message.  Xapain's docs say [1]:

  For a WritableDatabase, if a transaction is active it will be
  aborted, while if no transaction is active commit() will be
  implicitly called.

Cheers,
Trevor

[1]: http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#a59f5f8b137723dcaaabdbdccbc0cf1eb

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy