You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/05/20 17:11:07 UTC
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.
Modified:
subversion/trunk/subversion/libsvn_wc/adm_crawler.c
Modified: subversion/trunk/subversion/libsvn_wc/adm_crawler.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_crawler.c?rev=946661&r1=946660&r2=946661&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/adm_crawler.c (original)
+++ subversion/trunk/subversion/libsvn_wc/adm_crawler.c Thu May 20 15:11:07 2010
@@ -143,7 +143,7 @@ restore_node(svn_boolean_t *restored,
using DB. In that case send the externals definition and DEPTH to
EXTERNAL_FUNC. Use SCRATCH_POOL for temporary allocations. */
static svn_error_t *
-read_traversal_info(svn_wc__db_t *db,
+read_externals_info(svn_wc__db_t *db,
const char *local_abspath,
svn_wc_external_update_t external_func,
void *external_baton,
@@ -237,7 +237,7 @@ report_revisions_and_depths(svn_wc__db_t
apr_pool_t *scratch_pool)
{
const char *dir_abspath;
- const apr_array_header_t *children;
+ const apr_array_header_t *base_children;
apr_hash_t *dirents;
apr_pool_t *iterpool = svn_pool_create(scratch_pool);
int i;
@@ -249,7 +249,7 @@ report_revisions_and_depths(svn_wc__db_t
notice that we're picking up hidden entries too (read_children never
hides children). */
dir_abspath = svn_dirent_join(anchor_abspath, dir_path, scratch_pool);
- SVN_ERR(svn_wc__db_base_get_children(&children, db, dir_abspath,
+ SVN_ERR(svn_wc__db_base_get_children(&base_children, db, dir_abspath,
scratch_pool, iterpool));
SVN_ERR(svn_io_get_dir_filenames(&dirents, dir_abspath, scratch_pool));
@@ -270,18 +270,52 @@ report_revisions_and_depths(svn_wc__db_t
NULL, db, dir_abspath,
scratch_pool, iterpool));
- /* If "this dir" has "svn:externals" property set on it, store its name
- and depth in traversal_info. */
+ /* If "this dir" has "svn:externals" property set on it,
+ * call the external_func callback. */
if (external_func)
{
- SVN_ERR(read_traversal_info(db, dir_abspath, external_func,
+ const apr_array_header_t *all_children;
+
+ SVN_ERR(read_externals_info(db, dir_abspath, external_func,
external_baton, dir_depth, iterpool));
+
+ /* Also do this for added children. They aren't part of the base
+ * tree so we don't recurse into them. But our caller might still
+ * want to pull externals into them as part of the update operation. */
+ SVN_ERR(svn_wc__db_read_children(&all_children, db, dir_abspath,
+ scratch_pool, iterpool));
+ for (i = 0; i < all_children->nelts; ++i)
+ {
+ const char *child = APR_ARRAY_IDX(all_children, i, const char *);
+ const char *this_abspath;
+ svn_wc__db_status_t this_status;
+ svn_wc__db_kind_t this_kind;
+ svn_depth_t this_depth;
+
+ svn_pool_clear(iterpool);
+
+ this_abspath = svn_dirent_join(dir_abspath, child, iterpool);
+ SVN_ERR(svn_wc__db_read_info(&this_status, &this_kind, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ &this_depth, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL,
+ db, this_abspath, iterpool, iterpool));
+ if (this_kind == svn_wc__db_kind_dir &&
+ this_status == svn_wc__db_status_added)
+ {
+ SVN_ERR(read_externals_info(db, this_abspath, external_func,
+ external_baton, this_depth,
+ iterpool));
+ }
+ }
}
- /* Looping over current directory's SVN entries: */
- for (i = 0; i < children->nelts; ++i)
+
+ /* Looping over current directory's BASE children: */
+ for (i = 0; i < base_children->nelts; ++i)
{
- const char *child = APR_ARRAY_IDX(children, i, const char *);
+ const char *child = APR_ARRAY_IDX(base_children, i, const char *);
const char *this_path, *this_abspath;
const char *this_repos_root_url, *this_repos_relpath;
svn_wc__db_status_t this_status;
Re: svn commit: r946661 -
/subversion/trunk/subversion/libsvn_wc/adm_crawler.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 21, 2010 at 12:54:31PM +0200, Bert Huijben wrote:
> See the relegate_external() test in externals_tests.py for some ideas on how
> to test this.
I'll do that, thanks.
> > > 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).
I should have added "within reasonable limits" to that sentence :)
Though of course people have different ideas about such limits.
> 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)
I like the idea of separating the base table crawl from externals
handling. Since not all external information can be gathered during
this crawl, the crawl currently does double duty.
In 1.6.x these crawls were expensive, so it made sense to do as much
as possible during a single crawl. But in wc-ng we'll eventually be
crawling just database tables so it should be fine.
> > 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.
Right. So I guess you would not mind a fix within the CLI client?
> Did you check all the crawler users (in and outside the Subversion
> codebase).
No.
> Or we make it ignore all added nodes and handle the externals handling in
> libsvn_client via some different api. (or ...)
I'll try this approach.
It makes an 1.6.x backport unlikely though.
It's probably not worth the effort.
Thanks,
Stefan
RE: svn commit: r946661 - /subversion/trunk/subversion/libsvn_wc/adm_crawler.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: vrijdag 21 mei 2010 12:05
> To: Bert Huijben
> Cc: dev@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@apache.org [mailto:stsp@apache.org]
> > > Sent: donderdag 20 mei 2010 17:11
> > > To: commits@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
Re: svn commit: r946661 -
/subversion/trunk/subversion/libsvn_wc/adm_crawler.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 21, 2010 at 11:42:13AM +0200, Bert Huijben wrote:
> > -----Original Message-----
> > From: stsp@apache.org [mailto:stsp@apache.org]
> > Sent: donderdag 20 mei 2010 17:11
> > To: commits@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.
> 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.
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.
> 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.
Thanks,
Stefan
RE: svn commit: r946661 - /subversion/trunk/subversion/libsvn_wc/adm_crawler.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stsp@apache.org [mailto:stsp@apache.org]
> Sent: donderdag 20 mei 2010 17:11
> To: commits@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. (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.
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).
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?)
Looking further in the implementation: Why does the code walk the children of an added noded, but not the children of the children?
Bert