PathNotChild, port mismatch with "bzr info" for bzr:// smartserver

 status triaged
 importance critical

Thanks for reporting this, it looks like a probably easy to fix
regression.

Thanks Robert for the feedback.

I also noticed "Repository branch (format: unnamed)" --> format unnamed... and should be dirstate-tags.

Guess is another bug of SmartServer :-P

On (22/08/07 02:25), Luis Lavena wrote:
> PathNotChild: Path 'bzr://bzr.mmediasys.net/licensr/engine/trunk/' is not a child of path 'bzr://bzr.mmediasys.net/licensr/engine/trunk/': port mismatch
>
> ====
>
> The same request with 0.18, on the same machine (swapped both
> installation) return this:
>
> D:\Users\Developer>bzr info bzr://bzr.mmediasys.net/licensr/engine/trunk
> Repository branch (format: unnamed)
> Location:
> shared repository: bzr://bzr.mmediasys.net/licensr/engine/
> repository branch: trunk

Should this regression stop the release? Should we put out an RC2 with
the fix, or release a 0.90.1 with the fix?

Thanks,

James

--
  James Westby -- GPG Key ID: B577FE13 -- http://jameswestby.net/
  seccure key - (3+)k7|M*edCX/.A:n*N!>|&7U.L#9E)Tu)T0>AM - secp256r1/nistp256

On (22/08/07 20:50), James Westby wrote:
> On (22/08/07 02:25), Luis Lavena wrote:
> > PathNotChild: Path 'bzr://bzr.mmediasys.net/licensr/engine/trunk/' is not a child of path 'bzr://bzr.mmediasys.net/licensr/engine/trunk/': port mismatch
> >
> > ====
> >
> > The same request with 0.18, on the same machine (swapped both
> > installation) return this:
> >
> > D:\Users\Developer>bzr info bzr://bzr.mmediasys.net/licensr/engine/trunk
> > Repository branch (format: unnamed)
> > Location:
> > shared repository: bzr://bzr.mmediasys.net/licensr/engine/
> > repository branch: trunk
>
> Should this regression stop the release? Should we put out an RC2 with
> the fix, or release a 0.90.1 with the fix?
>

I don't think we can release with this as a known issue, so I'm not
going to go ahead and complete the release tonight.

Can someone please create a fix for this issue, and then we can discuss
how to proceed?

Thanks,

James

--
  James Westby -- GPG Key ID: B577FE13 -- http://jameswestby.net/
  seccure key - (3+)k7|M*edCX/.A:n*N!>|&7U.L#9E)Tu)T0>AM - secp256r1/nistp256

I think I have the fix, bundle on the way shortly.

Hi,

Attached is a bundle for the 0.90 branch that attempts to fix this
issue.

The commit message hopefully explains the issue and the fix quite well,
but I am not sure it is the correct one, specifically is it now being
too lenient, in that it will allow two URIs to match that refer to
different ports?

There is also another case that seems like it may also cause issues,
namely _reuse_for in bzrlib/transport/__init__.py, as that compares
self._port to the output of _split_url. If someone that understands the
transport framework better than me could evaluate whether this could
cause bugs that would be great.

The reason the tests didn't get this bug in the first place is that the
implementation tests use urls with a specific, non-default port, which
doesn't trigger the issue, and the general tests didn't use bzr://.

Thanks,

James

--
  James Westby -- GPG Key ID: B577FE13 -- http://jameswestby.net/
  seccure key - (3+)k7|M*edCX/.A:n*N!>|&7U.L#9E)Tu)T0>AM - secp256r1/nistp256

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: <email address hidden>-\
# b62gwlx3souge25q
# target_branch: http://bazaar-vcs.org/bzr/bzr.0.90/
# testament_sha1: efb96fa837eea942baa224351ca4323d9481bad3
# timestamp: 2007-08-23 20:12:44 +0100
# base_revision_id: <email address hidden>
#
# Begin patch
=== modified file 'NEWS'
--- NEWS 2007-08-21 20:54:11 +0000
+++ NEWS 2007-08-23 19:12:30 +0000
@@ -18,6 +18,11 @@
       from the developer document catalog back to the main one.
       (Ian Clatworthy, Sabin Iacob, Alexander Belchenko)

