Howdy Daniel,
On Sat, May 3, 2008 at 2:53 AM, Daniel Shahaf <d.s_at_daniel.shahaf.co.il> wrote:
> 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?
Exactly, this function will be operating directly on user input. The goal
being to simply replace '^/' with the root url.
>
> > + * 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.
Really this is just a helper function for this file. In this case no
assumptions are made as far as canonicallizing go on the inputs and are just
'passed' through back to the caller.
>
> > */
> > 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.
But I am assuming that relative_url is a repository root relative url so by
definition it begins with '^/'. (see next comment)
>
> [2] Why can you assume that (relative_url != "")?
Because it's the first thing that gets checked :) You can't see that in the
patch, but arg_is_repos_relative_url() is called on the relative_url argument
first thing.
>
> [2] Why the pointer arithmetic? To skip a leading '/'?
> If so, svn_path_canonicalize will do that anyway.
>
Actually to skip the leading '^'. I purposefully left the '/' in there and am
relying on later canonicallization to clean up any duplicated '/'s. This way
the bare '^/' is supported and the root url does not actually have to end with
a '/'.
>
> 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?
Doesn't this throw an exception? I'm not much of a python person and I
learned just enough of the unit test framework to get something working. If
not I guess I can change it (and my earlier tests not in this patch). Of
course I'm in good company as a majority of the svntest.main.run_svn() calls
do not check return values.
>
> > +
> > + # 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
Doing a blame on a directory would not have fit into my existing code here. I
simply wanted to make sure that the trailing '/' did not cause an assertion
failure!
I suppose that I could change it to a directory so that it would make more
sense. I guess I could use 'ls' instead of 'blame' as my subcommand. Do you
feel that it is really necessary?
>
> > +
> > #----------------------------------------------------------------------
> >
> > ########################################################################
> > @@ -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
>
Troy
--
Beware of spyware. If you can, use the Firefox browser. - USA Today
Download now at http://getfirefox.com
Registered Linux User #354814 ( http://counter.li.org/)
---------------------------------------------------------------------
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-04 03:56:36 CEST