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

[John Arbash Meinel]

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

Michael Ellerman wrote:
> On Thu, 2006-07-06 at 23:24 -0500, John Arbash Meinel wrote:

...

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

This looks okay. You should have also fixed the indentation of the
'exist.'... line.

In general, I agree with not catching Exception, or not having a blank
'except:'. But it turns out that:
try:
  foo()
except:
  raise

Actually returns the traceback from the 'foo' exception, it doesn't
reset it to start from where the 'raise' occurs.
So I think it is fine.

The only thing it really means is that if we get an exception, and then
delete_tree() also raises, it will hide the original exception.
The only way I know around it is:

except Exception, e:
  try:
    to_transport.delete_tree('.')
  except:
    raise e
  raise

That will raise the original exception delete_tree() fails, though you
lose the traceback. And if delete_tree() succeeds, it raises the
original exception with the original traceback.

Otherwise, I'm happy with the patch. I should probably review the whole
thing, though, rather than just this part.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFExmd/JdeBCYSNAAMRAoXHAJ4iinCfL8qdbWv8NohpMrsGc8oiWACgiMT/
7q4OQIrou8xILsdvEdBHEQo=
=xOps
-----END PGP SIGNATURE-----