[PATCH 0/3] delete directory documents on directory removal

[PATCH 0/3] delete directory documents on directory removal

[Jani Nikula]

Hi all, this is a bit RFC-ish series for fixing some issues on directory
renames and removals. The commit messages could use some polish, but I
think I added enough to make it reasonable to follow. I'm not sure if
the directory document removal API does enough or not, or whether it's a
good API. This series just proves we need to do this.

BR,
Jani.



Jani Nikula (3):
  test: flag one more notmuch new test as broken
  lib: add interface to delete directory documents
  cli: delete directory documents on directory removal

 lib/directory.cc | 18 ++++++++++++++++++
 lib/notmuch.h    |  7 +++++++
 notmuch-new.c    | 12 +++---------
 test/T050-new.sh |  2 +-
 4 files changed, 29 insertions(+), 10 deletions(-)

-- 
2.1.4

[PATCH 1/3] test: flag one more notmuch new test as broken

[Jani Nikula]

Drop the test update added in

commit e4e04bbc328f990e36b77f508aef904d156029b1
Author: David Bremner <david@tethera.net>
Date:   Tue Aug 4 08:48:33 2015 +0200

    cli/new: add more debugging output

and mark the test as broken, like the tests flagged as broken in

commit ed9ceda623d3f22fb66365b33db63c5c982067d3
Author: David Bremner <david@tethera.net>
Date:   Tue Aug 4 08:48:34 2015 +0200

    test: add debugging output to notmuch-new tests, mark 5 as broken
---
 test/T050-new.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/T050-new.sh b/test/T050-new.sh
index 625739232369..33ce1ab90a3d 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -224,6 +224,7 @@ output=$(NOTMUCH_NEW 2>&1)
 test_expect_equal "$output" "Added 1 new message to the database."
 
 test_begin_subtest "Ignore files and directories specified in new.ignore (multiple occurrences)"
+test_subtest_known_broken
 notmuch config set new.ignore .git ignored_file .ignored_hidden_file
 notmuch new > /dev/null # ensure that files/folders will be printed in ASCII order.
 touch "${MAIL_DIR}"/.git # change .git's mtime for notmuch new to rescan.
@@ -246,7 +247,6 @@ test_expect_equal "$output" \
 (D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/ignored_file
 (D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/.git
 (D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/ignored_file
-(D) add_files_recursive, pass 3: queuing leftover directory ${MAIL_DIR}/two for deletion from database
 No new mail."
 
 
-- 
2.1.4

[PATCH 2/3] lib: add interface to delete directory documents

[Jani Nikula]

As mentioned in

commit acd66cdec075312944e527febd46382e54d99367
Author: Jani Nikula <jani@nikula.org>
Date:   Sat Sep 5 12:35:31 2015 +0300

    cli: reset db directory mtime upon directory removal

we don't have an interface to delete directory documents, and they're
left behind. Add the interface.

XXX: Should this also remove the files under it, or assume that's been
done by the caller? Should this incorporate some or all of the
functionality of _remove_directory() in notmuch-new.c?
---
 lib/directory.cc | 18 ++++++++++++++++++
 lib/notmuch.h    |  7 +++++++
 2 files changed, 25 insertions(+)

diff --git a/lib/directory.cc b/lib/directory.cc
index b836ea2812c8..f23b71769aef 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -281,6 +281,24 @@ notmuch_directory_get_child_directories (notmuch_directory_t *directory)
     return child_directories;
 }
 
+notmuch_status_t
+notmuch_directory_delete (notmuch_directory_t *directory)
+{
+    notmuch_status_t status;
+    Xapian::WritableDatabase *db;
+
+    status = _notmuch_database_ensure_writable (directory->notmuch);
+    if (status)
+	return status;
+
+    db = static_cast <Xapian::WritableDatabase *> (directory->notmuch->xapian_db);
+    db->delete_document (directory->document_id);
+
+    notmuch_directory_destroy (directory);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 void
 notmuch_directory_destroy (notmuch_directory_t *directory)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 87756838d072..1feda4521e4d 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1720,6 +1720,13 @@ notmuch_filenames_t *
 notmuch_directory_get_child_directories (notmuch_directory_t *directory);
 
 /**
+ * Delete directory document from the database, and destroy the
+ * notmuch_directory_t object.
+ */
+notmuch_status_t
+notmuch_directory_delete (notmuch_directory_t *directory);
+
+/**
  * Destroy a notmuch_directory_t object.
  */
 void
-- 
2.1.4

[PATCH 3/3] cli: delete directory documents on directory removal

[Jani Nikula]

There was a problem with the directory documents being left behind
when the filesystem directory was removed. This was worked around in

commit acd66cdec075312944e527febd46382e54d99367
Author: Jani Nikula <jani@nikula.org>
Date:   Sat Sep 5 12:35:31 2015 +0300

    cli: reset db directory mtime upon directory removal

However, that ignored the fact that the directory documents are also
still listed by notmuch_directory_get_child_directories() leading to
confusing results when running notmuch new. The directory documents
are found and queued for removal over and over again.

Fix the problem for real by removing the directory documents. This
fixes the tests flagged as broken in

commit ed9ceda623d3f22fb66365b33db63c5c982067d3
Author: David Bremner <david@tethera.net>
Date:   Tue Aug 4 08:48:34 2015 +0200

    test: add debugging output to notmuch-new tests, mark 5 as broken

The (non-deterministic) hack test from [1] also still passes with this
change.

[1] id:1441445731-4362-1-git-send-email-jani@nikula.org
---
 notmuch-new.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 33645349cd5f..8bfed37fef96 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -878,17 +878,11 @@ _remove_directory (void *ctx,
 	    goto DONE;
     }
 
-    /*
-     * XXX: The library does not have a function to remove a directory
-     * document for a path. Usually this doesn't matter except for a
-     * slight waste of space. However, if the directory gets added to
-     * the filesystem again, the old directory document is found with
-     * the old mtime. Reset the directory mtime to avoid problems.
-     */
-    notmuch_directory_set_mtime (directory, 0);
+    status = notmuch_directory_delete (directory);
 
   DONE:
-    notmuch_directory_destroy (directory);
+    if (status)
+	notmuch_directory_destroy (directory);
     return status;
 }
 
-- 
2.1.4

Re: [PATCH 2/3] lib: add interface to delete directory documents

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

>
> XXX: Should this also remove the files under it, or assume that's been
> done by the caller? Should this incorporate some or all of the
> functionality of _remove_directory() in notmuch-new.c?

1) The top level _remove_directory function does seem to make sense in
  the lib, however
2) it calls remove_filename, which reads and writes add_files_state in
  pseudo-OO style.
