[PATCH 0/6] compactor changes

[PATCH 0/6] compactor changes

[Jani Nikula]

David was a bit hasty with pushing compact, so I missed the
review. Instead of just whining about it, here's a few changes I'd
really like to see merged before release. Completely untested, needs man
page updates and probably test changes too, so there's a bit more to do
still. Hint, if you have the time, just pick up from here. ;)

Cheers,
Jani.


Jani Nikula (6):
  lib: construct compactor within try block to catch any exceptions
  lib: add closure parameter to compact status update callback
  lib: use the backup path provided by the user, don't add anything to
    it
  cli: return error status if compaction fails
  cli: add compact --backup=FILE option, don't backup by default
  cli: add compact --verbose option and silence output without it

 lib/database.cc   |   29 ++++++++++++++---------------
 lib/notmuch.h     |    5 +++--
 notmuch-compact.c |   46 +++++++++++++++++++++++++++-------------------
 3 files changed, 44 insertions(+), 36 deletions(-)

-- 
1.7.2.5

[PATCH 1/6] lib: construct compactor within try block to catch any exceptions

[Jani Nikula]

Constructors may also throw exceptions. Catch them.
---
 lib/database.cc |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 20e5ec2..3dfea0f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -864,7 +864,6 @@ notmuch_database_compact (const char* path,
 			  notmuch_compact_status_cb_t status_cb)
 {
     void *local = talloc_new (NULL);
-    NotmuchCompactor compactor(status_cb);
     char *notmuch_path, *xapian_path, *compact_xapian_path;
     char *old_xapian_path = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
@@ -910,6 +909,8 @@ notmuch_database_compact (const char* path,
     }
 
     try {
+	NotmuchCompactor compactor(status_cb);
+
 	compactor.set_renumber(false);
 	compactor.add_source(xapian_path);
 	compactor.set_destdir(compact_xapian_path);
-- 
1.7.2.5

[PATCH 2/6] lib: add closure parameter to compact status update callback

[Jani Nikula]

This provides much more flexibility for the caller, and can still be
done without soname bumps.
---
 lib/database.cc   |   14 +++++++++-----
 lib/notmuch.h     |    5 +++--
 notmuch-compact.c |    8 +++-----
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3dfea0f..da549b4 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -821,9 +821,11 @@ static int rmtree (const char *path)
 class NotmuchCompactor : public Xapian::Compactor
 {
     notmuch_compact_status_cb_t status_cb;
+    void *status_closure;
 
 public:
-    NotmuchCompactor(notmuch_compact_status_cb_t cb) : status_cb(cb) { }
+    NotmuchCompactor(notmuch_compact_status_cb_t cb, void *closure) :
+	status_cb(cb), status_closure(closure) { }
 
     virtual void
     set_status (const std::string &table, const std::string &status)
@@ -842,7 +844,7 @@ public:
 	    return;
 	}
 
-	status_cb(msg);
+	status_cb(msg, status_closure);
 	talloc_free(msg);
     }
 };
