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

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

From: Lieven Govaerts <lgo_at_mobsol.be>
Date: 2006-04-08 00:40:05 CEST

 

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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Received on Sat Apr 8 00:44:16 2006

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.