Re: [PATCH] Fix issue #2116

On Sun, Jun 08, 2008 at 09:47:45PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Sun, 8 Jun 2008 at 19:24 +0200:
> > 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.

Great.

> > 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).

Doh!

> 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.

As far as I'm concerned, we can drop all the "don't lookup empty
string in DNS" patches, i.e. the ones for ra_neon, ra_serf, and ra_svn.

I added them because of a comment Julian appended to issue #2166,
he said:

  "I'm not sure that allowing "http://" through to a network look-up is
   acceptable."

But I don't think it does much harm. If people really want to look
up the empty string in DNS, why stop them? :)

Stefan