implement specific rename handling for SMB #23845
Conversation
|
By analyzing the blame information on this pull request, we identified @icewind1991, @jmaciasportela, @PVince81 and @Xenopathic to be potential reviewers |
|
It should be unnecessary to wrap the rename call in an exception catcher, if an exception is thrown it will bubble up into the filesystem layer anyway and be logged (and ownCloud is smart enough to recognize the rename failed) |
blizzz
force-pushed
the
smb-rename
branch
2 times, most recently
from
9539d5c to
afd2f56
Compare
True, seen that. Adjusted, thx! |
blizzz
changed the title
| $path2 = $this->buildPath($path2); | ||
| $result = $this->share->rename($path1, $path2); | ||
|
|
||
| // non-native Share implementation does not return bool in fact |
There was a problem hiding this comment.
Does it? According to the PHPDoc of icewine1991/SMB it should...
There was a problem hiding this comment.
|
You'll probably need to do some |
I believe this is because SMB doesn't allow you to rename + overwrite existing files. So you have to copy+overwrite then delete the source. At least that's how I had to do it back then when we had the smb4php lib. |
|
In WND, i believe, we used to delete the target, if existing, and then do the rename. I adopted it. |
|
@blizzz any specific reason why you decided to change the approach ? Does it fix any issue ? |
|
I expect a native rename on an external storage to be more performing than copying a file there, honestly I did not compare it. My main motivation back then was proper behaviour in SMB notifications. In case there are other applications listening, they will not do get an information about a rename with the old and new name, but copy and delete actions. If another application would do this while we listen, this would be bad and we could not recognize a move, as we plan. So this is an issue indeed. |
That is actually a very good point |
|
@icewind1991 @jvillafanez mind taking over this ? |
|
Updated This is good to review @owncloud/filesystem |
👍 |
|
Being picky, how much common is to rename to an already existing file? I don't think this is very common, and probably it usually is due to a mistake. So 90% of the times, we'll be trying to delete a missing file (the target one) before renaming. My point is that we might want to try the rename first, and if the rename fails due to the target file exists, we delete the file and retry. |
When uploading a file with the desktop client, internally it creates a part file. |
|
@owncloud/filesystem @jvillafanez @guruz @dragotin @mmattel second review ? |
|
Code looks ok 👍 |
|
👍 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
@icewind1991 were there reasons why there was no specifc rename for SMB and copy+delete was used instead? So far this changes work for me with and without php-libsmbclient.