On Feb 13, 2009, at 8:38 PM, Greg Stein wrote:
> On Wed, Feb 11, 2009 at 22:57, Hyrum K. Wright
> <hyrum_at_hyrumwright.org> wrote:
>> ...
>> +++ branches/explore-wc/subversion/libsvn_wc/entries.c Wed Feb 11
>> 13:57:45 2009 (r35823)
>> ...
>> @@ -1138,6 +1147,20 @@ fetch_actual_nodes(apr_hash_t **nodes,
>> }
>>
>> static svn_error_t *
>> +fetch_wc_id(apr_int64_t *wc_id, svn_sqlite__db_t *wc_db)
>> +{
>> + svn_sqlite__stmt_t *stmt;
>> + svn_boolean_t have_row;
>> +
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, wc_db,
>> STMT_SELECT_WCROOT_NULL));
>> + SVN_ERR(svn_sqlite__step(&have_row, stmt));
>> + if (!have_row)
>> + return svn_error_create(SVN_ERR_WC_DB_ERROR, NULL, _("No WC
>> table entry"));
>
> Simplify: use svn_sqlite__step_row(). And in lots of other places!
>
> And its companion svn_sqlite__step_done().
Maybe I'm obtuse, or maybe it's just late, but the use of those APIs
isn't readily intuitive. It appears that I need to know a priori how
many rows there are to fetch, which isn't the case here.
Oh, and svn_sqlite__step_done() finalizes the statement, and we'd
rather have it reset instead.
>
>> + *wc_id = svn_sqlite__column_int(stmt, 0);
>
> I just looked at that SQL statement, and it tests local_abspath for
> being NULL. But local_abspath is declared as NOT NULL.
>
> Further, looking at the INSERT for that row, the SQL has a value to
> insert, but we bind nothing to it.
>
> I think the right answer is to change the schema, and add a comment
> that NULL means "implied by the location this database in the
> filesystem. e.g. for /some/path/.svn/wc.db, the local_abspath is
> implied as '/some/path'"
I did this on the branch, and merged the change to wc-metadata.sql to
trunk in r35873.
>> ...
>> + /* Also remove from the sqlite database. */
>> + /* Open the wc.db sqlite database. */
>> + SVN_ERR(svn_sqlite__open(&wc_db, db_path(parent_dir,
>> scratch_pool),
>> + svn_sqlite__mode_readwrite, statements,
>> + SVN_WC__VERSION, upgrade_sql,
>> + scratch_pool, scratch_pool));
>
> Note that we "could" cache open databases in the adm_access stuff. But
> I believe that is going away entirely (for API consistency, we'll have
> a placebo adm_access structure). Further, all sqlite db handles will
> be managed (and cached/reused/etc) under the wc_db.h covers.
Agreed. Let's worry about caching sqlite handles once all this moves
behind the wc_db interface.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1155519
Received on 2009-02-14 06:05:07 CET