Bazaar

Merge lp:~mbp/bzr/484558-merge-directory into lp:bzr

  1. 484558-merge-directory
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Work in progress
Proposed branch: lp:~mbp/bzr/484558-merge-directory
Merge into: lp:bzr
Diff against target: 132 lines (+27/-34) (has conflicts)
4 files modified
NEWS (+10/-0)
bzrlib/bundle/__init__.py (+17/-5)
bzrlib/tests/test_http.py (+0/-8)
bzrlib/tests/test_read_bundle.py (+0/-21)
Text conflict in NEWS
Text conflict in bzrlib/bundle/__init__.py
To merge this branch: bzr merge lp:~mbp/bzr/484558-merge-directory
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+22430@code.launchpad.net

Description of the change

According to https://bugs.edge.launchpad.net/bzr/+bug/484558 openbsd's sftp server may hang if you try to read from a directory. The user who originally reported this can't reproduce the error in trunk, but on the whole I think this is still worth merging; it may occur in other cases.

To post a comment you must log in.
Revision history for this message
John Szakmeister (jszakmeister) wrote :

On Tue, Mar 30, 2010 at 2:00 AM, Martin Pool <email address hidden> wrote:
[snip]
> +<<<<<<< TREE
>  * Added ``cachedproperty`` decorator to ``bzrlib.decorators``.
>   (Andrew Bennetts)
>
> +=======
> +* Delete deprecated function ``read_bundle_from_url``
> +  (Martin Pool)
> +
> +>>>>>>> MERGE-SOURCE

It looks like conflict markers snuck in there.

>  * Many test features were renamed from ``FooFeature`` to ``foo_feature``
>   to be consistent with instances being lower case and classes being
>   CamelCase. For the features that were more likely to be used, we added a
>
> === modified file 'bzrlib/bundle/__init__.py'
> --- bzrlib/bundle/__init__.py   2010-02-17 17:11:16 +0000
> +++ bzrlib/bundle/__init__.py   2010-03-30 06:00:48 +0000
> @@ -1,4 +1,8 @@
> +<<<<<<< TREE
>  # Copyright (C) 2005-2010 Canonical Ltd
> +=======
> +# Copyright (C) 2005, 2006, 2010 Canonical Ltd
> +>>>>>>> MERGE-SOURCE

And here too. Although, is that right? How'd did it go from
2005-2010 to 2005, 2006, 2010?

-John

Reply
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Szakmeister wrote:
> On Tue, Mar 30, 2010 at 2:00 AM, Martin Pool <email address hidden> wrote:
> [snip]
>> +<<<<<<< TREE
>> * Added ``cachedproperty`` decorator to ``bzrlib.decorators``.
>> (Andrew Bennetts)
>>
>> +=======
>> +* Delete deprecated function ``read_bundle_from_url``
>> + (Martin Pool)
>> +
>> +>>>>>>> MERGE-SOURCE
>
> It looks like conflict markers snuck in there.
>
>> * Many test features were renamed from ``FooFeature`` to ``foo_feature``
>> to be consistent with instances being lower case and classes being
>> CamelCase. For the features that were more likely to be used, we added a
>>
>> === modified file 'bzrlib/bundle/__init__.py'
>> --- bzrlib/bundle/__init__.py 2010-02-17 17:11:16 +0000
>> +++ bzrlib/bundle/__init__.py 2010-03-30 06:00:48 +0000
>> @@ -1,4 +1,8 @@
>> +<<<<<<< TREE
>> # Copyright (C) 2005-2010 Canonical Ltd
>> +=======
>> +# Copyright (C) 2005, 2006, 2010 Canonical Ltd
>> +>>>>>>> MERGE-SOURCE
>
> And here too. Although, is that right? How'd did it go from
> 2005-2010 to 2005, 2006, 2010?
>
> -John

I don't know if he is using my plugin, but it tracks when the file was
actually touched. It is certainly possible that we didn't modify it in
2007/2008/2009.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuyL7cACgkQJdeBCYSNAAN7HQCfXQfp9i6cIu6q97z9obp7idnE
pTkAn2ncTZRMyNxUG9GLPVIrOv4pV8TG
=MFc/
-----END PGP SIGNATURE-----

Reply
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/484558-merge-directory into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #484558 bzr 2.0.0 hang on merge & pull from location when trying to read directory as a file
> https://bugs.launchpad.net/bugs/484558
>
>
> According to https://bugs.edge.launchpad.net/bzr/+bug/484558 openbsd's sftp server may hang if you try to read from a directory. The user who originally reported this can't reproduce the error in trunk, but on the whole I think this is still worth merging; it may occur in other cases.
>

