Rename files in cache too if they exist #3480

darthShadow · 2019-08-26T13:12:46Z

What is the purpose of this change?

Files in cache should be renamed too along with renames on the remote

Was the change discussed in an issue or in the forum before?

#2907

Checklist

I have read the contribution guidelines.
I have added tests for all changes in this PR if appropriate.
I have added documentation for the changes if appropriate.
All commit messages are in house style.
I'm done, this Pull Request is ready for review :-)

darthShadow · 2019-08-26T13:13:41Z

~~I still need to handle cases where the file is moved into or out of a directory.~~ Done.

vfs/cache.go

ncw

This is a great thing to do - thanks for tackling this.

I put some notes inline.

I think we should try to write a test for this, then we'll be sure it works properly on all OSes - what do you think?

ncw · 2019-08-26T16:26:41Z

vfs/cache.go

+			return true
+		}
+	}
+	if err = os.Link(osOldPath, osNewPath); err == nil {


Why not use os.Rename always? Isn't it equivalent to Link+Remove?

os.Link doesn't work on all windows fileystems (eg FAT)

os.Rename may have troubles renaming over an existing file on Windows.

Not much of difference either way. Didn't try it on windows, if it doesn't work there, I will just remove the section that tries to link.

As for rename, what do you think would be better? Remove the existing destination or do nothing and print an error log?

ncw · 2019-08-26T16:26:58Z

vfs/cache.go

@@ -270,12 +270,63 @@ func (c *cache) exists(name string) bool {
 	if err != nil {
 		return false
 	}
-	if fi.IsDir() {
+	// checks for non-regular files (e.g. directories, symlinks, devices, etc.)
+	if !fi.Mode().IsRegular() {


Good catch!

ncw · 2019-08-26T16:28:35Z

vfs/cache.go

 		return false
 	}
 	return true
 }

+// renames the file in cache
+func (c *cache) rename(name string, newName string) bool {


It would be more go style to return an annotated error, than a boolean here I think...

ncw · 2019-08-26T16:29:47Z

vfs/cache.go

+	}
+	if !sfi.Mode().IsRegular() {
+		// cannot copy non-regular files (e.g., directories, symlinks, devices, etc.)
+		fs.Errorf("rename: non-regular source file %s (%q)", sfi.Name(), sfi.Mode().String())


... so instead of fs.Errorf(...); return false

do

return errors.Errorf("rename: non-regular source file %s (%q)", sfi.Name(), sfi.Mode().String()))

ncw · 2019-08-26T16:30:33Z

vfs/cache.go

+	dfi, err := os.Stat(osNewPath)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			fs.Errorf(name, "rename: Failed to stat destination: %v", err)


...and likewise wrap the errors when you have them so this should be

return errors.Wrap(err, "rename: Failed to stat destination")

ncw · 2019-08-26T16:32:25Z

vfs/file.go

+
+		// Rename in the cache too if it exists
+		if f.d.vfs.Opt.CacheMode >= CacheModeWrites && f.d.vfs.cache.exists(f.Path()) {
+			if ok := f.d.vfs.cache.rename(f.Path(), newPath); !ok {


Then this should be something like

err = f.d.vfs.cache.rename(f.Path(), newPath) if err != nil { fs.Infof(f.Path(), "File.Rename failed in Cache: %v", err) }

ncw · 2019-08-26T16:33:37Z

vfs/cache.go

+	osNewPath := c.toOSPath(newName)
+	sfi, err := os.Stat(osOldPath)
+	if err != nil {
+		return false


return errors.Wrap(err, "rename: source file")

ncw · 2019-08-26T16:39:43Z

vfs/cache.go

+			fs.Errorf("rename: non-regular destination file: %s (%q)", dfi.Name(), dfi.Mode().String())
+			return false
+		}
+		if os.SameFile(sfi, dfi) {


How likely do you think os.SameFile is to return true here? I'm struggling to see how it might happen

Just thought it better to be safe than have an issue afterwards in case it ever happens. This avoids silent failures from rename in case both the source & destination point to the same file via links.

Source: https://forum.golangbridge.org/t/os-rename-and-hard-link-overwrite/7838

darthShadow · 2019-08-26T16:59:03Z

Thanks for the review. As for the tests, I may need some help with that. Haven't written any tests before 😅

ncw · 2019-08-26T17:11:51Z

Thanks for the review. As for the tests, I may need some help with that. Haven't written any tests before

Take a look in the cache_test.go file there is a fair amount of machinery in there. You'd write a test for the rename function in there.

It would be good if we could also adjust the rename file test to check the correct things are happening in the cache but that might be a bit trickier.

rclone/vfs/dir_test.go

Line 469 in d1a39dc

func TestDirRename(t *testing.T) {

ncw · 2019-09-02T15:33:03Z

How are you doing with this? Need help finishing it off?

darthShadow · 2019-09-02T15:36:27Z

Need to get some time together to finish off the tests. Will be updating it by the weekend at the latest.

ncw · 2019-09-03T11:07:17Z

Need to get some time together to finish off the tests. Will be updating it by the weekend at the latest.

Great :-) Let me know if you need any help.

Signed-off-by: Anagh Kumar Baranwal <6824881+darthShadow@users.noreply.github.com>

darthShadow · 2019-10-23T19:52:26Z

@ncw Sorry for the delay, but doesn't look like I will have any time in the next 2-3 months to write any tests and finish this. I have just pushed the latest changeset that I had started on and hope that you or someone else can use that and finish this PR.

ncw · 2019-10-26T16:40:58Z

@ncw Sorry for the delay, but doesn't look like I will have any time in the next 2-3 months to write any tests and finish this. I have just pushed the latest changeset that I had started on and hope that you or someone else can use that and finish this PR.

That is no problem. I'll try to get this merged one way or another in 1.51

@bdutro do you fancy finishing this off?

bdutro · 2019-10-28T17:09:13Z

@ncw, sure. How should I add my changes? It seems like the proper way is to do a pull request against @darthShadow's branch, which would then get added to this pull request when it's merged.

Edit: Also, does this just need a test added, or do we need to fix the test failures as well?

ncw · 2019-10-29T16:56:50Z

@ncw, sure. How should I add my changes? It seems like the proper way is to do a pull request against @darthShadow's branch, which would then get added to this pull request when it's merged.

I would pull the current changes into a local branch, add your commits there then make a new pull request I think.

Edit: Also, does this just need a test added, or do we need to fix the test failures as well?

Both I think.

ncw · 2019-11-10T12:00:19Z

This is now merged in #3688 - thank you @darthShadow for the initial work and @bdutro for finishing it off.

golangcibot reviewed Aug 26, 2019

View reviewed changes

vfs/cache.go Show resolved Hide resolved

vfs/cache.go Show resolved Hide resolved

darthShadow force-pushed the fix-2907-vfs-cache-rename branch from cb7fa09 to acd0bda Compare August 26, 2019 14:48

ncw reviewed Aug 26, 2019

View reviewed changes

ncw mentioned this pull request Oct 10, 2019

Replace close() call in Flush() with writeback of dirty data to backing storage (Fix for #3381) #3596

Merged

5 tasks

darthShadow added 2 commits October 24, 2019 01:15

On rename, rename in cache too if the file exists

19662c2

Signed-off-by: Anagh Kumar Baranwal <6824881+darthShadow@users.noreply.github.com>

Return errors instead of a boolean value

112f2d0

Signed-off-by: Anagh Kumar Baranwal <6824881+darthShadow@users.noreply.github.com>

darthShadow force-pushed the fix-2907-vfs-cache-rename branch from acd0bda to 112f2d0 Compare October 23, 2019 19:49

bdutro mentioned this pull request Oct 29, 2019

Rename files in cache (add tests for pull request 3480) #3688

Merged

5 tasks

ncw closed this Nov 10, 2019

darthShadow deleted the fix-2907-vfs-cache-rename branch October 13, 2020 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename files in cache too if they exist #3480

Rename files in cache too if they exist #3480

darthShadow commented Aug 26, 2019 •

edited

darthShadow commented Aug 26, 2019 •

edited

ncw left a comment

ncw Aug 26, 2019

darthShadow Aug 26, 2019

ncw Aug 26, 2019

ncw Aug 26, 2019

ncw Aug 26, 2019

ncw Aug 26, 2019

ncw Aug 26, 2019

ncw Aug 26, 2019

ncw Aug 26, 2019

darthShadow Aug 26, 2019

darthShadow commented Aug 26, 2019

ncw commented Aug 26, 2019

ncw commented Sep 2, 2019

darthShadow commented Sep 2, 2019

ncw commented Sep 3, 2019

darthShadow commented Oct 23, 2019

ncw commented Oct 26, 2019

bdutro commented Oct 28, 2019 •

edited

ncw commented Oct 29, 2019

ncw commented Nov 10, 2019

Rename files in cache too if they exist #3480

Rename files in cache too if they exist #3480

Conversation

darthShadow commented Aug 26, 2019 • edited

What is the purpose of this change?

Was the change discussed in an issue or in the forum before?

Checklist

darthShadow commented Aug 26, 2019 • edited

ncw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthShadow commented Aug 26, 2019

ncw commented Aug 26, 2019

ncw commented Sep 2, 2019

darthShadow commented Sep 2, 2019

ncw commented Sep 3, 2019

darthShadow commented Oct 23, 2019

ncw commented Oct 26, 2019

bdutro commented Oct 28, 2019 • edited

ncw commented Oct 29, 2019

ncw commented Nov 10, 2019

darthShadow commented Aug 26, 2019 •

edited

darthShadow commented Aug 26, 2019 •

edited

bdutro commented Oct 28, 2019 •

edited