[MERGE] don't invoke os.rename on children with no contents

[Aaron Bentley]

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

Robert Collins wrote:
> +1 conditional
> 
> On Tue, 2007-06-05 at 18:34 -0400, Aaron Bentley wrote:
> 
> I think this test method could have a clearer name - perhaps
> 
> test_cancel_with_deleted_child_should_succeed

Okay.

>> +        try:
>> +            transform.cancel_creation(parent)
>> +        except OSError:
>> +            self.fail('Transform tried to move a deleted child')
> 
> I am guessing the try:except: here is to provide a clear error when the
> test fails, but perhaps just
>     transform.cancel_creation(child)
>     transform.cancel_creation(parent)
> 
> is clear enough?

Well, it seems to me that tests should not error.  They should pass or
fail.  It should be clear which operation you're testing.  Without the
try/except, I don't think it's clear.

However, in the interest of getting the fix in, I'll do as you ask, and
leave the discussion for later.

> It could after all raise many different errors if
> various bugs are added, and catching all OSError and interpreting them
> as this specific defect seems error prone in the case of other future
> changes.

In production code, I can see that.  But it's not like this will cause a
silent pass for the test.  The worst that happens is you get the wrong
failure.

>>          for trans_id in trans_ids:
>>              old_path = self._limbo_files[trans_id]
>>              new_path = self._limbo_name(trans_id, from_scratch=True)
>> -            os.rename(old_path, new_path)
>> +            if trans_id in self._new_contents:
>> +                os.rename(old_path, new_path)
> 
> Wouldn't:
>     for trans_id in trans_ids:
>         if trans_id not in self._new_contents:
>             continue
>         old_path = self._limbo_files[trans_id]
>         new_path = self._limbo_name(trans_id, from_scratch=True)
>         os.rename(old_path, new_path)
> 
> be better? Less calculations for the its-missing-case, and the same
> number of checks for the its-present-case.

That would leave the old limbo name in self._limbo_files, and as soon as
you tried to provide contents for trans_id, TreeTransform would blow up
real good.

Now what you could do is this:

     for trans_id in trans_ids:
         if trans_id not in self._new_contents:
             del self._limbo_files[trans_id]
             continue
         old_path = self._limbo_files[trans_id]
         new_path = self._limbo_name(trans_id, from_scratch=True)
         os.rename(old_path, new_path)

Deleting the self._limbo_files entry forces it to be generated from
scratch on the next usage, rather than generating it from scratch now.

I will do that, too, which I assume cures the conditional.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGZgmf0F+nu1YWqI0RAkH+AJ9mT7Bkiq7TJ9zSi47WmbUWskNPcACggrnt
EClJCBO3gQYi3Pu+obiTG4c=
=T31K
-----END PGP SIGNATURE-----