[PATCH v2 00/11] compactor changes v2

[PATCH v2 00/11] compactor changes v2

[Jani Nikula]

Hi all, this is v2 of [1], incorporating compact related patches from
David, some changes from David's review, some new patches, man page
update, test update.

Cheers,
Jani.


[1] id:cover.1383315568.git.jani@nikula.org


David Bremner (2):
  test: fix compact backup / restore test
  lib: update documentation of callback functions for database_compact
    and database_upgrade.

Jani Nikula (9):
  lib: construct compactor within try block to catch any exceptions
  lib: check talloc success in compact
  lib: do not leak the database in compaction
  lib: add closure parameter to compact status update callback
  lib: use the compaction backup path provided by the caller
  cli: return error status if compaction fails
  cli: add compact --backup=DIRECTORY option, don't backup by default
  cli: add compact --verbose option and silence output without it
  man: document notmuch compact --verbose and --backup=DIRECTORY options

 lib/database.cc            | 41 +++++++++++++++++++++++------------------
 lib/notmuch.h              | 11 ++++++++---
 man/man1/notmuch-compact.1 | 28 +++++++++++++++++++++++++++-
 notmuch-compact.c          | 46 +++++++++++++++++++++++++++-------------------
 test/compact               |  9 +++++----
 5 files changed, 90 insertions(+), 45 deletions(-)

-- 
1.8.4.rc3

[PATCH v2 01/11] test: fix compact backup / restore test

[Jani Nikula]

From: David Bremner <david@tethera.net>

It was looking in completely the wrong place for the backup and the
(test) xapian database. Unfortunately test_begin_subtest hides the
relevant errors.
---
 test/compact | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/test/compact b/test/compact
index 5bb5cea..afab537 100755
--- a/test/compact
+++ b/test/compact
@@ -19,10 +19,11 @@ thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (inbox tag1 unread)
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 tag2 unread)
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Three (inbox tag3 unread)"
 
-test_begin_subtest "Restoring backup"
-rm -Rf ${TEST_TMPDIR}/mail/xapian
-mv ${TEST_TMPDIR}/mail/xapian.old ${TEST_TMPDIR}/mail/xapian
+test_expect_success 'Restoring Backup' \
+    'rm -Rf ${MAIL_DIR}/.notmuch/xapian &&
+     mv ${MAIL_DIR}/xapian.old ${MAIL_DIR}/.notmuch/xapian'
 
+test_begin_subtest "Checking restored backup"
 output=$(notmuch search \* | notmuch_search_sanitize)
 test_expect_equal "$output" "\
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (inbox tag1 unread)
-- 
1.8.4.rc3

[PATCH v2 02/11] lib: construct compactor within try block to catch any exceptions

[Jani Nikula]

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

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.8.4.rc3

[PATCH v2 03/11] lib: check talloc success in compact

[Jani Nikula]

