[PATCH 3/4] lib: move deallocation of memory from n_d_close to n_d_destroy

2020-07-14 Thread David Bremner
In order to mimic the "best effort" API of Xapian to provide
information from a closed database when possible, do not
destroy the Xapian database object too early.

Because the pointer to a Xapian database is no longer nulled on close,
introduce a flag to track whether the notmuch database is open or not.
---
 lib/database-private.h |  2 +-
 lib/database.cc| 35 ---
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/lib/database-private.h b/lib/database-private.h
index 76359007..d2c25313 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -194,7 +194,7 @@ struct _notmuch_database {
 /* true if changes have been made in this atomic section */
 bool atomic_dirty;
 Xapian::Database *xapian_db;
-
+bool open;
 /* Bit mask of features used by this database.  This is a
  * bitwise-OR of NOTMUCH_FEATURE_* values (above). */
 enum _notmuch_features features;
diff --git a/lib/database.cc b/lib/database.cc
index 2f794164..cfb19ccb 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1076,6 +1076,10 @@ notmuch_database_open_verbose (const char *path,
*database = notmuch;
 else
talloc_free (notmuch);
+
+if (notmuch)
+   notmuch->open = true;
+
 return status;
 }
 
@@ -1087,7 +1091,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
 /* Many Xapian objects (and thus notmuch objects) hold references to
  * the database, so merely deleting the database may not suffice to
  * close it.  Thus, we explicitly close it here. */
-if (notmuch->xapian_db != NULL) {
+if (notmuch->open) {
try {
/* If there's an outstanding transaction, it's unclear if
 * closing the Xapian database commits everything up to
@@ -1110,20 +1114,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
}
}
 }
-
-delete notmuch->term_gen;
-notmuch->term_gen = NULL;
-delete notmuch->query_parser;
-notmuch->query_parser = NULL;
-delete notmuch->xapian_db;
-notmuch->xapian_db = NULL;
-delete notmuch->value_range_processor;
-notmuch->value_range_processor = NULL;
-delete notmuch->date_range_processor;
-notmuch->date_range_processor = NULL;
-delete notmuch->last_mod_range_processor;
-notmuch->last_mod_range_processor = NULL;
-
+notmuch->open = false;
 return status;
 }
 
@@ -1336,6 +1327,20 @@ notmuch_database_destroy (notmuch_database_t *notmuch)
 notmuch_status_t status;
 
 status = notmuch_database_close (notmuch);
+
+delete notmuch->term_gen;
+notmuch->term_gen = NULL;
+delete notmuch->query_parser;
+notmuch->query_parser = NULL;
+delete notmuch->xapian_db;
+notmuch->xapian_db = NULL;
+delete notmuch->value_range_processor;
+notmuch->value_range_processor = NULL;
+delete notmuch->date_range_processor;
+notmuch->date_range_processor = NULL;
+delete notmuch->last_mod_range_processor;
+notmuch->last_mod_range_processor = NULL;
+
 talloc_free (notmuch);
 
 return status;
-- 
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


batch 5, API exception handling cleanup

2020-07-14 Thread David Bremner
This is a followup and extension to

 id:20200709001709.449217-1-da...@tethera.net

Since that finished with message.cc, this one starts on database.cc

There is one non-boring patch here, namely

  [PATCH 3/4] lib: move deallocation of memory from n_d_close to n_d_destroy

It (of course) passes the test suite, but it represents a non-trivial
behaviour change for the few people calling notmuch_database_close
directly rather than via notmuch_database_destroy

___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


[PATCH 4/4] lib/n_d_get_version: catch exceptions and clarify the API

2020-07-14 Thread David Bremner
notmuch_database_get_version previously returned 0 on some errors, but
did not document this. Luckily 0 is not a valid database version.
---
 lib/database.cc   | 19 ++-
 lib/notmuch.h |  3 +++
 test/T562-lib-database.sh |  1 -
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index cfb19ccb..27861970 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -58,6 +58,17 @@ typedef struct {
 #define DB_ACTION Xapian::DB_CREATE_OR_OPEN
 #endif
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception 
(__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_database_t *notmuch,  const 
Xapian::Error error) {
+_notmuch_database_log (notmuch,
+  "A Xapian exception occurred %s accessing %s : %s\n",
+  where,
+  error.get_msg ().c_str ());
+notmuch->exception_reported = true;
+}
+
 /* Here's the current schema for our database (for NOTMUCH_DATABASE_VERSION):
  *
  * We currently have three different types of documents (mail, ghost,
@@ -1360,7 +1371,13 @@ notmuch_database_get_version (notmuch_database_t 
*notmuch)
 const char *str;
 char *end;
 
-version_string = notmuch->xapian_db->get_metadata ("version");
+try {
+   version_string = notmuch->xapian_db->get_metadata ("version");
+} catch (const Xapian::Error ) {
+   LOG_XAPIAN_EXCEPTION (notmuch, error);
+   return 0;
+}
+
 if (version_string.empty ())
return 0;
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 97ebc17d..7ee0507a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -431,6 +431,8 @@ notmuch_database_get_path (notmuch_database_t *database);
 
 /**
  * Return the database format version of the given database.
+ *
+ * @retval 0 on error
  */
 unsigned int
 notmuch_database_get_version (notmuch_database_t *database);
@@ -444,6 +446,7 @@ notmuch_database_get_version (notmuch_database_t *database);
  * fail with NOTMUCH_STATUS_UPGRADE_REQUIRED.  This always returns
  * FALSE for a read-only database because there's no way to upgrade a
  * read-only database.
+ *
  */
 notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *database);
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index b8fba7d6..c9705b13 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -68,7 +68,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get version with closed db"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
 unsigned int version;
-- 
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


[PATCH 2/4] test: add known broken test for n_d_get_version on closed db

2020-07-14 Thread David Bremner
This should not crash, but it does currently.
---
 test/T562-lib-database.sh | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index c869341a..b8fba7d6 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-test_description="error reporting for library"
+test_description="notmuch_database_* API"
 
 . $(dirname "$0")/test-lib.sh || exit 1
 
@@ -67,4 +67,21 @@ MAIL_DIR
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "get version with closed db"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+unsigned int version;
+EXPECT0(notmuch_database_close (db));
+version = notmuch_database_get_version (db);
+printf ("%u\n", version);
+}
+EOF
+cat < EXPECTED
+== stdout ==
+0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


[PATCH 1/4] test: regression tests for n_d_status_string and n_d_get_path

2020-07-14 Thread David Bremner
These do not crash on a closed database, and we want to keep it that
way.

Start a new file of tests as T560-lib-error was starting to get unwieldy.
---
 test/T562-lib-database.sh | 70 +++
 1 file changed, 70 insertions(+)
 create mode 100755 test/T562-lib-database.sh

diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
new file mode 100755
index ..c869341a
--- /dev/null
+++ b/test/T562-lib-database.sh
@@ -0,0 +1,70 @@
+#!/usr/bin/env bash
+test_description="error reporting for library"
+
+. $(dirname "$0")/test-lib.sh || exit 1
+
+add_email_corpus
+
+test_begin_subtest "building database"
+test_expect_success "NOTMUCH_NEW"
+
+cat < c_head
+#include 
+#include 
+#include 
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   char *msg = NULL;
+
+   stat = notmuch_database_open_verbose (argv[1], 
NOTMUCH_DATABASE_MODE_READ_WRITE, , );
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+ fprintf (stderr, "error opening database: %d %s\n", stat, msg ? msg : "");
+ exit (1);
+   }
+EOF
+
+cat <<'EOF' > c_tail
+   if (stat) {
+   const char *stat_str = notmuch_database_status_string (db);
+   if (stat_str)
+   fputs (stat_str, stderr);
+}
+
+}
+EOF
+
+test_begin_subtest "get status_string with closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+const char *str;
+EXPECT0(notmuch_database_close (db));
+str = notmuch_database_status_string (db);
+printf("%d\n", str == NULL);
+}
+EOF
+cat < EXPECTED
+== stdout ==
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "get path with closed db"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+const char *path;
+EXPECT0(notmuch_database_close (db));
+path = notmuch_database_get_path (db);
+printf("%s\n", path);
+}
+EOF
+cat < EXPECTED
+== stdout ==
+MAIL_DIR
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
-- 
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 1/2] doc: replace use of environment variables with a generated config

