Hi,
attached is an updated patch, including the changes based on your remarks.
> -----Original Message-----
> From: Philip Martin [mailto:philip@codematters.co.uk]
[..]
> > + if (uri)
> > + {
> > + // convert hostname to lowercase
>
> What about the scheme, should that be converted as well?
Yes, the case of scheme schould be ignored too. I changed that in attached
patch. While rereading the patch, I saw that I'm using C++ style comments. I
replaced them by /* */
> Perhaps we could then remove the tolower hack in svn_ra_dav__open?
The question is, has the url that's used here always been canonicalized? My
tests seems to indicate that is the case, but I don't know if that's
guaranteed. I've removed that hack in the attached patch.
> > + apr_uri_t host_uri;
> > + apr_size_t offset;
> > + int i;
> > +
> > + apr_uri_parse(pool, path, &host_uri);
>
> The overlap between skip_uri_scheme and apr_uri_parse is a bit
> worrying, can we drop skip_uri_scheme just use apr_uri_parse
> everywhere?
I've dropped skip_uri_scheme in the svn_path_canonicalize function, which
made the code easier to understand as well. The function skip_uri_scheme is
still used in two other places (svn_path_is_url & svn_path_is_uri_safe).
Both can be replaced by something better, but I don't think that should be
part of this patch.
> > if (entry->repos)
> > {
> > + entry->repos = svn_path_canonicalize(entry->repos, pool);
> > if (entry->url && !
> svn_path_is_ancestor(entry->repos, entry->url))
> > return svn_error_createf(SVN_ERR_WC_CORRUPT, NULL,
> > _("Entry for '%s' has
> invalid repository "
>
> I guess this is repairing old working copies that have stored
> non-canonical URLs, a repair that could be done manually using
> --relocate? I don't know whether our policy these days is to trust
> .svn data or not but we don't appear to force name to be canonical so
> forcing the URLs is inconsistent. I suppose we can justify it since
> we know that non-canonical URLs exist whereas the only known way to
> get non-canonical names is to modify the .svn data outside of
> Subversion.
I'm not exactly repairing old working copies here, just supporting them. I
don't think it's a good idea to ask people who have been using
'http://HOST/svn/repos' to relocate their working copies when they upgrade
to a new SVN version.
thanks for the review.
Lieven.
[[[
Fix for issue #2475: Make sure scheme and hostname in URI's are handled in a
case insenitive manner.
Patch by: Lieven Govaerts <lgo@mobsol.be>
Found by: Andy Somerville
* subversion/libsvn_subr/path.c:
(svn_path_canonicalize): convert scheme and hostname to lowercase.
* subversion/libsvn_wc/entries.c:
(svn_wc__atts_to_entry): while reading from the entries file,
canonicalize url and repos.
* subversion/include/svn_path.h:
(svn_path_canonicalize): add comment
* subversion/tests/libsvn_subr/path-test.c:
(test_canonicalize): add test for uppercase hostnames
* subversion/tests/cmdline/path_tests.py:
new file, contains tests for issue #2475.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 30 02:42:40 2006