Good morning Troy,
Finally reviewing this patch too.
Troy Curtis Jr wrote on Wed, 30 Apr 2008 at 22:25 -0500:
> Per Blair's request I have split up my earlier repository root relative url
> fixes. This is the first part.
>
> [[[
> Fix an assertion failure when a user inputs a non-canonical repository root
> relative url.
>
> * subversion/libsvn_client/cmdline.c
> (resolve_repos_relative_url): Avoid an assertion failure in svn_path_join() by
> replacing it with apr_pstrcat() so that there is not a requirement that the
> input arguments be canonical. Also updated the doc string to reflect this.
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_relative_url_non_canonical): New test function.
> (test_list): Call the new test function.
> ]]]
>
>
> Index: subversion/libsvn_client/cmdline.c
> ===================================================================
> --- subversion/libsvn_client/cmdline.c (revision 30798)
> +++ subversion/libsvn_client/cmdline.c (working copy)
> @@ -56,6 +56,10 @@
> * REPOS_ROOT_URL is the absolute URL of the repository root.
> * All strings are in UTF-8 encoding.
> * Allocate *ABSOLUTE_URL in POOL.
> + *
> + * REPOS_ROOT_URL and RELATIVE_URL do not have to be properly URI-encoded,
> + * canonical, or valid in any other way.
(to make sure I understand correctly) Is the reason we cannot make any
assumptions on the arguments that they come directly from user input,
and will be URI encoded and canonicalized later by the caller?
> + * The caller is expected to perform
> + * canonicalization on *ABSOLUTE_URL after the call to the function.
Can callers do anything with the returned value without canonicalizing
it first? If not, you should canonicalize it here. The fewer
exceptions (to the "Paths are canonicalized" rule), the better.
> */
> static svn_error_t *
> resolve_repos_relative_url(const char **absolute_url,
> @@ -68,7 +72,11 @@
> _("Improper relative URL '%s'"),
> relative_url);
>
> - *absolute_url = svn_path_join(repos_root_url, relative_url + 2, pool);
> + /* No assumptions are made about the canonicalization of the input
> + * arguments, it is presumed that the output will be canonicalized after
> + * this function, which will remove any duplicate path seperator.
> + */
> + *absolute_url = apr_pstrcat(pool, repos_root_url, relative_url + 1, NULL);
^[1] ^[2]
[1] Don't you need to add a "/" between them? You're not assuming
that they are in canonical form.
[2] Why can you assume that (relative_url != "")?
[2] Why the pointer arithmetic? To skip a leading '/'?
If so, svn_path_canonicalize will do that anyway.
I think I might be misunderstanding something here, since obviously you do
get a '/' between the repos root and the relative parts of the URL; I just
don't see how...
> return SVN_NO_ERROR;
> }
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 30798)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -2291,7 +2291,41 @@
> svntest.verify.AnyOutput, 'blame',
> '^/A/mu', iota_url_repo1, iota_url_repo2)
>
> +def basic_relative_url_non_canonical(sbox):
> + "basic relative url non-canonical targets"
>
> + sbox.build()
> +
> + # First, make a new revision of iota.
> + iota = os.path.join(sbox.wc_dir, 'iota')
> + svntest.main.file_append(iota, "New contents for iota\n")
> + svntest.main.run_svn(None, 'ci',
> + '-m', '', iota)
Error checking?
> +
> + # Now, make a new revision of A/mu .
> + mu = os.path.join(sbox.wc_dir, 'A', 'mu')
> + mu_url = sbox.repo_url + '/A/mu'
> +
> + svntest.main.file_append(mu, "New contents for mu\n")
> + svntest.main.run_svn(None, 'ci',
> + '-m', '', mu)
> +
> +
> + expected_output = [
> + " 1 jrandom This is the file 'iota'.\n",
> + " 2 jrandom New contents for iota\n",
> + " 1 jrandom This is the file 'mu'.\n",
> + " 3 jrandom New contents for mu\n",
> + ]
> +
> + exit_code, output, error = svntest.actions.run_and_verify_svn(None,
> + expected_output, [], 'blame',
> + '^/iota/', mu_url)
> +
> + exit_code, output, error = svntest.actions.run_and_verify_svn(None,
> + expected_output, [], 'blame',
> + '^//iota/', mu_url)
^
No trailing slash on files, please. I know we allow this,
% svn ls https://svn.collab.net/repos/svn/trunk/CHANGES/
CHANGES
but I don't think we should rely on this.
% /bin/ls CHANGES/
/bin/ls: cannot access CHANGES/: Not a directory
> +
> #----------------------------------------------------------------------
>
> ########################################################################
> @@ -2342,6 +2376,7 @@
> basic_relative_url_using_current_dir,
> basic_relative_url_using_other_targets,
> basic_relative_url_multi_repo,
> + basic_relative_url_non_canonical,
> ]
>
> if __name__ == '__main__':
Looks good,
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-03 09:53:41 CEST