You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/03/12 19:06:36 UTC
[PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in
/subversion/trunk/subversion: libsvn_client/cleanup.c
tests/cmdline/upgrade_tests.py
tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
On Tue, Mar 09, 2010 at 02:19:52PM +0000, Julian Foad wrote:
> On Sun, 2010-03-07, dannas@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
]]]
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in
/subversion/trunk/subversion: libsvn_client/cleanup.c
tests/cmdline/upgrade_tests.py
tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-03-12, Daniel Näslund wrote:
> On Fri, Mar 12, 2010 at 06:15:27PM +0000, Philip Martin wrote:
> > Daniel Näslund <da...@longitudo.com> writes:
> >
> > > 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
> > > ]]]
> >
> > Yes.
>
> Commited in r922387.
Thanks!
- Julian
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in
/subversion/trunk/subversion: libsvn_client/cleanup.c
tests/cmdline/upgrade_tests.py
tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-03-12, Daniel Näslund wrote:
> On Fri, Mar 12, 2010 at 06:15:27PM +0000, Philip Martin wrote:
> > Daniel Näslund <da...@longitudo.com> writes:
> >
> > > 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
> > > ]]]
> >
> > Yes.
>
> Commited in r922387.
Thanks!
- Julian
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in
/subversion/trunk/subversion: libsvn_client/cleanup.c
tests/cmdline/upgrade_tests.py
tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Mar 12, 2010 at 06:15:27PM +0000, Philip Martin wrote:
> Daniel Näslund <da...@longitudo.com> writes:
>
> > 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
> > ]]]
>
> Yes.
Commited in r922387.
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in
/subversion/trunk/subversion: libsvn_client/cleanup.c
tests/cmdline/upgrade_tests.py
tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Mar 12, 2010 at 06:15:27PM +0000, Philip Martin wrote:
> Daniel Näslund <da...@longitudo.com> writes:
>
> > 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
> > ]]]
>
> Yes.
Commited in r922387.
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in
/subversion/trunk/subversion: libsvn_client/cleanup.c
tests/cmdline/upgrade_tests.py
tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Mar 12, 2010 at 06:15:27PM +0000, Philip Martin wrote:
> Daniel Näslund <da...@longitudo.com> writes:
>
> > 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
> > ]]]
>
> Yes.
Commited in r922387.
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in /subversion/trunk/subversion: libsvn_client/cleanup.c tests/cmdline/upgrade_tests.py tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Näslund <da...@longitudo.com> writes:
> 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
> ]]]
Yes.
--
Philip
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in /subversion/trunk/subversion: libsvn_client/cleanup.c tests/cmdline/upgrade_tests.py tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Näslund <da...@longitudo.com> writes:
> 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
> ]]]
Yes.
--
Philip