[PATCH 1/4] test: Add compact test

[PATCH 1/4] test: Add compact test

[Ben Gamari]

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 test/compact      | 35 +++++++++++++++++++++++++++++++++++
 test/notmuch-test |  1 +
 2 files changed, 36 insertions(+)
 create mode 100755 test/compact

diff --git a/test/compact b/test/compact
new file mode 100755
index 0000000..54e85ab
--- /dev/null
+++ b/test/compact
@@ -0,0 +1,35 @@
+#!/usr/bin/env bash
+test_description='"notmuch compact"'
+. ./test-lib.sh
+
+add_message '[subject]=One'
+add_message '[subject]=Two'
+add_message '[subject]=Three'
+
+notmuch tag +tag1 \*
+notmuch tag +tag2 subject:Two
+notmuch tag -tag1 +tag3 subject:Three
+
+test_begin_subtest "Compacting"
+notmuch compact
+test_expect_success "compact" "notmuch compact"
+
+notmuch search \*
+output=$(notmuch search \* | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+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
+
+notmuch search \*
+output=$(notmuch search \* | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+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_done
diff --git a/test/notmuch-test b/test/notmuch-test
index aa28bb0..ec94baf 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -19,6 +19,7 @@ cd $(dirname "$0")
 TESTS="
   basic
   help-test
+  compact
   config
   setup
   new
-- 
1.8.3.2

[PATCH 2/4] compact: Give user more feedback on failure renaming

[Ben Gamari]

Provide the user with instructions after we fail to move the old
un-compacted database out of the way.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 lib/database.cc | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/database.cc b/lib/database.cc
index 06f1c0a..57c2292 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -922,7 +922,14 @@ notmuch_database_compact (const char* path,
 
     if (old_xapian_path != NULL) {
 	if (rename(xapian_path, old_xapian_path)) {
-	    fprintf (stderr, "Error moving old database out of the way\n");
+	    fprintf (stderr, "Error moving old database out of the way: %s\n",
+		     strerror(errno));
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "Old database: %s\n", xapian_path);
+	    fprintf (stderr, "Compacted database: %s\n", compact_xapian_path);
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "At this point it's probably best to remove the compacted database,\n");
+	    fprintf (stderr, "find the cause of this error, and try compacting again.\n");
 	    ret = NOTMUCH_STATUS_FILE_ERROR;
 	    goto DONE;
 	}
-- 
1.8.3.2

[PATCH 3/4] compact: Provide user with more error feedback

[Ben Gamari]

Provide instructions on what to do when we couldn't move the compacted
database into place.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 lib/database.cc | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/database.cc b/lib/database.cc
index 57c2292..34753ab 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -938,7 +938,25 @@ notmuch_database_compact (const char* path,
     }
 
     if (rename(compact_xapian_path, xapian_path)) {
-	fprintf (stderr, "Error moving compacted database\n");
+	fprintf (stderr, "Error moving compacted database into place: %s\n", strerror(errno));
+	fprintf (stderr, "\n");
+	fprintf (stderr, "Encountered error while moving the compacted database\n");
+	fprintf (stderr, "\n");
+	fprintf (stderr, "    %s\n", compact_xapian_path);
+	fprintf (stderr, "\n");
+	fprintf (stderr, "to\n");
+	fprintf (stderr, "\n");
+	fprintf (stderr, "    %s\n", xapian_path);
+	fprintf (stderr, "\n");
+	fprintf (stderr, "Please identify the reason for this and move the compacted database\n");
+	fprintf (stderr, "into place manually.\n");
+	if (old_xapian_path != NULL) {
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "Alternatively you can revert to the uncompacted database with\n");
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "    mv %s %s\n", old_xapian_path, xapian_path);
+	    fprintf (stderr, "\n");
+	}
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
-- 
1.8.3.2

[PATCH 4/4] database: Handle error while deleting uncompacted database

[Ben Gamari]

We never checked to ensure that the rmtree() of the old database
succeeded.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 lib/database.cc | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/database.cc b/lib/database.cc
index 34753ab..bfc5dac 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -934,7 +934,19 @@ notmuch_database_compact (const char* path,
 	    goto DONE;
 	}
     } else {
-	rmtree(xapian_path);
+	if (rmtree(xapian_path)) {
+	    fprintf (stderr, "Error removing old database: %s\n",
+		     strerror(errno));
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "Old database: %s\n", xapian_path);
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "Please remove the old database and move the compacted one in to place manually with\n");
+	    fprintf (stderr, "\n");
+	    fprintf (stderr, "    mv %s %s\n", compact_xapian_path, xapian_path);
+	    fprintf (stderr, "\n");
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
     }
 
     if (rename(compact_xapian_path, xapian_path)) {
-- 
1.8.3.2

Re: [PATCH 1/4] test: Add compact test

[Tomi Ollila]

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

> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
> ---

Patches 2, 3 & 4 Looks OK To Me. Thanks. A few comments on this patch 
inline:

>  test/compact      | 35 +++++++++++++++++++++++++++++++++++
>  test/notmuch-test |  1 +
>  2 files changed, 36 insertions(+)
>  create mode 100755 test/compact
>
> diff --git a/test/compact b/test/compact
> new file mode 100755
> index 0000000..54e85ab
> --- /dev/null
> +++ b/test/compact
> @@ -0,0 +1,35 @@
> +#!/usr/bin/env bash
> +test_description='"notmuch compact"'
> +. ./test-lib.sh
> +
> +add_message '[subject]=One'
> +add_message '[subject]=Two'
> +add_message '[subject]=Three'
> +
> +notmuch tag +tag1 \*
> +notmuch tag +tag2 subject:Two
> +notmuch tag -tag1 +tag3 subject:Three
> +
> +test_begin_subtest "Compacting"
> +notmuch compact
> +test_expect_success "compact" "notmuch compact"

test_expect_success executes "$2" ("notmuch compact" in this case)
do you mean to run notmuch 'compact twice' ?

> +notmuch search \*
> +output=$(notmuch search \* | notmuch_search_sanitize)

Now do you mean to run 'notmuch search \*' twice ?

> +test_expect_equal "$output" "\
> +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
> +
> +notmuch search \*
> +output=$(notmuch search \* | notmuch_search_sanitize)

again... ?


... actually David Bremner had even more insightful comments in 
id:874n8cw2yq.fsf@zancas.localnet to these same issues :D


Tomi


> +test_expect_equal "$output" "\
> +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_done
> diff --git a/test/notmuch-test b/test/notmuch-test
> index aa28bb0..ec94baf 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -19,6 +19,7 @@ cd $(dirname "$0")
>  TESTS="
>    basic
>    help-test
> +  compact
>    config
>    setup
>    new
> -- 
> 1.8.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] test: Add compact test

