You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Noorul Islam K M <no...@collab.net> on 2011/02/01 05:58:04 UTC
Re: [PATCH] New XFail tests for issue 3609
Noorul Islam K M <no...@collab.net> writes:
> Log
>
> [[[
>
> New XFail tests for issue 3609.
>
> * subversion/tests/cmdline/mergeinfo_tests.py
> (mergeinfo_url_special_characters, test_list),
> subversion/tests/cmdline/prop_tests.py
> (props_url_special_characters, test_list),
> subversion/tests/cmdline/merge_tests.py
> (merge_url_special_characters, test_list),
> subversion/tests/cmdline/log_tests.py
> (log_url_special_characters, test_list),
> subversion/tests/cmdline/copy_tests.py
> (copy_url_special_characters, test_list),
> subversion/tests/cmdline/blame_tests.py
> (blame_url_special_characters, test_list):
> New XFail tests.
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
>
> Index: subversion/tests/cmdline/mergeinfo_tests.py
> ===================================================================
> --- subversion/tests/cmdline/mergeinfo_tests.py (revision 1062140)
> +++ subversion/tests/cmdline/mergeinfo_tests.py (working copy)
> @@ -479,6 +479,18 @@
> adjust_error_for_server_version(''),
> ['4', '5'], A_path, A_COPY_path + '@PREV', '--show-revs', 'eligible')
>
> +def mergeinfo_url_special_characters(sbox):
> + """special characters in svn mergeinfo URL"""
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + special_url = sbox.repo_url + '/%2E'
> +
> + svntest.actions.run_and_verify_svn(None, None, [], 'ps', SVN_PROP_MERGEINFO,
> + '/:1', wc_dir)
> + svntest.actions.run_and_verify_mergeinfo(adjust_error_for_server_version(""),
> + ['1'], special_url, wc_dir)
> +
> ########################################################################
> # Run the tests
>
> @@ -494,6 +506,7 @@
> SkipUnless(recursive_mergeinfo, server_has_mergeinfo),
> SkipUnless(mergeinfo_on_pegged_wc_path,
> server_has_mergeinfo),
> + XFail(mergeinfo_url_special_characters),
>
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/prop_tests.py
> ===================================================================
> --- subversion/tests/cmdline/prop_tests.py (revision 1062140)
> +++ subversion/tests/cmdline/prop_tests.py (working copy)
> @@ -2335,6 +2335,31 @@
> if ((len(expected_output) * 3) - 6) != len(pg_stdout_redir):
> raise svntest.Failure("Redirected pg -vR has unexpected duplicates")
>
> +def props_url_special_characters(sbox):
> + "set/get/list/del with special characters in URL"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + special_url = sbox.repo_url + '/%2E'
> +
> + svntest.actions.enable_revprop_changes(sbox.repo_dir)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', '--revprop', '-r', '0',
> + 'cash-sound', 'cha-ching!', special_url)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propget', '--revprop', '-r', '0',
> + 'cash-sound', special_url)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'proplist', '--revprop', '-r', '0',
> + special_url)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propdel', '--revprop', '-r', '0',
> + 'cash-sound', special_url)
> +
> ########################################################################
> # Run the tests
>
> @@ -2380,6 +2405,7 @@
> obstructed_subdirs,
> atomic_over_ra,
> propget_redirection,
> + XFail(props_url_special_characters),
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/merge_tests.py
> ===================================================================
> --- subversion/tests/cmdline/merge_tests.py (revision 1062140)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
> @@ -16402,6 +16402,33 @@
> if not os.access(beta_path, os.X_OK):
> raise svntest.Failure("beta is not marked as executable after commit")
>
> +def merge_url_special_characters(sbox):
> + """special characters in svn merge URL"""
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + a_dir = os.path.join(wc_dir, 'A')
> + new_file = os.path.join(a_dir, "new file")
> +
> + # Make r2.
> + svntest.main.file_append(new_file, "Initial text in the file.\n")
> + svntest.main.run_svn(None, "add", new_file)
> + svntest.actions.run_and_verify_svn(None, None, [],
> + "ci", "-m", "r2", wc_dir)
> +
> + # Make r3.
> + svntest.main.file_append(new_file, "Next line of text in the file.\n")
> + svntest.actions.run_and_verify_svn(None, None, [],
> + "ci", "-m", "r3", wc_dir)
> +
> + os.chdir(wc_dir)
> + svntest.actions.run_and_verify_svn(None, None, [],
> + "up")
> +
> + special_url = sbox.repo_url + '/A' + '/%2E'
> + svntest.actions.run_and_verify_svn(None, None, [],
> + "merge", "-r3:2", special_url)
> +
> ########################################################################
> # Run the tests
>
> @@ -16595,6 +16622,7 @@
> XFail(subtree_merges_inherit_invalid_working_mergeinfo),
> XFail(SkipUnless(merge_change_to_file_with_executable,
> svntest.main.is_posix_os)),
> + XFail(merge_url_special_characters),
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/log_tests.py
> ===================================================================
> --- subversion/tests/cmdline/log_tests.py (revision 1062140)
> +++ subversion/tests/cmdline/log_tests.py (working copy)
> @@ -1750,6 +1750,20 @@
> "differs from that on move source '%s'"
> % (psi_moved_path, psi_path))
>
> +def log_url_special_characters(sbox):
> + """special characters in svn log URL"""
> + sbox.build(create_wc = False)
> +
> + special_urls = [sbox.repo_url + '/A' + '/%2E',
> + sbox.repo_url + '%2F' + 'A']
> +
> + for url in special_urls:
> + exit_code, output, err = svntest.actions.run_and_verify_svn(None, None, [],
> + 'log', '-c',
> + 1, url)
> + log_chain = parse_log_output(output)
> + check_log_chain(log_chain, [1])
> +
> ########################################################################
> # Run the tests
>
> @@ -1792,6 +1806,7 @@
> SkipUnless(merge_sensitive_log_propmod_merge_inheriting_path,
> server_has_mergeinfo),
> log_of_local_copy,
> + XFail(log_url_special_characters),
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/copy_tests.py
> ===================================================================
> --- subversion/tests/cmdline/copy_tests.py (revision 1062140)
> +++ subversion/tests/cmdline/copy_tests.py (working copy)
> @@ -4958,6 +4958,23 @@
> '.*Cannot move URL.* into itself.*',
> 'move', repo_url, repo_url)
>
> +def copy_url_special_characters(sbox):
> + """special characters in svn cp URL"""
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + a_path = os.path.join(wc_dir, 'A')
> + new_path1 = os.path.join(a_path, 'Folder1')
> + os.mkdir(new_path1)
> + new_path2 = os.path.join(a_path, 'Folder2')
> + os.mkdir(new_path2)
> + svntest.main.run_svn(None, "add", new_path1, new_path2)
> + sbox.simple_commit()
> + from_special_url = sbox.repo_url + '/A/Folder1' + '/%2E'
> +
> + svntest.actions.run_and_verify_svn(None, None, [], 'copy',
> + from_special_url, new_path2)
> +
> ########################################################################
> # Run the tests
>
> @@ -5061,6 +5078,7 @@
> copy_wc_over_deleted_same_kind,
> copy_wc_over_deleted_other_kind,
> move_wc_and_repo_dir_to_itself,
> + XFail(copy_url_special_characters),
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/blame_tests.py
> ===================================================================
> --- subversion/tests/cmdline/blame_tests.py (revision 1062140)
> +++ subversion/tests/cmdline/blame_tests.py (working copy)
> @@ -703,7 +703,19 @@
> svntest.actions.run_and_verify_svn(None, expected_output, [],
> 'blame', '-g', mu_path)
>
> +def blame_url_special_characters(sbox):
> + """special characters in svn blame URL"""
> + sbox.build(create_wc = False)
>
> + special_urls = [sbox.repo_url + '/A' + '/%2E',
> + sbox.repo_url + '%2F' + 'A']
> +
> + expected_err = "svn: '/A' is not a file in revision 1\n"
> +
> + for url in special_urls:
> + svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,
> + 'blame', url)
> +
> ########################################################################
> # Run the tests
>
> @@ -724,6 +736,7 @@
> blame_peg_rev_file_not_in_head,
> blame_file_not_in_head,
> blame_output_after_merge,
> + XFail(blame_url_special_characters)
> ]
>
> if __name__ == '__main__':
It will be great if someone could review this patch.
Thanks and Regards
Noorul
RE: [PATCH] New XFail tests for issue 3609
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Noorul Islam K M [mailto:noorul@collab.net]
> Sent: dinsdag 1 februari 2011 12:41
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: [PATCH] New XFail tests for issue 3609
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> >> -----Original Message-----
> >> From: Noorul Islam K M [mailto:noorul@collab.net]
> >> Sent: dinsdag 1 februari 2011 5:58
> >> To: dev@subversion.apache.org
> >> Subject: Re: [PATCH] New XFail tests for issue 3609
> >>
> >> Noorul Islam K M <no...@collab.net> writes:
> >>
> >> > Log
> >> >
> >> > [[[
> >> >
> >> > New XFail tests for issue 3609.
> >> >
> >> > * subversion/tests/cmdline/mergeinfo_tests.py
> >> > (mergeinfo_url_special_characters, test_list),
> >> > subversion/tests/cmdline/prop_tests.py
> >> > (props_url_special_characters, test_list),
> >> > subversion/tests/cmdline/merge_tests.py
> >> > (merge_url_special_characters, test_list),
> >> > subversion/tests/cmdline/log_tests.py
> >> > (log_url_special_characters, test_list),
> >> > subversion/tests/cmdline/copy_tests.py
> >> > (copy_url_special_characters, test_list),
> >> > subversion/tests/cmdline/blame_tests.py
> >> > (blame_url_special_characters, test_list):
> >> > New XFail tests.
> >> >
> >> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> >> > ]]]
> >> >
> >> > Index: subversion/tests/cmdline/mergeinfo_tests.py
> >> >
> ===================================================================
> >> > --- subversion/tests/cmdline/mergeinfo_tests.py (revision 1062140)
> >> > +++ subversion/tests/cmdline/mergeinfo_tests.py (working copy)
> >> > @@ -479,6 +479,18 @@
> >> > adjust_error_for_server_version(''),
> >> > ['4', '5'], A_path, A_COPY_path + '@PREV', '--show-revs',
> >> 'eligible')
> >> >
> >> > +def mergeinfo_url_special_characters(sbox):
> >> > + """special characters in svn mergeinfo URL"""
> >> > +
> >> > + sbox.build()
> >> > + wc_dir = sbox.wc_dir
> >> > + special_url = sbox.repo_url + '/%2E'
> >
> > With an url like http://server/svn/repos
> > and %2E = '.'
> >
> > This would create an url as 'http://server/svn/repos/%2E'
> >
> > Following our canonicalization rules this would be a non-canonical
> > equivalent to http://server/svn/repos (via http://server/svn/repos/.)
> >
> > I just added a few tests for this specific scenarios in
> > subversion/tests/libsvn_subr/dirent_uri_tests.c, which showed that
> > svn_uri_canonicalize() doesn't handle this case correctly.
> > (svn_uri_canonicalize() returned "http://server/svn/repos/.", which
> is not
> > canonical by itself)
> >
> > I committed a fix in r 1066006, so you might need a different special
> > character to trigger your issue. (Or maybe this just fixed the
> issue?)
>
> Do you mean this test cases will pass with you fix? If so, is it not
> okay to add these test cases? May be not as XFail?
I haven't tested your tests.
The comments in your tests say you are testing 'special characters', while
the test really failed in the canonicalization of '.' which is handled
unlike any other 'special' character.
The tests in dirent_uri_tests.c explicitly test this behavior now, so I
don't think you need this number of commandline tests for the '.'
canonicalization problems any more. (The test suite is slow enough already
and these few tests don't really improve the coverage now).
But the tests might still be valid for other special characters like 'é',
'+' and '%'.
Bert
Re: [PATCH] New XFail tests for issue 3609
Posted by Noorul Islam K M <no...@collab.net>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> -----Original Message-----
>> From: Noorul Islam K M [mailto:noorul@collab.net]
>> Sent: dinsdag 1 februari 2011 5:58
>> To: dev@subversion.apache.org
>> Subject: Re: [PATCH] New XFail tests for issue 3609
>>
>> Noorul Islam K M <no...@collab.net> writes:
>>
>> > Log
>> >
>> > [[[
>> >
>> > New XFail tests for issue 3609.
>> >
>> > * subversion/tests/cmdline/mergeinfo_tests.py
>> > (mergeinfo_url_special_characters, test_list),
>> > subversion/tests/cmdline/prop_tests.py
>> > (props_url_special_characters, test_list),
>> > subversion/tests/cmdline/merge_tests.py
>> > (merge_url_special_characters, test_list),
>> > subversion/tests/cmdline/log_tests.py
>> > (log_url_special_characters, test_list),
>> > subversion/tests/cmdline/copy_tests.py
>> > (copy_url_special_characters, test_list),
>> > subversion/tests/cmdline/blame_tests.py
>> > (blame_url_special_characters, test_list):
>> > New XFail tests.
>> >
>> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
>> > ]]]
>> >
>> > Index: subversion/tests/cmdline/mergeinfo_tests.py
>> > ===================================================================
>> > --- subversion/tests/cmdline/mergeinfo_tests.py (revision 1062140)
>> > +++ subversion/tests/cmdline/mergeinfo_tests.py (working copy)
>> > @@ -479,6 +479,18 @@
>> > adjust_error_for_server_version(''),
>> > ['4', '5'], A_path, A_COPY_path + '@PREV', '--show-revs',
>> 'eligible')
>> >
>> > +def mergeinfo_url_special_characters(sbox):
>> > + """special characters in svn mergeinfo URL"""
>> > +
>> > + sbox.build()
>> > + wc_dir = sbox.wc_dir
>> > + special_url = sbox.repo_url + '/%2E'
>
> With an url like http://server/svn/repos
> and %2E = '.'
>
> This would create an url as 'http://server/svn/repos/%2E'
>
> Following our canonicalization rules this would be a non-canonical
> equivalent to http://server/svn/repos (via http://server/svn/repos/.)
>
> I just added a few tests for this specific scenarios in
> subversion/tests/libsvn_subr/dirent_uri_tests.c, which showed that
> svn_uri_canonicalize() doesn't handle this case correctly.
> (svn_uri_canonicalize() returned "http://server/svn/repos/.", which is not
> canonical by itself)
>
> I committed a fix in r 1066006, so you might need a different special
> character to trigger your issue. (Or maybe this just fixed the issue?)
Do you mean this test cases will pass with you fix? If so, is it not
okay to add these test cases? May be not as XFail?
Thanks and Regards
Noorul
RE: [PATCH] New XFail tests for issue 3609
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Noorul Islam K M [mailto:noorul@collab.net]
> Sent: dinsdag 1 februari 2011 5:58
> To: dev@subversion.apache.org
> Subject: Re: [PATCH] New XFail tests for issue 3609
>
> Noorul Islam K M <no...@collab.net> writes:
>
> > Log
> >
> > [[[
> >
> > New XFail tests for issue 3609.
> >
> > * subversion/tests/cmdline/mergeinfo_tests.py
> > (mergeinfo_url_special_characters, test_list),
> > subversion/tests/cmdline/prop_tests.py
> > (props_url_special_characters, test_list),
> > subversion/tests/cmdline/merge_tests.py
> > (merge_url_special_characters, test_list),
> > subversion/tests/cmdline/log_tests.py
> > (log_url_special_characters, test_list),
> > subversion/tests/cmdline/copy_tests.py
> > (copy_url_special_characters, test_list),
> > subversion/tests/cmdline/blame_tests.py
> > (blame_url_special_characters, test_list):
> > New XFail tests.
> >
> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> > ]]]
> >
> > Index: subversion/tests/cmdline/mergeinfo_tests.py
> > ===================================================================
> > --- subversion/tests/cmdline/mergeinfo_tests.py (revision 1062140)
> > +++ subversion/tests/cmdline/mergeinfo_tests.py (working copy)
> > @@ -479,6 +479,18 @@
> > adjust_error_for_server_version(''),
> > ['4', '5'], A_path, A_COPY_path + '@PREV', '--show-revs',
> 'eligible')
> >
> > +def mergeinfo_url_special_characters(sbox):
> > + """special characters in svn mergeinfo URL"""
> > +
> > + sbox.build()
> > + wc_dir = sbox.wc_dir
> > + special_url = sbox.repo_url + '/%2E'
With an url like http://server/svn/repos
and %2E = '.'
This would create an url as 'http://server/svn/repos/%2E'
Following our canonicalization rules this would be a non-canonical
equivalent to http://server/svn/repos (via http://server/svn/repos/.)
I just added a few tests for this specific scenarios in
subversion/tests/libsvn_subr/dirent_uri_tests.c, which showed that
svn_uri_canonicalize() doesn't handle this case correctly.
(svn_uri_canonicalize() returned "http://server/svn/repos/.", which is not
canonical by itself)
I committed a fix in r 1066006, so you might need a different special
character to trigger your issue. (Or maybe this just fixed the issue?)
Bert