3) In particular it needs to read synchronize_flags, and write
  renamed_messages and removed_messages.
  
I guess one solution would be to pass through three arguments. A fancier
version would be to pass in a "visitor" function and closure pointer.

The cowardly solution would be to point at POSIX rmdir, and leave the
discussion immediately above for the future.

d

V2 of directory delete patches

[David Bremner]

Changes since id:cover.1443213654.git.jani@nikula.org

  - tidy commit messages
  - update docs in lib/notmuch.h
  - make an executive decision not to push _remove_directory into lib
  for now; it can always be a new api layer later.
  - unmark fixed tests
  - wrap xapian calls in try/catch

[PATCH 1/3] test: flag one more notmuch new test as broken

[David Bremner]

From: Jani Nikula <jani@nikula.org>

Drop the test update added in [1] and mark the test as broken, like the
tests flagged as broken in [2]. These all reflect the same underlying
breakage with (lack of) directory deletion.

[1] commit e4e04bbc328f990e36b77f508aef904d156029b1
[2] commit ed9ceda623d3f22fb66365b33db63c5c982067d3
---
 test/T050-new.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/T050-new.sh b/test/T050-new.sh
index 6257392..33ce1ab 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -224,6 +224,7 @@ output=$(NOTMUCH_NEW 2>&1)
 test_expect_equal "$output" "Added 1 new message to the database."
 
 test_begin_subtest "Ignore files and directories specified in new.ignore (multiple occurrences)"
+test_subtest_known_broken
 notmuch config set new.ignore .git ignored_file .ignored_hidden_file
 notmuch new > /dev/null # ensure that files/folders will be printed in ASCII order.
 touch "${MAIL_DIR}"/.git # change .git's mtime for notmuch new to rescan.
