space in url causes failure

Bug #496721 reported by Billie Cleek
18
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Bazaar Subversion Plugin
Fix Released
Medium
Jelmer Vernooij
Launchpad itself
Fix Released
Medium
Jelmer Vernooij

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.

Tags: lp-code

Related branches

Revision history for this message
Billie Cleek (cleekbi) wrote :
Revision history for this message
Billie Cleek (cleekbi) wrote :

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

Revision history for this message
Jelmer Vernooij (jelmer) 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.

Revision history for this message
Billie Cleek (cleekbi) wrote : Re: [Bug 496721] Re: space in url causes failure

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...

Revision history for this message
Jelmer Vernooij (jelmer) 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.

Revision history for this message
Billie Cleek (cleekbi) 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...

Revision history for this message
Jelmer Vernooij (jelmer) 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...
>

Revision history for this message
Billie Cleek (cleekbi) wrote :

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
>

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Billie,

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

Revision history for this message
Billie Cleek (cleekbi) wrote :

Jelmer,

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

Revision history for this message
Billie Cleek (cleekbi) wrote :

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.

Jelmer Vernooij (jelmer)
Changed in bzr-svn:
status: New → Triaged
importance: Undecided → Medium
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This is fixed in trunk now.

Changed in bzr-svn:
status: Triaged → Fix Committed
assignee: nobody → Jelmer Vernooij (jelmer)
Jelmer Vernooij (jelmer)
Changed in bzr-svn:
milestone: none → 1.0.3
Jelmer Vernooij (jelmer)
Changed in bzr-svn:
status: Fix Committed → Fix Released
Revision history for this message
Max Bowsher (maxb) wrote :
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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.

Changed in launchpad-code:
status: New → Fix Released
importance: Undecided → Medium
assignee: nobody → Jelmer Vernooij (jelmer)
milestone: none → 10.08
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.