2020-07-14 Thread Floris Bruynooghe
On Sat 11 Jul 2020 at 17:00 +0300, Tomi Ollila wrote:

> On Sat, Jul 11 2020, David Bremner wrote:
>
>> I don't love the use of exec, but it is getting unwieldy to pass
>> configuration options on the sphinx-build command line, and I
>> anticipate further use of conditionals.
>
> Perhaps less "opinions" in commit message.
>
> (and as I think I don't comment 2/2, s/seperate/separate/ there)
>
>
>> ---
>>  configure  |  8 
>>  doc/Makefile.local |  2 +-
>>  doc/conf.py| 11 ---
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 80cbac4f..177432db 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1548,6 +1548,14 @@ NOTMUCH_HAVE_PYTHON3_PYTEST=${have_python3_pytest}
>>  PLATFORM=${platform}
>>  EOF
>>  
>> +cat > sphinx.config <> +# Generate by configure, run from doc/conf.py
>> +EOF
>> +if [ $WITH_EMACS = "1" ]; then
>> +printf "tags.add('WITH_EMACS')\n" >> sphinx.config
>> +fi
>> +printf "rsti_dir = '%s'\n" $(realpath emacs) >> sphinx.config
>> +
>
> perhaps instead of multiple redirections to the file, 
>
> {
> echo "# Generated by configure, run from doc/conf.py"
> echo
> if [ $WITH_EMACS = "1" ]; then
> printf "tags.add('WITH_EMACS')\n"
> fi 
> printf "rsti_dir = '%s'\n" "$(realpath emacs)"
>
> } > sphinx.config
> 
> alternative (someone might think less readable... ;/):
>
>  exec 3>&1 1> sphinx.config
>
>  echo "# Generated by configure, run from doc/conf.py"
>  ...
>
>  exec 1>&3 3>&-

I'd probably prefer this last one, but I don't mind any of the solutions or
even the current one.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH] doc: set up for autoapi / readthedocs compatibility

