> -----Original Message-----
> From: Stefan Sperling [mailto:stsp_at_elego.de]
> Sent: vrijdag 21 mei 2010 12:05
> To: Bert Huijben
> Cc: dev_at_subversion.apache.org
> Subject: Re: svn commit: r946661 -
> /subversion/trunk/subversion/libsvn_wc/adm_crawler.c
>
> On Fri, May 21, 2010 at 11:42:13AM +0200, Bert Huijben wrote:
> > > -----Original Message-----
> > > From: stsp_at_apache.org [mailto:stsp_at_apache.org]
> > > Sent: donderdag 20 mei 2010 17:11
> > > To: commits_at_subversion.apache.org
> > > Subject: svn commit: r946661 -
> > > /subversion/trunk/subversion/libsvn_wc/adm_crawler.c
> > >
> > > Author: stsp
> > > Date: Thu May 20 15:11:07 2010
> > > New Revision: 946661
> > >
> > > URL: http://svn.apache.org/viewvc?rev=946661&view=rev
> > > Log:
> > > Fix issue #2267, "support uncommitted svn:externals properties".
> > >
> > > * subversion/libsvn_wc/adm_crawler.c
> > > (read_traversal_info): Rename to ...
> > > (read_externals_info): ... this. We've been using an external_func
> > > callback instead of a traversal info for some time.
> > > (report_revisions_and_depths): Rename local variable CHILDREN to
> > > BASE_CHILDREN for clarity (the children come from the BASE tree).
> > > If the caller provided an EXTERNAL_FUNC callback, check locally
> > > added directories for svn:externals as well and pass them to the
> > > callback. The caller will then pull externals into the added
directory.
> >
> > Hi,
> >
> > I'm not sure if this really fixes this issue for the common use cases.
>
> What are the common use cases you have in mind?
>
> The one I have in mind is where you want to test externals you have
> configured on a locally added directory, before commit.
> I tried the following and could not get svn to misbehave:
>
> - reverting the locally added dir
> - removing the svn:externals propery from the locally added dir
> - removing an external from the svn:externals property
> - adding a new external to the svn:externals property
> - running svn status in all above cases
>
> I could not get svn to behave differently when I tried the
> above on a directory I had already committed.
>
> > (And it introduces libsvn_client specific support for updates in a
> > libsvn_wc common function that is used for more than just this update
> > scenario. E.g. svn status -u).
> >
> > The current code (well; before your patch) applies changes on
> > svn:externals on update. By comparing the old and new versions of the
> > svn:externals property it can add/remove/switch externals.
>
> When does it do that? I cannot get trunk to physically remove an
> external which was removed from svn:external. Here's what I tried:
>
> $ cd svn-sandbox/trunk
> $ ls
> alpha beta epsilon/ gamma/
> $ svn mkdir d
> A d
> $ svn ci -mm
> Adding d
>
> Committed revision 3.
> $ svn ps svn:externals '^/trunk/gamma gamma' d
> property 'svn:externals' set on 'd'
> $ svn up
>
> Fetching external item into 'd/gamma'
> A d/gamma/delta
> Updated external to revision 3.
>
> Updated to revision 3.
> $ svn ci
> Sending d
>
> Committed revision 4.
> $ svn ps svn:externals '^/trunk/epsilon epsilon' d
> property 'svn:externals' set on 'd'
> $ svn up
>
> Fetching external item into 'd/epsilon'
> A d/epsilon/zeta
> Updated external to revision 4.
>
> Updated to revision 4.
> $ svn diff
>
> Property changes on: d
> __________________________________________________________
> _________
> Modified: svn:externals
> ## -1 +1 ##
> -^/trunk/gamma gamma
> +^/trunk/epsilon epsilon
> $ svn ci -mm
> Sending d
>
> Committed revision 5.
> $ svn up
>
> Fetching external item into 'd/epsilon'
> External at revision 5.
>
> At revision 5.
> $ svn st
> ? d/gamma
> X d/epsilon
>
>
> The old external 'gamma' stays behind as a detached WC.
>
> > Your new code can just add externals, and if the directory is later
> > reverted the externals will stay as a detached working copy. (And if a
> > different svn:external value is set for a directory you will get some
> > hard to resolve issues, that you would never get into with just the
> > old code).
>
> The same happens with an already committed directory, see above.
See the relegate_external() test in externals_tests.py for some ideas on how
to test this.
(And I didn't know that we did things like this either... until I tried
refactoring the externals code in libsvn_client for WC-NG)
> > I think fixing issue ##2267 needs a good design instead of a quick and
> > dirty fix in one of the fundamental editor helper functions. (E.g.
> > does this break backwards compatibility of our APIs somewhere?)
>
> I don't know. But we usually don't have APIs with strict promises
> that would forbid svn to do something in addition to what it is
> currently doing.
I don't think we can make update perform a commit (or format your hard
disk).
Yes, we define what an api does and doesn't. And we try not to break our API
users until 2.0.
See the errata directory for a few cases where we did intentionally change
specific behavior.
For WC-NG the crawler can just look at the BASE_NODE table, while it
currently still needs to check some WORKING_NODE information for handling of
missing nodes.
Your change makes it impossible to start ignoring WORKING_NODE, because
libsvn_client seems to need it. (Maybe we should remove the entire externals
reporting from this internal function and handle it in a separate step and
from the deprecated api version)
> Note also that in the issue the reporter says:
> Note that TSVN does fetch the externals in this situation
> (a temp workaround for me at least).
>
> This was in 2005. So I believe that if pulling externals into
> newly added dirs causes real problems, we'd know about them by now.
You changed the crawler; not 'svn update'. TSVN just fixed update.
Did you check all the crawler users (in and outside the Subversion
codebase). The crawler is also an important part of svn st --update and svn
diff (and probably more). I expect that it is safe for most users, because
external users most likely don't use the externals handling.
> > Looking further in the implementation: Why does the code walk the
> children of
> > an added noded, but not the children of the children?
>
> That's a fair point. We should make it crawl the added tree recursively.
Or we make it ignore all added nodes and handle the externals handling in
libsvn_client via some different api. (or ...)
Bert
>
> Thanks,
> Stefan
Received on 2010-05-21 12:55:14 CEST