You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Westermann-Clark <dw...@tigris.org> on 2006/07/22 21:23:50 UTC
[PATCH] Issue #660: Add basic support for redirects to ra_dav
Please excuse me if this has been discussed before; I checked the list
archives but didn't find anything.
Issue #660 (ra_dav should have some support for redirects) contains a
patch to provide improved an improved error message as suggested by
Mark Benedetto King:
http://subversion.tigris.org/issues/show_bug.cgi?id=660#desc5
Garrett Rooney subsequently updated the patch to build with neon 0.25
in addition to 0.24. I've attached the updated patch (against trunk)
for review.
[[
Patch by: Daniel Westermann-Clark <dw...@tigris.org>
Garrett Rooney <ro...@tigris.org>
Suggested by: Mark Benedetto King <mb...@tigris.org>
* subversion/include/svn_error_codes.h
Add SVN_ERRDEF for moved/relocated repositories named
SVN_ERR_RA_DAV_RELOCATED
* subversion/libsvn_ra_dav/util.c
(parsed_request): For response status codes 301 and 302, provide an
error message with the new location of the repository, based on the
Location response header.
]]
--
Daniel Westermann-Clark
Re: [PATCH] Issue #660: Add basic support for redirects to ra_dav
Posted by Daniel Rall <dl...@collab.net>.
On Sat, 19 Aug 2006, Daniel Westermann-Clark wrote:
> Apologies for the slow response. I've attempted to take your
> suggestions into account, but the changes are a bit more significant
> this time around. :)
Very nice job, Daniel! From your response to the review, to the log
message, to change itself, fantastic work all around.
> On 2006-08-01 10:07:46 -0700, Daniel Rall wrote:
> > commit.c has the interrogate_for_location() routine for doing this
> > work. Would it be possible to refactor both variations of
> > "Location" header retreival logic into there, and maintain an API
> > which is compliant with Neon 0.25?
>
> I've done a bit of refactoring to svn_ra_dav__request_interrogator
> in order to make it backwards compatible.
>
> The function interrogate_for_location from commit.c has been moved to
> util.c.
>
> The two request interrogator arguments to svn_ra_dav__request_dispatch
> are now part of the function's prototype regardless of Neon version.
> This required making the function interrogate_for_content_type in
> fetch.c backwards compatible also.
>
> > While we don't use ne_free() in other places we access the
> > "Location" header, since we get the data from Neon I think we
> > should. (It's simply #define'd to free() on my Linux box.)
>
> I was unsure about this; I've changed it to ne_free in the latest
> version of the patch.
...
I've made some very minor tweaks, and have committed your patch to
Subversion's trunk in r21442.
Thanks!
- Dan
Re: [PATCH] Issue #660: Add basic support for redirects to ra_dav
Posted by Daniel Westermann-Clark <dw...@tigris.org>.
Apologies for the slow response. I've attempted to take your
suggestions into account, but the changes are a bit more significant
this time around. :)
On 2006-08-01 10:07:46 -0700, Daniel Rall wrote:
> commit.c has the interrogate_for_location() routine for doing this
> work. Would it be possible to refactor both variations of
> "Location" header retreival logic into there, and maintain an API
> which is compliant with Neon 0.25?
I've done a bit of refactoring to svn_ra_dav__request_interrogator
in order to make it backwards compatible.
The function interrogate_for_location from commit.c has been moved to
util.c.
The two request interrogator arguments to svn_ra_dav__request_dispatch
are now part of the function's prototype regardless of Neon version.
This required making the function interrogate_for_content_type in
fetch.c backwards compatible also.
> While we don't use ne_free() in other places we access the
> "Location" header, since we get the data from Neon I think we
> should. (It's simply #define'd to free() on my Linux box.)
I was unsure about this; I've changed it to ne_free in the latest
version of the patch.
[[
Patch by: Daniel Westermann-Clark <dw...@tigris.org>
Suggested by: Mark Benedetto King <mb...@tigris.org>
Greg Stein <gs...@tigris.org>
Reviewed by:
Fix issue #660: Notify ra_dav user of repository redirection.
* subversion/include/svn_error_codes.h
Add SVN_ERRDEF for moved/relocated repositories named
SVN_ERR_RA_DAV_RELOCATED
* subversion/libsvn_ra_dav/ra_dav.h
(interrogate_for_location): Add prototype.
(svn_ra_dav__request_dispatch): Update prototype to accept
interrogator regardless of Neon version.
* subversion/libsvn_ra_dav/commit.c
(interrogate_for_location): Move to util.c.
* subversion/libsvn_ra_dav/fetch.c
(interrogate_for_content_type): Make compatible with Neon < 0.25
using ne_add_response_header_handler.
(custom_get_request): Always pass interrogate_for_content_type when
dispatching the request.
* subversion/libsvn_ra_dav/util.c
(interrogate_for_location): Move from commit.c and make compatible
with Neon < 0.25.
(parsed_request): For response status codes 301 and 302, provide an
error message with the new location of the repository based on the
Location response header.
(svn_ra_dav__request_dispatch): Update prototype to accept
interrogator regardless of Neon version.
]]
--
Daniel Westermann-Clark
Re: [PATCH] Issue #660: Add basic support for redirects to ra_dav
Posted by Daniel Rall <dl...@collab.net>.
I started looking at this last week. Conceptually, it's a good
improvement in behavior. I had a few questions about implementation;
I'll try to re-capture them in-line below...
On Sat, 29 Jul 2006, Julian Foad wrote:
> Ping! Any ra_dav people able to have a look at this?
>
> Daniel Westermann-Clark wrote:
...
> >Issue #660 (ra_dav should have some support for redirects) contains a
> >patch to provide improved an improved error message as suggested by
> >Mark Benedetto King:
> >
> >http://subversion.tigris.org/issues/show_bug.cgi?id=660#desc5
> >
> >Garrett Rooney subsequently updated the patch to build with neon 0.25
> >in addition to 0.24. I've attached the updated patch (against trunk)
> >for review.
...
> >* subversion/include/svn_error_codes.h
> > Add SVN_ERRDEF for moved/relocated repositories named
> > SVN_ERR_RA_DAV_RELOCATED
> >
> >* subversion/libsvn_ra_dav/util.c
> > (parsed_request): For response status codes 301 and 302, provide an
> > error message with the new location of the repository, based on the
> > Location response header.
...
> >Index: subversion/libsvn_ra_dav/util.c
> >===================================================================
> >--- subversion/libsvn_ra_dav/util.c (revision 20827)
> >+++ subversion/libsvn_ra_dav/util.c (working copy)
> >@@ -595,7 +595,7 @@
> > #endif /* ! SVN_NEON_0_25 */
> > int code;
> > int expected_code;
> >- const char *msg;
> >+ const char *msg, *location;
> > spool_reader_baton_t spool_reader_baton;
> > svn_error_t *err = SVN_NO_ERROR;
> > svn_ra_dav__session_t *ras = ne_get_session_private(sess,
> >@@ -711,6 +711,12 @@
> > ne_add_response_body_reader(req, ra_dav_error_accepter,
> > ne_xml_parse_v, error_parser);
> >
> >+#ifndef SVN_NEON_0_25
> >+ /* Grab the Location header from the response if provided */
> >+ ne_add_response_header_handler(req, "Location", ne_duplicate_header,
> >+ &location);
> >+#endif /* SVN_NEON_0_25 */
> >+
We're doing similar operations to retrieve the "Location" header in
commit.c:do_checkout(), which attempt to follow a the value of a
"Location" header in response to a 404.
Odd that we're attempting to follow a 404 instead of 30X...
> > /* run the request and get the resulting status code. */
> > rv = ne_request_dispatch(req);
> >
> >@@ -774,6 +780,32 @@
> > msg = apr_psprintf(pool, _("'%s' path not found"), url);
> > err = svn_error_create(SVN_ERR_RA_DAV_PATH_NOT_FOUND, NULL,
> > msg);
> > }
> >+ else if (code == 301)
> >+ {
> >+#ifdef SVN_NEON_0_25
> >+ location = ne_get_response_header(req, "Location");
> >+#endif /* SVN_NEON_0_25 */
> >+
commit.c has the interrogate_for_location() routine for doing this
work. Would it be possible to refactor both variations of "Location"
header retreival logic into there, and maintain an API which is
compliant with Neon 0.25?
> >+ msg = apr_psprintf(pool,
> >+ _("Repository moved permanently to '%s'; "
> >+ "please relocate"),
> >+ location);
> >+
> >+ err = svn_error_create(SVN_ERR_RA_DAV_RELOCATED, NULL, msg);
> >+ }
> >+ else if (code == 302)
> >+ {
> >+#ifdef SVN_NEON_0_25
> >+ location = ne_get_response_header(req, "Location");
> >+#endif /* SVN_NEON_0_25 */
> >+
> >+ msg = apr_psprintf(pool,
> >+ _("Repository moved temporarily to '%s'; "
> >+ "consider relocating"),
> >+ location);
> >+
> >+ err = svn_error_create(SVN_ERR_RA_DAV_RELOCATED, NULL, msg);
> >+ }
> > else
> > {
> > msg = apr_psprintf(pool, _("%s of '%s'"), method, url);
> >@@ -815,6 +847,10 @@
> > err = SVN_NO_ERROR;
> >
> > cleanup:
> >+#ifndef SVN_NEON_0_25
> >+ if (location)
> >+ free(location);
> >+#endif /* SVN_NEON_0_25 */
While we don't use ne_free() in other places we access the "Location"
header, since we get the data from Neon I think we should. (It's
simply #define'd to free() on my Linux box.)
> >
> >
> > if (req)
> > ne_request_destroy(req);
> > if (success_parser)
Re: [PATCH] Issue #660: Add basic support for redirects to ra_dav
Posted by Julian Foad <ju...@btopenworld.com>.
Ping! Any ra_dav people able to have a look at this?
Daniel Westermann-Clark wrote:
> Please excuse me if this has been discussed before; I checked the list
> archives but didn't find anything.
>
> Issue #660 (ra_dav should have some support for redirects) contains a
> patch to provide improved an improved error message as suggested by
> Mark Benedetto King:
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=660#desc5
>
> Garrett Rooney subsequently updated the patch to build with neon 0.25
> in addition to 0.24. I've attached the updated patch (against trunk)
> for review.
>
> [[
> Patch by: Daniel Westermann-Clark <dw...@tigris.org>
> Garrett Rooney <ro...@tigris.org>
> Suggested by: Mark Benedetto King <mb...@tigris.org>
>
> * subversion/include/svn_error_codes.h
> Add SVN_ERRDEF for moved/relocated repositories named
> SVN_ERR_RA_DAV_RELOCATED
>
> * subversion/libsvn_ra_dav/util.c
> (parsed_request): For response status codes 301 and 302, provide an
> error message with the new location of the repository, based on the
> Location response header.
> ]]
>
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/include/svn_error_codes.h
> ===================================================================
> --- subversion/include/svn_error_codes.h (revision 20827)
> +++ subversion/include/svn_error_codes.h (working copy)
> @@ -721,6 +721,11 @@
> SVN_ERR_RA_DAV_CATEGORY_START + 10,
> "Unable to extract data from response header")
>
> + /** @since New in 1.5 */
> + SVN_ERRDEF(SVN_ERR_RA_DAV_RELOCATED,
> + SVN_ERR_RA_DAV_CATEGORY_START + 11,
> + "Repository has been moved")
> +
> /* ra_local errors */
>
> SVN_ERRDEF(SVN_ERR_RA_LOCAL_REPOS_NOT_FOUND,
> Index: subversion/libsvn_ra_dav/util.c
> ===================================================================
> --- subversion/libsvn_ra_dav/util.c (revision 20827)
> +++ subversion/libsvn_ra_dav/util.c (working copy)
> @@ -595,7 +595,7 @@
> #endif /* ! SVN_NEON_0_25 */
> int code;
> int expected_code;
> - const char *msg;
> + const char *msg, *location;
> spool_reader_baton_t spool_reader_baton;
> svn_error_t *err = SVN_NO_ERROR;
> svn_ra_dav__session_t *ras = ne_get_session_private(sess,
> @@ -711,6 +711,12 @@
> ne_add_response_body_reader(req, ra_dav_error_accepter,
> ne_xml_parse_v, error_parser);
>
> +#ifndef SVN_NEON_0_25
> + /* Grab the Location header from the response if provided */
> + ne_add_response_header_handler(req, "Location", ne_duplicate_header,
> + &location);
> +#endif /* SVN_NEON_0_25 */
> +
> /* run the request and get the resulting status code. */
> rv = ne_request_dispatch(req);
>
> @@ -774,6 +780,32 @@
> msg = apr_psprintf(pool, _("'%s' path not found"), url);
> err = svn_error_create(SVN_ERR_RA_DAV_PATH_NOT_FOUND, NULL, msg);
> }
> + else if (code == 301)
> + {
> +#ifdef SVN_NEON_0_25
> + location = ne_get_response_header(req, "Location");
> +#endif /* SVN_NEON_0_25 */
> +
> + msg = apr_psprintf(pool,
> + _("Repository moved permanently to '%s'; "
> + "please relocate"),
> + location);
> +
> + err = svn_error_create(SVN_ERR_RA_DAV_RELOCATED, NULL, msg);
> + }
> + else if (code == 302)
> + {
> +#ifdef SVN_NEON_0_25
> + location = ne_get_response_header(req, "Location");
> +#endif /* SVN_NEON_0_25 */
> +
> + msg = apr_psprintf(pool,
> + _("Repository moved temporarily to '%s'; "
> + "consider relocating"),
> + location);
> +
> + err = svn_error_create(SVN_ERR_RA_DAV_RELOCATED, NULL, msg);
> + }
> else
> {
> msg = apr_psprintf(pool, _("%s of '%s'"), method, url);
> @@ -815,6 +847,10 @@
> err = SVN_NO_ERROR;
>
> cleanup:
> +#ifndef SVN_NEON_0_25
> + if (location)
> + free(location);
> +#endif /* SVN_NEON_0_25 */
> if (req)
> ne_request_destroy(req);
> if (success_parser)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org