88/007fc097bc9a4eff936b8deb888ec33831bdd1

   1 Return-Path: <tomi.ollila@iki.fi>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id EF71C431FC4\r
   6         for <notmuch@notmuchmail.org>; Sat, 21 Mar 2015 02:28:06 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 2.438\r
  10 X-Spam-Level: **\r
  11 X-Spam-Status: No, score=2.438 tagged_above=-999 required=5\r
  12         tests=[DNS_FROM_AHBL_RHSBL=2.438] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id zDwMIsiO-siz for <notmuch@notmuchmail.org>;\r
  16         Sat, 21 Mar 2015 02:28:03 -0700 (PDT)\r
  17 Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34])\r
  18         by olra.theworths.org (Postfix) with ESMTP id 74DC2431FC2\r
  19         for <notmuch@notmuchmail.org>; Sat, 21 Mar 2015 02:28:03 -0700 (PDT)\r
  20 Received: from guru.guru-group.fi (localhost [IPv6:::1])\r
  21         by guru.guru-group.fi (Postfix) with ESMTP id A9A6010004A;\r
  22         Sat, 21 Mar 2015 11:27:41 +0200 (EET)\r
  23 From: Tomi Ollila <tomi.ollila@iki.fi>\r
  24 To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org\r
  25 Subject: Re: [Patch v4 4/9] lib: add "verbose" versions\r
  26         of      notmuch_database_{open, create}\r
  27 In-Reply-To: <1426352554-4383-5-git-send-email-david@tethera.net>\r
  28 References: <1426352554-4383-1-git-send-email-david@tethera.net>\r
  29         <1426352554-4383-5-git-send-email-david@tethera.net>\r
  30 User-Agent: Notmuch/0.19+53~gb45d2f9 (http://notmuchmail.org) Emacs/24.3.1\r
  31         (x86_64-unknown-linux-gnu)\r
  32 X-Face: HhBM'cA~<r"^Xv\KRN0P{vn'Y"Kd;zg_y3S[4)KSN~s?O\"QPoL\r
  33         $[Xv_BD:i/F$WiEWax}R(MPS`^UaptOGD`*/=@\1lKoVa9tnrg0TW?"r7aRtgk[F\r
  34         !)g;OY^,BjTbr)Np:%c_o'jj,Z\r
  35 Date: Sat, 21 Mar 2015 11:27:41 +0200\r
  36 Message-ID: <m2619u7l9u.fsf@guru.guru-group.fi>\r
  37 MIME-Version: 1.0\r
  38 Content-Type: text/plain\r
  39 X-BeenThere: notmuch@notmuchmail.org\r
  40 X-Mailman-Version: 2.1.13\r
  41 Precedence: list\r
  42 List-Id: "Use and development of the notmuch mail system."\r
  43         <notmuch.notmuchmail.org>\r
  44 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  45         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  46 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  47 List-Post: <mailto:notmuch@notmuchmail.org>\r
  48 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  49 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  50         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  51 X-List-Received-Date: Sat, 21 Mar 2015 09:28:07 -0000\r
  52 \r
  53 On Sat, Mar 14 2015, David Bremner <david@tethera.net> wrote:\r
  54 \r
  55 > The compatibility wrapper ensures that clients calling\r
  56 > notmuch_database_open will receive consistent output for now.\r
  57 >\r
  58 > The stdargs based infrastucture will be used in following commits for\r
  59 \r
  60 infrastructure :D\r
  61 \r
  62 > a more general logging mechanism.\r
  63 >\r
  64 > The changes to notmuch-{new,search} and test/symbol-test are just to\r
  65 > make the test suite pass.\r
  66 > ---\r
  67 >  lib/database.cc     | 96 ++++++++++++++++++++++++++++++++++++++++++++---------\r
  68 >  lib/notmuch.h       | 21 ++++++++++++\r
  69 >  notmuch-new.c       |  8 +++--\r
  70 >  notmuch-search.c    |  8 +++--\r
  71 >  test/symbol-test.cc |  6 +++-\r
  72 >  5 files changed, 119 insertions(+), 20 deletions(-)\r
  73 >\r
  74 > diff --git a/lib/database.cc b/lib/database.cc\r
  75 > index 3974e2e..b774edc 100644\r
  76 > --- a/lib/database.cc\r
  77 > +++ b/lib/database.cc\r
  78 > @@ -349,6 +349,35 @@ notmuch_status_to_string (notmuch_status_t status)\r
  79 >  }\r
  80 >  \r
  81 >  static void\r
  82 > +vlog_to_string (void *ctx,\r
  83 > +            char **status_string,\r
  84 > +            const char *format,\r
  85 > +            va_list va_args)\r
  86 > +{\r
  87 > +    if (!status_string)\r
  88 > +     return;\r
  89 \r
  90 Noticed just before sending: (! status_string) -- and one more deep down\r
  91 below... \r
  92 \r
  93 > +\r
  94 > +    if (*status_string)\r
  95 > +     talloc_free (*status_string);\r
  96 > +\r
  97 > +    *status_string = talloc_vasprintf (ctx, format, va_args);\r
  98 > +}\r
  99 > +\r
 100 > +static void\r
 101 > +log_to_string (char **str,\r
 102 > +            const char *format,\r
 103 > +            ...)\r
 104 > +{\r
 105 > +    va_list va_args;\r
 106 > +\r
 107 > +    va_start (va_args, format);\r
 108 > +\r
 109 > +    vlog_to_string (NULL, str, format, va_args);\r
 110 > +\r
 111 > +    va_end (va_args);\r
 112 > +}\r
 113 \r
 114 \r
 115 To me it looks a bit peculiar log_to_string () not taking `ctx` as first\r
 116 argument. I'd suggest\r
 117 \r
 118 1) add that ctx to first arg and tediously add NULL to all callers\r
 119 \r
 120 or \r
 121 \r
 122 2) rename the function so something less similar and call that instead\r
 123 (even leading _ could "mark" the function to be less generic interface to\r
 124 vlog_to_string) .\r
 125 \r
 126 \r
 127 Otherwise this (and rest of the series) looks pretty good to me.\r
 128 \r
 129 Tomi\r
 130 \r
 131 \r
 132 > +static void\r
 133 >  find_doc_ids_for_term (notmuch_database_t *notmuch,\r
 134 >                      const char *term,\r
 135 >                      Xapian::PostingIterator *begin,\r
 136 > @@ -608,28 +637,37 @@ parse_references (void *ctx,\r
 137 >  notmuch_status_t\r
 138 >  notmuch_database_create (const char *path, notmuch_database_t **database)\r
 139 >  {\r
 140 > +    return notmuch_database_create_verbose (path, database, NULL);\r
 141 > +}\r
 142 > +\r
 143 > +notmuch_status_t\r
 144 > +notmuch_database_create_verbose (const char *path,\r
 145 > +                              notmuch_database_t **database,\r
 146 > +                              char **status_string)\r
 147 > +{\r
 148 >      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;\r
 149 >      notmuch_database_t *notmuch = NULL;\r
 150 >      char *notmuch_path = NULL;\r
 151 > +    char *message = NULL;\r
 152 >      struct stat st;\r
 153 >      int err;\r
 154 >  \r
 155 >      if (path == NULL) {\r
 156 > -     fprintf (stderr, "Error: Cannot create a database for a NULL path.\n");\r
 157 > +     log_to_string (&message, "Error: Cannot create a database for a NULL path.\n");\r
 158 >       status = NOTMUCH_STATUS_NULL_POINTER;\r
 159 >       goto DONE;\r
 160 >      }\r
 161 >  \r
 162 >      err = stat (path, &st);\r
 163 >      if (err) {\r
 164 > -     fprintf (stderr, "Error: Cannot create database at %s: %s.\n",\r
 165 > +     log_to_string (&message, "Error: Cannot create database at %s: %s.\n",\r
 166 >                path, strerror (errno));\r
 167 >       status = NOTMUCH_STATUS_FILE_ERROR;\r
 168 >       goto DONE;\r
 169 >      }\r
 170 >  \r
 171 >      if (! S_ISDIR (st.st_mode)) {\r
 172 > -     fprintf (stderr, "Error: Cannot create database at %s: Not a directory.\n",\r
 173 > +     log_to_string (&message, "Error: Cannot create database at %s: Not a directory.\n",\r
 174 >                path);\r
 175 >       status = NOTMUCH_STATUS_FILE_ERROR;\r
 176 >       goto DONE;\r
 177 > @@ -640,15 +678,15 @@ notmuch_database_create (const char *path, notmuch_database_t **database)\r
 178 >      err = mkdir (notmuch_path, 0755);\r
 179 >  \r
 180 >      if (err) {\r
 181 > -     fprintf (stderr, "Error: Cannot create directory %s: %s.\n",\r
 182 > +     log_to_string (&message, "Error: Cannot create directory %s: %s.\n",\r
 183 >                notmuch_path, strerror (errno));\r
 184 >       status = NOTMUCH_STATUS_FILE_ERROR;\r
 185 >       goto DONE;\r
 186 >      }\r
 187 >  \r
 188 > -    status = notmuch_database_open (path,\r
 189 > -                                 NOTMUCH_DATABASE_MODE_READ_WRITE,\r
 190 > -                                 &notmuch);\r
 191 > +    status = notmuch_database_open_verbose (path,\r
 192 > +                                         NOTMUCH_DATABASE_MODE_READ_WRITE,\r
 193 > +                                         &notmuch, &message);\r
 194 >      if (status)\r
 195 >       goto DONE;\r
 196 >  \r
 197 > @@ -667,6 +705,8 @@ notmuch_database_create (const char *path, notmuch_database_t **database)\r
 198 >      if (notmuch_path)\r
 199 >       talloc_free (notmuch_path);\r
 200 >  \r
 201 > +    if (message)\r
 202 > +     *status_string = strdup(message);\r
 203 \r
 204 strdup (message);\r
 205 \r
 206 >      if (database)\r
 207 >       *database = notmuch;\r
 208 >      else\r
 209 > @@ -767,37 +807,55 @@ notmuch_database_open (const char *path,\r
 210 >                      notmuch_database_mode_t mode,\r
 211 >                      notmuch_database_t **database)\r
 212 >  {\r
 213 > +    char *status_string = NULL;\r
 214 > +    notmuch_status_t status;\r
 215 > +\r
 216 > +    status = notmuch_database_open_verbose(path, mode, database,\r
 217 > +                                        &status_string);\r
 218 > +\r
 219 > +    if (status_string) fputs(status_string, stderr);\r
 220 \r
 221 Also, does this above (and few similar below) match the style elsewhere ?\r
 222 (i personally like it but... `git grep fputs` does not reveal such done before)\r
 223 \r
 224 > +\r
 225 > +    return status;\r
 226 > +}\r
 227 > +\r
 228 > +notmuch_status_t\r
 229 > +notmuch_database_open_verbose (const char *path,\r
 230 > +                            notmuch_database_mode_t mode,\r
 231 > +                            notmuch_database_t **database,\r
 232 > +                            char **status_string)\r
 233 > +{\r
 234 >      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;\r
 235 >      void *local = talloc_new (NULL);\r
 236 >      notmuch_database_t *notmuch = NULL;\r
 237 >      char *notmuch_path, *xapian_path, *incompat_features;\r
 238 > +    char *message = NULL;\r
 239 >      struct stat st;\r
 240 >      int err;\r
 241 >      unsigned int i, version;\r
 242 >      static int initialized = 0;\r
 243 >  \r
 244 >      if (path == NULL) {\r
 245 > -     fprintf (stderr, "Error: Cannot open a database for a NULL path.\n");\r
 246 > +     log_to_string (&message, "Error: Cannot open a database for a NULL path.\n");\r
 247 >       status = NOTMUCH_STATUS_NULL_POINTER;\r
 248 >       goto DONE;\r
 249 >      }\r
 250 >  \r
 251 >      if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {\r
 252 > -     fprintf (stderr, "Out of memory\n");\r
 253 > +     log_to_string (&message, "Out of memory\n");\r
 254 >       status = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 255 >       goto DONE;\r
 256 >      }\r
 257 >  \r
 258 >      err = stat (notmuch_path, &st);\r
 259 >      if (err) {\r
 260 > -     fprintf (stderr, "Error opening database at %s: %s\n",\r
 261 > +     log_to_string (&message, "Error opening database at %s: %s\n",\r
 262 >                notmuch_path, strerror (errno));\r
 263 >       status = NOTMUCH_STATUS_FILE_ERROR;\r
 264 >       goto DONE;\r
 265 >      }\r
 266 >  \r
 267 >      if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {\r
 268 > -     fprintf (stderr, "Out of memory\n");\r
 269 > +     log_to_string (&message, "Out of memory\n");\r
 270 >       status = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 271 >       goto DONE;\r
 272 >      }\r
 273 > @@ -837,7 +895,7 @@ notmuch_database_open (const char *path,\r
 274 >        * means a dramatically incompatible change. */\r
 275 >       version = notmuch_database_get_version (notmuch);\r
 276 >       if (version > NOTMUCH_DATABASE_VERSION) {\r
 277 > -         fprintf (stderr,\r
 278 > +         log_to_string (&message,\r
 279 >                    "Error: Notmuch database at %s\n"\r
 280 >                    "       has a newer database format version (%u) than supported by this\n"\r
 281 >                    "       version of notmuch (%u).\n",\r
 282 > @@ -856,7 +914,7 @@ notmuch_database_open (const char *path,\r
 283 >           version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',\r
 284 >           &incompat_features);\r
 285 >       if (incompat_features) {\r
 286 > -         fprintf (stderr,\r
 287 > +         log_to_string (&message,\r
 288 >                    "Error: Notmuch database at %s\n"\r
 289 >                    "       requires features (%s)\n"\r
 290 >                    "       not supported by this version of notmuch.\n",\r
 291 > @@ -906,7 +964,7 @@ notmuch_database_open (const char *path,\r
 292 >           notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);\r
 293 >       }\r
 294 >      } catch (const Xapian::Error &error) {\r
 295 > -     fprintf (stderr, "A Xapian exception occurred opening database: %s\n",\r
 296 > +     log_to_string (&message, "A Xapian exception occurred opening database: %s\n",\r
 297 >                error.get_msg().c_str());\r
 298 >       notmuch_database_destroy (notmuch);\r
 299 >       notmuch = NULL;\r
 300 > @@ -916,6 +974,9 @@ notmuch_database_open (const char *path,\r
 301 >    DONE:\r
 302 >      talloc_free (local);\r
 303 >  \r
 304 > +    if (status_string && message)\r
 305 > +     *status_string = strdup (message);\r
 306 > +\r
 307 >      if (database)\r
 308 >       *database = notmuch;\r
 309 >      else\r
 310 > @@ -1039,13 +1100,18 @@ notmuch_database_compact (const char *path,\r
 311 >      notmuch_database_t *notmuch = NULL;\r
 312 >      struct stat statbuf;\r
 313 >      notmuch_bool_t keep_backup;\r
 314 > +    char *message = NULL;\r
 315 >  \r
 316 >      local = talloc_new (NULL);\r
 317 >      if (! local)\r
 318 >       return NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 319 >  \r
 320 > -    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);\r
 321 > +    ret = notmuch_database_open_verbose (path,\r
 322 > +                                      NOTMUCH_DATABASE_MODE_READ_WRITE,\r
 323 > +                                      &notmuch,\r
 324 > +                                      &message);\r
 325 >      if (ret) {\r
 326 > +     if (status_cb) status_cb (message, closure);\r
 327 >       goto DONE;\r
 328 >      }\r
 329 >  \r
 330 > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 331 > index f066245..c671d82 100644\r
 332 > --- a/lib/notmuch.h\r
 333 > +++ b/lib/notmuch.h\r
 334 > @@ -230,6 +230,16 @@ notmuch_status_t\r
 335 >  notmuch_database_create (const char *path, notmuch_database_t **database);\r
 336 >  \r
 337 >  /**\r
 338 > + * Like notmuch_database_create, except optionally return an error\r
 339 > + * message. This message is allocated by malloc and should be freed by\r
 340 > + * the caller.\r
 341 > + */\r
 342 > +notmuch_status_t\r
 343 > +notmuch_database_create_verbose (const char *path,\r
 344 > +                              notmuch_database_t **database,\r
 345 > +                              char **error_message);\r
 346 > +\r
 347 > +/**\r
 348 >   * Database open mode for notmuch_database_open.\r
 349 >   */\r
 350 >  typedef enum {\r
 351 > @@ -279,6 +289,17 @@ notmuch_status_t\r
 352 >  notmuch_database_open (const char *path,\r
 353 >                      notmuch_database_mode_t mode,\r
 354 >                      notmuch_database_t **database);\r
 355 > +/**\r
 356 > + * Like notmuch_database_open, except optionally return an error\r
 357 > + * message. This message is allocated by malloc and should be freed by\r
 358 > + * the caller.\r
 359 > + */\r
 360 > +\r
 361 > +notmuch_status_t\r
 362 > +notmuch_database_open_verbose (const char *path,\r
 363 > +                            notmuch_database_mode_t mode,\r
 364 > +                            notmuch_database_t **database,\r
 365 > +                            char **error_message);\r
 366 >  \r
 367 >  /**\r
 368 >   * Commit changes and close the given notmuch database.\r
 369 > diff --git a/notmuch-new.c b/notmuch-new.c\r
 370 > index ddf42c1..93b70bf 100644\r
 371 > --- a/notmuch-new.c\r
 372 > +++ b/notmuch-new.c\r
 373 > @@ -985,9 +985,13 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])\r
 374 >           return EXIT_FAILURE;\r
 375 >       add_files_state.total_files = count;\r
 376 >      } else {\r
 377 > -     if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,\r
 378 > -                                &notmuch))\r
 379 > +     char *status_string = NULL;\r
 380 > +     if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,\r
 381 > +                                        &notmuch, &status_string)) {\r
 382 > +         if (status_string) fputs (status_string, stderr);\r
 383 > +\r
 384 >           return EXIT_FAILURE;\r
 385 > +     }\r
 386 >  \r
 387 >       if (notmuch_database_needs_upgrade (notmuch)) {\r
 388 >           time_t now = time (NULL);\r
 389 > diff --git a/notmuch-search.c b/notmuch-search.c\r
 390 > index a591d45..d012af3 100644\r
 391 > --- a/notmuch-search.c\r
 392 > +++ b/notmuch-search.c\r
 393 > @@ -545,6 +545,7 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar\r
 394 >  {\r
 395 >      char *query_str;\r
 396 >      unsigned int i;\r
 397 > +    char *status_string = NULL;\r
 398 >  \r
 399 >      switch (ctx->format_sel) {\r
 400 >      case NOTMUCH_FORMAT_TEXT:\r
 401 > @@ -570,9 +571,12 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar\r
 402 >  \r
 403 >      notmuch_exit_if_unsupported_format ();\r
 404 >  \r
 405 > -    if (notmuch_database_open (notmuch_config_get_database_path (config),\r
 406 > -                            NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch))\r
 407 > +    if (notmuch_database_open_verbose (\r
 408 > +         notmuch_config_get_database_path (config),\r
 409 > +         NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch, &status_string)) {\r
 410 > +     if (status_string) fputs (status_string, stderr);\r
 411 >       return EXIT_FAILURE;\r
 412 > +    }\r
 413 >  \r
 414 >      query_str = query_string_from_args (ctx->notmuch, argc, argv);\r
 415 >      if (query_str == NULL) {\r
 416 > diff --git a/test/symbol-test.cc b/test/symbol-test.cc\r
 417 > index 3e96c03..9f8eea7 100644\r
 418 > --- a/test/symbol-test.cc\r
 419 > +++ b/test/symbol-test.cc\r
 420 > @@ -5,7 +5,11 @@\r
 421 >  \r
 422 >  int main() {\r
 423 >    notmuch_database_t *notmuch;\r
 424 > -  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);\r
 425 > +  char *message = NULL;\r
 426 > +\r
 427 > +  if (notmuch_database_open_verbose  ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch, &message))\r
 428 > +      if (message) fputs (message, stderr);\r
 429 > +\r
 430 >  \r
 431 >    try {\r
 432 >      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);\r
 433 > -- \r
 434 > 2.1.4\r
 435 >\r
 436 > _______________________________________________\r
 437 > notmuch mailing list\r
 438 > notmuch@notmuchmail.org\r
 439 > http://notmuchmail.org/mailman/listinfo/notmuch\r