[PATCH 09/11] lib/parse-sexp: add error handling to internal API

[David Bremner <david@tethera.net>]

The 'notmuch' argument is currently only used for logging in one
place, but this will most likely change in the future. Furthermore, it
will be needed for e.g. stored queries.
---
 lib/parse-sexp.cc         | 75 +++++++++++++++++++++++++--------------
 lib/query.cc              |  5 +--
 test/T081-sexpr-search.sh | 12 ++++++-
 3 files changed, 61 insertions(+), 31 deletions(-)

diff --git a/lib/parse-sexp.cc b/lib/parse-sexp.cc
index 81bf5cee..4a9e1aeb 100644
--- a/lib/parse-sexp.cc
+++ b/lib/parse-sexp.cc
@@ -40,40 +40,55 @@ static _sexp_field_t fields[] =
     { }
 };
 
-static Xapian::Query _sexp_to_xapian_query (sexp_t *sx);
+static notmuch_status_t _sexp_to_xapian_query (notmuch_database_t *notmuch, sexp_t *sx, Xapian::Query &output);
 
-static Xapian::Query
-_sexp_combine_query (Xapian::Query::op operation,
+static notmuch_status_t
+_sexp_combine_query (notmuch_database_t *notmuch,
+		     Xapian::Query::op operation,
 		     Xapian::Query left,
-		     sexp_t *sx)
+		     sexp_t *sx,
+		     Xapian::Query &output)
 {
+    Xapian::Query subquery;
+
+    notmuch_status_t status;
 
     /* if we run out elements, return accumulator */
 
-    if (! sx)
-	return left;
+    if (! sx) {
+	output = left;
+	return NOTMUCH_STATUS_SUCCESS;
+    }
 
-    return _sexp_combine_query (operation,
-				Xapian::Query (operation,
-					       left,
-					       _sexp_to_xapian_query (sx)),
-				sx->next);
+    status = _sexp_to_xapian_query (notmuch, sx, subquery);
+    if (status)
+	return status;
+
+    return _sexp_combine_query (notmuch,
+				operation,
+				Xapian::Query (operation, left, subquery),
+				sx->next, output);
 }
 
-Xapian::Query
-_notmuch_sexp_string_to_xapian_query (notmuch_database_t *notmuch, const char *querystr)
+notmuch_status_t
+_notmuch_sexp_string_to_xapian_query (notmuch_database_t *notmuch, const char *querystr,
+				      Xapian::Query &output)
 {
     sexp_t *sx = NULL;
     char *buf = talloc_strdup (notmuch, querystr);
 
     sx = parse_sexp (buf, strlen (querystr));
-    return _sexp_to_xapian_query (sx);
+    if (!sx)
+	return NOTMUCH_STATUS_ILLEGAL_ARGUMENT;
+
+    return _sexp_to_xapian_query (notmuch, sx, output);
 }
 
-static Xapian::Query
+static notmuch_status_t
 _sexp_combine_field (const char *prefix,
 		     Xapian::Query::op operation,
-		     sexp_t *sx)
+		     sexp_t *sx,
+		     Xapian::Query &output)
 {
     std::vector<std::string> terms;
 
@@ -107,15 +122,16 @@ _sexp_combine_field (const char *prefix,
 	    terms.push_back (pref_str + cur->val);
 	}
     }
-    return Xapian::Query (operation, terms.begin (), terms.end ());
+    output = Xapian::Query (operation, terms.begin (), terms.end ());
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 /* Here we expect the s-expression to be a proper list, with first
  * element defining and operation, or as a special case the empty
  * list */
 
-static Xapian::Query
-_sexp_to_xapian_query (sexp_t *sx)
+static notmuch_status_t
+_sexp_to_xapian_query (notmuch_database_t *notmuch, sexp_t *sx, Xapian::Query &output)
 {
 
     const _sexp_op_t *op;
@@ -124,18 +140,25 @@ _sexp_to_xapian_query (sexp_t *sx)
     assert (sx->ty == SEXP_LIST);
 
     /* Empty list */
-    if (! sx->list)
-	return Xapian::Query::MatchAll;
+    if (! sx->list) {
+	output = Xapian::Query::MatchAll;
+	return NOTMUCH_STATUS_SUCCESS;
+    }
 
     for (op = operations; op && op->name; op++) {
-	if (strcasecmp (op->name, hd_sexp (sx)->val) == 0)
-	    return _sexp_combine_query (op->xapian_op, op->initial, sx->list->next);
+	if (strcasecmp (op->name, hd_sexp (sx)->val) == 0) {
+	    return _sexp_combine_query (notmuch, op->xapian_op, op->initial, sx->list->next, output);
+	}
+
     }
 
     for (const _sexp_field_t *field = fields; field && field->name; field++) {
-	if (strcasecmp (field->name, hd_sexp (sx)->val) == 0)
-	    return _sexp_combine_field (_find_prefix (field->name), field->xapian_op, sx->list->next);
+	if (strcasecmp (field->name, hd_sexp (sx)->val) == 0) {
+	    return _sexp_combine_field (_find_prefix (field->name), field->xapian_op, sx->list->next,
+					output);
+	}
     }
 
-    INTERNAL_ERROR ("unimplemented prefix %s\n", sx->list->val);
+    _notmuch_database_log_append (notmuch, "unimplemented prefix %s\n", sx->list->val);
+    return NOTMUCH_STATUS_ILLEGAL_ARGUMENT;
 }
diff --git a/lib/query.cc b/lib/query.cc
index 6e89af60..4b42620e 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -191,10 +191,7 @@ _notmuch_query_ensure_parsed_xapian (notmuch_query_t *query)
 static notmuch_status_t
 _notmuch_query_ensure_parsed_sexpr (notmuch_query_t *query)
 {
-
-    query->xapian_query = _notmuch_sexp_string_to_xapian_query (query->notmuch, query->query_string);
-
-    return NOTMUCH_STATUS_SUCCESS;
+    return _notmuch_sexp_string_to_xapian_query (query->notmuch, query->query_string, query->xapian_query);
 }
 
 static notmuch_status_t
diff --git a/test/T081-sexpr-search.sh b/test/T081-sexpr-search.sh
index 9ee9de7b..b8229ebe 100755
--- a/test/T081-sexpr-search.sh
+++ b/test/T081-sexpr-search.sh
@@ -178,5 +178,15 @@ test_begin_subtest "Search by 'to' (name and address)"
 output=$(notmuch search --query-syntax=sexp '(to "Search By To Name <test@example.com>")' | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; search by to (name) (inbox unread)"
 
-
+test_begin_subtest "Unbalanced parens"
+# A code 1 indicates the error was handled (a crash will return e.g. 139).
+test_expect_code 1 "notmuch search --query-syntax=sexp '('"
+
+test_begin_subtest "unknown_prefix"
+notmuch search --query-syntax=sexp '(foo)' >OUTPUT 2>&1
+cat <<EOF > EXPECTED
+notmuch search: Illegal argument for function
+unimplemented prefix foo
+EOF
+test_expect_equal_file EXPECTED OUTPUT
 test_done
-- 
2.30.2