* [PATCH 0/3] delete directory documents on directory removal
@ 2015-09-25 20:48 Jani Nikula
2015-09-25 20:48 ` [PATCH 1/3] test: flag one more notmuch new test as broken Jani Nikula
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jani Nikula @ 2015-09-25 20:48 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] test: flag one more notmuch new test as broken 2015-09-25 20:48 [PATCH 0/3] delete directory documents on directory removal Jani Nikula @ 2015-09-25 20:48 ` Jani Nikula 2015-09-25 20:48 ` [PATCH 2/3] lib: add interface to delete directory documents Jani Nikula 2015-09-25 20:48 ` [PATCH 3/3] cli: delete directory documents on directory removal Jani Nikula 2 siblings, 0 replies; 11+ messages in thread From: Jani Nikula @ 2015-09-25 20:48 UTC (permalink / raw) To: notmuch 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] lib: add interface to delete directory documents 2015-09-25 20:48 [PATCH 0/3] delete directory documents on directory removal Jani Nikula 2015-09-25 20:48 ` [PATCH 1/3] test: flag one more notmuch new test as broken Jani Nikula @ 2015-09-25 20:48 ` Jani Nikula 2015-09-28 11:42 ` David Bremner 2015-09-25 20:48 ` [PATCH 3/3] cli: delete directory documents on directory removal Jani Nikula 2 siblings, 1 reply; 11+ messages in thread From: Jani Nikula @ 2015-09-25 20:48 UTC (permalink / raw) To: notmuch 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] lib: add interface to delete directory documents 2015-09-25 20:48 ` [PATCH 2/3] lib: add interface to delete directory documents Jani Nikula @ 2015-09-28 11:42 ` David Bremner 0 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2015-09-28 11:42 UTC (permalink / raw) To: Jani Nikula, notmuch 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] cli: delete directory documents on directory removal 2015-09-25 20:48 [PATCH 0/3] delete directory documents on directory removal Jani Nikula 2015-09-25 20:48 ` [PATCH 1/3] test: flag one more notmuch new test as broken Jani Nikula 2015-09-25 20:48 ` [PATCH 2/3] lib: add interface to delete directory documents Jani Nikula @ 2015-09-25 20:48 ` Jani Nikula 2015-10-10 12:27 ` V2 of directory delete patches David Bremner 2 siblings, 1 reply; 11+ messages in thread From: Jani Nikula @ 2015-09-25 20:48 UTC (permalink / raw) To: notmuch 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* V2 of directory delete patches 2015-09-25 20:48 ` [PATCH 3/3] cli: delete directory documents on directory removal Jani Nikula @ 2015-10-10 12:27 ` David Bremner 2015-10-10 12:27 ` [PATCH 1/3] test: flag one more notmuch new test as broken David Bremner ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: David Bremner @ 2015-10-10 12:27 UTC (permalink / raw) To: Jani Nikula, notmuch 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] test: flag one more notmuch new test as broken 2015-10-10 12:27 ` V2 of directory delete patches David Bremner @ 2015-10-10 12:27 ` David Bremner 2015-10-10 12:27 ` [PATCH 2/3] lib: add interface to delete directory documents David Bremner ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2015-10-10 12:27 UTC (permalink / raw) To: Jani Nikula, notmuch 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] lib: add interface to delete directory documents 2015-10-10 12:27 ` V2 of directory delete patches David Bremner 2015-10-10 12:27 ` [PATCH 1/3] test: flag one more notmuch new test as broken David Bremner @ 2015-10-10 12:27 ` David Bremner 2015-10-10 12:27 ` [PATCH 3/3] cli: delete directory documents on directory removal David Bremner 2015-10-11 16:45 ` V2 of directory delete patches Tomi Ollila 3 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2015-10-10 12:27 UTC (permalink / raw) To: Jani Nikula, notmuch 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] cli: delete directory documents on directory removal 2015-10-10 12:27 ` V2 of directory delete patches David Bremner 2015-10-10 12:27 ` [PATCH 1/3] test: flag one more notmuch new test as broken David Bremner 2015-10-10 12:27 ` [PATCH 2/3] lib: add interface to delete directory documents David Bremner @ 2015-10-10 12:27 ` David Bremner 2015-10-11 16:45 ` V2 of directory delete patches Tomi Ollila 3 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2015-10-10 12:27 UTC (permalink / raw) To: Jani Nikula, notmuch 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: V2 of directory delete patches 2015-10-10 12:27 ` V2 of directory delete patches David Bremner ` (2 preceding siblings ...) 2015-10-10 12:27 ` [PATCH 3/3] cli: delete directory documents on directory removal David Bremner @ 2015-10-11 16:45 ` Tomi Ollila 2015-10-14 10:58 ` David Bremner 3 siblings, 1 reply; 11+ messages in thread From: Tomi Ollila @ 2015-10-11 16:45 UTC (permalink / raw) To: David Bremner, Jani Nikula, notmuch 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: V2 of directory delete patches 2015-10-11 16:45 ` V2 of directory delete patches Tomi Ollila @ 2015-10-14 10:58 ` David Bremner 0 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2015-10-14 10:58 UTC (permalink / raw) To: Tomi Ollila, Jani Nikula, notmuch Tomi Ollila <tomi.ollila@iki.fi> writes: > > > Looks good to me. tests pass. > > Tomi Pushed, with one one commit message typo fix ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-10-14 11:00 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-25 20:48 [PATCH 0/3] delete directory documents on directory removal Jani Nikula 2015-09-25 20:48 ` [PATCH 1/3] test: flag one more notmuch new test as broken Jani Nikula 2015-09-25 20:48 ` [PATCH 2/3] lib: add interface to delete directory documents Jani Nikula 2015-09-28 11:42 ` David Bremner 2015-09-25 20:48 ` [PATCH 3/3] cli: delete directory documents on directory removal Jani Nikula 2015-10-10 12:27 ` V2 of directory delete patches David Bremner 2015-10-10 12:27 ` [PATCH 1/3] test: flag one more notmuch new test as broken David Bremner 2015-10-10 12:27 ` [PATCH 2/3] lib: add interface to delete directory documents David Bremner 2015-10-10 12:27 ` [PATCH 3/3] cli: delete directory documents on directory removal David Bremner 2015-10-11 16:45 ` V2 of directory delete patches Tomi Ollila 2015-10-14 10:58 ` David Bremner
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).