[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix issue #2116

From: Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
Date: Sun, 8 Jun 2008 21:47:45 +0300 (Jerusalem Daylight Time)

Stefan Sperling wrote on Sun, 8 Jun 2008 at 19:24 +0200:
> Hi,
>
> attached is a series of patches that I believe fix issue #2116
> ('svn log file:///' results in a failed assertion).
>
> Does anyone object to any of these?
>
> Stefan
>

> Index: subversion/tests/libsvn_subr/path-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/path-test.c (revision 31628)
> +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> @@ -736,6 +736,12 @@ test_canonicalize(const char **msg,
> { "http://hst", "http://hst" },
> { "http://hst/foo/../bar","http://hst/foo/../bar" },
> { "http://hst/", "http://hst" },
> + { "http:///", "http://" },
> + { "https://", "https://" },
> + { "file:///", "file://" },
> + { "file://", "file://" },
> + { "svn:///", "svn://" },
> + { "svn+ssh:///", "svn+ssh://" },

I added

  + { "foo//", "foo" },
  + { "foo///", "foo" },

in r31634.

>
> [[[
>
> * subversion/libsvn_ra_local/split_url.c
> (svn_ra_local__split_URL): Treat file:// equivalent to file:///.
> This function used to complain about a missing hostname when
> just passed "file://". But "file://" is the canonical version
> of "file:///", which is equivalent to "file://localhost/".
> skip_uri_scheme() (in subversion/libsvn_subr/path.c) and
> therefore svn_path_is_url() have been considering "file://"
> a valid URL since r14445.
>
> ]]]
>

For anyone interested, here is the 'diff -w' version of this patch:

> Index: subversion/libsvn_ra_local/split_url.c
> ===================================================================
> --- subversion/libsvn_ra_local/split_url.c (revision 31632)
> +++ subversion/libsvn_ra_local/split_url.c (working copy)
> /* Then, skip what's between the "file://" prefix and the next
> occurance of '/' -- this is the hostname, and we are considering
> everything from that '/' until the end of the URL to be the
> - absolute path portion of the URL. */
> + absolute path portion of the URL.
> + If we got just "file://", treat it the same as "file:///". */
> hostname = URL + 7;
> + if (*hostname == '\0')
> + {
> + path = "/";
> + hostname = NULL;
> + }
> + else
> + {
> path = strchr(hostname, '/');
> if (! path)
> return svn_error_createf
> (SVN_ERR_RA_ILLEGAL_URL, NULL,
> _("Local URL '%s' contains only a hostname, no path"), URL);
>
> /* Treat localhost as an empty hostname. */
> if (hostname != path)
> {
> hostname = svn_path_uri_decode(apr_pstrmemdup(pool, hostname,
> path - hostname), pool);
> if (strncmp(hostname, "localhost", 9) == 0)
> hostname = NULL;
> }
> else
> hostname = NULL;
> + }
>
> /* Duplicate the URL, starting at the top of the path.
> At the same time, we URI-decode the path. */
> #if defined(WIN32) || defined(__CYGWIN__)
> /* On Windows, we'll typically have to skip the leading / if the
> path starts with a drive letter. Like most Web browsers, We
> support two variants of this scheme:

> [[[
>
> * subversion/libsvn_ra_serf/serf.c
> (svn_ra_serf__open,
> svn_ra_serf__reparent): Don't accept an empty hostname.
>
> ]]]
>
> Index: subversion/libsvn_ra_serf/serf.c
> ===================================================================
> --- subversion/libsvn_ra_serf/serf.c (revision 31628)
> +++ subversion/libsvn_ra_serf/serf.c (working copy)
> @@ -500,7 +500,7 @@ svn_ra_serf__open(svn_ra_session_t *session,
> serf_sess->context = serf_context_create(serf_sess->pool);
>
> status = apr_uri_parse(serf_sess->pool, repos_URL, &url);
> - if (status)
> + if (status || strlen(url.hostname) == 0)

url.hostname may be NULL, see r31223 (including epg's comment there).
Actually, r31223 makes it unnecessary to check here for NULL, but for
the svn_ra_serf__reparent hunk it's still an issue. And I don't know
about the neon patch, which doesn't use apr_uri_parse().

> {
> return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
> _("Illegal repository URL '%s'"),

> [[[
>
> * subversion/libsvn_ra_svn/client.c
> (parse_url): Don't accept an empty hostname.
>

(I said that on IRC; repeating for the list)

This, I think, would break tunnels that hard-code the hostname, such as
svn+tau:///bionets/trunk which I currently have.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-08 20:48:05 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.