Re: [PATCH] fix for issue 2475: ignore case for hostnames (repost)

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

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