You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2003/04/02 02:20:53 UTC
Re: svn commit: rev 5516 - in trunk/subversion: include libsvn_client libsvn_ra_dav tests/clients/cmdline tests/clients/cmdline/svntest
On Tue, Apr 01, 2003 at 03:21:55PM -0600, sussman@tigris.org wrote:
>...
> Issue #1090: svn_client_copy(URL, wc) now recognizes different UUIDs.
> If the repository UUIDs are different, then schedule a normal add,
> rather than an add-with-history.
>
> Note that this is the first change which actually *depends* on the
> repository having a fetchable UUID. But this should be in accord with
> our compatibility policy, since we've had repository UUIDs since svn 0.19.
So what happens when you run it against an older repository? For example,
our own svn.collab.net? Graceful failure? Maybe an assumption that the
repositories are the same or different?
While our compat policy defines the *outermost* bounds of compatibility, it
doesn't say "feel free to break things" either. Is there a way to get this
to work acceptably against older repositories without much effort?
>...
> * main.py (copy_repos): new optional 'ignore_uuid' argument, so we can
> pass --ignore-uuid to 'svnadmin load'.
Could you maybe include partial paths to the filenames? It took me "a while"
to figure out which file was changed here. If it had said:
* tests/clients/cmdline/svntest/main.py
Then it would have been a no-brainer.
>...
> +++ trunk/subversion/libsvn_client/copy.c Tue Apr 1 15:21:53 2003
>...
> + /* Get the repository uuid for the *parent* of the destination,
> + since dst_path doesn't actually exist yet. */
How do you know the parent exists? Maybe somebody is copying into a new
directory? It would seem that you'd have to "walk up" the working copy until
you found a directory that had a URL in its entries file.
>...
> +++ trunk/subversion/libsvn_client/ra.c Tue Apr 1 15:21:53 2003
>...
> +svn_error_t *
> +svn_client_uuid_from_url (const char **uuid,
> + const char *url,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_ra_plugin_t *ra_lib;
> + void *ra_baton, *session;
> + apr_pool_t *subpool = svn_pool_create (pool);
Creating a local subpool like this doesn't really mesh with our pool
management policy. There is no iteration occurring here. Normally, we'd
defer this out to the caller to manage.
>...
> +++ trunk/subversion/libsvn_ra_dav/session.c Tue Apr 1 15:21:53 2003
> @@ -598,15 +598,23 @@
> apr_pool_t *pool)
> {
> svn_ra_session_t *ras = session_baton;
> + svn_error_t *err;
>
> if (! ras->uuid)
> {
> - apr_hash_t *props;
> const svn_string_t *value;
Why is the error out at the higher level? It can sit "inside" this block.
> - SVN_ERR(svn_ra_dav__get_dir(ras, "", 0, NULL, NULL, &props, pool));
> - value = apr_hash_get(props, SVN_PROP_ENTRY_UUID, APR_HASH_KEY_STRING);
> + ne_propname uuid_propname = { SVN_DAV_PROP_NS_DAV, "repository-uuid" };
static const! No need to keep rebuilding this on every entry. (not like
that is a big problem, given you're about to hit the network, but above is a
departure from our typical pattern)
>...
> +++ trunk/subversion/tests/clients/cmdline/svntest/main.py Tue Apr 1 15:21:53 2003
> @@ -310,7 +310,7 @@
> chmod_tree(path, 0666, 0666)
>
> # For copying a repository
> -def copy_repos(src_path, dst_path, head_revision):
> +def copy_repos(src_path, dst_path, head_revision, ignore_uuid = 0):
Personally, I don't put spaces around keyword args, but I'm not sure what
the convention is in our suite [without looking].
>...
> @@ -319,6 +319,9 @@
> create_repos(dst_path)
> dump_args = ' dump "' + src_path + '"'
> load_args = ' load "' + dst_path + '"'
> +
> + if (ignore_uuid):
> + load_args = load_args + " --ignore-uuid"
Parentheses in an "if" statement? :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5516 - in trunk/subversion: include libsvn_client libsvn_ra_dav tests/clients/cmdline tests/clients/cmdline/svntest
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> On Tue, Apr 01, 2003 at 03:21:55PM -0600, sussman@tigris.org wrote:
> >...
> > Issue #1090: svn_client_copy(URL, wc) now recognizes different UUIDs.
> > If the repository UUIDs are different, then schedule a normal add,
> > rather than an add-with-history.
> >
> > Note that this is the first change which actually *depends* on the
> > repository having a fetchable UUID. But this should be in accord with
> > our compatibility policy, since we've had repository UUIDs since svn 0.19.
>
> So what happens when you run it against an older repository? For example,
> our own svn.collab.net? Graceful failure? Maybe an assumption that the
> repositories are the same or different?
Heh, you have good precognition. I should have inspected this more
carefully. :-)
svn.collab.net doesn't yet have a repository UUID, so I tried running
$ svn cp http://svn.collab.net/repos/svn/trunk/subversion/include/svn_wc.h .
In the ethereal trace, we do a PROPFIND for the repository-uuid
property of svn_wc.h, and get back a 207 response containing a 404 for
the specific property.
But it seems that in this situation, svn_ra_dav__get_one_prop returns
an svn_string_t of "", rather than NULL, which is what would normally
throw a graceful error ("Repository has no UUID.")
So later on, when repos_to_wc_copy() compares UUIDs of src and dst,
it's comparing two empty strings, so it thinks they're the same. Doh.
I'll fix this bug -- svn_client_uuid_from_url() should do some sanity
checking on the value it returns: the value shouldn't be NULL, nor
should it have a strlen of 0.
> While our compat policy defines the *outermost* bounds of compatibility, it
> doesn't say "feel free to break things" either. Is there a way to get this
> to work acceptably against older repositories without much effort?
If one or both of the repository UUIDs is non-existent, I guess all we
can do is *assume* the UUIDs are different, and schedule an addition
without history. Is that preferable to just throwing an error?
> > * main.py (copy_repos): new optional 'ignore_uuid' argument, so we can
> > pass --ignore-uuid to 'svnadmin load'.
>
> Could you maybe include partial paths to the filenames? It took me "a while"
> to figure out which file was changed here. If it had said:
>
> * tests/clients/cmdline/svntest/main.py
>
> Then it would have been a no-brainer.
Yeah, sorry bout that.
> > +++ trunk/subversion/libsvn_client/copy.c Tue Apr 1 15:21:53 2003
> >...
> > + /* Get the repository uuid for the *parent* of the destination,
> > + since dst_path doesn't actually exist yet. */
>
> How do you know the parent exists? Maybe somebody is copying into a new
> directory? It would seem that you'd have to "walk up" the working copy until
> you found a directory that had a URL in its entries file.
If you create a new wc directory and schedule it for addition, it
still has a URL in its entry, even if the URL doesn't exist yet.
But yes, I'm thinking here that opening an RA session to a fictional
URL is still bound to cause problems: it will probably cause ra_local
and ra_svn to fail immediately, and ra_dav will choke when doing a
PROPFIND on that fictional url.
So I'll do what you say -- walk up the wc until we don't get an error
fetching the UUID.
>
> >...
> > +++ trunk/subversion/libsvn_client/ra.c Tue Apr 1 15:21:53 2003
> >...
> > +svn_error_t *
> > +svn_client_uuid_from_url (const char **uuid,
> > + const char *url,
> > + svn_client_ctx_t *ctx,
> > + apr_pool_t *pool)
> > +{
> > + svn_ra_plugin_t *ra_lib;
> > + void *ra_baton, *session;
> > + apr_pool_t *subpool = svn_pool_create (pool);
>
> Creating a local subpool like this doesn't really mesh with our pool
> management policy. There is no iteration occurring here. Normally, we'd
> defer this out to the caller to manage.
This is one of those annoying times when the caller wants to pass in
*two* pools: one to hold the UUID return value, and one for doing the
temporary RA session in. Should this function take two pools? One
for return data, one for scratchwork?
>
> >...
> > +++ trunk/subversion/libsvn_ra_dav/session.c Tue Apr 1 15:21:53 2003
> > @@ -598,15 +598,23 @@
> > apr_pool_t *pool)
> > {
> > svn_ra_session_t *ras = session_baton;
> > + svn_error_t *err;
> >
> > if (! ras->uuid)
> > {
> > - apr_hash_t *props;
> > const svn_string_t *value;
>
> Why is the error out at the higher level? It can sit "inside" this block.
Surely.
>
> > - SVN_ERR(svn_ra_dav__get_dir(ras, "", 0, NULL, NULL, &props, pool));
> > - value = apr_hash_get(props, SVN_PROP_ENTRY_UUID, APR_HASH_KEY_STRING);
> > + ne_propname uuid_propname = { SVN_DAV_PROP_NS_DAV, "repository-uuid" };
>
> static const! No need to keep rebuilding this on every entry. (not like
> that is a big problem, given you're about to hit the network, but above is a
> departure from our typical pattern)
Will fix.
> > +
> > + if (ignore_uuid):
> > + load_args = load_args + " --ignore-uuid"
>
> Parentheses in an "if" statement? :-)
*Blush*
Fixes coming up soon, thanks.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org