In line with the allocation checks all around.
---
 lib/database.cc | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3dfea0f..7a8702e 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -863,13 +863,17 @@ notmuch_database_compact (const char* path,
 			  const char* backup_path,
 			  notmuch_compact_status_cb_t status_cb)
 {
-    void *local = talloc_new (NULL);
+    void *local;
     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;
 
+    local = talloc_new (NULL);
+    if (! local)
+	return NOTMUCH_STATUS_OUT_OF_MEMORY;
+
     ret = notmuch_database_open(path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
     if (ret) {
 	goto DONE;
-- 
1.8.4.rc3

[PATCH v2 04/11] lib: do not leak the database in compaction

[Jani Nikula]

Destroy instead of close the database after compaction, and also on
error path, to not leak the database.
---
 lib/database.cc | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 7a8702e..eadf8a7 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -941,10 +941,12 @@ notmuch_database_compact (const char* path,
 	goto DONE;
     }
 
-    notmuch_database_close(notmuch);
-
 DONE:
+    if (notmuch)
+	notmuch_database_destroy (notmuch);
+
     talloc_free(local);
+
     return ret;
 }
 #else
-- 
1.8.4.rc3

[PATCH v2 05/11] lib: add closure parameter to compact status update callback

[Jani Nikula]

This provides much more flexibility for the caller.
---
 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 eadf8a7..5a01703 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;
     char *notmuch_path, *xapian_path, *compact_xapian_path;
@@ -913,7 +916,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);
@@ -953,7 +956,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.8.4.rc3

[PATCH v2 06/11] lib: update documentation of callback functions for database_compact and database_upgrade.

[Jani Nikula]

From: David Bremner <bremner@debian.org>

Compact was missing callback documentation entirely, and upgrade did not discuss the
closure parameter.
---
 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

[PATCH v2 07/11] lib: use the compaction backup path provided by the caller

[Jani Nikula]

The extra path component added by the lib is a magic value that the
caller just has to know. This is demonstrated by the current code,
which indeed has "xapian.old" both sides of the interface. Use the
backup path provided by the lib caller verbatim, without adding
anything to it.

---

v2: add xapian.old in cli
---
 lib/database.cc   | 14 ++++----------
 notmuch-compact.c | 10 +++++++---
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 5a01703..a021bf1 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -868,7 +868,6 @@ notmuch_database_compact (const char* path,
 {
     void *local;
     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;
@@ -898,13 +897,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;
 	}
@@ -928,8 +922,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..55dc731 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -32,9 +32,13 @@ notmuch_compact_command (notmuch_config_t *config,
 			 unused (char *argv[]))
 {
     const char *path = notmuch_config_get_database_path (config);
-    const char *backup_path = path;
+    const char *backup_path;
     notmuch_status_t ret;
 
+    backup_path = talloc_asprintf (config, "%s/xapian.old", path);
+    if (! backup_path)
+	return 1;
+
     printf ("Compacting database...\n");
     ret = notmuch_database_compact (path, backup_path, status_update_cb, NULL);
     if (ret) {
@@ -42,11 +46,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.8.4.rc3

[PATCH v2 08/11] cli: return error status if compaction fails

[Jani Nikula]

As is customary for any tool.
---
 notmuch-compact.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index 55dc731..b9461c2 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -43,16 +43,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.8.4.rc3

[PATCH v2 09/11] cli: add compact --backup=DIRECTORY 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.

---

v2: reorder prints
---
 notmuch-compact.c | 27 +++++++++++++--------------
 test/compact      |  4 ++--
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index b9461c2..359acfc 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -27,16 +27,19 @@ 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;
+    const char *backup_path = NULL;
     notmuch_status_t ret;
+    int opt_index;
 
-    backup_path = talloc_asprintf (config, "%s/xapian.old", path);
-    if (! backup_path)
+    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");
@@ -46,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");
+    if (backup_path)
+	printf ("The old database has been moved to %s.\n", backup_path);
+
+    printf ("Done.\n");
 
     return 0;
 }
diff --git a/test/compact b/test/compact
index afab537..ac174ce 100755
--- a/test/compact
+++ b/test/compact
@@ -10,7 +10,7 @@ notmuch tag +tag1 \*
 notmuch tag +tag2 subject:Two
 notmuch tag -tag1 +tag3 subject:Three
 
-test_expect_success "Running compact" "notmuch compact"
+test_expect_success "Running compact" "notmuch compact --backup=${TEST_DIRECTORY}/xapian.old"
 
 test_begin_subtest "Compact preserves database"
 output=$(notmuch search \* | notmuch_search_sanitize)
@@ -21,7 +21,7 @@ thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Three (inbox tag3 unread)"
 
 test_expect_success 'Restoring Backup' \
     'rm -Rf ${MAIL_DIR}/.notmuch/xapian &&
-     mv ${MAIL_DIR}/xapian.old ${MAIL_DIR}/.notmuch/xapian'
+     mv ${TEST_DIRECTORY}/xapian.old ${MAIL_DIR}/.notmuch/xapian'
 
 test_begin_subtest "Checking restored backup"
 output=$(notmuch search \* | notmuch_search_sanitize)
-- 
1.8.4.rc3

[PATCH v2 10/11] 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 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index 359acfc..1e7808c 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;
     }
 
-    if (backup_path)
-	printf ("The old database has been moved to %s.\n", backup_path);
+    if (verbose) {
+	if (backup_path)
+	    printf ("The old database has been moved to %s.\n", backup_path);
 
-    printf ("Done.\n");
+	printf ("Done.\n");
+    }
 
     return 0;
 }
-- 
1.8.4.rc3

[PATCH v2 11/11] man: document notmuch compact --verbose and --backup=DIRECTORY options

[Jani Nikula]

---
 man/man1/notmuch-compact.1 | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/man/man1/notmuch-compact.1 b/man/man1/notmuch-compact.1
index 1aeed22..dfffccb 100644
--- a/man/man1/notmuch-compact.1
+++ b/man/man1/notmuch-compact.1
@@ -4,6 +4,8 @@ notmuch-compact \- compact the notmuch database
 .SH SYNOPSIS
 
 .B notmuch compact
+.RI "[ --verbose ]"
+.RI "[ --backup=<" directory "> ]"
 
 .SH DESCRIPTION
 
@@ -14,11 +16,35 @@ the space required by the database and improve lookup performance.
 
 The compacted database is built in a temporary directory and is later
 moved into the place of the origin database. The original uncompacted
-database is preserved to be deleted by the user as desired.
+database is discarded, unless the
+.BR "\-\-backup=" <directory>
+option is used.
 
 Note that the database write lock will be held during the compaction
 process (which may be quite long) to protect data integrity.
 
+Supported options for
+.B compact
+include
+
+.RS 4
+.TP 4
+.BR "\-\-backup=" <directory>
+
+Save the current database to the given directory before replacing it
+with the compacted database. The backup directory must not exist and
+it must reside on the same mounted filesystem as the current database.
+
+.RE
+
+.RS 4
+.TP 4
+.BR \-\-verbose
+
+Report database compaction progress to stdout.
+
+.RE
+
 .RE
 .SH ENVIRONMENT
 The following environment variables can be used to control the
-- 
1.8.4.rc3

Re: [PATCH v2 07/11] lib: use the compaction backup path provided by the caller

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> +    backup_path = talloc_asprintf (config, "%s/xapian.old", path);
> +    if (! backup_path)
> +	return 1;
> +

Is it intentional that no message is printed here? A random glance at
notmuch-search.c suggests we usually print a message on OOM in the CLI.

d

Re: [PATCH v2 00/11] compactor changes v2

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Hi all, this is v2 of [1], incorporating compact related patches from
> David, some changes from David's review, some new patches, man page
> update, test update.
>
> Cheers,
> Jani.

Aside from the one quibble that I already sent seperately, the first 8
patches look OK to me. I have no technical objections to the last 3, but
since the change the UI, I'd like to wait a bit for more input from
other people.

Re: [PATCH v2 07/11] lib: use the compaction backup path provided by the caller

[Jani Nikula]

On Tue, 05 Nov 2013, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> +    backup_path = talloc_asprintf (config, "%s/xapian.old", path);
>> +    if (! backup_path)
>> +	return 1;
>> +
>
> Is it intentional that no message is printed here? A random glance at
> notmuch-search.c suggests we usually print a message on OOM in the CLI.

Oversight.

Jani.

Re: [PATCH v2 00/11] compactor changes v2

[Ben Gamari]

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

Jani Nikula <jani@nikula.org> writes:

> Hi all, this is v2 of [1], incorporating compact related patches from
> David, some changes from David's review, some new patches, man page
> update, test update.
>
Alas, more silly oversights on my part. Thanks for catching these!

No objections from my side. Given that I've never seen a compact fail, I
think switching off backups by default seems fine. Personally I'll
likely always run with --verbose as even with an SSD compact can take
quite a while and there's nothing like watching a teapot to pass the
day. Regardless, I have no objection to the flag's existence.

Cheers,

- Ben


[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [PATCH v2 00/11] compactor changes v2

[Tomi Ollila]

On Tue, Nov 05 2013, David Bremner <david@tethera.net> wrote:

> Jani Nikula <jani@nikula.org> writes:
>
>> Hi all, this is v2 of [1], incorporating compact related patches from
>> David, some changes from David's review, some new patches, man page
>> update, test update.
>>
>> Cheers,
>> Jani.
>
> Aside from the one quibble that I already sent seperately, the first 8
> patches look OK to me. I have no technical objections to the last 3, but
> since the change the UI, I'd like to wait a bit for more input from
> other people.


The whole series looks OK to me. The only thing I'm, a bit worried about
what happens is I Ctrl-C compaction at any state. Is it possible that I
end up with corrupted database ? Someone may accidentally execute
notmuch compact -- for example by picking the command from history.

If it is possible what is the likelihood that happening (also comparing
w/ notmuch tag, notmuch new & notmuch insert managing to corrupt database)

I personally would like to be sure. OTOH if others don't see issue
there (perhaps with a good reason) I'm fine with the patches as those
are now.


Tomi

Re: [PATCH v2 00/11] compactor changes v2

[Ben Gamari]

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

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Tue, Nov 05 2013, David Bremner <david@tethera.net> wrote:
>
>> Jani Nikula <jani@nikula.org> writes:
>>
>>> Hi all, this is v2 of [1], incorporating compact related patches from
>>> David, some changes from David's review, some new patches, man page
>>> update, test update.
>>>
>>> Cheers,
>>> Jani.
>>
>> Aside from the one quibble that I already sent seperately, the first 8
>> patches look OK to me. I have no technical objections to the last 3, but
>> since the change the UI, I'd like to wait a bit for more input from
>> other people.
>
>
> The whole series looks OK to me. The only thing I'm, a bit worried about
> what happens is I Ctrl-C compaction at any state. Is it possible that I
> end up with corrupted database ? Someone may accidentally execute
> notmuch compact -- for example by picking the command from history.
>
You should in principle never end up in a situation where the original
database is corrupted. Xapian's Compactor interface reads from the
existing database and writes out the compacted database to a new
directory. The notmuch compact command keeps the original database in
place until the last possible moment.

When the compacted database is completely constructed it, deletes or
moves the old database out of the way and then moves the new database
into place. The worst thing that could happen is that you manage to
interrupt the process between deleting the old database and moving the
new one into place (a very small window). In this case you'd simply need
to move the new database into place manually (although you'd need to
figure out where the compacted database is located).
  
Cheers,

- Ben


[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [PATCH v2 00/11] compactor changes v2

[Tomi Ollila]

On Tue, Nov 05 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>>
>> The whole series looks OK to me. The only thing I'm, a bit worried about
>> what happens is I Ctrl-C compaction at any state. Is it possible that I
>> end up with corrupted database ? Someone may accidentally execute
>> notmuch compact -- for example by picking the command from history.
>>
> You should in principle never end up in a situation where the original
> database is corrupted. Xapian's Compactor interface reads from the
> existing database and writes out the compacted database to a new
> directory. The notmuch compact command keeps the original database in
> place until the last possible moment.
>
> When the compacted database is completely constructed it, deletes or
> moves the old database out of the way and then moves the new database
> into place. The worst thing that could happen is that you manage to
> interrupt the process between deleting the old database and moving the
> new one into place (a very small window). In this case you'd simply need
> to move the new database into place manually (although you'd need to
> figure out where the compacted database is located).

Ah true. I failed to keep the whole picture in mind. Indeed the compactor
first compacts from .notmuch/xapian to .notmuch/xapian.compact (and
finally directory renaming is done in database.cc... (and with directories
we cannot do atomic file update using link()/rename() calls). 
(If we wanted to handle this "small window" we could try use 'xapian.compact'
directory in case 'xapian' was nonexistent but... :)

That understood I'm ok with these pathes (when that oversight fixed).

>   
> Cheers,
>
> - Ben

Tomi

[PATCH 1/2] cli: add compact --quiet option and silence output with it

[Jani Nikula]

Provide a way to silence the output.

---

v2: --verbose -> --quiet
---
 notmuch-compact.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/notmuch-compact.c b/notmuch-compact.c
index 359acfc..8022dfe 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 quiet;
     int opt_index;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &backup_path, "backup", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 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 (! quiet)
+	printf ("Compacting database...\n");
+    ret = notmuch_database_compact (path, backup_path,
+				    quiet ? NULL : status_update_cb, NULL);
     if (ret) {
 	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
 	return 1;
     }
 
-    if (backup_path)
-	printf ("The old database has been moved to %s.\n", backup_path);
+    if (! quiet) {
+	if (backup_path)
+	    printf ("The old database has been moved to %s.\n", backup_path);
 
-    printf ("Done.\n");
+	printf ("Done.\n");
+    }
 
     return 0;
 }
-- 
1.8.4.rc3

[PATCH 2/2] man: document notmuch compact --quiet and --backup=DIRECTORY options

[Jani Nikula]

---

v2: --verbose -> --quiet
---
 man/man1/notmuch-compact.1 | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/man/man1/notmuch-compact.1 b/man/man1/notmuch-compact.1
index 1aeed22..0c95873 100644
--- a/man/man1/notmuch-compact.1
+++ b/man/man1/notmuch-compact.1
@@ -4,6 +4,8 @@ notmuch-compact \- compact the notmuch database
 .SH SYNOPSIS
 
 .B notmuch compact
+.RI "[ --quiet ]"
+.RI "[ --backup=<" directory "> ]"
 
 .SH DESCRIPTION
 
@@ -14,11 +16,35 @@ the space required by the database and improve lookup performance.
 
 The compacted database is built in a temporary directory and is later
 moved into the place of the origin database. The original uncompacted
-database is preserved to be deleted by the user as desired.
+database is discarded, unless the
+.BR "\-\-backup=" <directory>
+option is used.
 
 Note that the database write lock will be held during the compaction
 process (which may be quite long) to protect data integrity.
 
+Supported options for
+.B compact
+include
+
+.RS 4
+.TP 4
+.BR "\-\-backup=" <directory>
+
+Save the current database to the given directory before replacing it
+with the compacted database. The backup directory must not exist and
+it must reside on the same mounted filesystem as the current database.
+
+.RE
+
+.RS 4
+.TP 4
+.BR \-\-quiet
+
+Do not report database compaction progress to stdout.
+
+.RE
+
 .RE
 .SH ENVIRONMENT
 The following environment variables can be used to control the
-- 
1.8.4.rc3

Re: [PATCH v2 03/11] lib: check talloc success in compact

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> In line with the allocation checks all around.

Pushed the first three. 

d

Re: [PATCH v2 09/11] cli: add compact --backup=DIRECTORY option, don't backup by default

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> It's the user's decision. The recommended way is to do a database dump
> anyway. Clean up the relevant printfs too.
>

Pushed up to here in the series.

d

Re: [PATCH 2/2] man: document notmuch compact --quiet and --backup=DIRECTORY options

[Tomi Ollila]

On Wed, Nov 06 2013, Jani Nikula <jani@nikula.org> wrote:

> ---
>
> v2: --verbose -> --quiet

Patches

id:1383755936-11862-1-git-send-email-jani@nikula.org &
id:1383755936-11862-2-git-send-email-jani@nikula.org

(notmuch compact --quiet and --backup=DIRECTORY options with docs)

LGTM

Tomi



> ---
>  man/man1/notmuch-compact.1 | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/man/man1/notmuch-compact.1 b/man/man1/notmuch-compact.1
> index 1aeed22..0c95873 100644
> --- a/man/man1/notmuch-compact.1
> +++ b/man/man1/notmuch-compact.1
> @@ -4,6 +4,8 @@ notmuch-compact \- compact the notmuch database
>  .SH SYNOPSIS
>  
>  .B notmuch compact
> +.RI "[ --quiet ]"
> +.RI "[ --backup=<" directory "> ]"
>  
>  .SH DESCRIPTION
>  
> @@ -14,11 +16,35 @@ the space required by the database and improve lookup performance.
>  
>  The compacted database is built in a temporary directory and is later
>  moved into the place of the origin database. The original uncompacted
> -database is preserved to be deleted by the user as desired.
> +database is discarded, unless the
> +.BR "\-\-backup=" <directory>
> +option is used.
>  
>  Note that the database write lock will be held during the compaction
>  process (which may be quite long) to protect data integrity.
>  
> +Supported options for
> +.B compact
> +include
> +
> +.RS 4
> +.TP 4
> +.BR "\-\-backup=" <directory>
> +
> +Save the current database to the given directory before replacing it
> +with the compacted database. The backup directory must not exist and
> +it must reside on the same mounted filesystem as the current database.
> +
> +.RE
> +
> +.RS 4
> +.TP 4
> +.BR \-\-quiet
> +
> +Do not report database compaction progress to stdout.
> +
> +.RE
> +
>  .RE
>  .SH ENVIRONMENT
>  The following environment variables can be used to control the
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/2] cli: add compact --quiet option and silence output with it

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Provide a way to silence the output.
>
> ---

pushed these two as well

d