You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rd...@apache.org on 2010/03/24 23:39:25 UTC
svn commit: r927211 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Author: rdonch
Date: Wed Mar 24 22:39:24 2010
New Revision: 927211
URL: http://svn.apache.org/viewvc?rev=927211&view=rev
Log:
* subversion/libsvn_wc/wc_db.c:
(svn_wc__db_wclock_set): Pass levels_to_lock to svn_sqlite__bindf
as a 64-bit value, as required by the latter.
Modified:
subversion/trunk/subversion/libsvn_wc/wc_db.c
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=927211&r1=927210&r2=927211&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Wed Mar 24 22:39:24 2010
@@ -6068,7 +6068,7 @@ svn_wc__db_wclock_set(svn_wc__db_t *db,
SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
STMT_INSERT_WC_LOCK));
SVN_ERR(svn_sqlite__bindf(stmt, "isi", pdh->wcroot->wc_id, local_relpath,
- levels_to_lock));
+ (apr_int64_t) levels_to_lock));
err = svn_sqlite__insert(NULL, stmt);
if (err)
return svn_error_createf(SVN_ERR_WC_LOCKED, err,
Re: svn commit: r927211 -
/subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
Greg Stein wrote:
> On Thu, Mar 25, 2010 at 09:06, Julian Foad <ju...@wandisco.com> wrote:
> > Hyrum K. Wright wrote:
> >> On Mar 25, 2010, at 7:06 AM, Philip Martin wrote:
> >> > OK. I've just noticed that every caller of svn_wc__db_wclock_set
> >> > passes zero for levels_to_lock, perhaps it doesn't need to be a
> >> > parameter at all.
> >>
> >> This is here strictly for backward compat. The access batons still
> >> have the notion of 'levels to lock' and we need to ensure that they
> >> still function correctly, even in the new world. 'levels to lock'
> >> should not be exposed through the wc-ng APIs at all: users either get
> >> to lock the entire tree (rooted at some subdir, of course), or none.
> >
> > I added this note of yours in r927378.
> >
> > Also I fixed another svn_sqlite__bindf "i" parameter type mismatch or
> > two in r927344. (Thought: Maybe we should aim to replace
> > svn_sqlite__bindf() or its "i" format with some alternative that's
> > harder to get wrong.)
>
> Feel free to suggest something, but bindf() is damned handy. I don't
> see moving away from that. And adding a second integer type in the
> format strings could simply increase the confusion and binding
> problems.
>
> Note that there are svn_sqlite__bind_$TYPE() functions that can be
> used, whose prototypes automatically correct their args' width.
Yup. I agree it's handy, and I don't have a better idea. Saw the
alternate functions.
- Julian
Re: svn commit: r927211 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 25, 2010 at 09:06, Julian Foad <ju...@wandisco.com> wrote:
> Hyrum K. Wright wrote:
>> On Mar 25, 2010, at 7:06 AM, Philip Martin wrote:
>> > OK. I've just noticed that every caller of svn_wc__db_wclock_set
>> > passes zero for levels_to_lock, perhaps it doesn't need to be a
>> > parameter at all.
>>
>> This is here strictly for backward compat. The access batons still
>> have the notion of 'levels to lock' and we need to ensure that they
>> still function correctly, even in the new world. 'levels to lock'
>> should not be exposed through the wc-ng APIs at all: users either get
>> to lock the entire tree (rooted at some subdir, of course), or none.
>
> I added this note of yours in r927378.
>
> Also I fixed another svn_sqlite__bindf "i" parameter type mismatch or
> two in r927344. (Thought: Maybe we should aim to replace
> svn_sqlite__bindf() or its "i" format with some alternative that's
> harder to get wrong.)
Feel free to suggest something, but bindf() is damned handy. I don't
see moving away from that. And adding a second integer type in the
format strings could simply increase the confusion and binding
problems.
Note that there are svn_sqlite__bind_$TYPE() functions that can be
used, whose prototypes automatically correct their args' width.
Cheers,
-g
Re: svn commit: r927211 -
/subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
Hyrum K. Wright wrote:
> On Mar 25, 2010, at 7:06 AM, Philip Martin wrote:
> > OK. I've just noticed that every caller of svn_wc__db_wclock_set
> > passes zero for levels_to_lock, perhaps it doesn't need to be a
> > parameter at all.
>
> This is here strictly for backward compat. The access batons still
> have the notion of 'levels to lock' and we need to ensure that they
> still function correctly, even in the new world. 'levels to lock'
> should not be exposed through the wc-ng APIs at all: users either get
> to lock the entire tree (rooted at some subdir, of course), or none.
I added this note of yours in r927378.
Also I fixed another svn_sqlite__bindf "i" parameter type mismatch or
two in r927344. (Thought: Maybe we should aim to replace
svn_sqlite__bindf() or its "i" format with some alternative that's
harder to get wrong.)
- Julian
Re: svn commit: r927211 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 25, 2010, at 7:06 AM, Philip Martin wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
>> But that would obscure the reason for it and "int levels_to_lock" is the
>> API idiom throughout Subversion.
>
> OK. I've just noticed that every caller of svn_wc__db_wclock_set
> passes zero for levels_to_lock, perhaps it doesn't need to be a
> parameter at all.
This is here strictly for backward compat. The access batons still have the notion of 'levels to lock' and we need to ensure that they still function correctly, even in the new world. 'levels to lock' should not be exposed through the wc-ng APIs at all: users either get to lock the entire tree (rooted at some subdir, of course), or none.
-Hyrum
Re: svn commit: r927211 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> But that would obscure the reason for it and "int levels_to_lock" is the
> API idiom throughout Subversion.
OK. I've just noticed that every caller of svn_wc__db_wclock_set
passes zero for levels_to_lock, perhaps it doesn't need to be a
parameter at all.
--
Philip
Re: svn commit: r927211 -
/subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
Philip Martin wrote:
> rdonch@apache.org writes:
>
> > Author: rdonch
> > Date: Wed Mar 24 22:39:24 2010
> > New Revision: 927211
> >
> > URL: http://svn.apache.org/viewvc?rev=927211&view=rev
> > Log:
> > * subversion/libsvn_wc/wc_db.c:
> > (svn_wc__db_wclock_set): Pass levels_to_lock to svn_sqlite__bindf
> > as a 64-bit value, as required by the latter.
> >
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_wc/wc_db.c
> >
> > Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=927211&r1=927210&r2=927211&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> > +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Wed Mar 24 22:39:24 2010
> > @@ -6068,7 +6068,7 @@ svn_wc__db_wclock_set(svn_wc__db_t *db,
> > SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
> > STMT_INSERT_WC_LOCK));
> > SVN_ERR(svn_sqlite__bindf(stmt, "isi", pdh->wcroot->wc_id, local_relpath,
> > - levels_to_lock));
> > + (apr_int64_t) levels_to_lock));
> > err = svn_sqlite__insert(NULL, stmt);
> > if (err)
> > return svn_error_createf(SVN_ERR_WC_LOCKED, err,
>
> Make the levels_to_lock parameter in svn_wc__db_wclock_set an
> apr_int64_t and the compiler will automatically convert and the cast
> would be unnecessary.
But that would obscure the reason for it and "int levels_to_lock" is the
API idiom throughout Subversion. Although casts are to be avoided when
possible, it's better to keep this impl detail as an impl detail so it's
clear why it's there.
- Julian
Re: svn commit: r927211 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
rdonch@apache.org writes:
> Author: rdonch
> Date: Wed Mar 24 22:39:24 2010
> New Revision: 927211
>
> URL: http://svn.apache.org/viewvc?rev=927211&view=rev
> Log:
> * subversion/libsvn_wc/wc_db.c:
> (svn_wc__db_wclock_set): Pass levels_to_lock to svn_sqlite__bindf
> as a 64-bit value, as required by the latter.
>
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/wc_db.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=927211&r1=927210&r2=927211&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Wed Mar 24 22:39:24 2010
> @@ -6068,7 +6068,7 @@ svn_wc__db_wclock_set(svn_wc__db_t *db,
> SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
> STMT_INSERT_WC_LOCK));
> SVN_ERR(svn_sqlite__bindf(stmt, "isi", pdh->wcroot->wc_id, local_relpath,
> - levels_to_lock));
> + (apr_int64_t) levels_to_lock));
> err = svn_sqlite__insert(NULL, stmt);
> if (err)
> return svn_error_createf(SVN_ERR_WC_LOCKED, err,
Make the levels_to_lock parameter in svn_wc__db_wclock_set an
apr_int64_t and the compiler will automatically convert and the cast
would be unnecessary.
--
Philip