You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/03/14 10:55:23 UTC
Re: svn commit: r36537 - trunk/subversion/libsvn_wc
On Fri, Mar 13, 2009 at 23:02, Hyrum K. Wright <hy...@hyrumwright.org> wrote:
>...
> +++ trunk/subversion/libsvn_wc/entries.c Fri Mar 13 15:02:06 2009 (r36537)
for entries.c, note that we could have a temp function to get an sdb
from the db.
>...
> @@ -1851,6 +1810,21 @@ write_entry(svn_sqlite__db_t *wc_db,
> base_node->changed_author = entry->cmt_author;
>
> SVN_ERR(insert_base_node(wc_db, base_node, scratch_pool));
> +
> + /* We have to insert the lock after the base node, because the node
> + must exist to lookup various bits of repos related information for
> + the abs path. */
> + if (entry->lock_token)
> + {
> + svn_wc__db_lock_t *lock = apr_pcalloc(scratch_pool, sizeof(*lock));
gah. dude. put it on the stack.
>...
> @@ -1981,9 +1963,11 @@ entries_write_body(svn_sqlite__db_t *wc_
>
> /* Write out "this dir" */
> SVN_ERR(fetch_wc_id(&wc_id, wc_db));
> - SVN_ERR(write_entry(wc_db, wc_id, repos_id, repos_root, this_dir,
> - SVN_WC_ENTRY_THIS_DIR, this_dir,
> - scratch_pool));
> + SVN_ERR(write_entry(db, wc_db, wc_id, repos_id, repos_root, this_dir,
> + SVN_WC_ENTRY_THIS_DIR,
> + svn_dirent_join(local_abspath, SVN_WC_ENTRY_THIS_DIR,
> + scratch_pool),
> + this_dir, scratch_pool));
SVN_WC_ENTRY_THIS_DIR is defined as "". no need for the join, and
(really) no need for the dumb symbol. in ancient history, we had a
name like "svn:this_dir" or somesuch when we recorded the item in the
'entries' file. it just doesn't make sense to keep the symbol.
>...
> @@ -2844,14 +2832,17 @@ svn_wc__entries_init(const char *path,
> || depth == svn_depth_immediates
> || depth == svn_depth_infinity);
>
> - /* Check that the entries sqlite database does not yet exist. */
> - SVN_ERR(svn_io_check_path(wc_db_path, &kind, scratch_pool));
> - if (kind != svn_node_none)
> - return svn_error_createf(SVN_ERR_WC_DB_ERROR, NULL,
> - _("Existing sqlite database found at '%s'"),
> - svn_path_local_style(wc_db_path, pool));
> -
> - /* Create the entries database, and start a transaction. */
> + /* ### chicken and egg problem: we don't have an adm_access baton to
> + ### use to open the initial entries database, we we've got to do it
> + ### manually. */
> + SVN_ERR(svn_wc__db_open(&db, svn_wc__db_openmode_readwrite, path,
> + NULL, scratch_pool, scratch_pool));
> +
> + /* Open the sqlite database, and insert the REPOS and WCROOT.
> + ### this is redundant, but we currently need it for the entry
> + ### inserting API. DB and WC_DB should be pointing to the *same*
> + ### sqlite database, and it works fine thanks for sqlite's
> + ### concurrency handling. However, this should eventually disappear. */
> SVN_ERR(svn_sqlite__open(&wc_db, wc_db_path, svn_sqlite__mode_rwcreate,
> statements,
> SVN_WC__VERSION_EXPERIMENTAL, upgrade_sql,
could get this from the db? we could also have something like:
svn_wc__db_construct() to construct a wc for the given directory. (or
do nothing in the future, if it finds a parent directory is already a
wc)
>...
> +++ trunk/subversion/libsvn_wc/wc_db.c Fri Mar 13 15:02:06 2009 (r36537)
>...
> @@ -2657,6 +2663,55 @@ svn_wc__db_global_commit(svn_wc__db_t *d
>
>
> svn_error_t *
> +svn_wc__db_lock_add(svn_wc__db_t *db,
> + const char *local_abspath,
> + const svn_wc__db_lock_t *lock,
> + apr_pool_t *scratch_pool)
> +{
> + svn_wc__db_pdh_t *pdh;
> + const char *local_relpath;
> + svn_sqlite__stmt_t *stmt;
> + const char *repos_root_url;
> + const char *repos_relpath;
> + const char *repos_uuid;
> + apr_int64_t repos_id;
> +
> + SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
> +
> + /* Fetch the repos root and repos uuid from the base node, we we can
> + then create or get the repos id.
> + ### is there a better way to do this? */
> + SVN_ERR(svn_wc__db_base_get_info(NULL, NULL, NULL, &repos_relpath,
> + &repos_root_url, &repos_uuid,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL,
> + db, local_abspath, scratch_pool,
> + scratch_pool));
Yeah, there might be a better way, but this isn't bad. It handles the
case of the row containing the data, or needing to inherit the
information. And it gets the repos_relpath.
> +
> + SVN_ERR(parse_local_abspath(&pdh, &local_relpath, db, local_abspath,
> + svn_sqlite__mode_readwrite,
> + scratch_pool, scratch_pool));
> + SVN_ERR(create_repos_id(&repos_id, repos_root_url, repos_uuid, pdh->sdb,
> + scratch_pool));
These two lines are the best reason to find another way. base_get_info
already does a parse_local_abspath, and we want to share that. Also,
it discovered a repos_id, so we don't want to look it up again.
> + SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->sdb, STMT_INSERT_LOCK));
> + SVN_ERR(svn_sqlite__bind_int64(stmt, 1, repos_id));
> + SVN_ERR(svn_sqlite__bind_text(stmt, 2, repos_relpath));
> + SVN_ERR(svn_sqlite__bind_text(stmt, 3, lock->token));
> +
> + if (lock->owner != NULL)
> + SVN_ERR(svn_sqlite__bind_text(stmt, 4, lock->owner));
> +
> + if (lock->comment != NULL)
> + SVN_ERR(svn_sqlite__bind_text(stmt, 5, lock->comment));
> +
> + SVN_ERR(svn_sqlite__bind_int64(stmt, 6, lock->date));
If the date is not specified (0), then should we avoid binding this
column? Leave it null?
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1321370
Re: svn commit: r36537 - trunk/subversion/libsvn_wc
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 14, 2009, at 5:55 AM, Greg Stein wrote:
> On Fri, Mar 13, 2009 at 23:02, Hyrum K. Wright
> <hy...@hyrumwright.org> wrote:
>> ...
>> +++ trunk/subversion/libsvn_wc/entries.c Fri Mar 13 15:02:06
>> 2009 (r36537)
>
> for entries.c, note that we could have a temp function to get an sdb
> from the db.
I'm going to mess with it a bit more, which will hopefully make this
type of function unneeded. Noted for future reference, thought.
>> ...
>> @@ -1851,6 +1810,21 @@ write_entry(svn_sqlite__db_t *wc_db,
>> base_node->changed_author = entry->cmt_author;
>>
>> SVN_ERR(insert_base_node(wc_db, base_node, scratch_pool));
>> +
>> + /* We have to insert the lock after the base node, because
>> the node
>> + must exist to lookup various bits of repos related
>> information for
>> + the abs path. */
>> + if (entry->lock_token)
>> + {
>> + svn_wc__db_lock_t *lock = apr_pcalloc(scratch_pool,
>> sizeof(*lock));
>
> gah. dude. put it on the stack.
heh. fixed.
>> ...
>> @@ -1981,9 +1963,11 @@ entries_write_body(svn_sqlite__db_t *wc_
>>
>> /* Write out "this dir" */
>> SVN_ERR(fetch_wc_id(&wc_id, wc_db));
>> - SVN_ERR(write_entry(wc_db, wc_id, repos_id, repos_root, this_dir,
>> - SVN_WC_ENTRY_THIS_DIR, this_dir,
>> - scratch_pool));
>> + SVN_ERR(write_entry(db, wc_db, wc_id, repos_id, repos_root,
>> this_dir,
>> + SVN_WC_ENTRY_THIS_DIR,
>> + svn_dirent_join(local_abspath,
>> SVN_WC_ENTRY_THIS_DIR,
>> + scratch_pool),
>> + this_dir, scratch_pool));
>
> SVN_WC_ENTRY_THIS_DIR is defined as "". no need for the join, and
> (really) no need for the dumb symbol. in ancient history, we had a
> name like "svn:this_dir" or somesuch when we recorded the item in the
> 'entries' file. it just doesn't make sense to keep the symbol.
>
>> ...
>> @@ -2844,14 +2832,17 @@ svn_wc__entries_init(const char *path,
>> || depth == svn_depth_immediates
>> || depth == svn_depth_infinity);
>>
>> - /* Check that the entries sqlite database does not yet exist. */
>> - SVN_ERR(svn_io_check_path(wc_db_path, &kind, scratch_pool));
>> - if (kind != svn_node_none)
>> - return svn_error_createf(SVN_ERR_WC_DB_ERROR, NULL,
>> - _("Existing sqlite database found at
>> '%s'"),
>> - svn_path_local_style(wc_db_path,
>> pool));
>> -
>> - /* Create the entries database, and start a transaction. */
>> + /* ### chicken and egg problem: we don't have an adm_access
>> baton to
>> + ### use to open the initial entries database, we we've got to
>> do it
>> + ### manually. */
>> + SVN_ERR(svn_wc__db_open(&db, svn_wc__db_openmode_readwrite, path,
>> + NULL, scratch_pool, scratch_pool));
>> +
>> + /* Open the sqlite database, and insert the REPOS and WCROOT.
>> + ### this is redundant, but we currently need it for the entry
>> + ### inserting API. DB and WC_DB should be pointing to the
>> *same*
>> + ### sqlite database, and it works fine thanks for sqlite's
>> + ### concurrency handling. However, this should eventually
>> disappear. */
>> SVN_ERR(svn_sqlite__open(&wc_db, wc_db_path,
>> svn_sqlite__mode_rwcreate,
>> statements,
>> SVN_WC__VERSION_EXPERIMENTAL, upgrade_sql,
>
> could get this from the db? we could also have something like:
> svn_wc__db_construct() to construct a wc for the given directory. (or
> do nothing in the future, if it finds a parent directory is already a
> wc)
I don't know about getting it from the db, but I found myself wanting
a "initialize the db for this directory" API when writing this. I'd
like to finish the other work I have on converting entries.c to use
the wc_db APIs, and then come back to see if this is needed.
>> ...
>> +++ trunk/subversion/libsvn_wc/wc_db.c Fri Mar 13 15:02:06
>> 2009 (r36537)
>> ...
>> @@ -2657,6 +2663,55 @@ svn_wc__db_global_commit(svn_wc__db_t *d
>>
>>
>> svn_error_t *
>> +svn_wc__db_lock_add(svn_wc__db_t *db,
>> + const char *local_abspath,
>> + const svn_wc__db_lock_t *lock,
>> + apr_pool_t *scratch_pool)
>> +{
>> + svn_wc__db_pdh_t *pdh;
>> + const char *local_relpath;
>> + svn_sqlite__stmt_t *stmt;
>> + const char *repos_root_url;
>> + const char *repos_relpath;
>> + const char *repos_uuid;
>> + apr_int64_t repos_id;
>> +
>> + SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
>> +
>> + /* Fetch the repos root and repos uuid from the base node, we we
>> can
>> + then create or get the repos id.
>> + ### is there a better way to do this? */
>> + SVN_ERR(svn_wc__db_base_get_info(NULL, NULL, NULL, &repos_relpath,
>> + &repos_root_url, &repos_uuid,
>> + NULL, NULL, NULL, NULL, NULL,
>> NULL,
>> + NULL, NULL, NULL,
>> + db, local_abspath, scratch_pool,
>> + scratch_pool));
>
> Yeah, there might be a better way, but this isn't bad. It handles the
> case of the row containing the data, or needing to inherit the
> information. And it gets the repos_relpath.
>
>> +
>> + SVN_ERR(parse_local_abspath(&pdh, &local_relpath, db,
>> local_abspath,
>> + svn_sqlite__mode_readwrite,
>> + scratch_pool, scratch_pool));
>> + SVN_ERR(create_repos_id(&repos_id, repos_root_url, repos_uuid,
>> pdh->sdb,
>> + scratch_pool));
>
> These two lines are the best reason to find another way. base_get_info
> already does a parse_local_abspath, and we want to share that. Also,
> it discovered a repos_id, so we don't want to look it up again.
Yeah, it would definitely save some space. However, we'd need to
either refactor, or end up doing some copy-pasting, and both seem a
bit premature at this point. We can revisit this section when it we
go through an optimization round on the wc_db API.
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->sdb,
>> STMT_INSERT_LOCK));
>> + SVN_ERR(svn_sqlite__bind_int64(stmt, 1, repos_id));
>> + SVN_ERR(svn_sqlite__bind_text(stmt, 2, repos_relpath));
>> + SVN_ERR(svn_sqlite__bind_text(stmt, 3, lock->token));
>> +
>> + if (lock->owner != NULL)
>> + SVN_ERR(svn_sqlite__bind_text(stmt, 4, lock->owner));
>> +
>> + if (lock->comment != NULL)
>> + SVN_ERR(svn_sqlite__bind_text(stmt, 5, lock->comment));
>> +
>> + SVN_ERR(svn_sqlite__bind_int64(stmt, 6, lock->date));
>
> If the date is not specified (0), then should we avoid binding this
> column? Leave it null?
Fixed.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1322950