[Ben Gamari]

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

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

> On Tue, Oct 29 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:
>
>> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
>> ---
>
> Patches 2, 3 & 4 Looks OK To Me. Thanks. A few comments on this patch 
> inline:
>
Thanks again for the review!

>>  test/compact      | 35 +++++++++++++++++++++++++++++++++++
>>  test/notmuch-test |  1 +
>>  2 files changed, 36 insertions(+)
>>  create mode 100755 test/compact
>>
>> diff --git a/test/compact b/test/compact
>> new file mode 100755
>> index 0000000..54e85ab
>> --- /dev/null
>> +++ b/test/compact
>> @@ -0,0 +1,35 @@
>> +#!/usr/bin/env bash
>> +test_description='"notmuch compact"'
>> +. ./test-lib.sh
>> +
>> +add_message '[subject]=One'
>> +add_message '[subject]=Two'
>> +add_message '[subject]=Three'
>> +
>> +notmuch tag +tag1 \*
>> +notmuch tag +tag2 subject:Two
>> +notmuch tag -tag1 +tag3 subject:Three
>> +
>> +test_begin_subtest "Compacting"
>> +notmuch compact
>> +test_expect_success "compact" "notmuch compact"
>
> test_expect_success executes "$2" ("notmuch compact" in this case)
> do you mean to run notmuch 'compact twice' ?
>
It's been a while but I suspect I just never cleaned up the patch after
coming to this realization. Anyways, it's fixed in the revised patch
coming shortly.

>> +notmuch search \*
>> +output=$(notmuch search \* | notmuch_search_sanitize)
>
> Now do you mean to run 'notmuch search \*' twice ?
>
>> +test_expect_equal "$output" "\
>> +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
>> +
>> +notmuch search \*
>> +output=$(notmuch search \* | notmuch_search_sanitize)
>
> again... ?
>
>
> ... actually David Bremner had even more insightful comments in 
> id:874n8cw2yq.fsf@zancas.localnet to these same issues :D
>
Hmm, it seems I overlooked these. Thanks for the reference.

