branch locking mk2.

[Robey Pointer]

On 14 Feb 2006, at 7:08, John A Meinel wrote:

> Robey Pointer wrote:
>>
>> 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?
>
> 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.

I guess there are a few things going on here.

The name "fancy_rename" implies (to me) something like rename, but  
with added magic.  It sounds like it's really just an attempt to  
implement "atomic_rename" of files for Transports that don't have  
it.  Having it in osutils further implies that it's related to the  
local OS.

What if we moved it into Transport.__init__.py and called it  
"atomic_rename", to emphasize that it's really a base implementation  
for Transports that don't have an atomic rename?  Then  
Transport.rename (or .replace) could promise to always be atomic.

Another issue is that if file-renaming isn't atomic on some  
platforms, and we're relying on atomic behavior, this seems... bad.   
I don't have an answer.  But it troubles me.  Over SFTP, you can't  
even guarantee that the race is short -- we may get disconnected from  
the server in the middle of the atomic_rename, and now there's no  
weave file.  Or the user may just have hit ^C.  Scary.  Will the  
append-only knit stuff make this a non-issue?

robey