On Sun, 2010-03-07, dan...@apache.org wrote: > Author: dannas > Date: Sun Mar 7 21:21:58 2010 > New Revision: 920118 > > URL: http://svn.apache.org/viewvc?rev=920118&view=rev > Log: > Fix issue #3483 - extend svn_client_upgrade() to include externals. I've > done the externals upgrading after wc upgrade is finished. In that way > no errors in the externals will affect the wc.
Hi Daniel. A few post-commit review comments. > * subversion/libsvn_client/cleanup.c > (svn_client_upgrade): Get all svn:externals. We need the target_dir so > we have to parse the description. For each target_dir we call > svn_wc_upgrade() which will recursively upgrade the external. > > * subversion/tests/cmdline/upgrade_tests.py > (upgrade_with_externals): New. Checks the format of a 1.6 wc upgraded > to wc-ng. > (test_list): Add upgrade with_externals. > > * subversion/tests/cmdline/upgrade_tests_data/upgrade_with_extenals.tar.bz2 > (...): New. An 1.6 wc with the same structure as those used in > externals_tests.py. > > Approved by: rhuijben > > Added: > > subversion/trunk/subversion/tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2 > (with props) > Modified: > subversion/trunk/subversion/libsvn_client/cleanup.c > subversion/trunk/subversion/tests/cmdline/upgrade_tests.py > > Modified: subversion/trunk/subversion/libsvn_client/cleanup.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/cleanup.c?rev=920118&r1=920117&r2=920118&view=diff > ============================================================================== > --- subversion/trunk/subversion/libsvn_client/cleanup.c (original) > +++ subversion/trunk/subversion/libsvn_client/cleanup.c Sun Mar 7 21:21:58 > 2010 > @@ -34,8 +34,11 @@ > #include "svn_dirent_uri.h" > #include "svn_pools.h" > #include "client.h" > +#include "svn_pools.h" Included twice now. Bert already included it, just above, a few days before :-) > +#include "svn_props.h" > > #include "svn_private_config.h" > +#include "private/svn_wc_private.h" > > > /*** Code. ***/ > @@ -110,6 +113,10 @@ > apr_pool_t *scratch_pool) > { > const char *local_abspath; > + apr_hash_t *externals; > + apr_hash_index_t *hi; > + apr_pool_t *iterpool; > + svn_opt_revision_t rev = {svn_opt_revision_unspecified, {0}}; > struct repos_info_baton info_baton; > info_baton.pool = scratch_pool; > info_baton.ctx = ctx; > @@ -123,5 +130,79 @@ > ctx->notify_func2, ctx->notify_baton2, > scratch_pool)); > > + /* Now it's time to upgrade the externals too. We do it after the wc > + upgrade to avoid that errors in the externals causes the wc upgrade to > + fail. Thanks to caching the performance penalty of walking the wc a > + second time shouldn't be too severe */ > + SVN_ERR(svn_client_propget3(&externals, SVN_PROP_EXTERNALS, path, &rev, > + &rev, NULL, svn_depth_infinity, NULL, ctx, > + scratch_pool)); > + > + iterpool = svn_pool_create(scratch_pool); > + > + for (hi = apr_hash_first(scratch_pool, externals); hi; > + hi = apr_hash_next(hi)) > + { > + const char *key; > + int i; > + apr_ssize_t klen; > + svn_string_t *external_desc; > + apr_array_header_t *externals_p; > + > + svn_pool_clear(iterpool); > + externals_p = apr_array_make(iterpool, 1, > + sizeof(svn_wc_external_item2_t*)); > + > + apr_hash_this(hi, (void*)&key, &klen, NULL); > + > + external_desc = apr_hash_get(externals, key, klen); apr_hash_this() can give you the current item's key and value, so you don't need to look up the value separately: you can just apr_hash_this(hi, &key, NULL, &value). However, I discourage use of apr_hash_this() as it requires (void *) pointers, as you have seen. A neater solution enables the key and value to be assigned straight in to variables of the appropriate type: for (hi = ...) { const char *external_path = svn_apr_hash_index_key(hi); svn_string_t *external_desc = svn_apr_hash_index_value(hi); ... } And by calling variable 'external_path' rather than 'key' you can describe the data it holds. (I see you have an 'external_path' in the inner loop too, so you might want to choose a different name or eliminate the inner one.) > + SVN_ERR(svn_wc_parse_externals_description3(&externals_p, > + svn_dirent_dirname(path, > + iterpool), > + external_desc->data, TRUE, > + iterpool)); > + for (i = 0; i < externals_p->nelts; i++) > + { > + svn_wc_external_item2_t *item; > + const char *external_abspath; > + const char *external_path; > + svn_node_kind_t kind; > + svn_error_t *err; > + > + item = APR_ARRAY_IDX(externals_p, i, svn_wc_external_item2_t*); > + > + /* The key is the path to the dir the svn:externals was set on */ > + external_path = svn_dirent_join(key, item->target_dir, iterpool); > + > + SVN_ERR(svn_dirent_get_absolute(&external_abspath, external_path, > + iterpool)); > + > + /* This is hack. We can only send dirs to svn_wc_upgrade(). This > + way we will get an exception saying that the wc must be > + upgraded if it's a dir. If it's a file then the lookup is done > + in an adm_dir belonging to the real wc and since that was > + updated before the externals no error is returned. */ > + err = svn_wc__node_get_kind(&kind, ctx->wc_ctx, external_abspath, > + FALSE, iterpool); > + > + if (err && err->apr_err == SVN_ERR_WC_UPGRADE_REQUIRED) > + { > + SVN_ERR(svn_wc_upgrade(ctx->wc_ctx, external_abspath, > + fetch_repos_info, &info_baton, > + ctx->cancel_func, ctx->cancel_baton, > + ctx->notify_func2, ctx->notify_baton2, > + iterpool)); > + svn_error_clear(err); > + } [...] - Julian