2020-07-14 Thread Floris Bruynooghe
On Sun 12 Jul 2020 at 09:02 -0300, David Bremner wrote:

> sphinx-autoapi seems nicer conceptually (it parses the docs rather
> than importing them),

TIL about sphinx-autoapi, agree it's nicer conceptually.

> but it also generates a ton of warnings, so
> leave the default as autodoc.
> ---
>
> You can see the results of this (for now) at 
> https://notmuch.readthedocs.io/en/rtdtest/
> We'd presumable want to integrate the whole tree somehow into notmuchmail.org

Saldy it seems to struggle with a fair bit of things.  E.g. it does
manage to create a Database.MODE attribute, but it doesn't figure out
that this is the Mode enum and thus doesn't document the fact you have
two options: MODE.READ_ONLY and MODE.READ_WRITE.  There are obviously a
bunch of other enums where this matters.

It could be that these things are solvable by using more autodoc-style
directives:
https://sphinx-autoapi.readthedocs.io/en/latest/reference/directives.html

But I wonder if the autodoc one will always be better because of the
dynamic nature.  E.g. mapping notmuch2._database.Mode to
notmuch2.Database.MODE on the actual Public API.

I didn't actually try out how much better autodoc does, I should
probably try that too before commenting further.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/2] doc: add new python bindings to main documentatation tree.

2020-07-14 Thread Floris Bruynooghe
Oh, this is very nice!  I've been thinking for a while I should attempt
this.  Great to see it being done!

On Sat 11 Jul 2020 at 10:20 -0300, David Bremner wrote:

> A seperate conf.py and doc directory will be needed if someone wants
> to build the bindings docs separately from notmuch.
> ---
>  configure   | 4 
>  doc/conf.py | 8 
>  doc/index.rst   | 1 +
>  doc/python-bindings.rst | 5 +
>  4 files changed, 18 insertions(+)
>  create mode 100644 doc/python-bindings.rst
>
> diff --git a/configure b/configure
> index 177432db..36fe4a9d 100755
> --- a/configure
> +++ b/configure
> @@ -801,6 +801,7 @@ if [ $have_python3 -eq 1 ]; then
>  if "$python" -c 'import cffi,setuptools; cffi.FFI().verify()' >/dev/null 
> 2>&1; then
>  printf "Yes.\n"
>  have_python3_cffi=1
> +WITH_PYTHON_DOCS=1
>  else
>  printf "No (will not install CFFI-based python bindings).\n"
>  fi
> @@ -1554,6 +1555,9 @@ EOF
>  if [ $WITH_EMACS = "1" ]; then
>  printf "tags.add('WITH_EMACS')\n" >> sphinx.config
>  fi
> +if [ $WITH_PYTHON_DOCS = "1" ]; then
> +printf "tags.add('WITH_PYTHON')\n" >> sphinx.config
> +fi
>  printf "rsti_dir = '%s'\n" $(realpath emacs) >> sphinx.config
>  
>  # Finally, after everything configured, inform the user how to continue.
> diff --git a/doc/conf.py b/doc/conf.py
> index fdff2a2c..94e266af 100644
> --- a/doc/conf.py
> +++ b/doc/conf.py
> @@ -4,6 +4,8 @@
>  import sys
>  import os
>  
> +extensions = [ 'sphinx.ext.autodoc' ]
> +
>  # The suffix of source filenames.
>  source_suffix = '.rst'
>  
> @@ -22,6 +24,9 @@ for pathdir in ['.', '..']:
>  with open(version_file,'r') as infile:
>  version=infile.read().replace('\n','')
>  
> +# for autodoc
> +sys.path.insert(0, os.path.join(location, '..', 'bindings', 'python-cffi', 
> 'notmuch2'))
> +
>  # read generated config
>  for pathdir in ['.', '..']:
>  conf_file = os.path.join(location,pathdir,'sphinx.config')
> @@ -50,6 +55,9 @@ else:
>  # the docstring include files
>  exclude_patterns.append('notmuch-emacs.rst')
>  
> +if not tags.has('WITH_PYTHON'):
> +exclude_patterns.append('python-bindings.rst')
> +
>  # The name of the Pygments (syntax highlighting) style to use.
>  pygments_style = 'sphinx'
>  
> diff --git a/doc/index.rst b/doc/index.rst
> index 4440d93a..a3bf3480 100644
> --- a/doc/index.rst
> +++ b/doc/index.rst
> @@ -26,6 +26,7 @@ Contents:
> man7/notmuch-search-terms
> man1/notmuch-show
> man1/notmuch-tag
> +   python-bindings
>  
>  Indices and tables
>  ==
> diff --git a/doc/python-bindings.rst b/doc/python-bindings.rst
> new file mode 100644
> index ..e1ad26ad
> --- /dev/null
> +++ b/doc/python-bindings.rst
> @@ -0,0 +1,5 @@
> +Python Bindings
> +===
> +
> +.. automodule:: notmuch2
> +   :members:
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 1/2] doc: replace use of environment variables with a generated config

