From 6760ab6bb323b3e4e6ef10a7a4c6d80eff217af4 Mon Sep 17 00:00:00 2001
From: Nick Craig-Wood <nick@craig-wood.com>
Date: Thu, 2 Jul 2020 10:15:47 +0100
Subject: [PATCH] fs/cache: fix moveto/copyto remote:file remote:file2

Before this change, if the cache was given a source `remote:file` it
stored `remote:` with the error `fs.ErrorIsFile` attached. This meant
that if it `remote:` was subsequently looked up it would return the
`fs.ErrorIsFile` error.

This broke `moveto remote:file remote:file2` as moveto would lookup
`remote:` from the second argument and erroneously get the
`fs.ErrorIsFile` error.

This likely broke other commands too.

This was broken in

4c9836035 fs/cache: Add Pin and Unpin and canonicalised lookup

Which was released in v1.52.0

The fix is to make a new cache entry for `remote:` with no error
attached in the case that the original call returned `fs.ErrorIsFile`.
---
 fs/cache/cache.go      | 18 +++++++++++++-----
 fs/cache/cache_test.go | 37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/fs/cache/cache.go b/fs/cache/cache.go
index 731477bd8..7d996fc8f 100644
--- a/fs/cache/cache.go
+++ b/fs/cache/cache.go
@@ -56,12 +56,20 @@ func GetFn(fsString string, create func(fsString string) (fs.Fs, error)) (f fs.F
 	if created {
 		canonicalName := fs.ConfigString(f)
 		if canonicalName != fsString {
-			fs.Debugf(nil, "fs cache: renaming cache item %q to be canonical %q", fsString, canonicalName)
-			value, found := c.Rename(fsString, canonicalName)
-			if found {
-				f = value.(fs.Fs)
+			// Note that if err == fs.ErrorIsFile at this moment
+			// then we can't rename the remote as it will have the
+			// wrong error status, we need to add a new one.
+			if err == nil {
+				fs.Debugf(nil, "fs cache: renaming cache item %q to be canonical %q", fsString, canonicalName)
+				value, found := c.Rename(fsString, canonicalName)
+				if found {
+					f = value.(fs.Fs)
+				}
+				addMapping(fsString, canonicalName)
+			} else {
+				fs.Debugf(nil, "fs cache: adding new entry for parent of %q, %q", fsString, canonicalName)
+				Put(canonicalName, f)
 			}
-			addMapping(fsString, canonicalName)
 		}
 	}
 	return f, err
diff --git a/fs/cache/cache_test.go b/fs/cache/cache_test.go
index e261eda15..56ad4231c 100644
--- a/fs/cache/cache_test.go
+++ b/fs/cache/cache_test.go
@@ -23,7 +23,7 @@ func mockNewFs(t *testing.T) (func(), func(path string) (fs.Fs, error)) {
 		switch path {
 		case "mock:/":
 			return mockfs.NewFs("mock", "/"), nil
-		case "mock:/file.txt":
+		case "mock:/file.txt", "mock:file.txt":
 			return mockfs.NewFs("mock", "/"), fs.ErrorIsFile
 		case "mock:/error":
 			return nil, errSentinel
@@ -64,13 +64,46 @@ func TestGetFile(t *testing.T) {
 	require.Equal(t, fs.ErrorIsFile, err)
 	require.NotNil(t, f)
 
-	assert.Equal(t, 1, c.Entries())
+	assert.Equal(t, 2, c.Entries())
 
 	f2, err := GetFn("mock:/file.txt", create)
 	require.Equal(t, fs.ErrorIsFile, err)
 	require.NotNil(t, f2)
 
 	assert.Equal(t, f, f2)
+
+	// check parent is there too
+	f2, err = GetFn("mock:/", create)
+	require.Nil(t, err)
+	require.NotNil(t, f2)
+
+	assert.Equal(t, f, f2)
+}
+
+func TestGetFile2(t *testing.T) {
+	cleanup, create := mockNewFs(t)
+	defer cleanup()
+
+	assert.Equal(t, 0, c.Entries())
+
+	f, err := GetFn("mock:file.txt", create)
+	require.Equal(t, fs.ErrorIsFile, err)
+	require.NotNil(t, f)
+
+	assert.Equal(t, 2, c.Entries())
+
+	f2, err := GetFn("mock:file.txt", create)
+	require.Equal(t, fs.ErrorIsFile, err)
+	require.NotNil(t, f2)
+
+	assert.Equal(t, f, f2)
+
+	// check parent is there too
+	f2, err = GetFn("mock:/", create)
+	require.Nil(t, err)
+	require.NotNil(t, f2)
+
+	assert.Equal(t, f, f2)
 }
 
 func TestGetError(t *testing.T) {