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/02/14 02:38:14 UTC
Re: svn commit: r35823 - branches/explore-wc/subversion/libsvn_wc
On Wed, Feb 11, 2009 at 22:57, Hyrum K. Wright <hy...@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().
> + *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'"
>...
> + /* 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.
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1154870
Re: svn commit: r35823 - branches/explore-wc/subversion/libsvn_wc
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Feb 17, 2009 at 22:34, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
>
> On Feb 14, 2009, at 4:56 AM, Greg Stein wrote:
>
>> On Sat, Feb 14, 2009 at 06:04, Hyrum K. Wright
>> <hy...@mail.utexas.edu> wrote:
>>>
>>> On Feb 13, 2009, at 8:38 PM, Greg Stein wrote:
>>>>
>>>> On Wed, Feb 11, 2009 at 22:57, Hyrum K. Wright <hy...@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.
>>
>> Feel free to change those two APIs into something that makes more
>> sense (just checked; nothing uses them). In the above case, you *know*
>> that the step should return a row, so step_row() is Goodness. Shoot...
>> if a row isn't returned, you raise an error, which is *exactly* what
>> step_row() does. Just wrapped up into a simple API.
>>
>> For DTML statements, no rows are expected, so step_done() makes sense.
>> That said... we already have an __insert() call, so maybe step_done is
>> bogus, in favor of __delete() and various __create() entry points. But
>> I'd say keep step_done -- we have the __insert() in order to return
>> the rowid. That isn't the case for other DTML statements.
>
> Ah gotcha. I'll take a look at where they can be used.
>
>>> Oh, and svn_sqlite__step_done() finalizes the statement, and we'd rather
>>> have it reset instead.
>>
>> No, it doesn't.
>
> In which case, the docs are lying:
>
> /* Steps the given statement; raises an SVN error (and finalizes the
> statement) if it doesn't return SQLITE_DONE. */
> svn_error_t *
> svn_sqlite__step_done(svn_sqlite__stmt_t *stmt);
Doc strings?!
Okay. Thanks, Grandma.
Cheers,
-Greg "Reads the Code" Stein
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1181350
Re: svn commit: r35823 - branches/explore-wc/subversion/libsvn_wc
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Feb 14, 2009, at 4:56 AM, Greg Stein wrote:
> On Sat, Feb 14, 2009 at 06:04, Hyrum K. Wright
> <hy...@mail.utexas.edu> wrote:
>> On Feb 13, 2009, at 8:38 PM, Greg Stein wrote:
>>> On Wed, Feb 11, 2009 at 22:57, Hyrum K. Wright <hyrum@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.
>
> Feel free to change those two APIs into something that makes more
> sense (just checked; nothing uses them). In the above case, you *know*
> that the step should return a row, so step_row() is Goodness. Shoot...
> if a row isn't returned, you raise an error, which is *exactly* what
> step_row() does. Just wrapped up into a simple API.
>
> For DTML statements, no rows are expected, so step_done() makes sense.
> That said... we already have an __insert() call, so maybe step_done is
> bogus, in favor of __delete() and various __create() entry points. But
> I'd say keep step_done -- we have the __insert() in order to return
> the rowid. That isn't the case for other DTML statements.
Ah gotcha. I'll take a look at where they can be used.
>> Oh, and svn_sqlite__step_done() finalizes the statement, and we'd
>> rather
>> have it reset instead.
>
> No, it doesn't.
In which case, the docs are lying:
/* Steps the given statement; raises an SVN error (and finalizes the
statement) if it doesn't return SQLITE_DONE. */
svn_error_t *
svn_sqlite__step_done(svn_sqlite__stmt_t *stmt);
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1181226
Re: svn commit: r35823 - branches/explore-wc/subversion/libsvn_wc
Posted by Greg Stein <gs...@gmail.com>.
On Sat, Feb 14, 2009 at 06:04, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
> On Feb 13, 2009, at 8:38 PM, Greg Stein wrote:
>> On Wed, Feb 11, 2009 at 22:57, Hyrum K. Wright <hy...@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.
Feel free to change those two APIs into something that makes more
sense (just checked; nothing uses them). In the above case, you *know*
that the step should return a row, so step_row() is Goodness. Shoot...
if a row isn't returned, you raise an error, which is *exactly* what
step_row() does. Just wrapped up into a simple API.
For DTML statements, no rows are expected, so step_done() makes sense.
That said... we already have an __insert() call, so maybe step_done is
bogus, in favor of __delete() and various __create() entry points. But
I'd say keep step_done -- we have the __insert() in order to return
the rowid. That isn't the case for other DTML statements.
> Oh, and svn_sqlite__step_done() finalizes the statement, and we'd rather
> have it reset instead.
No, it doesn't.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1157193
Re: svn commit: r35823 - branches/explore-wc/subversion/libsvn_wc
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Feb 13, 2009, at 8:38 PM, Greg Stein wrote:
> On Wed, Feb 11, 2009 at 22:57, Hyrum K. Wright
> <hy...@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