+ BUGFIXES:
+
+ * Make the bzr:// transport work again for some operations (e.g. info)
+ when using the default port. (James Westby, #133965)
+

 bzr 0.90rc1 2007-08-14
 ======================

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py 2007-07-22 15:44:59 +0000
+++ bzrlib/tests/test_transport.py 2007-08-23 19:12:30 +0000
@@ -54,6 +54,7 @@
 from bzrlib.transport.memory import MemoryTransport
 from bzrlib.transport.local import (LocalTransport,
                                     EmulatedWin32LocalTransport)
+from bzrlib.transport.remote import RemoteTCPTransport

 # TODO: Should possibly split transport-specific tests into their own files.
@@ -665,6 +666,17 @@
         t = ConnectedTransport('sftp://host.com/dev/%path')
         self.assertEquals(t.relpath('sftp://host.com/dev/%path/sub'), 'sub')

+ def test_relpath_when_transport_uses_a_port(self):
+ """If no port is specified then it is the same as the default.
+
+ When the transport uses a port then relpath assumes that
+ the request must have the same port, otherwise it is not a child.
+ However if the request has no port then the default port is
+ used, and this will match what the server is listening on.
+ """
+ t = RemoteTCPTransport('bzr://host.com/')
+ self.assertEquals(t.relpath('bzr://host.com/'), '')
+
     def test_connection_sharing_propagate_credentials(self):
         t = ConnectedTransport('foo://<email address hidden>/abs/path')
         self.assertIs(None, t._get_connection())

=== modi...

bb:resubmit

On Thu, 2007-08-23 at 21:39 +0100, James Westby wrote:
> Hi,
>
> Attached is a bundle for the 0.90 branch that attempts to fix this
> issue.
>
> The commit message hopefully explains the issue and the fix quite well,
> but I am not sure it is the correct one, specifically is it now being
> too lenient, in that it will allow two URIs to match that refer to
> different ports?

This is clearly wrong.

The right way to handle this is to use the default port number if none
has been explicitly provided.

E.g. to do a comparison with http, you need to supply 80, for https 443,
etc.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

Hi all,

Attached is the fix that I am proposing for 0.90. It is John's fix, with
the parts of Andrew's test case that are appropriate for this fix.
Robert was keen that the same fix be applied to 0.90 and trunk, but I
can't see that a fix has been decided on for trunk. Also this is the fix
that Vincent suggested would be good for this release.

Assuming this is accepted and merged soon I would like to make another
release tomorrow. The question is whether it should be a second release
candidate or the final release. It would normally be a second release
candidate, but there are a couple of factors in play. The first is that
Gutsy is freezing soon, and I guess some people would want 0.90 in the
release. The second is that a release candidate and then a release in a
week means that there will only be one week before the release candidate
of 0.91.

I propose to release a second release candidate tomorrow, but if anyone
feels strongly that it should be the final release please speak up. If
that is the case then perhaps Martin should have the final say in the
matter.

If the second release candidate is decided on then could someone please
merge the second bundle attached in to the 0.90 branch so that it is
ready for me to do the release from it when I get home, and to avoid
unnecessarily dragging out the release further. The third bundle is in
the event that a full release is chosen, it merely updates the date
recorded in NEWS.

Thanks,

James

--
  James Westby -- GPG Key ID: B577FE13 -- http://jameswestby.net/
  seccure key - (3+)k7|M*edCX/.A:n*N!>|&7U.L#9E)Tu)T0>AM - secp256r1/nistp256

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: <email address hidden>-\
# 672l6nisr283ybdc
# target_branch: http://bazaar-vcs.org/bzr/bzr.0.90/
# testament_sha1: 5fe509622ea1c6904199e90ff155694122eadee4
# timestamp: 2007-08-27 22:04:22 +0100
# base_revision_id: <email address hidden>
#
# Begin patch
=== modified file 'NEWS'
--- NEWS 2007-08-21 20:54:11 +0000
+++ NEWS 2007-08-27 21:01:57 +0000
@@ -18,6 +18,13 @@
       from the developer document catalog back to the main one.
       (Ian Clatworthy, Sabin Iacob, Alexander Belchenko)

+ BUGFIXES:
+
+ * Fix ''bzr info bzr://host/'' and other operations on ''bzr://' URLs with
+ an implicit port. We were incorrectly raising PathNotChild due to
+ inconsistent treatment of the ''_port'' attribute on the Transport object.
+ (John Arbash Meinel, Andrew Bennetts, #133965)
+

 bzr 0.90rc1 2007-08-14
 ======================

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py 2007-07-22 15:44:59 +0000
+++ bzrlib/tests/test_transport.py 2007-08-27 21:01:57 +0000
@@ -54,6 +54,9 @@
 from bzrlib.transport.memory import MemoryTransport
 from bzrlib.transport.local import (LocalTransport,
                                     EmulatedWin32LocalTransport)
+from bzrlib.transport.remote import (BZR_DEFAULT_PORT,
+ RemoteTCPTransport,
+ )

 # TODO: Should possibly split transport-specific tests into their own files.
@@ -714,6 +...

On Mon, 2007-08-27 at 21:18 +0000, James Westby wrote:
> Hi all,
>
> Attached is the fix that I am proposing for 0.90. It is John's fix, with
> the parts of Andrew's test case that are appropriate for this fix.
> Robert was keen that the same fix be applied to 0.90 and trunk, but I
> can't see that a fix has been decided on for trunk. Also this is the fix
> that Vincent suggested would be good for this release.
>
> Assuming this is accepted and merged soon I would like to make another
> release tomorrow. The question is whether it should be a second release
> candidate or the final release. It would normally be a second release
> candidate, but there are a couple of factors in play. The first is that
> Gutsy is freezing soon, and I guess some people would want 0.90 in the
> release. The second is that a release candidate and then a release in a
> week means that there will only be one week before the release candidate
> of 0.91.
>
> I propose to release a second release candidate tomorrow, but if anyone
> feels strongly that it should be the final release please speak up. If
> that is the case then perhaps Martin should have the final say in the
> matter.

I feel strongly that as no other issues have arrived it should be the
final release. We're 2 weeks into 0.91 at this point.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

Robert Collins wrote:

>> I propose to release a second release candidate tomorrow, but if anyone
>> feels strongly that it should be the final release please speak up. If
>> that is the case then perhaps Martin should have the final say in the
>> matter.
>
> I feel strongly that as no other issues have arrived it should be the
> final release. We're 2 weeks into 0.91 at this point.

I agree with Rob - let's ship 0.90 instead of another RC. In theory, the
risk is isolated to side effects of this change. Quite a few eyeballs
have looked over this change and debated it, so the risk ought to be
low, yes?

Ian C.

James Westby wrote:
> Hi all,
>
> Attached is the fix that I am proposing for 0.90. It is John's fix, with
> the parts of Andrew's test case that are appropriate for this fix.
> Robert was keen that the same fix be applied to 0.90 and trunk, but I
> can't see that a fix has been decided on for trunk. Also this is the fix
> that Vincent suggested would be good for this release.

FWIW, this fix looks fine to me.

-Andrew.

>>>>> "James" == James Westby <email address hidden> writes:

    James> Hi all,
    James> Attached is the fix that I am proposing for 0.90. It is John's fix, with
    James> the parts of Andrew's test case that are appropriate for this fix.
    James> Robert was keen that the same fix be applied to 0.90 and trunk, but I
    James> can't see that a fix has been decided on for trunk. Also this is the fix
    James> that Vincent suggested would be good for this release.

Still catching up so no time to propose a better fix, I think
this one is appropriate for the time being as it fixes the
regression.

Having read further remarks by spiv and Robert I also agree with
them about a better fix and will comment in the other threads RSN.

     Vincent

On (27/08/07 21:40), Robert Collins wrote:
> I feel strongly that as no other issues have arrived it should be the
> final release. We're 2 weeks into 0.91 at this point.

Ok, two for the final release, and no one else speaking up against, so
we should go for this. Could someone please merge the attached two
bundles. (The second wont show up in BB I believe, but it is trivial).

The second does rely on both getting merged by the end of today. Would
it be acceptable to do a release from my local branch rather than a PQM
branch if that is not the case?

Thanks,

James

--
  James Westby -- GPG Key ID: B577FE13 -- http://jameswestby.net/
  seccure key - (3+)k7|M*edCX/.A:n*N!>|&7U.L#9E)Tu)T0>AM - secp256r1/nistp256

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: <email address hidden>-\
# 672l6nisr283ybdc
# target_branch: http://bazaar-vcs.org/bzr/bzr.0.90/
# testament_sha1: 5fe509622ea1c6904199e90ff155694122eadee4
# timestamp: 2007-08-27 22:04:22 +0100
# base_revision_id: <email address hidden>
#
# Begin patch
=== modified file 'NEWS'
--- NEWS 2007-08-21 20:54:11 +0000
+++ NEWS 2007-08-27 21:01:57 +0000
@@ -18,6 +18,13 @@
       from the developer document catalog back to the main one.
       (Ian Clatworthy, Sabin Iacob, Alexander Belchenko)

+ BUGFIXES:
+
+ * Fix ''bzr info bzr://host/'' and other operations on ''bzr://' URLs with
+ an implicit port. We were incorrectly raising PathNotChild due to
+ inconsistent treatment of the ''_port'' attribute on the Transport object.
+ (John Arbash Meinel, Andrew Bennetts, #133965)
+

 bzr 0.90rc1 2007-08-14
 ======================

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py 2007-07-22 15:44:59 +0000
+++ bzrlib/tests/test_transport.py 2007-08-27 21:01:57 +0000
@@ -54,6 +54,9 @@
 from bzrlib.transport.memory import MemoryTransport
 from bzrlib.transport.local import (LocalTransport,
                                     EmulatedWin32LocalTransport)
+from bzrlib.transport.remote import (BZR_DEFAULT_PORT,
+ RemoteTCPTransport,
+ )

 # TODO: Should possibly split transport-specific tests into their own files.
@@ -714,6 +717,31 @@
         self.assertIsNot(t1, t2)

+class TestRemoteTCPTransport(TestCase):
+ """Tests for bzr:// transport (RemoteTCPTransport)."""
+
+ def test_relpath_with_implicit_port(self):
+ """Connected transports with the same URL are the same, even if the
+ port is implicit.
+
+ So t.relpath(url) should always be '' if t.base is the same as url, or
+ if the only difference is that one explicitly specifies the default
+ port and the other doesn't specify a port.
+ """
+ t_implicit_port = RemoteTCPTransport('bzr://host.com/')
+ self.assertEquals('', t_implicit_port.relpath('bzr://host.com/'))
+ t_explicit_port = RemoteTCPTransport('bzr://host.com:4155/')
+ self.assertEquals('', t_explicit_port.relpath('bzr://host.com:4155/'))
+
+ def test_url_includes_non_default_port(self):
+ """Non-defau...

The fix for 0.90 wasn't liked for bzr.dev, so we need another.

Andrew posted a 0.91 patch for review.

Merged to 0.91; still needs merge for bzr.dev?

The fix from 0.91 got merged into bzr.dev.