Cheers,

- Ben

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

[PATCH 1/4] test: Add compact test

[Ben Gamari]

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 test/compact      | 32 ++++++++++++++++++++++++++++++++
 test/notmuch-test |  1 +
 2 files changed, 33 insertions(+)
 create mode 100755 test/compact

diff --git a/test/compact b/test/compact
new file mode 100755
index 0000000..5bb5cea
--- /dev/null
+++ b/test/compact
@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+test_description='"notmuch compact"'
+. ./test-lib.sh
+
+add_message '[subject]=One'
+add_message '[subject]=Two'
+add_message '[subject]=Three'
+
+notmuch tag +tag1 \*
+notmuch tag +tag2 subject:Two
+notmuch tag -tag1 +tag3 subject:Three
+
+test_expect_success "Running compact" "notmuch compact"
+
+test_begin_subtest "Compact preserves database"
+output=$(notmuch search \* | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+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
+
+output=$(notmuch search \* | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+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_done
diff --git a/test/notmuch-test b/test/notmuch-test
index aa28bb0..ec94baf 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -19,6 +19,7 @@ cd $(dirname "$0")
 TESTS="
   basic
   help-test
+  compact
   config
   setup
   new
-- 
1.8.3.2

Re: [PATCH 1/4] test: Add compact test

[Tomi Ollila]

On Thu, Oct 31 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:

> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>

LGTM.

Tomi


> ---
>  test/compact      | 32 ++++++++++++++++++++++++++++++++
>  test/notmuch-test |  1 +
>  2 files changed, 33 insertions(+)
>  create mode 100755 test/compact
>
> diff --git a/test/compact b/test/compact
> new file mode 100755
> index 0000000..5bb5cea
> --- /dev/null
> +++ b/test/compact
> @@ -0,0 +1,32 @@
> +#!/usr/bin/env bash
> +test_description='"notmuch compact"'
> +. ./test-lib.sh
> +
> +add_message '[subject]=One'
> +add_message '[subject]=Two'
> +add_message '[subject]=Three'
> +
> +notmuch tag +tag1 \*
> +notmuch tag +tag2 subject:Two
> +notmuch tag -tag1 +tag3 subject:Three
> +
> +test_expect_success "Running compact" "notmuch compact"
> +
> +test_begin_subtest "Compact preserves database"
> +output=$(notmuch search \* | notmuch_search_sanitize)
> +test_expect_equal "$output" "\
> +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
> +
> +output=$(notmuch search \* | notmuch_search_sanitize)
> +test_expect_equal "$output" "\
> +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_done
> diff --git a/test/notmuch-test b/test/notmuch-test
> index aa28bb0..ec94baf 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -19,6 +19,7 @@ cd $(dirname "$0")
>  TESTS="
>    basic
>    help-test
> +  compact
>    config
>    setup
>    new
> -- 
> 1.8.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] test: Add compact test

[David Bremner]

Ben Gamari <bgamari.foss@gmail.com> writes:

> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
> ---
>  test/compact      | 32 ++++++++++++++++++++++++++++++++
>  test/notmuch-test |  1 +

Pushed the tests.

d

Re: [PATCH 4/4] database: Handle error while deleting uncompacted database

[David Bremner]

Ben Gamari <bgamari.foss@gmail.com> writes:

> We never checked to ensure that the rmtree() of the old database
> succeeded.
>

Is there some reason these patches (and the existing compact code in the
lib) is using fprintf and not the status callback function? Most places
we don't (yet) have logging callbacks, but here we do.

d
 

Re: [PATCH 4/4] database: Handle error while deleting uncompacted database

[Ben Gamari]

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

David Bremner <david@tethera.net> writes:

> Ben Gamari <bgamari.foss@gmail.com> writes:
>
>> We never checked to ensure that the rmtree() of the old database
>> succeeded.
>>
>
> Is there some reason these patches (and the existing compact code in the
> lib) is using fprintf and not the status callback function? Most places
> we don't (yet) have logging callbacks, but here we do.
>
Nope. I simply wasn't aware of this infrastructure. I can put together a
patch to fix this.

Cheers,

- Ben

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