Only concern is FTP, which I think pretends stat is available, but I'm
not sure what data it returns is actually reliable. (I know it does bad
things with mode bits, for example.)

Anyway, this looks good in general.

 merge: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuyM7EACgkQJdeBCYSNAANZ6QCgxm3N2btfEtIrjYti8ro1gEvc
wxgAniSrCUUoYahS7kDyHQ2Orv1UG/ne
=92K0
-----END PGP SIGNATURE-----

review: Approve
Reply
Revision history for this message
Martin Pool (mbp) wrote :

> On Tue, Mar 30, 2010 at 2:00 AM, Martin Pool <email address hidden> wrote:
> [snip]
> > +<<<<<<< TREE
> >  * Added ``cachedproperty`` decorator to ``bzrlib.decorators``.
> >   (Andrew Bennetts)
> >
> > +=======
> > +* Delete deprecated function ``read_bundle_from_url``
> > +  (Martin Pool)
> > +
> > +>>>>>>> MERGE-SOURCE
>
> It looks like conflict markers snuck in there.

No, it's actually that Launchpad is doing merge --preview and telling us there will be conflicts if it's merged. I need to resolve them by merging trunk before submitting.

> > === modified file 'bzrlib/bundle/__init__.py'
> > --- bzrlib/bundle/__init__.py   2010-02-17 17:11:16 +0000
> > +++ bzrlib/bundle/__init__.py   2010-03-30 06:00:48 +0000
> > @@ -1,4 +1,8 @@
> > +<<<<<<< TREE
> >  # Copyright (C) 2005-2010 Canonical Ltd
> > +=======
> > +# Copyright (C) 2005, 2006, 2010 Canonical Ltd
> > +>>>>>>> MERGE-SOURCE
>
> And here too. Although, is that right? How'd did it go from
> 2005-2010 to 2005, 2006, 2010?

I manually added 2010 to the list. In trunk somebody did a bulk update to the dash format.

Reply
Revision history for this message
Martin Pool (mbp) wrote :

> Only concern is FTP, which I think pretends stat is available, but I'm
> not sure what data it returns is actually reliable. (I know it does bad
> things with mode bits, for example.)

That's a really good point: if it says things that are really directories are files, we could end up not being able to merge from ftp branches, which would certainly suck - previously we would have just tried it. Similarly for http for that matter.

Perhaps we should be specifically checking "is the type known and is it not a directory" and then we can allow some transports to give noncommittal answers.

Given the only report of this bug has gone away perhaps we should reject this.

Reply

Unmerged revisions

4964. By Martin Pool

Check first whether a bundle location is a directory

4963. By Martin Pool

Remove tests for read_bundle_from_url

4962. By Martin Pool

