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
1=== modified file 'NEWS'
2--- NEWS 2010-03-30 04:09:15 +0000
3+++ NEWS 2010-03-30 06:00:48 +0000
4@@ -543,6 +543,10 @@
5 another process already removed the file.
6 (John Arbash Meinel, Gareth White, #507557)
7
8+* Check whether a location is a directory before trying to read a bundle
9+ from it, to avoid problems on BSD where you can ``read()`` a directory.
10+ (Martin Pool, #484558)
11+
12 * Fix "Too many concurrent requests" in reconcile when network connection
13 fails. (Andrew Bennetts, #503878)
14
15@@ -628,9 +632,15 @@
16 API Changes
17 ***********
18
19+<<<<<<< TREE
20 * Added ``cachedproperty`` decorator to ``bzrlib.decorators``.
21 (Andrew Bennetts)
22
23+=======
24+* Delete deprecated function ``read_bundle_from_url``
25+ (Martin Pool)
26+
27+>>>>>>> MERGE-SOURCE
28 * Many test features were renamed from ``FooFeature`` to ``foo_feature``
29 to be consistent with instances being lower case and classes being
30 CamelCase. For the features that were more likely to be used, we added a
31
32=== modified file 'bzrlib/bundle/__init__.py'
33--- bzrlib/bundle/__init__.py 2010-02-17 17:11:16 +0000
34+++ bzrlib/bundle/__init__.py 2010-03-30 06:00:48 +0000
35@@ -1,4 +1,8 @@
36+<<<<<<< TREE
37 # Copyright (C) 2005-2010 Canonical Ltd
38+=======
39+# Copyright (C) 2005, 2006, 2010 Canonical Ltd
40+>>>>>>> MERGE-SOURCE
41 #
42 # This program is free software; you can redistribute it and/or modify
43 # it under the terms of the GNU General Public License as published by
44@@ -15,6 +19,7 @@
45 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
46
47 from cStringIO import StringIO
48+from stat import S_ISDIR
49
50 from bzrlib.symbol_versioning import deprecated_function, deprecated_in
51 from bzrlib.lazy_import import lazy_import
52@@ -33,11 +38,6 @@
53 from bzrlib.trace import note
54
55
56-@deprecated_function(deprecated_in((1, 12, 0)))
57-def read_bundle_from_url(url):
58- return read_mergeable_from_url(url, _do_directive=False)
59-
60-
61 def read_mergeable_from_url(url, _do_directive=True, possible_transports=None):
62 """Read mergable object from a given URL.
63
64@@ -55,6 +55,18 @@
65
66 def read_mergeable_from_transport(transport, filename, _do_directive=True):
67 def get_bundle(transport):
68+ try:
69+ stat = transport.stat(filename)
70+ if not S_ISDIR(stat.st_mode):
71+ # check first whether this is a directory; otherwise trying to read
72+ # from it may cause misbehaviour like bug 484558 on systems
73+ # like openbsd that let you read from directories.
74+ # XXX: Arguably this should check it's a file (or maybe a pipe)
75+ # instead?
76+ raise errors.NotABundle(
77+ '%s is a directory' % transport.abspath(filename))
78+ except errors.TransportNotPossible:
79+ pass
80 return StringIO(transport.get_bytes(filename)), transport
81
82 def redirected_transport(transport, exception, redirection_notice):
83
84=== modified file 'bzrlib/tests/test_http.py'
85--- bzrlib/tests/test_http.py 2010-03-18 23:11:15 +0000
86+++ bzrlib/tests/test_http.py 2010-03-30 06:00:48 +0000
87@@ -1334,14 +1334,6 @@
88 t = self._transport(self.new_server.get_url())
89 self.assertEqual('0123456789', t.get('a').read())
90
91- def test_read_redirected_bundle_from_url(self):
92- from bzrlib.bundle import read_bundle_from_url
93- url = self.old_transport.abspath('bundle')
94- bundle = self.applyDeprecated(deprecated_in((1, 12, 0)),
95- read_bundle_from_url, url)
96- # If read_bundle_from_url was successful we get an empty bundle
97- self.assertEqual([], bundle.revisions)
98-
99
100 class RedirectedRequest(_urllib2_wrappers.Request):
101 """Request following redirections. """
102
103=== modified file 'bzrlib/tests/test_read_bundle.py'
104--- bzrlib/tests/test_read_bundle.py 2009-07-10 07:14:02 +0000
105+++ bzrlib/tests/test_read_bundle.py 2010-03-30 06:00:48 +0000
106@@ -60,27 +60,6 @@
107 return out, wt
108
109
110-class TestDeprecations(tests.TestCaseInTempDir):
111-
112- def create_test_bundle(self):
113- out, wt = create_bundle_file(self)
114- f = open('test_bundle', 'wb')
115- try:
116- f.write(out.getvalue())
117- finally:
118- f.close()
119- return wt
120-
121- def test_read_bundle_from_url_deprecated(self):
122- wt = self.create_test_bundle()
123- t = bzrlib.transport.get_transport(self.test_dir)
124- url = t.abspath('test_bundle')
125- self.callDeprecated([deprecated_in((1, 12, 0))
126- % 'bzrlib.bundle.read_bundle_from_url'],
127- bzrlib.bundle.read_bundle_from_url,
128- url)
129-
130-
131 class TestReadBundleFromURL(TestTransportImplementation):
132 """Test that read_bundle works properly across multiple transports"""
133