On Tue, Mar 09, 2010 at 02:19:52PM +0000, Julian Foad wrote:
> 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/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 :-)
Fixed!
> > @@ -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.)
Fixed!
Can this be committed?
[[[
Follow up to r920118.
* subversion/libsvn_client/cleanup.c
(svn_client_upgrade): Use svn__apr_hash_index_{key,val} to avoid
casts. Use a more descriptive variable name for the path holding the
svn:externals declaration.
Suggested by: philipm
julianfoad
]]]
Index: subversion/libsvn_client/cleanup.c
===================================================================
--- subversion/libsvn_client/cleanup.c (revision 922358)
+++ subversion/libsvn_client/cleanup.c (arbetskopia)
@@ -34,7 +34,6 @@
#include "svn_dirent_uri.h"
#include "svn_pools.h"
#include "client.h"
-#include "svn_pools.h"
#include "svn_props.h"
#include "svn_private_config.h"
@@ -143,20 +142,15 @@
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;
+ const char *externals_parent = svn__apr_hash_index_key(hi);
+ svn_string_t *external_desc = svn__apr_hash_index_val(hi);
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);
-
SVN_ERR(svn_wc_parse_externals_description3(&externals_p,
svn_dirent_dirname(path,
iterpool),
@@ -172,8 +166,8 @@
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);
+ external_path = svn_dirent_join(externals_parent, item->target_dir,
+ iterpool);
SVN_ERR(svn_dirent_get_absolute(&external_abspath, external_path,
iterpool));
