Troy Curtis Jr wrote on Sat, 3 May 2008 at 20:56 -0500:
> 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.
OK.
>
> >
> > > + * 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.
>
OK. There is only one caller, anyway.
> >
> > > */
> > > 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)
>
(see below)
> >
> > [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.
>
(see below)
> >
> > [2] Why the pointer arithmetic? To skip a leading '/'?
> > If so, svn_path_canonicalize will do that anyway.
> >
>
> Actually to skip the leading '^'.
Got it. I got confused here: I assumed that the leading '^' is already
removed by the time this function gets called. That line looks fine
now, when I know what to assume.
> 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,
(I did)
> > 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.
>
Interesting question. The docstrings didn't say that it does, and a few
weeks ago, when I wrote a test (wc_uuid) that used this function,
I copied existing code that explicitly checked for errors:
exit_code, output, errput = svntest.main.run_svn(None, 'info', wc_dir)
if errput:
raise svntest.verify.SVNUnexpectedStderr(errput)
However, looking deeper into the implementation, run_svn() calls
run_command() which calls run_command_stdin(stdin_lines=None) which does
the error checking by itself:
if (not error_expected) and (stderr_lines):
map(sys.stdout.write, stderr_lines)
raise Failure
So no error checking is necessary around run_svn() here.
> >
> > > +
> > > + # 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?
>
As far as I know, trailing slashes on files are allowed because we
canonicalize paths before we check whether they are directories, not
because it is our policy to allow them in the UI. Also, it would not
be much work to use 'ls' or 'info' here.
> >
> > > +
> > > #----------------------------------------------------------------------
> > >
> > > ########################################################################
> > > @@ -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
>
>
---------------------------------------------------------------------
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 08:33:56 CEST