> -----Original Message-----
> From: Philip Martin [mailto:philip.martin_at_wandisco.com]
> Sent: vrijdag 6 mei 2011 13:51
> To: dev_at_subversion.apache.org
> Subject: Re: svn commit: r1100169 -
> /subversion/trunk/subversion/libsvn_wc/adm_ops.c
>
> rhuijben_at_apache.org writes:
>
> > Author: rhuijben
> > Date: Fri May 6 11:21:15 2011
> > New Revision: 1100169
> >
> > URL: http://svn.apache.org/viewvc?rev=1100169&view=rev
> > Log:
> > Following up on r1100163, add a write lock check to the revert handling.
> >
> > * subversion/libsvn_wc/adm_ops.c
> > (new_revert_internal): Expect a write lock on the directory containing
> > this node, which might be the directory itself for the wcroot.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_wc/adm_ops.c
> >
> > Modified: subversion/trunk/subversion/libsvn_wc/adm_ops.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm
> _ops.c?rev=1100169&r1=1100168&r2=1100169&view=diff
> >
> ==========================================================
> ====================
> > --- subversion/trunk/subversion/libsvn_wc/adm_ops.c (original)
> > +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Fri May 6 11:21:15
> 2011
> > @@ -1590,6 +1590,20 @@ new_revert_internal(svn_wc__db_t *db,
> > {
> > SVN_ERR_ASSERT(depth == svn_depth_empty || depth ==
> svn_depth_infinity);
> >
> > + /* We should have a write lock on the parent of local_abspath, except
> > + when local_abspath is the working copy root. */
> > + {
> > + const char *dir_abspath;
> > +
> > + SVN_ERR(svn_wc__db_get_wcroot(&dir_abspath, db, local_abspath,
> > + scratch_pool, scratch_pool));
> > +
> > + if (svn_dirent_is_child(dir_abspath, local_abspath, NULL))
> > + dir_abspath = svn_dirent_dirname(local_abspath, scratch_pool);
> > +
> > + SVN_ERR(svn_wc__write_check(db, dir_abspath, scratch_pool));
> > + }
> > +
> > SVN_ERR(svn_wc__db_op_revert(db, local_abspath, depth,
> > scratch_pool, scratch_pool));
>
> I suppose that is sort of how the checking worked in 1.6, but is it
> right for 1.7? Perhaps 1.7 should be checking for the write lock inside
> the SQLite transaction?
>
> There is the question about depth as well, a recursive revert needs a
> recursive lock.
At least we should check the lock somewhere.
The svn_wc_* functions perform database reads and assume the database still
has that state when we call db_op_*, so the lock must exist before the
checks. But it doesn't really matter where we check them (as long as we do
check them).
Currently I don't think any of the db_op_* functions check for the lock
(except when they remove locks); all the checks are in the higher layers,
just like before.
The new apis provide only fully recursive locks, so this check is 'safe'
when you assume that the function is called by somebody who uses it via a
wc_ctx.
So theoretically we could move the recursive vs nonrecursive lock problems
to deprecated.c
Or we just assume that our caller knows what he is doing and just check in
some places... like we have been doing for +- the past two years.
Bert
Received on 2011-05-06 14:30:46 CEST