Re: [PATCH] Issue #660: Add basic support for redirects to ra_dav

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)