@@ -861,7 +863,8 @@ public:
 notmuch_status_t
 notmuch_database_compact (const char* path,
 			  const char* backup_path,
-			  notmuch_compact_status_cb_t status_cb)
+			  notmuch_compact_status_cb_t status_cb,
+			  void *closure)
 {
     void *local = talloc_new (NULL);
     char *notmuch_path, *xapian_path, *compact_xapian_path;
@@ -909,7 +912,7 @@ notmuch_database_compact (const char* path,
     }
 
     try {
-	NotmuchCompactor compactor(status_cb);
+	NotmuchCompactor compactor(status_cb, closure);
 
 	compactor.set_renumber(false);
 	compactor.add_source(xapian_path);
@@ -947,7 +950,8 @@ DONE:
 notmuch_status_t
 notmuch_database_compact (unused (const char* path),
 			  unused (const char* backup_path),
-			  unused (notmuch_compact_status_cb_t status_cb))
+			  unused (notmuch_compact_status_cb_t status_cb),
+			  unused (void *closure))
 {
     fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
     return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 9dab555..cd301a4 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -219,7 +219,7 @@ notmuch_database_close (notmuch_database_t *database);
 /* A callback invoked by notmuch_database_compact to notify the user
  * of the progress of the compaction process.
  */
-typedef void (*notmuch_compact_status_cb_t)(const char*);
+typedef void (*notmuch_compact_status_cb_t)(const char *message, void *closure);
 
 /* Compact a notmuch database, backing up the original database to the
  * given path.
@@ -231,7 +231,8 @@ typedef void (*notmuch_compact_status_cb_t)(const char*);
 notmuch_status_t
 notmuch_database_compact (const char* path,
 			  const char* backup_path,
-			  notmuch_compact_status_cb_t status_cb);
+			  notmuch_compact_status_cb_t status_cb,
+			  void *closure);
 
 /* Destroy the notmuch database, closing it if necessary and freeing
  * all associated resources.
diff --git a/notmuch-compact.c b/notmuch-compact.c
index bfda40e..ee7afcf 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -20,10 +20,8 @@
 
 #include "notmuch-client.h"
 
-void status_update_cb (const char *msg);
-
-void
-status_update_cb (const char *msg)
+static void
+status_update_cb (const char *msg, unused (void *closure))
 {
     printf("%s\n", msg);
 }
@@ -38,7 +36,7 @@ notmuch_compact_command (notmuch_config_t *config,
     notmuch_status_t ret;
 
     printf ("Compacting database...\n");
-    ret = notmuch_database_compact (path, backup_path, status_update_cb);
+    ret = notmuch_database_compact (path, backup_path, status_update_cb, NULL);
     if (ret) {
 	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
     } else {
-- 
1.7.2.5

[PATCH] lib: update documentation of callback functions for database_compact and database_upgrade.

[david]

From: David Bremner <bremner@debian.org>

Compact was missing callback documentation entirely, and upgrade did not discuss the
closure parameter.
---
This patch depends on 
     
     id:2a58adbdc1257f16579692544b4bcbadca3d3045.1383315568.git.jani@nikula.org

BTW, I didn't completely understand the remark about SONAME bumps;
since we're providing new symbols, it doesn't really matter what the
signature is?

 lib/notmuch.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index cd301a4..82fd599 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -227,6 +227,9 @@ typedef void (*notmuch_compact_status_cb_t)(const char *message, void *closure);
  * The database will be opened with NOTMUCH_DATABASE_MODE_READ_WRITE
  * during the compaction process to ensure no writes are made.
  *
+ * If the optional callback function 'status_cb' is non-NULL, it will
+ * be called with diagnostic and informational messages. The argument
+ * 'closure' is passed verbatim to any callback invoked.
  */
 notmuch_status_t
 notmuch_database_compact (const char* path,
@@ -270,7 +273,8 @@ notmuch_database_needs_upgrade (notmuch_database_t *database);
  * provide progress indication to the user. If non-NULL it will be
  * called periodically with 'progress' as a floating-point value in
  * the range of [0.0 .. 1.0] indicating the progress made so far in
- * the upgrade process.
+ * the upgrade process.  The argument 'closure' is passed verbatim to
+ * any callback invoked.
  */
 notmuch_status_t
 notmuch_database_upgrade (notmuch_database_t *database,
-- 
1.8.4.rc3

Re: [PATCH] lib: update documentation of callback functions for database_compact and database_upgrade.

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

The point was, we can do this *now* without soname bumps, because we
haven't released this yet.
 On Nov 2, 2013 1:16 AM, <david@tethera.net> wrote:

> From: David Bremner <bremner@debian.org>
>
> Compact was missing callback documentation entirely, and upgrade did not
> discuss the
> closure parameter.
> ---
> This patch depends on
>
>
> id:2a58adbdc1257f16579692544b4bcbadca3d3045.1383315568.git.jani@nikula.org
>
> BTW, I didn't completely understand the remark about SONAME bumps;
> since we're providing new symbols, it doesn't really matter what the
> signature is?
>
>  lib/notmuch.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index cd301a4..82fd599 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -227,6 +227,9 @@ typedef void (*notmuch_compact_status_cb_t)(const char
> *message, void *closure);
>   * The database will be opened with NOTMUCH_DATABASE_MODE_READ_WRITE
>   * during the compaction process to ensure no writes are made.
>   *
> + * If the optional callback function 'status_cb' is non-NULL, it will
> + * be called with diagnostic and informational messages. The argument
> + * 'closure' is passed verbatim to any callback invoked.
>   */
>  notmuch_status_t
>  notmuch_database_compact (const char* path,
> @@ -270,7 +273,8 @@ notmuch_database_needs_upgrade (notmuch_database_t
> *database);
>   * provide progress indication to the user. If non-NULL it will be
>   * called periodically with 'progress' as a floating-point value in
>   * the range of [0.0 .. 1.0] indicating the progress made so far in
> - * the upgrade process.
> + * the upgrade process.  The argument 'closure' is passed verbatim to
> + * any callback invoked.
>   */
>  notmuch_status_t
>  notmuch_database_upgrade (notmuch_database_t *database,
> --
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
>

[-- Attachment #2: Type: text/html, Size: 2703 bytes --]

Re: [PATCH] lib: update documentation of callback functions for database_compact and database_upgrade.

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> The point was, we can do this *now* without soname bumps, because we
> haven't released this yet.

OK, I think we are agreeing violently. This is a bit ephemeral for a
commit message, but not worth re-doing the patch just for that.

d

[PATCH 3/6] lib: use the backup path provided by the user, don't add anything to it

[Jani Nikula]

Adding another level is just useless cruft.
---
 lib/database.cc   |   14 ++++----------
 notmuch-compact.c |    4 ++--
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index da549b4..9c99ac6 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -868,7 +868,6 @@ notmuch_database_compact (const char* path,
 {
     void *local = talloc_new (NULL);
     char *notmuch_path, *xapian_path, *compact_xapian_path;
-    char *old_xapian_path = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
     notmuch_database_t *notmuch = NULL;
     struct stat statbuf;
@@ -894,13 +893,8 @@ notmuch_database_compact (const char* path,
     }
 
     if (backup_path != NULL) {
-	if (! (old_xapian_path = talloc_asprintf (local, "%s/xapian.old", backup_path))) {
-	    ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
-	    goto DONE;
-	}
-
-	if (stat(old_xapian_path, &statbuf) != -1) {
-	    fprintf (stderr, "Backup path already exists: %s\n", old_xapian_path);
+	if (stat(backup_path, &statbuf) != -1) {
+	    fprintf (stderr, "Backup path already exists: %s\n", backup_path);
 	    ret = NOTMUCH_STATUS_FILE_ERROR;
 	    goto DONE;
 	}
@@ -924,8 +918,8 @@ notmuch_database_compact (const char* path,
 	goto DONE;
     }
 
-    if (old_xapian_path != NULL) {
-	if (rename(xapian_path, old_xapian_path)) {
+    if (backup_path) {
+	if (rename(xapian_path, backup_path)) {
 	    fprintf (stderr, "Error moving old database out of the way\n");
 	    ret = NOTMUCH_STATUS_FILE_ERROR;
 	    goto DONE;
diff --git a/notmuch-compact.c b/notmuch-compact.c
index ee7afcf..043710f 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -42,11 +42,11 @@ notmuch_compact_command (notmuch_config_t *config,
     } else {
 	printf ("\n");
 	printf ("\n");
-	printf ("The old database has been moved to %s/xapian.old", backup_path);
+	printf ("The old database has been moved to %s", backup_path);
 	printf ("\n");
 	printf ("To delete run,\n");
 	printf ("\n");
-	printf ("    rm -R %s/xapian.old\n", backup_path);
+	printf ("    rm -R %s\n", backup_path);
 	printf ("\n");
     }
 
-- 
1.7.2.5

Re: [PATCH 3/6] lib: use the backup path provided by the user, don't add anything to it

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Adding another level is just useless cruft.
> ---
>  lib/database.cc   |   14 ++++----------
>  notmuch-compact.c |    4 ++--
>  2 files changed, 6 insertions(+), 12 deletions(-)

The first two patches are fine, modulo my comments about documentation.
I think the backup path needs to be updated in notmuch-compact.c, and
possible the test updated (after applying
id:1383350515-24320-1-git-send-email-david@tethera.net)

d

Re: [PATCH 3/6] lib: use the backup path provided by the user, don't add anything to it

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Nov 2, 2013 2:14 PM, "David Bremner" <david@tethera.net> wrote:
>
> Jani Nikula <jani@nikula.org> writes:
>
> > Adding another level is just useless cruft.
> > ---
> >  lib/database.cc   |   14 ++++----------
> >  notmuch-compact.c |    4 ++--
> >  2 files changed, 6 insertions(+), 12 deletions(-)
>
> The first two patches are fine, modulo my comments about documentation.
> I think the backup path needs to be updated in notmuch-compact.c, and
> possible the test updated (after applying
> id:1383350515-24320-1-git-send-email-david@tethera.net)
>
> d

Ah, right, I was juggling the patch order back and forth a bit, and I
missed this (as I drop the path completely in a later patch).

Jani.

[-- Attachment #2: Type: text/html, Size: 1078 bytes --]

[PATCH 4/6] cli: return error status if compaction fails

[Jani Nikula]

---
 notmuch-compact.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index 043710f..2afa725 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -39,16 +39,17 @@ notmuch_compact_command (notmuch_config_t *config,
     ret = notmuch_database_compact (path, backup_path, status_update_cb, NULL);
     if (ret) {
 	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
-    } else {
-	printf ("\n");
-	printf ("\n");
-	printf ("The old database has been moved to %s", backup_path);
-	printf ("\n");
-	printf ("To delete run,\n");
-	printf ("\n");
-	printf ("    rm -R %s\n", backup_path);
-	printf ("\n");
+	return 1;
     }
 
+    printf ("\n");
+    printf ("\n");
+    printf ("The old database has been moved to %s", backup_path);
+    printf ("\n");
+    printf ("To delete run,\n");
+    printf ("\n");
+    printf ("    rm -R %s\n", backup_path);
+    printf ("\n");
+
     return 0;
 }
-- 
1.7.2.5

[PATCH 5/6] cli: add compact --backup=FILE option, don't backup by default

[Jani Nikula]

It's the user's decision. The recommended way is to do a database dump
anyway. Clean up the relevant printfs too.
---
 notmuch-compact.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index 2afa725..ecac86a 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -27,13 +27,20 @@ status_update_cb (const char *msg, unused (void *closure))
 }
 
 int
-notmuch_compact_command (notmuch_config_t *config,
-			 unused (int argc),
-			 unused (char *argv[]))
+notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 {
     const char *path = notmuch_config_get_database_path (config);
-    const char *backup_path = path;
+    const char *backup_path = NULL;
     notmuch_status_t ret;
+    int opt_index;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_STRING, &backup_path, "backup", 0, 0 },
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
+	return 1;
 
     printf ("Compacting database...\n");
     ret = notmuch_database_compact (path, backup_path, status_update_cb, NULL);
@@ -42,14 +49,10 @@ notmuch_compact_command (notmuch_config_t *config,
 	return 1;
     }
 
-    printf ("\n");
-    printf ("\n");
-    printf ("The old database has been moved to %s", backup_path);
-    printf ("\n");
-    printf ("To delete run,\n");
-    printf ("\n");
-    printf ("    rm -R %s\n", backup_path);
-    printf ("\n");
+    printf ("Done.\n");
+
+    if (backup_path)
+	printf ("The old database has been moved to %s.\n", backup_path);
 
     return 0;
 }
-- 
1.7.2.5

[PATCH 6/6] cli: add compact --verbose option and silence output without it

[Jani Nikula]

If there's nothing surprising to say, don't say anything. Unless asked
for by the user.
---
 notmuch-compact.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index ecac86a..c0ae22e 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -32,27 +32,33 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
     const char *path = notmuch_config_get_database_path (config);
     const char *backup_path = NULL;
     notmuch_status_t ret;
+    notmuch_bool_t verbose;
     int opt_index;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &backup_path, "backup", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
     };
 
     opt_index = parse_arguments (argc, argv, options, 1);
     if (opt_index < 0)
 	return 1;
 
-    printf ("Compacting database...\n");
-    ret = notmuch_database_compact (path, backup_path, status_update_cb, NULL);
+    if (verbose)
+	printf ("Compacting database...\n");
+    ret = notmuch_database_compact (path, backup_path,
+				    verbose ? status_update_cb : NULL, NULL);
     if (ret) {
 	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
 	return 1;
     }
 
-    printf ("Done.\n");
+    if (verbose) {
+	printf ("Done.\n");
 
-    if (backup_path)
-	printf ("The old database has been moved to %s.\n", backup_path);
+	if (backup_path)
+	    printf ("The old database has been moved to %s.\n", backup_path);
+    }
 
     return 0;
 }
-- 
1.7.2.5