Author: rhuijben
Date: Sat Jan 19 20:41:47 2013
New Revision: 1435684
URL: http://svn.apache.org/viewvc?rev=1435684&view=rev
Log:
Resolve an ugly regression introduced by the inherited property support.
An inaccessible switched location would make the update run against the
wrong target, producing a nice error instead of running an update.
* subversion/libsvn_client/client.h
Document changed error behavior.
* subversion/libsvn_client/iprops.c
(get_inheritable_props): New function, extracted from
svn_client__get_inheritable_props.
(svn_client__get_inheritable_props): Wrap get_inheritable_props to make
sure this function never returns without trying to restore the ra session.
* subversion/libsvn_client/update.c
(update_internal): Wrap svn_client__get_inheritable_props call with SVN_ERR()
* subversion/tests/cmdline/update_tests.py
(update_removes_switched): Remove XFail marker. Expect the current behavior to
show and verify that the problem identified in this patch is fixed.
Modified:
subversion/trunk/subversion/libsvn_client/client.h
subversion/trunk/subversion/libsvn_client/iprops.c
subversion/trunk/subversion/libsvn_client/update.c
subversion/trunk/subversion/tests/cmdline/update_tests.py
Modified: subversion/trunk/subversion/libsvn_client/client.h
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/client.h?rev=1435684&r1=1435683&r2=1435684&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/client.h (original)
+++ subversion/trunk/subversion/libsvn_client/client.h Sat Jan 19 20:41:47 2013
@@ -673,10 +673,13 @@ svn_client__iprop_relpaths_to_urls(apr_a
If LOCAL_ABSPATH has no base then do nothing.
RA_SESSION should be an open RA session pointing at the URL of PATH,
- or NULL, in which case this function will open its own temporary session.
+ or NULL, in which case this function will use its own temporary session.
Allocate *WCROOT_IPROPS in RESULT_POOL, use SCRATCH_POOL for temporary
allocations.
+
+ If one or more of the paths are not available in the repository at the
+ specified revision, these paths will not be added to the hashtable.
*/
svn_error_t *
svn_client__get_inheritable_props(apr_hash_t **wcroot_iprops,
Modified: subversion/trunk/subversion/libsvn_client/iprops.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/iprops.c?rev=1435684&r1=1435683&r2=1435684&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/iprops.c (original)
+++ subversion/trunk/subversion/libsvn_client/iprops.c Sat Jan 19 20:41:47 2013
@@ -123,53 +123,54 @@ svn_client__iprop_relpaths_to_urls(apr_a
return SVN_NO_ERROR;
}
-svn_error_t *
-svn_client__get_inheritable_props(apr_hash_t **wcroot_iprops,
- const char *local_abspath,
- svn_revnum_t revision,
- svn_depth_t depth,
- svn_ra_session_t *ra_session,
- svn_client_ctx_t *ctx,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool)
+/* The real implementation of svn_client__get_inheritable_props */
+static svn_error_t *
+get_inheritable_props(apr_hash_t **wcroot_iprops,
+ const char *local_abspath,
+ svn_revnum_t revision,
+ svn_depth_t depth,
+ svn_ra_session_t *ra_session,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
+ apr_hash_t *iprop_paths;
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+ apr_pool_t *session_pool = NULL;
*wcroot_iprops = apr_hash_make(result_pool);
+ SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(revision));
+
/* If we don't have a base revision for LOCAL_ABSPATH then it can't
possibly be a working copy root, nor can it contain any WC roots
in the form of switched subtrees. So there is nothing to cache. */
- if (SVN_IS_VALID_REVNUM(revision))
+
+ SVN_ERR(svn_wc__get_cached_iprop_children(&iprop_paths, depth,
+ ctx->wc_ctx, local_abspath,
+ scratch_pool, iterpool));
+
+ /* If we are in the midst of a checkout or an update that is bringing in
+ an external, then svn_wc__get_cached_iprop_children won't return
+ LOCAL_ABSPATH in IPROPS_PATHS because the former has no cached iprops
+ yet. So make sure LOCAL_ABSPATH is present if it's a WC root. */
+ if (!apr_hash_get(iprop_paths, local_abspath, APR_HASH_KEY_STRING))
{
- apr_hash_t *iprop_paths;
- apr_hash_index_t *hi;
- const char *old_session_url = NULL;
- apr_pool_t *iterpool = svn_pool_create(scratch_pool);
-
- SVN_ERR(svn_wc__get_cached_iprop_children(&iprop_paths, depth,
- ctx->wc_ctx, local_abspath,
- scratch_pool, iterpool));
-
- /* If we are in the midst of a checkout or an update that is bringing in
- an external, then svn_wc__get_cached_iprop_children won't return
- LOCAL_ABSPATH in IPROPS_PATHS because the former has no cached iprops
- yet. So make sure LOCAL_ABSPATH is present if it's a WC root. */
- if (!apr_hash_get(iprop_paths, local_abspath, APR_HASH_KEY_STRING))
- {
- svn_boolean_t needs_cached_iprops;
+ svn_boolean_t needs_cached_iprops;
- SVN_ERR(need_to_cache_iprops(&needs_cached_iprops, local_abspath,
- ra_session, ctx, iterpool));
- if (needs_cached_iprops)
- {
- const char *target_abspath = apr_pstrdup(scratch_pool,
- local_abspath);
+ SVN_ERR(need_to_cache_iprops(&needs_cached_iprops, local_abspath,
+ ra_session, ctx, iterpool));
+ if (needs_cached_iprops)
+ {
+ const char *target_abspath = apr_pstrdup(scratch_pool,
+ local_abspath);
- /* As value we set TARGET_ABSPATH, but any string besides ""
- would do */
- apr_hash_set(iprop_paths, target_abspath,
- APR_HASH_KEY_STRING, target_abspath);
- }
+ /* As value we set TARGET_ABSPATH, but any string besides ""
+ would do */
+ apr_hash_set(iprop_paths, target_abspath,
+ APR_HASH_KEY_STRING, target_abspath);
}
+ }
for (hi = apr_hash_first(scratch_pool, iprop_paths);
hi;
@@ -179,6 +180,7 @@ svn_client__get_inheritable_props(apr_ha
const char *child_repos_relpath = svn__apr_hash_index_val(hi);
const char *url;
apr_array_header_t *inherited_props;
+ svn_error_t *err;
svn_pool_clear(iterpool);
@@ -191,38 +193,82 @@ svn_client__get_inheritable_props(apr_ha
SVN_ERR(svn_wc__node_get_url(&url, ctx->wc_ctx, child_abspath,
iterpool, iterpool));
if (ra_session)
- {
- if (old_session_url)
- SVN_ERR(svn_ra_reparent(ra_session, url, scratch_pool));
- else
- SVN_ERR(svn_client__ensure_ra_session_url(&old_session_url,
- ra_session, url,
- scratch_pool));
- }
+ SVN_ERR(svn_ra_reparent(ra_session, url, scratch_pool));
else
{
+ if (! session_pool)
+ session_pool = svn_pool_create(scratch_pool);
+
SVN_ERR(svn_client__open_ra_session_internal(&ra_session,
NULL, url,
NULL, NULL,
FALSE, TRUE,
ctx,
- scratch_pool));
+ session_pool));
+ }
+
+ err = svn_ra_get_inherited_props(ra_session, &inherited_props,
+ "", revision,
+ result_pool, iterpool);
+
+ if (err)
+ {
+ if (err->apr_err != SVN_ERR_FS_NOT_FOUND)
+ return svn_error_trace(err);
+
+ svn_error_clear(err);
+ continue;
}
- SVN_ERR(svn_ra_get_inherited_props(ra_session, &inherited_props,
- "", revision,
- result_pool, iterpool));
apr_hash_set(*wcroot_iprops,
apr_pstrdup(result_pool, child_abspath),
APR_HASH_KEY_STRING,
inherited_props);
}
- if (old_session_url)
- SVN_ERR(svn_ra_reparent(ra_session, old_session_url,
- iterpool));
- svn_pool_destroy(iterpool);
- }
+
+ svn_pool_destroy(iterpool);
+ if (session_pool)
+ svn_pool_destroy(session_pool);
return SVN_NO_ERROR;
+
+}
+
+svn_error_t *
+svn_client__get_inheritable_props(apr_hash_t **wcroot_iprops,
+ const char *local_abspath,
+ svn_revnum_t revision,
+ svn_depth_t depth,
+ svn_ra_session_t *ra_session,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ const char *old_session_url;
+ svn_error_t *err;
+
+ if (!SVN_IS_VALID_REVNUM(revision))
+ return SVN_NO_ERROR;
+
+ if (ra_session)
+ SVN_ERR(svn_ra_get_session_url(ra_session, &old_session_url,
scratch_pool));
+
+ /* We just wrap a simple helper function, as it is to easy to leave the ra
+ session rooted at some wrong path without a wrapper like this.
+
+ During development we had problems where some now deleted switched path
+ made the update try to update to that url instead of the intended url
+ */
+
+ err = get_inheritable_props(wcroot_iprops, local_abspath, revision, depth,
+ ra_session, ctx, result_pool, scratch_pool);
+
+ if (ra_session)
+ {
+ err = svn_error_compose_create(
+ err,
+ svn_ra_reparent(ra_session, old_session_url, scratch_pool));
+ }
+ return svn_error_trace(err);
}
Modified: subversion/trunk/subversion/libsvn_client/update.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/update.c?rev=1435684&r1=1435683&r2=1435684&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/update.c (original)
+++ subversion/trunk/subversion/libsvn_client/update.c Sat Jan 19 20:41:47 2013
@@ -376,23 +376,9 @@ update_internal(svn_revnum_t *result_rev
dfb.target_revision = revnum;
dfb.anchor_url = anchor_loc->url;
- err = svn_client__get_inheritable_props(&wcroot_iprops, local_abspath,
- revnum, depth, ra_session,
- ctx, pool, pool);
-
- /* We might be trying to update to a non-existant path-rev. */
- if (err)
- {
- if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
- {
- svn_error_clear(err);
- err = NULL;
- }
- else
- {
- return svn_error_trace(err);
- }
- }
+ SVN_ERR(svn_client__get_inheritable_props(&wcroot_iprops, local_abspath,
+ revnum, depth, ra_session,
+ ctx, pool, pool));
/* Fetch the update editor. If REVISION is invalid, that's okay;
the RA driver will call editor->set_target_revision later on. */
Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/update_tests.py?rev=1435684&r1=1435683&r2=1435684&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/update_tests.py Sat Jan 19
20:41:47 2013
@@ -6153,7 +6153,6 @@ def break_moved_replaced_dir(sbox):
expected_status.tweak('A/B/E2', moved_from=None)
svntest.actions.run_and_verify_status(wc_dir, expected_status)
-@XFail()
def update_removes_switched(sbox):
"update completely removes switched node"
sbox.build(create_wc = False)
@@ -6168,19 +6167,38 @@ def update_removes_switched(sbox):
svntest.main.run_svn(None, 'rm', sbox.repo_url + '/AA/B', '-m', 'Q')
expected_output = svntest.wc.State(wc_dir, {
+ 'B' : Item(status='D '),
})
expected_status = svntest.wc.State(wc_dir, {
+ 'D' : Item(status=' ', wc_rev='3'),
+ 'D/G' : Item(status=' ', wc_rev='3'),
+ 'D/G/rho' : Item(status=' ', wc_rev='3'),
+ 'D/G/pi' : Item(status=' ', wc_rev='3'),
+ 'D/G/tau' : Item(status=' ', wc_rev='3'),
+ 'D/H' : Item(status=' ', wc_rev='3'),
+ 'D/H/omega' : Item(status=' ', wc_rev='3'),
+ 'D/H/chi' : Item(status=' ', wc_rev='3'),
+ 'D/H/psi' : Item(status=' ', wc_rev='3'),
+ 'D/gamma' : Item(status=' ', wc_rev='3'),
+ 'C' : Item(status=' ', wc_rev='3'),
+ 'mu' : Item(status=' ', wc_rev='3'),
})
- # This update should remove 'A/B', because its in-repository location is
removed
- # ### But somehow this just fails much earlier than in 1.7... Serious
regression???
- # svn: E160005: Target path '/AA/B' does not exist.
- # (This should have been verified at r2, where it DOES exist.
- # It was only deleted at r3.)
+ # Before r<to-be-filled-in> the inherited properties code would try to fetch
+ # inherited properties for ^/AA/B and fail. (2013-01-19)
+ #
+ # The inherited properties fetch code would then bail and forget to reset
+ # the ra-session URL back to its original value.
+ #
+ # After that the update code (which ignored the specific error code) was
+ # continued the update against /AA/B (url of missing switched path)
+ # instead of against A (the working copy url).
+
+ # This update removes 'A/B', since its in-repository location is removed.
svntest.actions.run_and_verify_update(wc_dir,
+ expected_output,
None,
- None,
- None)
+ expected_status)
#expected_output = svntest.wc.State(wc_dir, {
#})
