> -----Original Message-----
> From: Stefan Sperling [mailto:stsp_at_apache.org]
> Sent: dinsdag 9 juli 2013 14:23
> To: Bert Huijben
> Cc: dev_at_subversion.apache.org
> Subject: Re: svn commit: r1501163 - in /subversion/trunk/subversion:
> include/private/svn_wc_private.h libsvn_client/checkout.c
> libsvn_wc/adm_files.c
>
> On Tue, Jul 09, 2013 at 12:39:30PM +0200, Bert Huijben wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bert Huijben [mailto:bert_at_qqmail.nl]
> > > Sent: dinsdag 9 juli 2013 12:26
> > > To: dev_at_subversion.apache.org; stsp_at_apache.org
> > > Subject: RE: svn commit: r1501163 - in /subversion/trunk/subversion:
> > > include/private/svn_wc_private.h libsvn_client/checkout.c
> > > libsvn_wc/adm_files.c
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: stsp_at_apache.org [mailto:stsp_at_apache.org]
> > > > Sent: dinsdag 9 juli 2013 11:35
> > > > To: commits_at_subversion.apache.org
> > > > Subject: svn commit: r1501163 - in /subversion/trunk/subversion:
> > > > include/private/svn_wc_private.h libsvn_client/checkout.c
> > > > libsvn_wc/adm_files.c
> > > >
> > > > Author: stsp
> > > > Date: Tue Jul 9 09:35:12 2013
> > > > New Revision: 1501163
> > > >
> > > > URL: http://svn.apache.org/r1501163
> > > > Log:
> > > > Allow 'svn checkout' to work within a working copy that is locked.
> > > > Fixes a regression from 1.7.
> > > >
> > > > Reported by: Frank Loeffler <knarf_at_cct lsu edu>
> > > > See http://svn.haxx.se/users/archive-2013-07/0066.shtml
> > > >
> > > > * subversion/include/private/svn_wc_private.h
> > > > (svn_wc__init_adm): Declare.
> > > >
> > > > * subversion/libsvn_client/checkout.c
> > > > (initialize_area): Use svn_wc__init_adm() instead of
> > > > svn_wc_ensure_adm4().
> > > > The latter scans upwards for an existing admin area to check for
> existing
> > > > working copies, which we don't need to do when creating a new WC.
> > > >
> > > > * subversion/libsvn_wc/adm_files.c
> > > > (svn_wc__init_adm): New function, a thin wrapper around
init_adm().
> > > > This creates a new admin area at a specified local abspath,
without
> > > > first scanning upwards for an existing admin area. We could also
have
> > > > created svn_wc_ensure_adm5() with a new 'is_checkout' argument,
> but
> > > > we're trying to reduce the public set of libsvn_wc API functions.
> > >
> > > I haven't tested this, but this currently appears to remove the safety
net
> > > around:
> > > $ rm trunk
> > > $ svn co URL trunk
> > > (which would produce an error and now two working copies)
> > >
> > > Or:
> > > $ svn rm trunk
> > > $ svn co URL trunk
> > > (which will now produce two working copies, with the first partially
> > > obstructed)
> > >
> > > Or even:
> > > $ svn up --set-depth excluded trunk
> > > $ svn co URL trunk
> > >
> > > In all these cases 1.6 would have behaved one way, and with single-db
> we
> > > behave in a different way as we don't just attach subdirectories in
the
> parent
> > > wc.db.
> > >
> > > This patch might fix a few use cases, but I don't think it solves a
real
> > > problem... And it might create a whole heap of new proplems
> >
> > And detecting any of these in a user friendly way would require reading
the
> parent working copy...
> >
> > ... but the only reason you added this patch (as far as I can tell) was
> because reading the parent working copy wasn't possible?
> >
> >
> > I think the recommended approach is making sure the parent working copy
> is readable, instead of trying to do things 100% without the parent
working
> copy.
> >
> > Doing things without the parent working copy just gives you all kinds of
> obstruction scenarios where a simple local_abspath doesn't work as that
will
> just describe a path that is now part of two working copies.
> >
> > I don't think want to go back to the access batons/wc.db per directory
> where we could describe these paths that are part of two working copy
> scenarios.
> >
> > Bert
>
> Bert, I think you're missing the point.
>
> All the "scary" obstruction scenarios you are worried about are already
> possible without my patch if the parent working copy has no outstanding
> work queue items. My patch has an effect only in the situation where
> the parent working copy has outstanding work queue items. This is not
> obvious from the diff. It is the result of avoiding the call to
> svn_wc__internal_check_wc() in svn_wc_ensure_adm4().
Yes, but after your patch we cannot fix any of these 'scary' scenarios
because anything that would just touch the parent working copy would cause a
regression of this 'fix'.
> Here's what's happening without my patch. I'm running commands in
> a working copy that is currently being checked out. I've got a
> breakpoint set at svn_wc__db_wcroot_parse_local_abspath in the
> svn client that runs the enclosing checkout operation. The enclosing
> working copy is created bit by bit, and as soon as the parent gets
> oustanding work queue items, the nested checkout stops working.
> But it works up to a point, even with the enclosing WC already locked.
Yes, this behavior is part of the WC-NG design. I think some of these
restrictions can be lifted in 1.8 since we finally converted the remaining
workqueue operations that changed db state.
Before we fixed this the DB state was assumed to be unstable until the
workqueue was empty. (E.g. there could be nodes that should have been
deleted in the db).
We can now assume the db to be always stable (guarded by sqlite
transactions), but some in-wc-state can still be lagging behind until the
workqueue is cleared.
> $ svn st
> ! L .
> $ svn co file:///tmp/svn-sandbox/repos/trunk
> A trunk/alpha
> A trunk/epsilon
> A trunk/epsilon/zeta
> A trunk/beta
> A trunk/gamma
> A trunk/gamma/delta
> Checked out revision 2.
>
> (let the parent checkout proceed a bit but don't let it finish)
>
> $ rm -rf trunk
> $ svn st
> ! L .
> $ svn co file:///tmp/svn-sandbox/repos/trunk
> subversion/svn/checkout-cmd.c:168,
> subversion/libsvn_client/checkout.c:197,
> subversion/libsvn_client/checkout.c:123,
> subversion/libsvn_client/checkout.c:63,
> subversion/libsvn_wc/adm_files.c:516,
> subversion/libsvn_wc/adm_files.c:422,
> subversion/libsvn_wc/lock.c:103,
> subversion/libsvn_wc/wc_db.c:12781,
> subversion/libsvn_wc/wc_db_wcroot.c:694,
> subversion/libsvn_wc/wc_db_wcroot.c:310,
> subversion/libsvn_wc/wc_db_wcroot.c:160:
> (apr_err=SVN_ERR_WC_CLEANUP_REQUIRED)
> svn: E155037: Previous operation has not finished; run 'cleanup' if it was
inter
> rupted
>
> So the success of the checkout operation depends on private state (work
> queue items) of an unrelated enclosing working copy. That is bogus.
The design of WC-NG disagrees...
In the old entries world we were always allowed to read the entries, but the
current wc-ng code was designed to explicitly not allow this.
(If you use exclusive locking things get even worse)
The fact that a 'scheduled for delete' directory no longer lives on disk,
appears to be unknown by the checkout code in libsvn_client. It currently
only checks for pre-existing working copies if the directory exists on disk.
(Different issue, but should also be fixed)
>
> While my patch fixes the bogus error, I'm not saying my patch is perfect.
> What it does is that it skips the format check performed by
> svn_wc_ensure_adm4(), which only works correctly if there is no working
> copy with outstanding work queue items in the way. Perhaps a better fix
> would be applied within svn_wc_ensure_adm4(). But I haven't yet found
> a good way of fixing it without passing down a flag that says "we're
> doing a checkout operation right now" into the WC layer.
>
> Does this make sense?
I think the only fix that really makes sense is lifting the restriction that
you can't read from a wc that has workqueue items left.
We should be able to fix this by moving this check to where we obtain a
wc-write-lock. And at this point we are also free to just run the workqueue,
so the combination should reduce the number of times the user sees errors
quite drastically.
Bert
Received on 2013-07-09 15:56:40 CEST