[MERGE] TreeTransform rolls back on exception (#67699)

[Kuno Meyer]

On 23.08.2007 21:35, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Kuno Meyer wrote:
>> It's really great news to see a TreeTransform rollback. I played around
>> a bit, and I've got the following points:
>>
>> On 21.08.2007 17:42, Aaron Bentley wrote:
>>
>> [skip]
>>> === modified file 'bzrlib/transform.py'
>>> --- bzrlib/transform.py    2007-08-16 05:37:08 +0000
>>> +++ bzrlib/transform.py    2007-08-21 15:26:59 +0000
>>> @@ -108,6 +108,9 @@
>>>              except OSError, e:
>>>                  if e.errno == errno.EEXIST:
>>>                      raise ExistingLimbo(self._limbodir)
>>> +            self._deletiondir = urlutils.local_path_from_url(
>>> +                control_files.controlfilename('pending-deletion'))
>>> +            os.mkdir(self._deletiondir)
>> Why not complain about an already existing pending-deletion dir? As far
>> as I can see, the reasons for complaining are not that different than in
>> the case of the already existing limbo dir: if pending-deletion exists,
>> then a tree transformation has unexpectedly terminated and name
>> conflicts during further operation are very likely to occur.
> 
> Yes, it's pretty much the same.  I just assume pending-deletion and
> limbo will either both exist or both not exist.

Ok. And why not enforce this assumption?

> 
>>> +            os.rmdir(self._deletiondir)
>> Why not use osutils.rmtree()? It would deal with write-protected files
>> which can be moved, but not deleted (on Windows).
> 
> At this point, the directory should be empty.

My misunderstanding. I realized it after I sent the mail.

> 
>> It would be a big gain in user-friendliness if bazaar would tell the
>> user which file causes the conflict
> 
> I think Python exceptions should include the pathname.
> 
> Failing that, we should be raising our own exceptions that include the
> pathname.

At least on my platform is the 'filename' attribute of the 'OSError' 
exception None when trying to revert a locked file.

(Python 2.4.3; Win32. I was not able to find an according error report 
in the Python issue tracker, so I'm not sure whether this is intended.)

> 
>> , for example by a warning message
>> before re-raising exceptions at
> 
> I think warnings aren't the right way to handle errors.
> 
> But this submission is aimed at ending the data-loss potential, not
> making it more friendly.

Nevertheless, it would be the very point where we have this information.

Having the filename in the exception certainly makes it easier to treat 
the error condition programatically, but having it in the warning output 
at least gives the (human) user a hint.

Kuno