space in url causes failure

I mistakenly attached transport.py instead of the patch last time. Here is the patch as an attachment.

Thanks for the patch.

Please avoid UTF8-encoding the url, it should already be in UTF8.

It would also be nice to have a test for this, as we'd like to avoid double escaping spaces.

The url was already being encoded in utf-8; all I did was to add
urllib.quote. would you like for me to submit a patch that tests for the
need to quote before calling urllib.quote? Or did you mean to create a unit
test?

Billie

sent from my droid

On Dec 25, 2009 4:55 AM, "Jelmer Vernooij" <email address hidden> wrote:

Thanks for the patch.

Please avoid UTF8-encoding the url, it should already be in UTF8.

It would also be nice to have a test for this, as we'd like to avoid
double escaping spaces.

-- space in url causes failure https://bugs.launchpad.net/bugs/496721 You
received this bug notifi...

- ret = subvertpy.ra.RemoteAccess(url, auth=auth,
+ ret = subvertpy.ra.RemoteAccess(urllib.quote(url.encode('utf8'), '/:'), auth=auth,

You added an additional .encode('utf8') there.

If you could add a unit test that made sure that URLs with spaces in them worked, that would be great. Such a test would probably belong in tests/test_transport.py.

You are right. Sorry about that. I am pretty sure that was the only way I
could get it to work correctly. I will strip out the utf-8 encoding, add a
unit test and resubmit. Do you prefer a patch or a pushed branch?

Billie

sent from my droid

On Dec 25, 2009 8:15 AM, "Jelmer Vernooij" <email address hidden> wrote:

- ret = subvertpy.ra.RemoteAccess(url, auth=auth,
+ ret = subvertpy.ra.RemoteAccess(urllib.quote(url.encode('utf8'),
'/:'), auth=auth,

You added an additional .encode('utf8') there.

If you could add a unit test that made sure that URLs with spaces in
them worked, that would be great. Such a test would probably belong in
tests/test_transport.py.

-- space in url causes failure https://bugs.launchpad.net/bugs/496721

You received this bug notification because you are a direct subscriber of
the bug. Status in Subver...

Thanks! I'd prefer a branch.

Cheers,

Jelmer

On Fri, 2009-12-25 at 22:36 +0000, Billie Cleek wrote:
> You are right. Sorry about that. I am pretty sure that was the only way I
> could get it to work correctly. I will strip out the utf-8 encoding, add a
> unit test and resubmit. Do you prefer a patch or a pushed branch?
>
> Billie
>
> sent from my droid
>
> On Dec 25, 2009 8:15 AM, "Jelmer Vernooij" <email address hidden> wrote:
>
> - ret = subvertpy.ra.RemoteAccess(url, auth=auth,
> + ret = subvertpy.ra.RemoteAccess(urllib.quote(url.encode('utf8'),
> '/:'), auth=auth,
>
> You added an additional .encode('utf8') there.
>
> If you could add a unit test that made sure that URLs with spaces in
> them worked, that would be great. Such a test would probably belong in
> tests/test_transport.py.
>
> -- space in url causes failure https://bugs.launchpad.net/bugs/496721
>
> You received this bug notification because you are a direct subscriber of
> the bug. Status in Subver...
>

Jelmer,

Sorry for the delay. I have some unit tests coded, and I have made some
changes to the patch to use _url_encode_uri instead of urllib.quote
directly. However, I am encountering problems getting the unit tests to run
on my Windows machine. I am going to push the branch o Launchpad and then
pull it to my OS X box to try the unit tests. In the event that I cannot
easily get the unit tests to run on Snow Leopard, would you be willing to
run the unit tests and possibly make tweaks as necessary? The changes are
VERY small.

On Fri, Dec 25, 2009 at 6:38 PM, Jelmer Vernooij <email address hidden> wrote:

> Thanks! I'd prefer a branch.
>
> Cheers,
>
> Jelmer
>
> On Fri, 2009-12-25 at 22:36 +0000, Billie Cleek wrote:
> > You are right. Sorry about that. I am pretty sure that was the only way I
> > could get it to work correctly. I will strip out the utf-8 encoding, add
> a
> > unit test and resubmit. Do you prefer a patch or a pushed branch?
> >
> > Billie
> >
> > sent from my droid
> >
> > On Dec 25, 2009 8:15 AM, "Jelmer Vernooij" <email address hidden> wrote:
> >
> > - ret = subvertpy.ra.RemoteAccess(url, auth=auth,
> > + ret = subvertpy.ra.RemoteAccess(urllib.quote(url.encode('utf8'),
> > '/:'), auth=auth,
> >
> > You added an additional .encode('utf8') there.
> >
> > If you could add a unit test that made sure that URLs with spaces in
> > them worked, that would be great. Such a test would probably belong in
> > tests/test_transport.py.
> >
> > -- space in url causes failure https://bugs.launchpad.net/bugs/496721
> >
> > You received this bug notification because you are a direct subscriber of
> > the bug. Status in Subver...
> >
>
> --
> space in url causes failure
> https://bugs.launchpad.net/bugs/496721
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in Subversion branch support for Bazaar: New
>
> Bug description:
> A space in the branch name of an https URL always causes bzr-svn commands
> to fail. Attempting to checkout a Bazaar branch of a Subversion branch
> fails, as does attempting to perform a pull operation on a Bazaar branch
> created by using svn-import when the Subversion branch name contains a
> space. I tested using double quotes (e.g. "
> https://server/svn/proj/branches/my branch"), escape sequences (e.g.
> https://server/svn/proj/branches/my%20branch), and combining the two. I
> was able to track the problem down to transport.py and its failure to escape
> urls.
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/bzr-svn/+bug/496721/+subscribe
>

Hi Billie,

Yeah, I'd be happy to run the unit tests.

Jelmer,

I pushed the branch to ~cleekbi/bzr-svn/allow-spaces. If you could please run the unit tests, I would be much obliged.

I would really like to see this bug resolved, as it is a barrier to adoption of Bazaar for our organization. I have updated the branch with relevant information, and I am happy to do whatever work needs to be done to see this through.

I believe I may have found a related bug that affects repositories whose names need to be escaped. Every repository that I have tested against whose name does not conform to URL specs (e.g. gtk+, an internal repository whose name contains a space) results in the first character of the name of the the directory following the root being removed (e.g. https://svn.gnome.org/svn/gtk+/trunk becomes https://svn.gnome.org/svn/gtk+/runk). For this reason, testing against gtk+ may not be the best way to validate my fixes. Is there another repository that we can test against? Should I just attach a sample repository to this issue?

I will try to tackle the related bug that I described above very soon.

This is fixed in trunk now.

Seemingly not quite fixed on Launchpad:

https://code.edge.launchpad.net/~paul-climbing/winff/trunk
https://code.edge.launchpad.net/~gusions/ubucompilator/trunk

That's a different bug, related to the way launchpad calls bzr-svn. Both imports are for paths inside of a branch rather than a branch itself.