Re: [patch] Don't discard sqlite's error code

--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
> To: Julian Foad <julianfoad_at_btopenworld.com>
> Cc: "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>
> Sent: Thursday, 14 February 2013, 18:45
> Subject: Re: [patch] Don't discard sqlite's error code
> 
> Julian Foad wrote on Thu, Feb 14, 2013 at 22:11:13 +0000:
>>  Daniel Shahaf wrote:
>> 
>>  > Julian Foad wrote on Thu, Feb 14, 2013 at 21:17:03 +0000:
>>  >>  Daniel Shahaf wrote:
>>  >>  > @@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t 
> *pool)
>>  >>  >    {
>>  >>  >      int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
>>  >>  >      if (err != SQLITE_OK && err != SQLITE_MISUSE)
>>  >>  > -      return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
>>  >>  > -                              _("Could not configure 
>>  > SQLite"));
>>  >>  > +      return svn_error_createf(SQLITE_ERROR_CODE(err), 
> NULL,
>>  >>  > +                               _("Could not configure 
> SQLite 
>>  > (%d)"), 
>>  >>  > err);
>>  >> 
>>  >>  In cases like this one, it seems we should be using one of the 
> above sqlite 
>>  > -> svn error converters so that we get the full SQLite description, 
> and then 
>>  > wrapping the resulting svn error object with "Could not configure 
> 
>>  > SQLite".
>>  >> 
>>  > 
>>  > As in:
>>  > 
>>  >   return svn_error_quick_wrap(svn_error_create(SQLITE_ERROR_CODE(err),
>>  >                                                NULL, NULL)
>>  >                               _("Could not configure SQLite 
> (%d)"));
>>  > 
>>  > ?
>> 
>>  No.  I mean a chain of two svn error objects, one containing SQLite's 
> expansion of 'err' (including both its number and its message), which 
> you can get from e.g. the SQLITE_ERR() macro; and one containing the wrapper 
> message "Could not configure SQLite".  Such as:
>> 
>>  return svn_error_quick_wrap(SQLITE_ERR(err, db),
>>                              _("Could not configure SQLite"));
>> 
> 
> That won't compile because SQLITE_ERR() is a statement, not an
> expression.
> 
> I suppose I could write a function wrapper around SQLITE_ERR, but maybe
> there's a better option...
Huh... And there's no 'db' here anyway.  I believe, on delving a little deeper, that what we really want is:
[[[
   /* If SQLite has been already initialized, sqlite3_config() returns
      SQLITE_MISUSE. */
   {
     int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
     if (err != SQLITE_OK && err != SQLITE_MISUSE)
-      return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
-                              _("Could not configure SQLite"));
+      return svn_error_quick_wrap(
+               svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
+                                 "sqlite: %s", sqlite3_errstr(err))
+               _("Could not configure SQLite to multi-thread mode"));
   }
   SQLITE_ERR_MSG(sqlite3_initialize(), _("Could not initialize SQLite"));
]]]
The problem is that sqlite3_errstr() seems to be rather recent -- it doesn't exist in my installed version which is sqlite v3.7.7.
Feel free to commit what you have and we could re-visit this part later or never.
- Julian