From: Jani Nikula <jani@nikula.org>
To: Ben Gamari <bgamari.foss@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] Add notmuch_database_close_compact
In-Reply-To: <1350487737-32058-2-git-send-email-bgamari.foss@gmail.com>
References: <1350487737-32058-1-git-send-email-bgamari.foss@gmail.com>
	<1350487737-32058-2-git-send-email-bgamari.foss@gmail.com>
User-Agent: Notmuch/0.14+46~g272a1f1 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Wed, 17 Oct 2012 21:56:37 +0300
Message-ID: <874nlt2ahm.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi Ben -

I'd like some meaningful commit messages here. Please find other
comments inline.

BR,
Jani.

On Wed, 17 Oct 2012, Ben Gamari <bgamari.foss@gmail.com> wrote:
> ---
>  configure       |   21 ++++++++++++++++++++-
>  lib/database.cc |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/notmuch.h   |   14 ++++++++++++++
>  3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index acb90a8..6551b13 100755
> --- a/configure
> +++ b/configure
> @@ -270,7 +270,8 @@ printf "Checking for Xapian development files... "
>  have_xapian=0
>  for xapian_config in ${XAPIAN_CONFIG}; do
>      if ${xapian_config} --version > /dev/null 2>&1; then
> -	printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')
> +	xapian_version=$(${xapian_config} --version | sed -e 's/.* //')
> +	printf "Yes (%s).\n" ${xapian_version}
>  	have_xapian=1
>  	xapian_cxxflags=$(${xapian_config} --cxxflags)
>  	xapian_ldflags=$(${xapian_config} --libs)
> @@ -282,6 +283,20 @@ if [ ${have_xapian} = "0" ]; then
>      errors=$((errors + 1))
>  fi
>  
> +have_xapian_compact=0
> +if [ ${have_xapian} = "1" ]; then
> +    printf "Checking for Xapian compact support... "
> +    case "${xapian_version}" in
> +        0.*|1.[01].*|1.2.[0-5])
> +            printf "No.\n" ;;
> +        [1-9]*.[0-9]*.[0-9]*) 
> +            have_xapian_compact=1
> +            printf "Yes.\n" ;;
> +        *)
> +            printf "Unknown version.\n" ;;
> +    esac
> +fi
> +
>  printf "Checking for GMime development files... "
>  have_gmime=0
>  IFS=';'
> @@ -679,6 +694,9 @@ LINKER_RESOLVES_LIBRARY_DEPENDENCIES = ${linker_resolves_library_dependencies}
>  XAPIAN_CXXFLAGS = ${xapian_cxxflags}
>  XAPIAN_LDFLAGS = ${xapian_ldflags}
>  
> +# Whether compact is supported by this version of Xapian
> +HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
> +
>  # Flags needed to compile and link against GMime-2.4
>  GMIME_CFLAGS = ${gmime_cflags}
>  GMIME_LDFLAGS = ${gmime_ldflags}
> @@ -715,6 +733,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
>  CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
>  		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
>  		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
> +                     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)       \\
>                       -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)
>  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
>  EOF
> diff --git a/lib/database.cc b/lib/database.cc
> index 761dc1a..6e83a61 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -781,6 +781,60 @@ notmuch_database_close (notmuch_database_t *notmuch)
>  }
>  
>  void
> +notmuch_database_close_compact (notmuch_database_t *notmuch)

It is clear that there are error conditions, so this should be of type
notmuch_status_t. Even if you don't know what to do with the errors now,
you'll find that changing the API is a PITA later.

> +{
> +    void *local = talloc_new (NULL);
> +    Xapian::Compactor compactor;
> +    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian_path;
> +
> +#if HAVE_XAPIAN_COMPACT
> +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", notmuch->path, ".notmuch"))) {
> +	fprintf (stderr, "Out of memory\n");
> +	goto DONE;
> +    }

You could include notmuch->path and .notmuch above into the asprintf
below.

A personal preference, I think this style would be much more readable:

    xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian");
    if (!xapian_path) {
        ...
    }

> +
> +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
> +	fprintf (stderr, "Out of memory\n");
> +	goto DONE;
> +    }
> +
> +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
> +	fprintf (stderr, "Out of memory\n");
> +	goto DONE;
> +    }
> +
> +    if (! (old_xapian_path = talloc_asprintf (local, "%s.old", xapian_path))) {
> +	fprintf (stderr, "Out of memory\n");
> +	goto DONE;
> +    }
> +
> +    try {
> +	compactor.set_renumber(false);
> +	compactor.add_source(xapian_path);
> +	compactor.set_destdir(compact_xapian_path);
> +	compactor.compact();
> +
> +	if (rename(xapian_path, old_xapian_path)) {
> +	    fprintf (stderr, "Error moving old database out of the way\n");
> +	    goto DONE;
> +	}
> +
> +	if (rename(compact_xapian_path, xapian_path)) {
> +	    fprintf (stderr, "Error moving compacted database\n");
> +	    goto DONE;
> +	}

Please add strerror(errno) into the two error prints above. The user
would probably like to know why the errors occurred.

> +    } catch (Xapian::InvalidArgumentError e) {

Should this be catch (const Xapian::Error &error) ?

> +	fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());
> +    }
> +    
> +#endif
> +
> +    notmuch_database_close(notmuch);

The database gets closed on Xapian errors, but not on talloc or rename
errors, and the caller has no way of knowing. See the return status
above, but also read on...

> +DONE:
> +    talloc_free(local);
> +}
> +
> +void
>  notmuch_database_destroy (notmuch_database_t *notmuch)
>  {
>      notmuch_database_close (notmuch);
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 3633bed..50babfb 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -215,6 +215,20 @@ notmuch_database_open (const char *path,
>  void
>  notmuch_database_close (notmuch_database_t *database);
>  
> +/* Close the given notmuch database and then compact it.

It's the other way round, isn't it?

But do we want the call to do two things, one of which we already have a
call for (closing the database)? That's not orthogonal. Maybe closing
the database after compaction is the right thing to do (is it?) but even
so, could we leave that responsibility to the API user? The caller does
have to open the database too, and calls to open, compact, close seem
good.

> + *
> + * After notmuch_database_close_compact has been called, calls to
> + * other functions on objects derived from this database may either
> + * behave as if the database had not been closed (e.g., if the
> + * required data has been cached) or may fail with a
> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
> + *
> + * notmuch_database_close_compact can be called multiple times.  Later
> + * calls have no effect.
> + */
> +void
> +notmuch_database_close_compact (notmuch_database_t *notmuch);
> +
>  /* Destroy the notmuch database, closing it if necessary and freeing
>  * all associated resources. */
>  void
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch