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().
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.
$ 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.
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?
Received on 2013-07-09 14:23:26 CEST