Re: [Patch v5 4/8] lib: add "verbose" versions of notmuch_database_{open, create}

diff --git a/9a/3e5b22a9c575879a05eb22d0cf66948ea415c0 b/9a/3e5b22a9c575879a05eb22d0cf66948ea415c0
new file mode 100644 (file)
index 0000000..d3792bc
--- /dev/null
+++ b/9a/3e5b22a9c575879a05eb22d0cf66948ea415c0
@@ -0,0 +1,424 @@
+Return-Path: <tomi.ollila@iki.fi>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id C5D92431FC0\r
+       for <notmuch@notmuchmail.org>; Wed, 25 Mar 2015 09:39:49 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: 2.438\r
+X-Spam-Level: **\r
+X-Spam-Status: No, score=2.438 tagged_above=-999 required=5\r
+       tests=[DNS_FROM_AHBL_RHSBL=2.438] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id BgZaEHkKtE2x for <notmuch@notmuchmail.org>;\r
+       Wed, 25 Mar 2015 09:39:46 -0700 (PDT)\r
+Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34])\r
+       by olra.theworths.org (Postfix) with ESMTP id 49D86431FBC\r
+       for <notmuch@notmuchmail.org>; Wed, 25 Mar 2015 09:39:46 -0700 (PDT)\r
+Received: from guru.guru-group.fi (localhost [IPv6:::1])\r
+       by guru.guru-group.fi (Postfix) with ESMTP id B3CA7100086;\r
+       Wed, 25 Mar 2015 18:39:24 +0200 (EET)\r
+From: Tomi Ollila <tomi.ollila@iki.fi>\r
+To: David Bremner <david@tethera.net>, David Bremner <david@tethera.net>,\r
+       notmuch@notmuchmail.org\r
+Subject: Re: [Patch v5 4/8] lib: add "verbose" versions\r
+       of      notmuch_database_{open, create}\r
+In-Reply-To: <1427203451-1540-5-git-send-email-david@tethera.net>\r
+References: <1426352554-4383-10-git-send-email-david@tethera.net>\r
+       <1427203451-1540-1-git-send-email-david@tethera.net>\r
+       <1427203451-1540-5-git-send-email-david@tethera.net>\r
+User-Agent: Notmuch/0.19+92~g402df12 (http://notmuchmail.org) Emacs/24.3.1\r
+       (x86_64-unknown-linux-gnu)\r
+X-Face: HhBM'cA~<r"^Xv\KRN0P{vn'Y"Kd;zg_y3S[4)KSN~s?O\"QPoL\r
+       $[Xv_BD:i/F$WiEWax}R(MPS`^UaptOGD`*/=@\1lKoVa9tnrg0TW?"r7aRtgk[F\r
+       !)g;OY^,BjTbr)Np:%c_o'jj,Z\r
+Date: Wed, 25 Mar 2015 18:39:24 +0200\r
+Message-ID: <m2bnjhuj43.fsf@guru.guru-group.fi>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Wed, 25 Mar 2015 16:39:49 -0000\r
+\r
+On Tue, Mar 24 2015, David Bremner <david@tethera.net> wrote:\r
+\r
+> The compatibility wrapper ensures that clients calling\r
+> notmuch_database_open will receive consistent output for now.\r
+>\r
+> The changes to notmuch-{new,search} and test/symbol-test are just to\r
+> make the test suite pass.\r
+>\r
+> The use of IGNORE_RESULT is justified by two things. 1) I don't know\r
+> what else to do.  2) asprintf guarantees the output string is NULL if\r
+> an error occurs, so at least we are not passing garbage back.\r
+> ---\r
+>  lib/database.cc     | 94 +++++++++++++++++++++++++++++++++++++----------------\r
+>  lib/notmuch.h       | 21 ++++++++++++\r
+>  notmuch-new.c       |  8 +++--\r
+>  notmuch-search.c    |  8 +++--\r
+>  test/symbol-test.cc |  6 +++-\r
+>  5 files changed, 104 insertions(+), 33 deletions(-)\r
+>\r
+> diff --git a/lib/database.cc b/lib/database.cc\r
+> index 3974e2e..36849d7 100644\r
+> --- a/lib/database.cc\r
+> +++ b/lib/database.cc\r
+> @@ -608,29 +608,39 @@ parse_references (void *ctx,\r
+>  notmuch_status_t\r
+>  notmuch_database_create (const char *path, notmuch_database_t **database)\r
+>  {\r
+> +    return notmuch_database_create_verbose (path, database, NULL);\r
+\r
+Hmm, is is so that nothing is printed if creating database fails, should\r
+this provide &message to _verbose function and if message changed, fputs()\r
+it (and then free ())?\r
+\r
+... after looking below -- notmuch_database_open () does it.\r
+\r
+> +}\r
+> +\r
+> +notmuch_status_t\r
+> +notmuch_database_create_verbose (const char *path,\r
+> +                             notmuch_database_t **database,\r
+> +                             char **status_string)\r
+> +{\r
+>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;\r
+>      notmuch_database_t *notmuch = NULL;\r
+>      char *notmuch_path = NULL;\r
+> +    char *message = NULL;\r
+>      struct stat st;\r
+>      int err;\r
+>  \r
+>      if (path == NULL) {\r
+> -    fprintf (stderr, "Error: Cannot create a database for a NULL path.\n");\r
+> +    message = strdup ("Error: Cannot create a database for a NULL path.\n");\r
+>      status = NOTMUCH_STATUS_NULL_POINTER;\r
+>      goto DONE;\r
+>      }\r
+>  \r
+>      err = stat (path, &st);\r
+>      if (err) {\r
+> -    fprintf (stderr, "Error: Cannot create database at %s: %s.\n",\r
+> -             path, strerror (errno));\r
+> +    IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: %s.\n",\r
+> +                            path, strerror (errno)));\r
+>      status = NOTMUCH_STATUS_FILE_ERROR;\r
+>      goto DONE;\r
+>      }\r
+>  \r
+>      if (! S_ISDIR (st.st_mode)) {\r
+> -    fprintf (stderr, "Error: Cannot create database at %s: Not a directory.\n",\r
+> -             path);\r
+> +    IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: "\r
+> +                             "Not a directory.\n",\r
+> +                             path));\r
+>      status = NOTMUCH_STATUS_FILE_ERROR;\r
+>      goto DONE;\r
+>      }\r
+> @@ -640,15 +650,15 @@ notmuch_database_create (const char *path, notmuch_database_t **database)\r
+>      err = mkdir (notmuch_path, 0755);\r
+>  \r
+>      if (err) {\r
+> -    fprintf (stderr, "Error: Cannot create directory %s: %s.\n",\r
+> -             notmuch_path, strerror (errno));\r
+> +    IGNORE_RESULT (asprintf (&message, "Error: Cannot create directory %s: %s.\n",\r
+> +                             notmuch_path, strerror (errno)));\r
+>      status = NOTMUCH_STATUS_FILE_ERROR;\r
+>      goto DONE;\r
+>      }\r
+>  \r
+> -    status = notmuch_database_open (path,\r
+> -                                NOTMUCH_DATABASE_MODE_READ_WRITE,\r
+> -                                &notmuch);\r
+> +    status = notmuch_database_open_verbose (path,\r
+> +                                        NOTMUCH_DATABASE_MODE_READ_WRITE,\r
+> +                                        &notmuch, &message);\r
+>      if (status)\r
+>      goto DONE;\r
+>  \r
+> @@ -667,6 +677,8 @@ notmuch_database_create (const char *path, notmuch_database_t **database)\r
+>      if (notmuch_path)\r
+>      talloc_free (notmuch_path);\r
+>  \r
+> +    if (message)\r
+> +    *status_string = message;\r
+\r
+Hmm, what if status_string == NULL -- do we have a test for this (or am I\r
+missing something ?)\r
+\r
+>      if (database)\r
+>      *database = notmuch;\r
+>      else\r
+> @@ -767,37 +779,55 @@ notmuch_database_open (const char *path,\r
+>                     notmuch_database_mode_t mode,\r
+>                     notmuch_database_t **database)\r
+>  {\r
+> +    char *status_string = NULL;\r
+> +    notmuch_status_t status;\r
+> +\r
+> +    status = notmuch_database_open_verbose(path, mode, database,\r
+> +                                       &status_string);\r
+> +\r
+> +    if (status_string) fputs(status_string, stderr);\r
+\r
+and  free (status_string);\r
+\r
+(and fputs (...) (i.e. space which is in all other hunks in this patch :)\r
+\r
+> +\r
+> +    return status;\r
+> +}\r
+> +\r
+> +notmuch_status_t\r
+> +notmuch_database_open_verbose (const char *path,\r
+> +                           notmuch_database_mode_t mode,\r
+> +                           notmuch_database_t **database,\r
+> +                           char **status_string)\r
+> +{\r
+>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;\r
+>      void *local = talloc_new (NULL);\r
+>      notmuch_database_t *notmuch = NULL;\r
+>      char *notmuch_path, *xapian_path, *incompat_features;\r
+> +    char *message = NULL;\r
+>      struct stat st;\r
+>      int err;\r
+>      unsigned int i, version;\r
+>      static int initialized = 0;\r
+>  \r
+>      if (path == NULL) {\r
+> -    fprintf (stderr, "Error: Cannot open a database for a NULL path.\n");\r
+> +    message = strdup ("Error: Cannot open a database for a NULL path.\n");\r
+>      status = NOTMUCH_STATUS_NULL_POINTER;\r
+>      goto DONE;\r
+>      }\r
+>  \r
+>      if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {\r
+> -    fprintf (stderr, "Out of memory\n");\r
+> +    message = strdup ("Out of memory\n");\r
+>      status = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>      goto DONE;\r
+>      }\r
+>  \r
+>      err = stat (notmuch_path, &st);\r
+>      if (err) {\r
+> -    fprintf (stderr, "Error opening database at %s: %s\n",\r
+> -             notmuch_path, strerror (errno));\r
+> +    IGNORE_RESULT (asprintf (&message, "Error opening database at %s: %s\n",\r
+> +                             notmuch_path, strerror (errno)));\r
+>      status = NOTMUCH_STATUS_FILE_ERROR;\r
+>      goto DONE;\r
+>      }\r
+>  \r
+>      if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {\r
+> -    fprintf (stderr, "Out of memory\n");\r
+> +    message = strdup ("Out of memory\n");\r
+>      status = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>      goto DONE;\r
+>      }\r
+> @@ -837,11 +867,11 @@ notmuch_database_open (const char *path,\r
+>       * means a dramatically incompatible change. */\r
+>      version = notmuch_database_get_version (notmuch);\r
+>      if (version > NOTMUCH_DATABASE_VERSION) {\r
+> -        fprintf (stderr,\r
+> -                 "Error: Notmuch database at %s\n"\r
+> -                 "       has a newer database format version (%u) than supported by this\n"\r
+> -                 "       version of notmuch (%u).\n",\r
+> -                 notmuch_path, version, NOTMUCH_DATABASE_VERSION);\r
+> +        IGNORE_RESULT (asprintf (&message,\r
+> +                  "Error: Notmuch database at %s\n"\r
+> +                  "       has a newer database format version (%u) than supported by this\n"\r
+> +                  "       version of notmuch (%u).\n",\r
+> +                                 notmuch_path, version, NOTMUCH_DATABASE_VERSION));\r
+>          notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;\r
+>          notmuch_database_destroy (notmuch);\r
+>          notmuch = NULL;\r
+> @@ -856,11 +886,11 @@ notmuch_database_open (const char *path,\r
+>          version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',\r
+>          &incompat_features);\r
+>      if (incompat_features) {\r
+> -        fprintf (stderr,\r
+> -                 "Error: Notmuch database at %s\n"\r
+> -                 "       requires features (%s)\n"\r
+> -                 "       not supported by this version of notmuch.\n",\r
+> -                 notmuch_path, incompat_features);\r
+> +        IGNORE_RESULT (asprintf (&message,\r
+> +            "Error: Notmuch database at %s\n"\r
+> +            "       requires features (%s)\n"\r
+> +            "       not supported by this version of notmuch.\n",\r
+> +                                 notmuch_path, incompat_features));\r
+>          notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;\r
+>          notmuch_database_destroy (notmuch);\r
+>          notmuch = NULL;\r
+> @@ -906,8 +936,8 @@ notmuch_database_open (const char *path,\r
+>          notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);\r
+>      }\r
+>      } catch (const Xapian::Error &error) {\r
+> -    fprintf (stderr, "A Xapian exception occurred opening database: %s\n",\r
+> -             error.get_msg().c_str());\r
+> +    IGNORE_RESULT (asprintf (&message, "A Xapian exception occurred opening database: %s\n",\r
+> +                             error.get_msg().c_str()));\r
+>      notmuch_database_destroy (notmuch);\r
+>      notmuch = NULL;\r
+>      status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;\r
+> @@ -916,6 +946,9 @@ notmuch_database_open (const char *path,\r
+>    DONE:\r
+>      talloc_free (local);\r
+>  \r
+> +    if (status_string && message)\r
+> +    *status_string = message;\r
+\r
+here it is !!! \o/ !!! :D\r
+\r
+> +\r
+>      if (database)\r
+>      *database = notmuch;\r
+>      else\r
+> @@ -1039,13 +1072,18 @@ notmuch_database_compact (const char *path,\r
+>      notmuch_database_t *notmuch = NULL;\r
+>      struct stat statbuf;\r
+>      notmuch_bool_t keep_backup;\r
+> +    char *message = NULL;\r
+>  \r
+>      local = talloc_new (NULL);\r
+>      if (! local)\r
+>      return NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>  \r
+> -    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);\r
+> +    ret = notmuch_database_open_verbose (path,\r
+> +                                     NOTMUCH_DATABASE_MODE_READ_WRITE,\r
+> +                                     &notmuch,\r
+> +                                     &message);\r
+>      if (ret) {\r
+> +    if (status_cb) status_cb (message, closure);\r
+>      goto DONE;\r
+>      }\r
+\r
+Is this ever printing the message (if any?)\r
+\r
+>  \r
+> diff --git a/lib/notmuch.h b/lib/notmuch.h\r
+> index f066245..c671d82 100644\r
+> --- a/lib/notmuch.h\r
+> +++ b/lib/notmuch.h\r
+> @@ -230,6 +230,16 @@ notmuch_status_t\r
+>  notmuch_database_create (const char *path, notmuch_database_t **database);\r
+>  \r
+>  /**\r
+> + * Like notmuch_database_create, except optionally return an error\r
+> + * message. This message is allocated by malloc and should be freed by\r
+> + * the caller.\r
+> + */\r
+> +notmuch_status_t\r
+> +notmuch_database_create_verbose (const char *path,\r
+> +                             notmuch_database_t **database,\r
+> +                             char **error_message);\r
+> +\r
+> +/**\r
+>   * Database open mode for notmuch_database_open.\r
+>   */\r
+>  typedef enum {\r
+> @@ -279,6 +289,17 @@ notmuch_status_t\r
+>  notmuch_database_open (const char *path,\r
+>                     notmuch_database_mode_t mode,\r
+>                     notmuch_database_t **database);\r
+> +/**\r
+> + * Like notmuch_database_open, except optionally return an error\r
+> + * message. This message is allocated by malloc and should be freed by\r
+> + * the caller.\r
+> + */\r
+> +\r
+> +notmuch_status_t\r
+> +notmuch_database_open_verbose (const char *path,\r
+> +                           notmuch_database_mode_t mode,\r
+> +                           notmuch_database_t **database,\r
+> +                           char **error_message);\r
+>  \r
+>  /**\r
+>   * Commit changes and close the given notmuch database.\r
+> diff --git a/notmuch-new.c b/notmuch-new.c\r
+> index ddf42c1..93b70bf 100644\r
+> --- a/notmuch-new.c\r
+> +++ b/notmuch-new.c\r
+> @@ -985,9 +985,13 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])\r
+>          return EXIT_FAILURE;\r
+>      add_files_state.total_files = count;\r
+>      } else {\r
+> -    if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,\r
+> -                               &notmuch))\r
+> +    char *status_string = NULL;\r
+> +    if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,\r
+> +                                       &notmuch, &status_string)) {\r
+> +        if (status_string) fputs (status_string, stderr);\r
+\r
+and free (status_string);\r
+\r
+> +\r
+>          return EXIT_FAILURE;\r
+> +    }\r
+>  \r
+>      if (notmuch_database_needs_upgrade (notmuch)) {\r
+>          time_t now = time (NULL);\r
+> diff --git a/notmuch-search.c b/notmuch-search.c\r
+> index a591d45..d012af3 100644\r
+> --- a/notmuch-search.c\r
+> +++ b/notmuch-search.c\r
+> @@ -545,6 +545,7 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar\r
+>  {\r
+>      char *query_str;\r
+>      unsigned int i;\r
+> +    char *status_string = NULL;\r
+>  \r
+>      switch (ctx->format_sel) {\r
+>      case NOTMUCH_FORMAT_TEXT:\r
+> @@ -570,9 +571,12 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar\r
+>  \r
+>      notmuch_exit_if_unsupported_format ();\r
+>  \r
+> -    if (notmuch_database_open (notmuch_config_get_database_path (config),\r
+> -                           NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch))\r
+> +    if (notmuch_database_open_verbose (\r
+> +        notmuch_config_get_database_path (config),\r
+> +        NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch, &status_string)) {\r
+> +    if (status_string) fputs (status_string, stderr);\r
+\r
+diiitto (or do we skip freeing 'small' allocations just before exit)\r
+\r
+>      return EXIT_FAILURE;\r
+> +    }\r
+>  \r
+>      query_str = query_string_from_args (ctx->notmuch, argc, argv);\r
+>      if (query_str == NULL) {\r
+> diff --git a/test/symbol-test.cc b/test/symbol-test.cc\r
+> index 3e96c03..9f8eea7 100644\r
+> --- a/test/symbol-test.cc\r
+> +++ b/test/symbol-test.cc\r
+> @@ -5,7 +5,11 @@\r
+>  \r
+>  int main() {\r
+>    notmuch_database_t *notmuch;\r
+> -  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);\r
+> +  char *message = NULL;\r
+> +\r
+> +  if (notmuch_database_open_verbose  ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch, &message))\r
+> +      if (message) fputs (message, stderr);\r
+\r
+free... (for consistency)\r
+\r
+> +\r
+>  \r
+>    try {\r
+>      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);\r
+> -- \r
+> 2.1.4\r