branch locking mk2.

[John A Meinel]

Robey Pointer wrote:
> On 13 Feb 2006, at 14:54, Robert Collins wrote:
> 
>> I think we have a slightly confusing transport api, which this branch
>> has shown up...
>>
>> fancy_rename is more of a fancy_replace function IMO, and its use by
>> windows and sftp to implement 'semi atomic renames that replace' is
>> misleading.
>>
>> So I propose that we
>>  * rename fancy_rename to fancy_replace or fancy_move
>>  * add a 'replace' or 'move' api to transport which explicitly is
>>    not atomic (lowest common denominator) [but is when it can be ] and
>>    which will replace existing files or directories.
>>  * define rename as being explicitly an atomic rename - it either
>>    succeeds or it does not. Its behaviour with respect to existing files
>>    and directories should then be only defined as well as we can:
>>      + it may or may not replace existing files.
>>      + renaming an empty directory on top of another empty one may
>>        succeed
>>      + renaming a non empty directory on top of another non empty
>>        directory will not etc.
> 
> Agreed, +1 in general.
> 
> There are still a few "leaks" of high-level implementation into
> Transports.  I guess once Martin's LockFile lands, we can remove
> lock_read and lock_write too.
> 
> After staring sideways at fancy_rename, I think this is high-level
> functionality that should *use* Transport instead of being *used by*
> Transport.  I think instead of passing in rename_func & unlink_func,
> fancy_rename really just wants to be passed a Transport which knows how
> to do these lower-level operations.  We could call it osutils.replace
> (or something) and not have it in Transport at all.  Does that make sense?
> 
> robey

The whole reason fancy_rename exists is because on Windows and sftp you
cannot rename a file over an existing file. And we had been depending on
"os.rename" to support atomic replace like that.
Transport.move() was hijacked, because it was effectively os.rename, and
we were expecting that behavior.

I suppose we could audit the code, and everywhere we want 'rename/move'
we switch to 'replace' to make it clear what we are doing.

I *don't* think we want to make fancy_rename a higher level than
transport. Because then all the clients would need to know if they are
using a Transport (or platform) which doesn't support atomic rename, and
then they need to activate the fancy rename.

It is just meant to give posix-like semantics to the rename function.
Obviously it has a failing in that it will move directories around. It
was never designed to be used on directories, which probably needs to be
corrected.

The issue is that simple things like AtomicFile, expect to be able to
replace an existing file. Its how we replace all weaves, everything.

I don't really care how it is done. But I think people need to realize
why fancy_rename exists, and where it is actually used. (It is only used
on linux for sftp support, though that also effects the locking issues
as well).

I really think that we should try to standardize functionality across
platforms. If we want a rename which *doesn't* replace, then we should
force it to check if the target exists on Linux. And then we can have
another function which does replace.
Yes there is a race condition there, but we have a race condition for
'replace' on Windows.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060214/dc089cad/attachment.pgp