On Sun, May 4, 2008 at 1:33 AM, Daniel Shahaf <d.s_at_daniel.shahaf.co.il> wrote:
> 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.
>
>
Ah, such a stickler! Any python code is painful code! Oh ok I guess
it was all of 5 minutes of work. I have attached the modified patch,
and duplicated the log message below.
>
> > >
> > > > +
> > > > #----------------------------------------------------------------------
> > > >
> > > > ########################################################################
> > > > @@ -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
> >
> >
>
[[[
Fix an assertion failure when a user inputs a non-canonical repository root
elative 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.
]]]
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 15:57:51 CEST