Re: PATCH][merge-tracking]Create mergeinfo.db with proper schema in case it does not exist.

Kamesh wrote:

> Create mergeinfo.db with the necessary schema if the db does not
> exist with one.

I'm in favor of this concept.

> * subversion/libsvn_fs_util/merge-info-sqlite-index.c
> (CREATE_MERGEINFO_SCHEMA): New macro.

If the DB doesn't exist, is this be sufficient to create the DB
(empty), then also create the schema in it?

> (svn_fs_merge_info__update_index):
                                       ^
Please start the change log text on the same line as the symbol name.

> Create the default merge-tracking sqlite schema on first failure of sqlite
> operation.
> (svn_fs_merge_info__get_merge_info):
> Create the default merge-tracking sqlite schema on first failure of sqlite
> operation.

Since you say the same thing here, you can condense the change log by
listing both symbol names inside the same set of parenthesis,
separated by commas. Example:

    (svn_fs_merge_info__update_index, svn_fs_merge_info__get_merge_info): ...

In this particular case, your change logs should actually have been a
little different, since you introduced a behavioral difference in the
get_merge_info() function.

Index: subversion/libsvn_fs_util/merge-info-sqlite-index.c
===================================================================
--- subversion/libsvn_fs_util/merge-info-sqlite-index.c (revision 22777)
+++ subversion/libsvn_fs_util/merge-info-sqlite-index.c (working copy)
@@ -96,6 +96,21 @@
   "create index mi_c_revision_idx on mergeinfo_changed (revision);"
   APR_EOL_STR;
 
This doc string could be cleaned up a bit. A few suggestions below,
but it the entire thing could flow a little better.

+/* Create the mergeinfo database with the proper schema if one already does
+ not exist. Sqlite was not friendlier enough to explicitly say that Table

                     "is not friendly enough" "table"

+ does not exist.
+ As per sqlite docs SQLITE_ERROR === "SQL error or missing database".

        "the sqlite docs, SQLITE_ERROR corresponds to "

+ Our queries will never cause SQL error, so we can assume SQLITE_ERROR to be

You hope. What if a well-meaning sysadmin modified our table
structure, or fiddled around in some other fashion? I wouldn't make
such an absolute statement.

+ missing schema. Most likely caused by the newly upgraded repository created
+ originally by merge-tracking unaware svnadmin. */

Nice last comment.

+#define CREATE_MERGEINFO_SCHEMA(x, db) do \

"err" would be a more meaningful name than "x".

+{ \
+ if ((x) == SQLITE_ERROR) \
+ SVN_ERR(util_sqlite_exec(db, SVN_MTD_CREATE_SQL, NULL, NULL)); \
+ else \
+ SQLITE_ERR((x), db); \
+} while (0)
+
 /* Create an sqlite DB for our merge info index under PATH. Use POOL
    for temporary allocations. */
 svn_error_t *
@@ -242,7 +257,7 @@
   deletestring = apr_psprintf(pool,
                               "delete from mergeinfo_changed where revision = %ld;",
                               new_rev);
- SVN_ERR(util_sqlite_exec(db, deletestring, NULL, NULL));
+ CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, deletestring, NULL, NULL, NULL), db);

I guess we don't need this SQL statement to succeed, if the schema
hasn't been created yet.

   deletestring = apr_psprintf(pool,
                               "delete from mergeinfo where revision = %ld;",
                               new_rev);
@@ -502,6 +517,9 @@
   sqlite3_trace (db, sqlite_tracer, db);
 #endif
 
+ CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, "select 1 from mergeinfo",
+ NULL, NULL, NULL),
+ db);

-1 on issuing a "select 1" on this often-invoked code path.

   *mergeinfo = apr_hash_make (pool);
   for (i = 0; i < paths->nelts; i++)
     {