Merge lp:~spiv/bzr/dir_to-423563 into lp:bzr/2.0

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This fixes bug 423563, a traceback when doing push --use-existing-dir to a location that exists and contains a .bzr directory that is not actually a valid BzrDir. This patch will cause a simple error instead: "Target directory FOO already contains a .bzr directory, but it is not valid."

(I think there's a deeper API issue I haven't addressed here: part of the problem is that BzrDir.open_from_transport doesn't differentiate between "doesn't exist" and "exists but has corrupt/invalid bzrdir". Similarly, Branch.create_clone_on_transport raises FileExists both for "the destination directory already exists (use_existing_dir would suppress this)" and for "cannot create a bzrdir in the destination because one already exists." So the obvious use of the API that tries open_from_transport, and simply falls back to create_clone_on_transport needs some pretty precise error handling to do the right thing in all cases. I think solving that properly would be a lot of work, though.)

This patch I think is small and safe enough that it's a candidate for 2.0.x.

[Martin Pool (mbp)]

blah

[Martin Pool (mbp)]

sorry, i thought i was on staging ;-)

[Martin Pool (mbp)]

this seems plausible.

you need news.

i feel like you shouldn't be hardcoding '.bzr' here but it's an improvement for now.

[Martin Pool (mbp)]

spurious review

review: Approve (doc)

[Martin Pool (mbp)]

review: Approve

[Robert Collins (lifeless)]

Andrew, just a ping, to say 'please land' :)

=== modified file 'NEWS'
--- NEWS	2009-11-20 01:39:36 +0000
+++ NEWS	2009-12-02 01:36:11 +0000
@@ -20,6 +20,10 @@
 Bug Fixes
 *********
 
+* ``bzr push --use-existing-dir`` no longer crashes if the directory
+  exists but contains an invalid ``.bzr`` directory.
+  (Andrew Bennetts, #423563)
+
 * Content filters are now applied correctly after pull, merge and switch.
   (Ian Clatworthy, #385879)
 

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2009-08-19 18:04:49 +0000
+++ bzrlib/branch.py	2009-12-02 01:36:12 +0000
@@ -1284,16 +1284,9 @@
         # clone call. Or something. 20090224 RBC/spiv.
         if revision_id is None:
             revision_id = self.last_revision()
-        try:
-            dir_to = self.bzrdir.clone_on_transport(to_transport,
-                revision_id=revision_id, stacked_on=stacked_on,
-                create_prefix=create_prefix, use_existing_dir=use_existing_dir)
-        except errors.FileExists:
-            if not use_existing_dir:
-                raise
-        except errors.NoSuchFile:
-            if not create_prefix:
-                raise
+        dir_to = self.bzrdir.clone_on_transport(to_transport,
+            revision_id=revision_id, stacked_on=stacked_on,
+            create_prefix=create_prefix, use_existing_dir=use_existing_dir)
         return dir_to.open_branch()
 
     def create_checkout(self, to_location, revision_id=None,

=== modified file 'bzrlib/push.py'
--- bzrlib/push.py	2009-07-18 21:09:00 +0000
+++ bzrlib/push.py	2009-12-02 01:36:11 +0000
@@ -90,12 +90,19 @@
             br_to = br_from.create_clone_on_transport(to_transport,
                 revision_id=revision_id, stacked_on=stacked_on,
                 create_prefix=create_prefix, use_existing_dir=use_existing_dir)
-        except errors.FileExists:
+        except errors.FileExists, err:
+            if err.path.endswith('/.bzr'):
+                raise errors.BzrCommandError(
+                    "Target directory %s already contains a .bzr directory, "
+                    "but it is not valid." % (location,))
             if not use_existing_dir:
                 raise errors.BzrCommandError("Target directory %s"
-                     " already exists, but does not have a valid .bzr"
+                     " already exists, but does not have a .bzr"
                      " directory. Supply --use-existing-dir to push"
                      " there anyway." % location)
+            # This shouldn't occur, but if it does the FileExists error will be
+            # more informative than an UnboundLocalError for br_to.
+            raise
         except errors.NoSuchFile:
             if not create_prefix:
                 raise errors.BzrCommandError("Parent directory of %s"

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2009-08-20 04:09:58 +0000
+++ bzrlib/tests/blackbox/test_push.py	2009-12-02 01:36:11 +0000
@@ -319,6 +319,17 @@
         # The push should have created target/a
         self.failUnlessExists('target/a')
 
+    def test_push_use_existing_into_empty_bzrdir(self):
+        """'bzr push --use-existing-dir' into a dir with an empty .bzr dir
+        fails.
+        """
+        tree = self.create_simple_tree()
+        self.build_tree(['target/', 'target/.bzr/'])
+        self.run_bzr_error(
+            ['Target directory ../target already contains a .bzr directory, '
+             'but it is not valid.'],
+            'push ../target --use-existing-dir', working_dir='tree')
+
     def test_push_onto_repo(self):
         """We should be able to 'bzr push' into an existing bzrdir."""
         tree = self.create_simple_tree()