[MERGE] Make cmd_branch check the destination before doing any I/O on the source.

[Michael Ellerman]

On Thu, 2006-07-06 at 23:24 -0500, John Arbash Meinel wrote:
> Michael Ellerman wrote:
> > On Thu, 2006-07-06 at 09:06 -0500, John Arbash Meinel wrote:
> >> Michael Ellerman wrote:
> >>> Hi list,
> >>>
> >>> This rearranges some logic in cmd_branch so that we check the destination
> >>> before doing any I/O on the source branch. This is nice for branching
> >>> from a remote to a local branch on slow links, if there's something wrong
> >>> with the destination you find out quickly. A test is included.
> >>  
> >> Good but not great. The reason we connect to the remote branch first is
> >> that we don't want to create the local directory if the user types the
> >> wrong remote path. Which is probably more common than a user running
> >> into a local error.

> I wasn't thinking to completely scrap it, just cleanup the mkdir if you
> fail the rest.
> (Just like our locking lock A, then B, but if B fails, unlock A)

OK, I've got this, which works, but triggers my "never catch Exception"
handler. Any ideas on how to do it better?

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py  2006-07-22 07:34:45 +0000
+++ bzrlib/builtins.py  2006-07-22 07:56:53 +0000
@@ -659,13 +659,17 @@
                                   to_location)

         try:
-            br_from = Branch.open(from_location)
-        except OSError, e:
-            if e.errno == errno.ENOENT:
-                raise BzrCommandError('Source location "%s" does not'
+            try:
+                br_from = Branch.open(from_location)
+            except OSError, e:
+                if e.errno == errno.ENOENT:
+                    raise BzrCommandError('Source location "%s" does not'
                                       ' exist.' % to_location)
-            else:
-                raise
+                else:
+                    raise
+        except Exception:
+            to_transport.delete_tree('.')
+            raise

         br_from.lock_read()
         try:

=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py        2006-07-22 07:49:34 +0000
+++ bzrlib/tests/blackbox/test_branch.py        2006-07-22 07:54:48 +0000
@@ -166,12 +166,20 @@
         self.run_bzr('branch', '--no-checkout', 'a', 'repo/d')
         self.assertFalse(os.path.exists('repo/d/file'))

-    def test_branch_dest_first(self):
+    def test_branch_failure_local_first(self):
         """Make sure branch checks the destination before doing any I/O
         on the source branch. This is nice for branching from a remote
         to a local branch on slow links, if there's something wrong with
-        the destination you find out quickly."""
+        the destination you find out quickly. We know it's doing no I/O
+        on the source because it doesn't exist and so would raise a different
+        error."""
         wt = self.make_branch_and_tree('a')
         self.build_tree_contents([('a/file', 'hello world\n')])
         out, err = self.runbzr('branch non-existant a/file', retcode=3)
         self.assertContainsRe(err, 'Target directory "a/file" already exists')
+
+    def test_branch_failure_open_cleansup(self):
+        """Make sure if we can't open the source branch that the destination
+        directory is deleted."""
+        out, err = self.runbzr('branch non-existant a', retcode=3)
+        self.assertFalse(os.path.exists('a'))



-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060722/b001256f/attachment.pgp