You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lieven Govaerts <lg...@mobsol.be> on 2006/03/28 20:25:20 UTC
[PATCH] fix for issue 2475: ignore case for hostnames (repost)
Hi,
this patch has been sitting on the mailing list for some weeks, so I thought I
bring it back under your attention.
Previous discussion on this patch can be found here:
http://svn.haxx.se/dev/archive-2006-03/0292.shtml
In short, the issue is that hostnames should be handled in an case-insitive way.
Subversion now does all its url handling case sensitive, which leads to certain
problems, eg. moving a file/folder between working copies that point to the
same repository but with different case will fail.
I propose to resolve it as such:
1. When creating a new working copy, make sure the hostname part of the url in
the entries file is stored in lowercase.
2. When reading from an 'old' working copy, lower the case of the hostname while
reading the url. This allows us to be still backwards compatible.
This patch contains both white & blackbox tests for this issue.
thanks for committing if accepted.
Lieven.
[[[
Fix for issue #2475: Make sure hostnames in URI's are handled in a case
insenitive manner.
Patch by: Lieven Govaerts <lg...@mobsol.be>
Found by: Andy Somerville
* subversion/libsvn_subr/path.c:
(svn_path_canonicalize): convert 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.
]]]
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
RE: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: woensdag 5 april 2006 23:34
> To: Lieven Govaerts
> Cc: julianfoad@btopenworld.com; dev@subversion.tigris.org
> Subject: Re: [PATCH] fix for issue 2475: ignore case for
> hostnames (repost)
>
> +====================================================================
> > +# Copyright (c) 2000-2006 CollabNet. All rights reserved.
>
> 2006 only?
Hmm, what do you mean?
> > +# General modules
> > +import os
> > +from urlparse import urlparse, urlunparse
>
> Are these needed?
Nope, leftovers from an earlier try, removed them.
> > + # Do a checkout, and verify the resulting output and
> disk contents.
> > + svntest.actions.run_and_verify_checkout(repo_url2,
> > + wc_dir2,
> > + expected_output,
> > + expected_wc)
>
> I've not done it in a python test, but it might be simpler
> and quicker to do an OS copy of sbox.wc_dir rather than a
> separate checkout.
That doesn't make any sense here, because our url(repo_url2) is different
(uppercase) from the one in our working copy.
> > +
> > + # Move a file from wc to wc2
> > + mu_path = os.path.join(sbox.wc_dir, 'A', 'mu') E_path =
> > + os.path.join(wc_dir2, 'A', 'B', 'E')
> > +
> > + svntest.main.run_svn(None, 'mv', mu_path, E_path)
>
> run_svn on it's own isn't much of a test, add
> run_and_verify_status calls to check what has happend.
Yup, added them.
> > +
> > + # Move a folder from wc to wc2
> > + E_path = os.path.join(sbox.wc_dir, 'A', 'B', 'E') A_path =
> > + os.path.join(wc_dir2, 'A')
> > +
> > + svntest.main.run_svn(None, 'mv', E_path, A_path)
>
> So "move between working copies" works, I didn't know that!
> It's not clear to me whether this was intentional or
> accidental. This also needs run_and_verify_status, maybe
> even run_and_verify_commit.
I added run_and_verify_status. Since directories are completely
self-contained I don't see a reason for this not to work!
> > +
> > + # Copy a folder from wc to wc2
> > + E_path = os.path.join(sbox.wc_dir, 'A', 'B', 'E') A_path =
> > + os.path.join(wc_dir2, 'A')
> > +
> > + svntest.main.run_svn(None, 'cp', E_path, A_path)
>
> Rather than duplicate the whole test, I'd just put the copies
> into the previous test along with the moves.
Hm, I tend to prefer more but smaller unit tests, but with the overlap here
it makes sense to combine them.
> > +
> > +# regression test for issue #2475 - direct copy in the
> repository #
> > +this test handles the 'direct move' case too, that uses
> the same code.
> > +def path_copy_in_repo(sbox):
> > + "case issue #2475 - direct copy in the repository"
> > + sbox.build()
> > +
> > + repo_url2 =
> change_case_of_hostname(svntest.main.current_repo_url)
> > +
> > + # Copy a file from repo to repo2
> > + mu_url = svntest.main.current_repo_url + '/A/mu'
> > + E_url = repo_url2 + '/A/B/E'
> > +
> > + svntest.main.run_svn(None, 'cp', mu_url, E_url, '-m',
> 'copy mu to
> > + /A/B/E')
>
> Again run_svn isn't much of a test, how about a run_and_verify_update?
Added that.
> > +
> > +# regression test for issue #2475 - checkout in a working copy def
> > +path_checkout(sbox):
> > + "case issue #2475 - checkout in wc"
> > + sbox.build()
> > +
> > + # checkout a working copy non-recursively
> > + wc_dir2 = sbox.wc_dir + '2'
> > +
> > + output, errput = svntest.main.run_svn (None, 'co',
> > + '--username',
> svntest.main.wc_author,
> > + '--password',
> svntest.main.wc_passwd,
> > + '-N',
> > + svntest.main.current_repo_url,
> > + wc_dir2)
>
> Why is this using a separate, non-recursive, working copy?
> Your comment tells me what you are doing, something that is
> obvious from the code, but it doesn't tell me why you are doing it.
I've removed this test altogether.
> > +
> > + # checkout in the same working copy, use repository url with
> > + different case
> > + repo_url2 =
> change_case_of_hostname(svntest.main.current_repo_url)
> > +
> > + # Generate the expected output tree.
> > + expected_output = svntest.main.greek_state.copy()
> > + expected_output.wc_dir = wc_dir2
> expected_output.tweak(status='A ',
> > + contents=None)
> > +
> > + # Generate an expected wc tree.
>
> You have put in lots of "trivial" comments like that one,
> they don't make the code easier to understand they just make
> it longer.
Mostly copy paste from other tests, removed them.
> > +
> > +### End of file.
> > \ No newline at end of file
>
> You need to add path_tests.py to build.conf to get it to run.
That was new for me, so I applied that in the attached patch, and used it to
fix a patch for issue 2486 which was committed last week.
Lieven.
[[[
Fix for issue #2475: Make sure scheme and hostname in URI's are handled in a
case insenitive manner.
Patch by: Lieven Govaerts <lg...@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/libsvn_ra_dav/session.c:
(svn_ra_dav__open): cleaned up now obsolete tolower hack.
* subversion/tests/cmdline/path_tests.py:
new file, contains tests for issue #2475.
* build.conf:
added path_tests.py to list of python tests.
]]]
Re: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Philip Martin <ph...@codematters.co.uk>.
"Lieven Govaerts" <lg...@mobsol.be> writes:
> Index: subversion/tests/cmdline/path_tests.py
> ===================================================================
> --- subversion/tests/cmdline/path_tests.py (revision 0)
> +++ subversion/tests/cmdline/path_tests.py (revision 0)
> @@ -0,0 +1,187 @@
> +#!/usr/bin/env python
> +#
> +# path_tests.py: testing case insensitive hostname handling.
> +#
> +# Subversion is a tool for revision control.
> +# See http://subversion.tigris.org for more information.
> +#
> +# ====================================================================
> +# Copyright (c) 2000-2006 CollabNet. All rights reserved.
2006 only?
> +#
> +# This software is licensed as described in the file COPYING, which
> +# you should have received as part of this distribution. The terms
> +# are also available at http://subversion.tigris.org/license-1.html.
> +# If newer versions of this license are posted there, you may use a
> +# newer version instead, at your option.
> +#
> +######################################################################
> +
> +# General modules
> +import os
> +from urlparse import urlparse, urlunparse
Are these needed?
> +from string import lower, upper
> +
> +# Our testing module
> +import svntest
> +
> +import re
> +
> +######################################################################
> +# Utilities
> +#
> +
> +def change_case_of_hostname(input):
> + "Change the case of the hostname, try uppercase first"
> +
> + m = re.match(r"^(.*://)([^/]*)(.*)", input)
> + if m:
> + scheme = m.group(1)
> + host = upper(m.group(2))
> + if host == m.group(2):
> + host = lower(m.group(2))
> +
> + path = m.group(3)
> +
> + return scheme + host + path
> +
> +######################################################################
> +# Tests
> +#
> +# Each test must return on success or raise on failure.
> +
> +
> +#----------------------------------------------------------------------
> +
> +# regression test for issue #2475 - move file and folder
> +def path_move_between_wcs(sbox):
> + "case issue #2475 - move file and folder"
> + sbox.build()
> +
> + # checkout a second working copy, use repository url with different case
> + wc_dir2 = sbox.wc_dir + '2'
> + repo_url2 = change_case_of_hostname(svntest.main.current_repo_url)
> +
> + # Generate the expected output tree.
> + expected_output = svntest.main.greek_state.copy()
> + expected_output.wc_dir = wc_dir2
> + expected_output.tweak(status='A ', contents=None)
> +
> + # Generate an expected wc tree.
> + expected_wc = svntest.main.greek_state
> +
> + # Do a checkout, and verify the resulting output and disk contents.
> + svntest.actions.run_and_verify_checkout(repo_url2,
> + wc_dir2,
> + expected_output,
> + expected_wc)
I've not done it in a python test, but it might be simpler and quicker
to do an OS copy of sbox.wc_dir rather than a separate checkout.
> +
> + # Move a file from wc to wc2
> + mu_path = os.path.join(sbox.wc_dir, 'A', 'mu')
> + E_path = os.path.join(wc_dir2, 'A', 'B', 'E')
> +
> + svntest.main.run_svn(None, 'mv', mu_path, E_path)
run_svn on it's own isn't much of a test, add run_and_verify_status
calls to check what has happend.
> +
> + # Move a folder from wc to wc2
> + E_path = os.path.join(sbox.wc_dir, 'A', 'B', 'E')
> + A_path = os.path.join(wc_dir2, 'A')
> +
> + svntest.main.run_svn(None, 'mv', E_path, A_path)
So "move between working copies" works, I didn't know that! It's not
clear to me whether this was intentional or accidental. This also
needs run_and_verify_status, maybe even run_and_verify_commit.
> +
> +# regression test for issue #2475 - move file and folder
> +def path_copy_between_wcs(sbox):
> + "case issue #2475 - copy file and folder"
> + sbox.build()
> +
> + # checkout a second working copy, use repository url with different case
> + wc_dir2 = sbox.wc_dir + '2'
> + repo_url2 = change_case_of_hostname(svntest.main.current_repo_url)
> +
> + # Generate the expected output tree.
> + expected_output = svntest.main.greek_state.copy()
> + expected_output.wc_dir = wc_dir2
> + expected_output.tweak(status='A ', contents=None)
> +
> + # Generate an expected wc tree.
> + expected_wc = svntest.main.greek_state
> +
> + # Do a checkout, and verify the resulting output and disk contents.
> + svntest.actions.run_and_verify_checkout(repo_url2,
> + wc_dir2,
> + expected_output,
> + expected_wc)
> +
> + # Copy a file from wc to wc2
> + mu_path = os.path.join(sbox.wc_dir, 'A', 'mu')
> + E_path = os.path.join(wc_dir2, 'A', 'B', 'E')
> +
> + svntest.main.run_svn(None, 'cp', mu_path, E_path)
> +
> + # Copy a folder from wc to wc2
> + E_path = os.path.join(sbox.wc_dir, 'A', 'B', 'E')
> + A_path = os.path.join(wc_dir2, 'A')
> +
> + svntest.main.run_svn(None, 'cp', E_path, A_path)
Rather than duplicate the whole test, I'd just put the copies into the
previous test along with the moves.
> +
> +# regression test for issue #2475 - direct copy in the repository
> +# this test handles the 'direct move' case too, that uses the same code.
> +def path_copy_in_repo(sbox):
> + "case issue #2475 - direct copy in the repository"
> + sbox.build()
> +
> + repo_url2 = change_case_of_hostname(svntest.main.current_repo_url)
> +
> + # Copy a file from repo to repo2
> + mu_url = svntest.main.current_repo_url + '/A/mu'
> + E_url = repo_url2 + '/A/B/E'
> +
> + svntest.main.run_svn(None, 'cp', mu_url, E_url, '-m', 'copy mu to /A/B/E')
Again run_svn isn't much of a test, how about a run_and_verify_update?
> +
> +# regression test for issue #2475 - checkout in a working copy
> +def path_checkout(sbox):
> + "case issue #2475 - checkout in wc"
> + sbox.build()
> +
> + # checkout a working copy non-recursively
> + wc_dir2 = sbox.wc_dir + '2'
> +
> + output, errput = svntest.main.run_svn (None, 'co',
> + '--username', svntest.main.wc_author,
> + '--password', svntest.main.wc_passwd,
> + '-N',
> + svntest.main.current_repo_url, wc_dir2)
Why is this using a separate, non-recursive, working copy? Your
comment tells me what you are doing, something that is obvious from
the code, but it doesn't tell me why you are doing it.
> +
> + # checkout in the same working copy, use repository url with different case
> + repo_url2 = change_case_of_hostname(svntest.main.current_repo_url)
> +
> + # Generate the expected output tree.
> + expected_output = svntest.main.greek_state.copy()
> + expected_output.wc_dir = wc_dir2
> + expected_output.tweak(status='A ', contents=None)
> +
> + # Generate an expected wc tree.
You have put in lots of "trivial" comments like that one, they don't
make the code easier to understand they just make it longer.
> + expected_wc = svntest.main.greek_state
> +
> + # Do a checkout, and verify the resulting output and disk contents.
> + svntest.actions.run_and_verify_checkout(repo_url2,
> + wc_dir2,
> + expected_output,
> + expected_wc)
> +
> +########################################################################
> +# Run the tests
> +
> +
> +# list all tests here, starting with None:
> +test_list = [ None,
> + path_move_between_wcs,
> + path_copy_between_wcs,
> + path_copy_in_repo,
> + path_checkout
> + ]
> +
> +if __name__ == '__main__':
> + svntest.main.run_tests(test_list)
> + # NOTREACHED
> +
> +
> +### End of file.
> \ No newline at end of file
You need to add path_tests.py to build.conf to get it to run.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Lieven Govaerts <lg...@mobsol.be>.
Thanks for the review. Attached a new patch with the changes based on your
remarks.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> >
[..]
> > *
> > + * Convert the scheme and hostname to lowercase case-insensitive
> > + handling
> > + * of hostnames ( see issue 2475 )
>
> That comment doesn't make very much sense to me; are there
> some words missing or something?
Fixed.
>
> > Index: subversion/libsvn_subr/path.c
> > ===================================================================
> > --- subversion/libsvn_subr/path.c (revision 19084)
> > +++ subversion/libsvn_subr/path.c (working copy)
>
[..]
> > + /* path will be pointing to a new memory location,
> so update src to
> > + * point to the new location too. */
> > + offset = strlen(host_uri.scheme)+3; // "(scheme)://"
>
> You've still got a C++/C99-style comment there. (Also we
> like spaces around
> operators like "+".)
Fixed.
[..]
> > +
> > +### End of file.
> > +#!/usr/bin/env python
>
> Er, three.
That happens when you revert a patch to test the unit tests, and then
reapply it. Fixed.
> > +
> > +### End of file.
> > Index: subversion/tests/libsvn_subr/path-test.c
> > ===================================================================
> > --- subversion/tests/libsvn_subr/path-test.c (revision 19084)
> > +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> > @@ -670,6 +670,12 @@
> > { "http://hst", "http://hst" },
> > { "http://hst/foo/../bar","http://hst/foo/../bar" },
> > { "http://hst/", "http://hst" },
> > + { "http://HST/", "http://hst" },
> > + { "http://HST/FOO/BaR", "http://hst/FOO/BaR" },
> > + { "svn+ssh://j.random@HST/HST/FOO/BaR",
> "svn+ssh://j.random@hst/HST/FOO/BaR" },
> > + { "svn+ssh://j.random:jray@HST/HST/FOO/BaR",
> "svn+ssh://j.random:jray@hst/HST/FOO/BaR" },
> > + { "SVN+ssh://j.random:jray@HST/HST/FOO/BaR",
> "svn+ssh://j.random:jray@hst/HST/FOO/BaR" },
>
> I suggest using at least some upper-case letters in every
> part of the URL
> (incl. user name and password), to ensure that they don't
> accidentally get
> lower-cased. (Assuming that's the correct behaviour.)
Good point, it provides a better test (with minimal effort :). Fixed.
> > + { "file:///Users/jrandom/wc", "file:///Users/jrandom/wc" },
> > #if defined(WIN32) || defined(__CYGWIN__)
> > /* We permit UNC paths on Windows. By definition UNC
>
> Regards,
>
> - Julian
[[[
Fix for issue #2475: Make sure scheme and hostname in URI's are handled in a
case insenitive manner.
Patch by: Lieven Govaerts <lg...@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/libsvn_ra_dav/session.c:
(svn_ra_dav__open): cleaned up now obsolete tolower hack.
* subversion/tests/cmdline/path_tests.py:
new file, contains tests for issue #2475.
]]]
Re: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Julian Foad <ju...@btopenworld.com>.
Lieven Govaerts wrote:
[...]
Lieven, I don't have time to do a full review but it looks like Philip is doing
that. I just want to say that all your answers in that post sound good to me,
and point out a few things in the patch.
>
> [[[
> Fix for issue #2475: Make sure scheme and hostname in URI's are handled in a
> case insenitive manner.
>
> Patch by: Lieven Govaerts <lg...@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.
Also mention the change you are making to "session.c".
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 19084)
> +++ subversion/include/svn_path.h (working copy)
> @@ -185,6 +185,9 @@
> * separator characters, and possibly other semantically inoperative
> * transformations.
> *
> + * Convert the scheme and hostname to lowercase case-insensitive handling
> + * of hostnames ( see issue 2475 )
That comment doesn't make very much sense to me; are there some words missing
or something?
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 19084)
> +++ subversion/libsvn_subr/path.c (working copy)
> + uri = host_uri.scheme ? TRUE : FALSE;
>
> /* If this is an absolute path, then just copy over the initial
> separator character. */
> @@ -1139,6 +1132,33 @@
>
> }
>
> + if (uri)
> + {
> + /* convert scheme and hostname to lowercase */
> + apr_size_t offset;
> + int i;
> +
> + for(i = 0; host_uri.scheme[i]; i++)
> + host_uri.scheme[i] = tolower(host_uri.scheme[i]);
> + for(i = 0; host_uri.hostname[i]; i++)
> + host_uri.hostname[i] = tolower(host_uri.hostname[i]);
> +
> + /* path will be pointing to a new memory location, so update src to
> + * point to the new location too. */
> + offset = strlen(host_uri.scheme)+3; // "(scheme)://"
You've still got a C++/C99-style comment there. (Also we like spaces around
operators like "+".)
> + path = apr_uri_unparse(pool, &host_uri, APR_URI_UNP_REVEALPASSWORD);
> +
> + /* skip 3rd '/' in file:/// uri */
> + if (path[offset] == '/')
> + offset++;
> +
> + /* copy src to dst */
> + memcpy(dst, path, offset);
> + dst += offset;
> +
> + src = path + offset;
> + }
> +
> Index: subversion/tests/cmdline/path_tests.py
> ===================================================================
> --- subversion/tests/cmdline/path_tests.py (revision 0)
> +++ subversion/tests/cmdline/path_tests.py (revision 0)
> @@ -0,0 +1,561 @@
> +#!/usr/bin/env python
> +#
> +# path_tests.py: testing case insensitive hostname handling.
> +#
> +# Subversion is a tool for revision control.
> +# See http://subversion.tigris.org for more information.
> +#
> +# ====================================================================
> +# Copyright (c) 2000-2006 CollabNet. All rights reserved.
[...]
> +# list all tests here, starting with None:
> +test_list = [ None,
> + path_move_between_wcs,
> + path_copy_between_wcs,
> + path_copy_in_repo,
> + path_checkout
> + ]
> +
> +if __name__ == '__main__':
> + svntest.main.run_tests(test_list)
> + # NOTREACHED
> +
> +
> +### End of file.
> +#!/usr/bin/env python
Oops! You seem to have concatenated two files here.
> +#
> +# path_tests.py: testing case insensitive hostname handling.
> +#
> +# Subversion is a tool for revision control.
> +# See http://subversion.tigris.org for more information.
> +#
> +# ====================================================================
> +# Copyright (c) 2000-2006 CollabNet. All rights reserved.
[...]
> +if __name__ == '__main__':
> + svntest.main.run_tests(test_list)
> + # NOTREACHED
> +
> +
> +### End of file.
> +#!/usr/bin/env python
Er, three.
> +#
> +# path_tests.py: testing case insensitive hostname handling.
> +#
> +# Subversion is a tool for revision control.
> +# See http://subversion.tigris.org for more information.
> +#
> +# ====================================================================
> +# Copyright (c) 2000-2006 CollabNet. All rights reserved.
[...]
> +if __name__ == '__main__':
> + svntest.main.run_tests(test_list)
> + # NOTREACHED
> +
> +
> +### End of file.
> Index: subversion/tests/libsvn_subr/path-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/path-test.c (revision 19084)
> +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> @@ -670,6 +670,12 @@
> { "http://hst", "http://hst" },
> { "http://hst/foo/../bar","http://hst/foo/../bar" },
> { "http://hst/", "http://hst" },
> + { "http://HST/", "http://hst" },
> + { "http://HST/FOO/BaR", "http://hst/FOO/BaR" },
> + { "svn+ssh://j.random@HST/HST/FOO/BaR", "svn+ssh://j.random@hst/HST/FOO/BaR" },
> + { "svn+ssh://j.random:jray@HST/HST/FOO/BaR", "svn+ssh://j.random:jray@hst/HST/FOO/BaR" },
> + { "SVN+ssh://j.random:jray@HST/HST/FOO/BaR", "svn+ssh://j.random:jray@hst/HST/FOO/BaR" },
I suggest using at least some upper-case letters in every part of the URL
(incl. user name and password), to ensure that they don't accidentally get
lower-cased. (Assuming that's the correct behaviour.)
> + { "file:///Users/jrandom/wc", "file:///Users/jrandom/wc" },
> #if defined(WIN32) || defined(__CYGWIN__)
> /* We permit UNC paths on Windows. By definition UNC
Regards,
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Lieven Govaerts <lg...@mobsol.be>.
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 <lg...@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.
]]]
Re: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
>> + for(i = 0; host_uri.hostname[i]; i++)
>> + host_uri.hostname[i] = tolower(host_uri.hostname[i]);
>> +
>> + // path will be pointing to a new memory location, so update src to
>> + // point to the new location too.
>> + offset = src - path;
>> + path = apr_uri_unparse(pool, &host_uri, APR_URI_UNP_REVEALPASSWORD);
>> + src = path + offset;
>
> Rather than overwriting host_uri and calculating a new path can you
> simply copy the lower case stuff into dst?
I see now, the unparse call is to handle things like username and
password.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue 2475: ignore case for hostnames (repost)
Posted by Philip Martin <ph...@codematters.co.uk>.
Lieven Govaerts <lg...@mobsol.be> writes:
> --- subversion/libsvn_subr/path.c (revision 18872)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -23,6 +23,7 @@
>
> #include <apr_file_info.h>
> #include <apr_lib.h>
> +#include <apr_uri.h>
>
> #include "svn_string.h"
> #include "svn_path.h"
> @@ -1139,6 +1140,25 @@
>
> }
>
> + if (uri)
> + {
> + // convert hostname to lowercase
What about the scheme, should that be converted as well? Perhaps we
could then remove the tolower hack in svn_ra_dav__open?
> + 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?
> +
> + for(i = 0; host_uri.hostname[i]; i++)
> + host_uri.hostname[i] = tolower(host_uri.hostname[i]);
> +
> + // path will be pointing to a new memory location, so update src to
> + // point to the new location too.
> + offset = src - path;
> + path = apr_uri_unparse(pool, &host_uri, APR_URI_UNP_REVEALPASSWORD);
> + src = path + offset;
Rather than overwriting host_uri and calculating a new path can you
simply copy the lower case stuff into dst?
> + }
> +
> while (*src)
> {
> /* Parse each segment, find the closing '/' */
> Index: subversion/libsvn_wc/entries.c
> ===================================================================
> --- subversion/libsvn_wc/entries.c (revision 18872)
> +++ subversion/libsvn_wc/entries.c (working copy)
> @@ -240,9 +240,12 @@
> {
> entry->url
> = apr_hash_get(atts, SVN_WC__ENTRY_ATTR_URL, APR_HASH_KEY_STRING);
> -
> - if (entry->url)
> - *modify_flags |= SVN_WC__ENTRY_MODIFY_URL;
> +
> + if (entry->url)
> + {
> + *modify_flags |= SVN_WC__ENTRY_MODIFY_URL;
> + entry->url = svn_path_canonicalize(entry->url, pool);
> + }
> }
>
> /* Set up repository root. Make sure it is a prefix of url. */
> @@ -251,6 +254,7 @@
> APR_HASH_KEY_STRING);
> 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.
In passing I see some old code handling svn:this_dir with a comment
saying it should be removed before 1.0!
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org