Branko Čibej wrote:
It is a confusing error message, though. The redirect "cycle" is only
Subversion that made it a cycle, and is not the real problem. We could
do better.
We could. Not canonicalizing the value of the Location header is an
option; if it's not a valid URL, that's the server admin's problem, not
ours (and as noted a number of times in other conversations, Subversion
is not a browser so we don't have to worry about producing user-friendly
URLs).
The attached patch, 'serf-no-canonicalize-redirect-1.patch', stops
canonicalizing the redirect, solving this problem.
I have only tested that it fixes the non-repository case I reported, and
verified that the existing test suite still passes. I haven't tested
what difference it makes to any currently untested forms of redirect
when accessing a repo.
WDYT?
ps. It's my favourite kind of patch: a code reduction/simplification.
- Julian
When following an HTTP redirect, use the Location header URL exactly.
Previously we canonicalized the redirect URL, which could lead to a redirect
loop. Then Subversion would report a redirect loop as the error, potentially
hiding a more interesting error such as when the target is not in fact a
Subversion repository.
A manual test case (on a non-repository):
before:
$ svn ls https://archive.apache.org/dist
Redirecting to URL 'https://archive.apache.org/dist':
Redirecting to URL 'https://archive.apache.org/dist':
svn: E195019: Redirect cycle detected for URL 'https://archive.apache.org/dist'
after:
$ svn ls https://archive.apache.org/dist
Redirecting to URL 'https://archive.apache.org/dist/':
svn: E170013: Unable to connect to a repository at URL 'https://archive.apache.org/dist/'
svn: E175003: The server at 'https://archive.apache.org/dist/' does not support the HTTP/DAV protocol
* subversion/libsvn_ra_serf/options.c
(svn_ra_serf__exchange_capabilities): Don't canonicalize the redirect URL.
* subversion/libsvn_ra_serf/util.c
(response_get_location): Don't canonicalize the redirect URL.
--This line, and those below, will be ignored--
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 1866676)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -572,29 +572,26 @@ svn_ra_serf__exchange_capabilities(svn_r
return svn_error_create(
SVN_ERR_RA_DAV_RESPONSE_HEADER_BADNESS, NULL,
_("Location header not set on redirect response"));
}
else if (svn_path_is_url(opt_ctx->handler->location))
{
- *corrected_url = svn_uri_canonicalize(opt_ctx->handler->location,
- result_pool);
+ *corrected_url = apr_pstrdup(result_pool, opt_ctx->handler->location);
}
else
{
/* RFC1945 and RFC2616 state that the Location header's value
(from whence this CORRECTED_URL comes), if present, must be an
absolute URI. But some Apache versions (those older than 2.2.11,
it seems) transmit only the path portion of the URI.
See issue #3775 for details. */
apr_uri_t corrected_URI = serf_sess->session_url;
corrected_URI.path = (char *)corrected_url;
- *corrected_url = svn_uri_canonicalize(
- apr_uri_unparse(scratch_pool, &corrected_URI, 0),
- result_pool);
+ *corrected_url = apr_uri_unparse(result_pool, &corrected_URI, 0);
}
return SVN_NO_ERROR;
}
else if (opt_ctx->handler->sline.code >= 300
&& opt_ctx->handler->sline.code < 399)
Index: subversion/libsvn_ra_serf/util.c
===================================================================
--- subversion/libsvn_ra_serf/util.c (revision 1866676)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -1113,25 +1113,23 @@ response_get_location(serf_bucket_t *res
status = apr_uri_parse(scratch_pool, base_url, &uri);
if (status != APR_SUCCESS)
return NULL;
/* Replace the path path with what we got */
- uri.path = (char*)svn_urlpath__canonicalize(location, scratch_pool);
+ uri.path = apr_pstrdup(scratch_pool, location);
/* And make APR produce a proper full url for us */
- location = apr_uri_unparse(scratch_pool, &uri, 0);
-
- /* Fall through to ensure our canonicalization rules */
+ return apr_uri_unparse(result_pool, &uri, 0);
}
else if (!svn_path_is_url(location))
{
return NULL; /* Any other formats we should support? */
}
- return svn_uri_canonicalize(location, result_pool);
+ return apr_pstrdup(result_pool, location);
}
/* Implements svn_ra_serf__response_handler_t */
svn_error_t *
svn_ra_serf__expect_empty_body(serf_request_t *request,
