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

[Martin Pool]

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.

[John Szakmeister (jszakmeister)]

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

[John A Meinel (jameinel)]

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

[John A Meinel (jameinel)]

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

[Martin Pool (mbp)]

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

[Martin Pool (mbp)]

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

Unmerged revisions

4964. By Martin Pool on 2010-01-15

Check first whether a bundle location is a directory

4963. By Martin Pool on 2010-01-15

Remove tests for read_bundle_from_url

4962. By Martin Pool on 2010-01-14

Delete deprecated read_bundle_from_url

=== modified file 'NEWS'
--- NEWS	2010-03-30 04:09:15 +0000
+++ NEWS	2010-03-30 06:00:48 +0000
@@ -543,6 +543,10 @@
   another process already removed the file.
   (John Arbash Meinel, Gareth White, #507557)
 
+* Check whether a location is a directory before trying to read a bundle
+  from it, to avoid problems on BSD where you can ``read()`` a directory.
+  (Martin Pool, #484558)
+
 * Fix "Too many concurrent requests" in reconcile when network connection
   fails.  (Andrew Bennetts, #503878)
 
@@ -628,9 +632,15 @@
 API Changes
 ***********
 
+<<<<<<< TREE
 * Added ``cachedproperty`` decorator to ``bzrlib.decorators``.
   (Andrew Bennetts)
 
+=======
+* Delete deprecated function ``read_bundle_from_url``
+  (Martin Pool)
+
+>>>>>>> MERGE-SOURCE
 * 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
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -15,6 +19,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 from cStringIO import StringIO
+from stat import S_ISDIR
 
 from bzrlib.symbol_versioning import deprecated_function, deprecated_in
 from bzrlib.lazy_import import lazy_import
@@ -33,11 +38,6 @@
 from bzrlib.trace import note
 
 
-@deprecated_function(deprecated_in((1, 12, 0)))
-def read_bundle_from_url(url):
-    return read_mergeable_from_url(url, _do_directive=False)
-
-
 def read_mergeable_from_url(url, _do_directive=True, possible_transports=None):
     """Read mergable object from a given URL.
 
@@ -55,6 +55,18 @@
 
 def read_mergeable_from_transport(transport, filename, _do_directive=True):
     def get_bundle(transport):
+        try:
+            stat = transport.stat(filename)
+            if not S_ISDIR(stat.st_mode):
+                # check first whether this is a directory; otherwise trying to read
+                # from it may cause misbehaviour like bug 484558 on systems
+                # like openbsd that let you read from directories.  
+                # XXX: Arguably this should check it's a file (or maybe a pipe)
+                # instead?
+                raise errors.NotABundle(
+                    '%s is a directory' % transport.abspath(filename))
+        except errors.TransportNotPossible:
+            pass
         return StringIO(transport.get_bytes(filename)), transport
 
     def redirected_transport(transport, exception, redirection_notice):

=== 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 @@
         t = self._transport(self.new_server.get_url())
         self.assertEqual('0123456789', t.get('a').read())
 
-    def test_read_redirected_bundle_from_url(self):
-        from bzrlib.bundle import read_bundle_from_url
-        url = self.old_transport.abspath('bundle')
-        bundle = self.applyDeprecated(deprecated_in((1, 12, 0)),
-                read_bundle_from_url, url)
-        # If read_bundle_from_url was successful we get an empty bundle
-        self.assertEqual([], bundle.revisions)
-
 
 class RedirectedRequest(_urllib2_wrappers.Request):
     """Request following redirections. """

=== 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 @@
     return out, wt
 
 
-class TestDeprecations(tests.TestCaseInTempDir):
-
-    def create_test_bundle(self):
-        out, wt = create_bundle_file(self)
-        f = open('test_bundle', 'wb')
-        try:
-            f.write(out.getvalue())
-        finally:
-            f.close()
-        return wt
-
-    def test_read_bundle_from_url_deprecated(self):
-        wt = self.create_test_bundle()
-        t = bzrlib.transport.get_transport(self.test_dir)
-        url = t.abspath('test_bundle')
-        self.callDeprecated([deprecated_in((1, 12, 0))
-                             % 'bzrlib.bundle.read_bundle_from_url'],
-                            bzrlib.bundle.read_bundle_from_url,
-                            url)
-
-
 class TestReadBundleFromURL(TestTransportImplementation):
     """Test that read_bundle works properly across multiple transports"""