Piotr Trojanek <piotr.trojanek@gmail.com> writes: > On Wed, Jun 21, 2017 at 5:11 PM, Daniel Kahn Gillmor > <dkg@fifthhorseman.net> wrote: >> patch 5 adds FIXMEs that should probably actually be fixed, though, rather than just flagged. > > Thanks for merging the uncontroversial patches. Fixing the flagged > problems is not obvious to me, it really depends on your intentions. > > For the first FIXME, the documentation for notmuch_directory_delete > says (lib/notmuch.h:1971): > > * Delete directory document from the database, and destroy the > * notmuch_directory_t object. > > but that is not what happens, for example, if the call to > _notmuch_database_ensure_writable fails. Then the notmuch_directory_t > object is only destroyed by the caller (see the end of > _remove_directory function, notmuch-new.c:886). > > The comment should clearly say if the object is always destroyed or > only if no error happened. It seems like it would be cleaner to do something like the patch below, then amend the comment to say the notmuch_directory_t object is destroyed only on successful deletion of the document. diff --git a/lib/directory.cc b/lib/directory.cc index 5de3319c..d8f7763b 100644 --- a/lib/directory.cc +++ b/lib/directory.cc @@ -297,6 +297,7 @@ notmuch_directory_delete (notmuch_directory_t *directory) try { db = static_cast <Xapian::WritableDatabase *> (directory->notmuch->xapian_db); db->delete_document (directory->document_id); + notmuch_directory_destroy (directory); } catch (const Xapian::Error &error) { _notmuch_database_log (directory->notmuch, "A Xapian exception occurred deleting directory entry: %s.\n", @@ -304,9 +305,8 @@ notmuch_directory_delete (notmuch_directory_t *directory) directory->notmuch->exception_reported = TRUE; status = NOTMUCH_STATUS_XAPIAN_EXCEPTION; } - notmuch_directory_destroy (directory); - return NOTMUCH_STATUS_SUCCESS; + return status; } void