@@ -246,7 +247,6 @@ test_expect_equal "$output" \
 (D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/ignored_file
 (D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/.git
 (D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/ignored_file
-(D) add_files_recursive, pass 3: queuing leftover directory ${MAIL_DIR}/two for deletion from database
 No new mail."
 
 
-- 
2.5.3

[PATCH 2/3] lib: add interface to delete directory documents

[David Bremner]

From: Jani Nikula <jani@nikula.org>

As mentioned in acd66cdec075312944e527febd46382e54d99367 we don't have
an interface to delete directory documents, and they're left behind. Add
the interface.
---
 lib/directory.cc | 25 +++++++++++++++++++++++++
 lib/notmuch.h    | 10 ++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lib/directory.cc b/lib/directory.cc
index b836ea2..78637b3 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -281,6 +281,31 @@ notmuch_directory_get_child_directories (notmuch_directory_t *directory)
     return child_directories;
 }
 
+notmuch_status_t
+notmuch_directory_delete (notmuch_directory_t *directory)
+{
+    notmuch_status_t status;
+    Xapian::WritableDatabase *db;
+
+    status = _notmuch_database_ensure_writable (directory->notmuch);
+    if (status)
+	return status;
+
+    try {
+	db = static_cast <Xapian::WritableDatabase *> (directory->notmuch->xapian_db);
+	db->delete_document (directory->document_id);
+    } catch (const Xapian::Error &error) {
+	_notmuch_database_log (directory->notmuch,
+			       "A Xapian exception occurred deleting directory entry: %s.\n",
+			       error.get_msg().c_str());
+	directory->notmuch->exception_reported = TRUE;
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+    notmuch_directory_destroy (directory);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 void
 notmuch_directory_destroy (notmuch_directory_t *directory)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index c5f7dcb..85b56bf 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1762,6 +1762,16 @@ notmuch_filenames_t *
 notmuch_directory_get_child_directories (notmuch_directory_t *directory);
 
 /**
+ * Delete directory document from the database, and destroy the
+ * notmuch_directory_t object. Assumes any child directories and files
+ * have been deleted by the caller.
+ *
+ * @since libnotmuch 4.3 (notmuch 0.21)
+ */
+notmuch_status_t
+notmuch_directory_delete (notmuch_directory_t *directory);
+
+/**
  * Destroy a notmuch_directory_t object.
  */
 void
-- 
2.5.3

[PATCH 3/3] cli: delete directory documents on directory removal

[David Bremner]

From: Jani Nikula <jani@nikula.org>

There was a problem with the directory documents being left behind when
the filesystem directory was removed. This was worked around in [1].
However, that ignored the fact that the directory documents are also
still listed by notmuch_directory_get_child_directories() leading to
confusing results when running notmuch new. The directory documents are
found and queued for removal over and over again.

Fix the problem for real by removing the directory documents. This fixes
the tests flagged as broken in [2].

The (non-deterministic) hack test from [1] also still passes with this
change.

[1] commit acd66cdec075312944e527febd46382e54d99367
[2] commit ed9ceda623d3f22fb66365b33db63c5c982067d3
[3] id:1441445731-4362-1-git-send-email-jani@nikula.org
---
 notmuch-new.c    | 12 +++---------
 test/T050-new.sh |  6 ------
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 442a2f0..d45d0af 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -878,17 +878,11 @@ _remove_directory (void *ctx,
 	    goto DONE;
     }
 
-    /*
-     * XXX: The library does not have a function to remove a directory
-     * document for a path. Usually this doesn't matter except for a
-     * slight waste of space. However, if the directory gets added to
-     * the filesystem again, the old directory document is found with
-     * the old mtime. Reset the directory mtime to avoid problems.
-     */
-    notmuch_directory_set_mtime (directory, 0);
+    status = notmuch_directory_delete (directory);
 
   DONE:
-    notmuch_directory_destroy (directory);
+    if (status)
+	notmuch_directory_destroy (directory);
     return status;
 }
 
diff --git a/test/T050-new.sh b/test/T050-new.sh
index 33ce1ab..81cf2fa 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -93,7 +93,6 @@ No new mail. Detected 3 file renames."
 
 
 test_begin_subtest "Deleted directory"
-test_subtest_known_broken
 rm -rf "${MAIL_DIR}"/dir-renamed
 
 output=$(NOTMUCH_NEW --debug)
@@ -102,7 +101,6 @@ No new mail. Removed 3 messages."
 
 
 test_begin_subtest "New directory (at end of list)"
-test_subtest_known_broken
 
 generate_message [dir]=zzz
 generate_message [dir]=zzz
@@ -114,7 +112,6 @@ test_expect_equal "$output" "Added 3 new messages to the database."
 
 test_begin_subtest "Deleted directory (end of list)"
 
-test_subtest_known_broken
 rm -rf "${MAIL_DIR}"/zzz
 
 output=$(NOTMUCH_NEW --debug)
@@ -173,7 +170,6 @@ test_expect_equal "$output" "(D) add_files_recursive, pass 3: queuing leftover d
 No new mail. Removed 3 messages."
 
 test_begin_subtest "Support single-message mbox"
-test_subtest_known_broken
 cat > "${MAIL_DIR}"/mbox_file1 <<EOF
 From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
 From: Notmuch Test Suite <test_suite@notmuchmail.org>
@@ -187,7 +183,6 @@ test_expect_equal "$output" "Added 1 new message to the database."
 
 # This test requires that notmuch new has been run at least once.
 test_begin_subtest "Skip and report non-mail files"
-test_subtest_known_broken
 generate_message
 mkdir -p "${MAIL_DIR}"/.git && touch "${MAIL_DIR}"/.git/config
 touch "${MAIL_DIR}"/ignored_file
@@ -224,7 +219,6 @@ output=$(NOTMUCH_NEW 2>&1)
 test_expect_equal "$output" "Added 1 new message to the database."
 
 test_begin_subtest "Ignore files and directories specified in new.ignore (multiple occurrences)"
-test_subtest_known_broken
 notmuch config set new.ignore .git ignored_file .ignored_hidden_file
 notmuch new > /dev/null # ensure that files/folders will be printed in ASCII order.
 touch "${MAIL_DIR}"/.git # change .git's mtime for notmuch new to rescan.
-- 
2.5.3

Re: V2 of directory delete patches

[Tomi Ollila]

On Sat, Oct 10 2015, David Bremner <david@tethera.net> wrote:

> Changes since id:cover.1443213654.git.jani@nikula.org
>
>   - tidy commit messages
>   - update docs in lib/notmuch.h
>   - make an executive decision not to push _remove_directory into lib
>   for now; it can always be a new api layer later.
>   - unmark fixed tests
>   - wrap xapian calls in try/catch


Looks good to me. tests pass.

Tomi

Re: V2 of directory delete patches

[David Bremner]

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

>
>
> Looks good to me. tests pass.
>
> Tomi

Pushed, with one one commit message typo fix