Date: Sun, 11 Sep 2011 20:23:27 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Martin Owens <doctormo@gmail.com>
Subject: Re: Patch: Flush and Reopen
Message-ID: <20110912002327.GC23603@mit.edu>
References: <1315445674.32058.183.camel@delen>
	<CAH-f9WtuYYuksfTHL1ZMmoRF6kyL7iFu9BXDzGn-wWGniSvf9w@mail.gmail.com>
	<1315536861.2435.34.camel@delen> <87obyuj7q1.fsf@zancas.localnet>
	<1315590950.2435.59.camel@delen>
	<CAH-f9WtuL48RtaJ9njxHyCy5cqhhrxdz+30ydE11rq41Lu06pA@mail.gmail.com>
	<1315615433.2435.62.camel@delen>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1315615433.2435.62.camel@delen>
User-Agent: Mutt/1.5.20 (2009-06-14)
Cc: Notmuch developer list <notmuch@notmuchmail.org>
Precedence: list

BTW, in the future, you should send patches inline (see the patch
formatting guide I linked to earlier for easy ways to do this).  It
makes them much easier to review and reply to.

Quoth Martin Owens on Sep 09 at  8:43 pm:
> On Fri, 2011-09-09 at 19:40 -0400, Austin Clements wrote:
> > 
> > (indented correctly, of course).  Reopen is a method of
> > Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
> > flush, which is a member of the subclass WritableDatabase and hence
> > requires the cast).  And reopening is a sensible thing to do on both
> > read-only and read/write databases.
> 
> See attached for the removal of the cast and conditional for reopen.

Great.  That looks like how I would expect a reopen function to work.
I assume you've tested it?

Speaking of testing, I'm not sure what the best way to test this is,
but it needs something.  notmuch's testing system is great for testing
the CLI, but it's poorly suited to lower-level library things.

> > Also, in keeping with notmuch code style, you should use tabs for
> > indentation and put a space before the open paren argument lists. 
> 
> I couldn't detect a consistant style for the tab/spacing and it confused
> me greatly. Especially coming from python where spacing is critical.
> 
> I've put in tabs, not sure if I've put in enough in the form required.

Notmuch is quite consistent about indentation, though the rule may not
be something you'd expect coming from Python.  It uses four space
indentation, but each group of eight spaces is replaced with a tab, so
for example

notmuch_database_reopen (notmuch_database_t *notmuch)
{
____try {
->      notmuch->xapian_db->reopen ();
____} catch (const Xapian::Error &error) {
->      if (! notmuch->exception_reported) {
->      ____fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
->      ->      _____error.get_msg().c_str());
->      }
____}
}

Yeah, it's weird; blame RMS.  If you're using Emacs, the
.dir-locals.el should set this up automatically for you.  (If you're
not using Emacs, the easiest way might be to pop it up in Emacs,
select the code to indent, and do M-x indent-region.)

> diff --git a/debian/changelog b/debian/changelog
> index c2b7552..565edfb 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,8 +1,12 @@
>  notmuch (0.8~rc1-1) experimental; urgency=low
>  
> +  [ David Bremner ]
>    * Upstream release candidate.
>  
> - -- David Bremner <bremner@debian.org>  Tue, 06 Sep 2011 22:24:24 -0300
> +  [ Martin Owens (DoctorMO) ]
> +  * Re-new-world-order
> +
> + -- Martin Owens (DoctorMO) <doctormo@gmail.com>  Wed, 07 Sep 2011 18:19:50 -0400
>  
>  notmuch (0.7-1) unstable; urgency=low
>  
> diff --git a/debian/control b/debian/control
> index 03afdf4..a72fe1b 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -5,7 +5,7 @@ Maintainer: Carl Worth <cworth@debian.org>
>  Uploaders: Jameson Graef Rollins <jrollins@finestructure.net>, martin f. krafft <madduck@debian.org>, 
>            David Bremner <bremner@debian.org>
>  Build-Depends: debhelper (>= 7.0.50~), pkg-config, libxapian-dev, 
> - libgmime-2.4-dev, libtalloc-dev, libz-dev,  python-all (>= 2.6.6-3~),
> + libgmime-2.4-dev, libtalloc-dev, libz-dev,  python-all (>= 2.6.6),
>   emacs23-nox | emacs23 (>=23~) | emacs23-lucid (>=23~)
>  Standards-Version: 3.9.2
>  Homepage: http://notmuchmail.org/

Even if you found the above two changes necessary, they shouldn't be
included in this patch because they aren't related to reopen/flush.

> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -700,18 +700,36 @@ notmuch_database_open (const char *path,
>  }
>  
>  void
> -notmuch_database_close (notmuch_database_t *notmuch)
> +notmuch_database_reopen (notmuch_database_t *notmuch)
> +{
> +       try {
> +               notmuch->xapian_db->reopen ();
> +       } catch (const Xapian::Error &error) {
> +               if (! notmuch->exception_reported) {
> +               fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
> +               error.get_msg().c_str());
> +               }
> +       }
> +}
> +
> +void
> +notmuch_database_flush (notmuch_database_t *notmuch)
>  {
>      try {
>         if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
> -           (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
> +               (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
>      } catch (const Xapian::Error &error) {
> -       if (! notmuch->exception_reported) {
> -           fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
> -                    error.get_msg().c_str());
> +               if (! notmuch->exception_reported) {
> +               fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
> +               error.get_msg().c_str());
> +               }
>         }
> -    }
> +}

Both catch blocks should set notmuch->exception_reported = TRUE;
(notmuch_database_close happens to be the one exception to this since
it's closing the database; see other catch blocks in lib/database.cc).

The catch block in notmuch_database_flush shouldn't appear as changed
in the diff; you should leave the spacing the way it originally was.