On Sun, 2008-09-07 at 15:29 +0400, Fyodor Sheremetyev wrote:
> On Tue, Sep 2, 2008 at 11:13 AM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> > "Fyodor Sheremetyev" <fyodor_at_visualsvn.com> writes:
> >> I'm trying to fix this problem. Attached is a short and dirty patch
> >> that makes my reproduction script work. But I'm absolutely unsure that
> >> it fixes the problem correctly.
> >>
> >> I see no consistency in using svn_path_uri_encode and
> >> svn_path_uri_decode in the code and couldn't find any guidelines
> >> regarding uri encoding. Which level should do that by design? Please
> >> could somebody point at the right direction.
> >
> > I think your patch is correct. I've quoted it below so others can see
> > it. A couple of comments:
> >
> > 1. A log message always helps :-). See
> > http://subversion.tigris.org/hacking.html#log-messages
> >
> > 2. You attached it with MIME-type "application/octet-stream", which
> > means that mailreaders can't display it inline. If you use
> > "text/plain" instead, that makes things a bit easier. See
> > http://subversion.tigris.org/hacking.html#patches
> >
> > The guideline for URI encoding and decoding is just to follow the doc
> > strings of whatever functions the data will be passed to. If the
> > function says it needs URI-encoded data, then make sure it receives
> > that. If it does not, then usually it takes un-encoded data.
> >
> > -Karl
>
> Please find attached a bit more polished patch with an appropriate test.
>
> [[[
> Fix "Malformed URL" error on "merge --reintegrate" for a path with spaces.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_file_with_space_in_its_path): New test.
> (test_list): Run the new test.
>
> * subversion/libsvn_client/merge.c
> (calculate_left_hand_side): URI-encode URLs constructed from URI-decoded
> relative paths.
> ]]]
1) Can you (or someone) confirm that the test fails without the fix, and
passes with the fix? I can only test on ra-local and ra-svn at the
moment, and this new test passed both, even without the fix. I'm not
sure if that's expected. I guess it means those protocols don't check
for well-formed URLs.
If the test and the fix are behaving, we should commit this.
2) Having become aware of this problem, if we look further, there are
lots more calls to svn_path_join() that look wrong. Especially, 'grep
"url.*svn_path_join"' in Subversion source code reveals a bunch that
mostly look wrong. It also reveals a few that use uri_encode() on the
second argument that could be replaced by svn_path_url_add_component().
Do you fancy (a) fixing those and/or (b) figuring out tests for
exercising those?
3) Can we modify our test suite to include paths with spaces in, and
thereby catch many URI encoding errors automatically? We can add
specific tests for specific cases, but I'm wondering if we can modify
the Greek Tree to include some nasty names that will get exercised
through several different code paths.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-08 23:22:59 CEST