2020-07-14 Thread Floris Bruynooghe
On Sat 11 Jul 2020 at 10:20 -0300, David Bremner wrote:

> I don't love the use of exec, but it is getting unwieldy to pass

It's already a config file written in Python which is a terrible sin.
So no need to apologise, I think it makes sense in this context.

> configuration options on the sphinx-build command line, and I
> anticipate further use of conditionals.
> ---
>  configure  |  8 
>  doc/Makefile.local |  2 +-
>  doc/conf.py| 11 ---
>  3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/configure b/configure
> index 80cbac4f..177432db 100755
> --- a/configure
> +++ b/configure
> @@ -1548,6 +1548,14 @@ NOTMUCH_HAVE_PYTHON3_PYTEST=${have_python3_pytest}
>  PLATFORM=${platform}
>  EOF
>  
> +cat > sphinx.config < +# Generate by configure, run from doc/conf.py

Minor insignificant typo: Generated

> +EOF
> +if [ $WITH_EMACS = "1" ]; then
> +printf "tags.add('WITH_EMACS')\n" >> sphinx.config
> +fi
> +printf "rsti_dir = '%s'\n" $(realpath emacs) >> sphinx.config
> +
>  # Finally, after everything configured, inform the user how to continue.
>  cat <  
> diff --git a/doc/Makefile.local b/doc/Makefile.local
> index 769438ed..598b6028 100644
> --- a/doc/Makefile.local
> +++ b/doc/Makefile.local
> @@ -4,7 +4,7 @@ dir := doc
>  
>  # You can set these variables from the command line.
>  SPHINXOPTS:= -q
> -SPHINXBUILD   = WITH_EMACS=${WITH_EMACS} RSTI_DIR=$(realpath emacs) 
> sphinx-build
> +SPHINXBUILD   = sphinx-build
>  DOCBUILDDIR  := $(dir)/_build
>  
>  # Internal variables.
> diff --git a/doc/conf.py b/doc/conf.py
> index 70987ac5..fdff2a2c 100644
> --- a/doc/conf.py
> +++ b/doc/conf.py
> @@ -22,6 +22,13 @@ for pathdir in ['.', '..']:
>  with open(version_file,'r') as infile:
>  version=infile.read().replace('\n','')
>  
> +# read generated config
> +for pathdir in ['.', '..']:
> +conf_file = os.path.join(location,pathdir,'sphinx.config')
> +if os.path.exists(conf_file):
> +with open(conf_file,'r') as infile:
> +exec(''.join(infile.readlines()))
> +
>  # The full version, including alpha/beta/rc tags.
>  release = version
>  
> @@ -29,12 +36,10 @@ release = version
>  # directories to ignore when looking for source files.
>  exclude_patterns = ['_build']
>  
> -if os.environ.get('WITH_EMACS') == '1':
> +if tags.has('WITH_EMACS'):
>  # Hacky reimplementation of include to workaround limitations of
>  # sphinx-doc
>  lines = ['.. include:: /../emacs/rstdoc.rsti\n\n'] # in the source tree
> -rsti_dir = os.environ.get('RSTI_DIR')
> -# the other files are from the build tree
>  for file in ('notmuch.rsti', 'notmuch-lib.rsti', 'notmuch-show.rsti', 
> 'notmuch-tag.rsti'):
>  lines.extend(open(rsti_dir+'/'+file))
>  rst_epilog = ''.join(lines)
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: Third batch of API cleanup for exception safety

2020-07-14 Thread David Bremner
David Bremner  writes:

> This is a continuation of
>
>  id:20200704151805.3717715-1-da...@tethera.net
>
> and probably needs to be applied on top.
>
> There are two patches not fitting the pattern of "add test" or "add
> try/catch to fix test".

pushed this batch of patches to master.

d
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org