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

diff --git a/88/007fc097bc9a4eff936b8deb888ec33831bdd1 b/88/007fc097bc9a4eff936b8deb888ec33831bdd1
new file mode 100644 (file)
index 0000000..8cc3d81
--- /dev/null
+++ b/88/007fc097bc9a4eff936b8deb888ec33831bdd1
@@ -0,0 +1,439 @@
+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 EF71C431FC4\r
+       for <notmuch@notmuchmail.org>; Sat, 21 Mar 2015 02:28:06 -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 zDwMIsiO-siz for <notmuch@notmuchmail.org>;\r
+       Sat, 21 Mar 2015 02:28:03 -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 74DC2431FC2\r
+       for <notmuch@notmuchmail.org>; Sat, 21 Mar 2015 02:28:03 -0700 (PDT)\r
+Received: from guru.guru-group.fi (localhost [IPv6:::1])\r
+       by guru.guru-group.fi (Postfix) with ESMTP id A9A6010004A;\r
+       Sat, 21 Mar 2015 11:27:41 +0200 (EET)\r
+From: Tomi Ollila <tomi.ollila@iki.fi>\r
+To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org\r
+Subject: Re: [Patch v4 4/9] lib: add "verbose" versions\r
+       of      notmuch_database_{open, create}\r
+In-Reply-To: <1426352554-4383-5-git-send-email-david@tethera.net>\r
+References: <1426352554-4383-1-git-send-email-david@tethera.net>\r
+       <1426352554-4383-5-git-send-email-david@tethera.net>\r
+User-Agent: Notmuch/0.19+53~gb45d2f9 (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: Sat, 21 Mar 2015 11:27:41 +0200\r
+Message-ID: <m2619u7l9u.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: Sat, 21 Mar 2015 09:28:07 -0000\r
+\r
+On Sat, Mar 14 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 stdargs based infrastucture will be used in following commits for\r
+\r
+infrastructure :D\r
+\r
+> a more general logging mechanism.\r
+>\r
+> The changes to notmuch-{new,search} and test/symbol-test are just to\r
+> make the test suite pass.\r
+> ---\r
+>  lib/database.cc     | 96 ++++++++++++++++++++++++++++++++++++++++++++---------\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, 119 insertions(+), 20 deletions(-)\r
+>\r
+> diff --git a/lib/database.cc b/lib/database.cc\r
+> index 3974e2e..b774edc 100644\r
+> --- a/lib/database.cc\r
+> +++ b/lib/database.cc\r
+> @@ -349,6 +349,35 @@ notmuch_status_to_string (notmuch_status_t status)\r
+>  }\r
+>  \r
+>  static void\r
+> +vlog_to_string (void *ctx,\r
+> +           char **status_string,\r
+> +           const char *format,\r
+> +           va_list va_args)\r
+> +{\r
+> +    if (!status_string)\r
+> +    return;\r
+\r
+Noticed just before sending: (! status_string) -- and one more deep down\r
+below... \r
+\r
+> +\r
+> +    if (*status_string)\r
+> +    talloc_free (*status_string);\r
+> +\r
+> +    *status_string = talloc_vasprintf (ctx, format, va_args);\r
+> +}\r
+> +\r
+> +static void\r
+> +log_to_string (char **str,\r
+> +           const char *format,\r
+> +           ...)\r
+> +{\r
+> +    va_list va_args;\r
+> +\r
+> +    va_start (va_args, format);\r
+> +\r
+> +    vlog_to_string (NULL, str, format, va_args);\r
+> +\r
+> +    va_end (va_args);\r
+> +}\r
+\r
+\r
+To me it looks a bit peculiar log_to_string () not taking `ctx` as first\r
+argument. I'd suggest\r
+\r
+1) add that ctx to first arg and tediously add NULL to all callers\r
+\r
+or \r
+\r
+2) rename the function so something less similar and call that instead\r
+(even leading _ could "mark" the function to be less generic interface to\r
+vlog_to_string) .\r
+\r
+\r
+Otherwise this (and rest of the series) looks pretty good to me.\r
+\r
+Tomi\r
+\r
+\r
+> +static void\r
+>  find_doc_ids_for_term (notmuch_database_t *notmuch,\r
+>                     const char *term,\r
+>                     Xapian::PostingIterator *begin,\r
+> @@ -608,28 +637,37 @@ 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
+> +\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
+> +    log_to_string (&message, "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
+> +    log_to_string (&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
+> +    log_to_string (&message, "Error: Cannot create database at %s: Not a directory.\n",\r
+>               path);\r
+>      status = NOTMUCH_STATUS_FILE_ERROR;\r
+>      goto DONE;\r
+> @@ -640,15 +678,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
+> +    log_to_string (&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 +705,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 = strdup(message);\r
+\r
+strdup (message);\r
+\r
+>      if (database)\r
+>      *database = notmuch;\r
+>      else\r
+> @@ -767,37 +807,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
+Also, does this above (and few similar below) match the style elsewhere ?\r
+(i personally like it but... `git grep fputs` does not reveal such done before)\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
+> +    log_to_string (&message, "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
+> +    log_to_string (&message, "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
+> +    log_to_string (&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
+> +    log_to_string (&message, "Out of memory\n");\r
+>      status = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>      goto DONE;\r
+>      }\r
+> @@ -837,7 +895,7 @@ 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
+> +        log_to_string (&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
+> @@ -856,7 +914,7 @@ 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
+> +        log_to_string (&message,\r
+>                   "Error: Notmuch database at %s\n"\r
+>                   "       requires features (%s)\n"\r
+>                   "       not supported by this version of notmuch.\n",\r
+> @@ -906,7 +964,7 @@ 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
+> +    log_to_string (&message, "A Xapian exception occurred opening database: %s\n",\r
+>               error.get_msg().c_str());\r
+>      notmuch_database_destroy (notmuch);\r
+>      notmuch = NULL;\r
+> @@ -916,6 +974,9 @@ notmuch_database_open (const char *path,\r
+>    DONE:\r
+>      talloc_free (local);\r
+>  \r
+> +    if (status_string && message)\r
+> +    *status_string = strdup (message);\r
+> +\r
+>      if (database)\r
+>      *database = notmuch;\r
+>      else\r
+> @@ -1039,13 +1100,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
+> 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
+>          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
+>      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
+>  \r
+>    try {\r
+>      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);\r
+> -- \r
+> 2.1.4\r
+>\r
+> _______________________________________________\r
+> notmuch mailing list\r
+> notmuch@notmuchmail.org\r
+> http://notmuchmail.org/mailman/listinfo/notmuch\r