Delete deprecated read_bundle_from_url

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-03-30 04:09:15 +0000
+++ NEWS 2010-03-30 06:00:48 +0000
@@ -543,6 +543,10 @@
543 another process already removed the file.543 another process already removed the file.
544 (John Arbash Meinel, Gareth White, #507557)544 (John Arbash Meinel, Gareth White, #507557)
545545
546* Check whether a location is a directory before trying to read a bundle
547 from it, to avoid problems on BSD where you can ``read()`` a directory.
548 (Martin Pool, #484558)
549
546* Fix "Too many concurrent requests" in reconcile when network connection550* Fix "Too many concurrent requests" in reconcile when network connection
547 fails. (Andrew Bennetts, #503878)551 fails. (Andrew Bennetts, #503878)
548552
@@ -628,9 +632,15 @@
628API Changes632API Changes
629***********633***********
630634
635<<<<<<< TREE
631* Added ``cachedproperty`` decorator to ``bzrlib.decorators``.636* Added ``cachedproperty`` decorator to ``bzrlib.decorators``.
632 (Andrew Bennetts)637 (Andrew Bennetts)
633638
639=======
640* Delete deprecated function ``read_bundle_from_url``
641 (Martin Pool)
642
643>>>>>>> MERGE-SOURCE
634* Many test features were renamed from ``FooFeature`` to ``foo_feature``644* Many test features were renamed from ``FooFeature`` to ``foo_feature``
635 to be consistent with instances being lower case and classes being645 to be consistent with instances being lower case and classes being
636 CamelCase. For the features that were more likely to be used, we added a646 CamelCase. For the features that were more likely to be used, we added a
637647
=== modified file 'bzrlib/bundle/__init__.py'
--- bzrlib/bundle/__init__.py 2010-02-17 17:11:16 +0000
+++ bzrlib/bundle/__init__.py 2010-03-30 06:00:48 +0000
@@ -1,4 +1,8 @@
1<<<<<<< TREE
1# Copyright (C) 2005-2010 Canonical Ltd2# Copyright (C) 2005-2010 Canonical Ltd
3=======
4# Copyright (C) 2005, 2006, 2010 Canonical Ltd
5>>>>>>> MERGE-SOURCE
2#6#
3# This program is free software; you can redistribute it and/or modify7# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by8# it under the terms of the GNU General Public License as published by
@@ -15,6 +19,7 @@
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA19# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
1620
17from cStringIO import StringIO21from cStringIO import StringIO
22from stat import S_ISDIR
1823
19from bzrlib.symbol_versioning import deprecated_function, deprecated_in24from bzrlib.symbol_versioning import deprecated_function, deprecated_in
20from bzrlib.lazy_import import lazy_import25from bzrlib.lazy_import import lazy_import
@@ -33,11 +38,6 @@
33from bzrlib.trace import note38from bzrlib.trace import note
3439
3540
36@deprecated_function(deprecated_in((1, 12, 0)))
37def read_bundle_from_url(url):
38 return read_mergeable_from_url(url, _do_directive=False)
39
40
41def read_mergeable_from_url(url, _do_directive=True, possible_transports=None):41def read_mergeable_from_url(url, _do_directive=True, possible_transports=None):
42 """Read mergable object from a given URL.42 """Read mergable object from a given URL.
4343
@@ -55,6 +55,18 @@
5555
56def read_mergeable_from_transport(transport, filename, _do_directive=True):56def read_mergeable_from_transport(transport, filename, _do_directive=True):
57 def get_bundle(transport):57 def get_bundle(transport):
58 try:
59 stat = transport.stat(filename)
60 if not S_ISDIR(stat.st_mode):
61 # check first whether this is a directory; otherwise trying to read
62 # from it may cause misbehaviour like bug 484558 on systems
63 # like openbsd that let you read from directories.
64 # XXX: Arguably this should check it's a file (or maybe a pipe)
65 # instead?
66 raise errors.NotABundle(
67 '%s is a directory' % transport.abspath(filename))
68 except errors.TransportNotPossible:
69 pass
58 return StringIO(transport.get_bytes(filename)), transport70 return StringIO(transport.get_bytes(filename)), transport
5971
60 def redirected_transport(transport, exception, redirection_notice):72 def redirected_transport(transport, exception, redirection_notice):
6173
=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py 2010-03-18 23:11:15 +0000
+++ bzrlib/tests/test_http.py 2010-03-30 06:00:48 +0000
@@ -1334,14 +1334,6 @@
1334 t = self._transport(self.new_server.get_url())1334 t = self._transport(self.new_server.get_url())
1335 self.assertEqual('0123456789', t.get('a').read())1335 self.assertEqual('0123456789', t.get('a').read())
13361336
1337 def test_read_redirected_bundle_from_url(self):
1338 from bzrlib.bundle import read_bundle_from_url
1339 url = self.old_transport.abspath('bundle')
1340 bundle = self.applyDeprecated(deprecated_in((1, 12, 0)),
1341 read_bundle_from_url, url)
1342 # If read_bundle_from_url was successful we get an empty bundle
1343 self.assertEqual([], bundle.revisions)
1344
13451337
1346class RedirectedRequest(_urllib2_wrappers.Request):1338class RedirectedRequest(_urllib2_wrappers.Request):
1347 """Request following redirections. """1339 """Request following redirections. """
13481340
=== modified file 'bzrlib/tests/test_read_bundle.py'
--- bzrlib/tests/test_read_bundle.py 2009-07-10 07:14:02 +0000
+++ bzrlib/tests/test_read_bundle.py 2010-03-30 06:00:48 +0000
@@ -60,27 +60,6 @@
60 return out, wt60 return out, wt
6161
6262
63class TestDeprecations(tests.TestCaseInTempDir):
64
65 def create_test_bundle(self):
66 out, wt = create_bundle_file(self)
67 f = open('test_bundle', 'wb')
68 try:
69 f.write(out.getvalue())
70 finally:
71 f.close()
72 return wt
73
74 def test_read_bundle_from_url_deprecated(self):
75 wt = self.create_test_bundle()
76 t = bzrlib.transport.get_transport(self.test_dir)
77 url = t.abspath('test_bundle')
78 self.callDeprecated([deprecated_in((1, 12, 0))
79 % 'bzrlib.bundle.read_bundle_from_url'],
80 bzrlib.bundle.read_bundle_from_url,
81 url)
82
83
84class TestReadBundleFromURL(TestTransportImplementation):63class TestReadBundleFromURL(TestTransportImplementation):
85 """Test that read_bundle works properly across multiple transports"""64 """Test that read_bundle works properly across multiple transports"""
8665