Fixing PR comments

treeverse · Dec 6, 2020 · 891e9d4 · 891e9d4
1 parent d7da950
commit 891e9d4
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 28 deletions.
diff --git a/pyramid/file_test.go b/pyramid/file_test.go
@@ -12,17 +12,17 @@ import (
 )
 
 func TestPyramidWriteFile(t *testing.T) {
-	filename := uuid.Must(uuid.NewRandom()).String()
-	filepath := path.Join("/tmp", filename)
-	defer os.Remove(filepath)
+	filename := uuid.New().String()
 
-	fh, err := os.Create(filepath)
+	fh, err := ioutil.TempFile("", filename)
 	if err != nil {
 		panic(err)
 	}
 
-	storeCalled := false
+	filepath := fh.Name()
+	defer os.Remove(filepath)
 
+	storeCalled := false
 	sut := File{
 		fh: fh,
 		store: func(string) error {
@@ -47,15 +47,15 @@ func TestPyramidWriteFile(t *testing.T) {
 }
 
 func TestWriteValidate(t *testing.T) {
-	filename := uuid.Must(uuid.NewRandom()).String()
-	filepath := path.Join("/tmp", filename)
-	defer os.Remove(filepath)
-
-	fh, err := os.Create(filepath)
+	filename := uuid.New().String()
+	fh, err := ioutil.TempFile("", filename)
 	if err != nil {
 		panic(err)
 	}
 
+	filepath := fh.Name()
+	defer os.Remove(filepath)
+
 	storeCalled := false
 
 	sut := File{
@@ -81,7 +81,7 @@ func TestWriteValidate(t *testing.T) {
 }
 
 func TestPyramidReadFile(t *testing.T) {
-	filename := uuid.Must(uuid.NewRandom()).String()
+	filename := uuid.New().String()
 	filepath := path.Join("/tmp", filename)
 	content := "some content to write to file"
 	if err := ioutil.WriteFile(filepath, []byte(content), os.ModePerm); err != nil {
@@ -112,7 +112,7 @@ func TestPyramidReadFile(t *testing.T) {
 	require.Equal(t, content, string(bytes))
 	require.NoError(t, sut.Close())
 
-	require.Equal(t, 1, mockEv.touchedTimes[relativePath(filename)])
+	require.Equal(t, 2, mockEv.touchedTimes[relativePath(filename)])
 }
 
 type mockEviction struct {

diff --git a/pyramid/ro_file.go b/pyramid/ro_file.go
@@ -24,6 +24,7 @@ func (f *ROFile) ReadAt(p []byte, off int64) (n int, err error) {
 }
 
 func (f *ROFile) Stat() (os.FileInfo, error) {
+	f.eviction.touch(f.rPath)
 	return f.fh.Stat()
 }
 

diff --git a/pyramid/tierFS.go b/pyramid/tierFS.go
@@ -122,10 +122,39 @@ func (tfs *TierFS) removeFromLocal(rPath relativePath) {
 func removeFromLocal(logger logging.Logger, fsLocalBaseDir string, rPath relativePath) {
 	p := path.Join(fsLocalBaseDir, string(rPath))
 	if err := os.Remove(p); err != nil {
-		logger.WithError(err).Errorf("Removing file %s", p)
+		logger.WithError(err).WithField("path", p).Error("Removing file failed")
+		return
+	}
+
+	dir := path.Dir(p)
+	empty, err := isDirEmpty(dir)
+	if err != nil {
+		logger.WithError(err).WithField("dir", dir).Error("Checking if dir empty failed")
+		return
+	}
+	if !empty {
+		return
+	}
+
+	if err := os.Remove(dir); err != nil {
+		logger.WithError(err).WithField("dir", dir).Error("Removing dir failed")
 	}
 }
 
+func isDirEmpty(name string) (bool, error) {
+	f, err := os.Open(name)
+	if err != nil {
+		return false, err
+	}
+	defer f.Close()
+
+	_, err = f.Readdirnames(1)
+	if err == io.EOF {
+		return true, nil
+	}
+	return false, err
+}
+
 func (tfs *TierFS) store(namespace, originalPath, filename string) error {
 	f, err := os.Open(originalPath)
 	if err != nil {
@@ -264,12 +293,16 @@ func validateArgs(namespace, filename string) error {
 var (
 	errSeparatorInFS   = errors.New("path contains separator")
 	errPathInWorkspace = errors.New("file cannot be located in the workspace")
+	errEmptyDirInPath  = errors.New("file path cannot contain an empty directory")
 )
 
 func validateFilename(filename string) error {
 	if strings.HasPrefix(filename, workspaceDir+string(os.PathSeparator)) {
 		return errPathInWorkspace
 	}
+	if strings.Contains(filename, strings.Repeat(string(os.PathSeparator), 2)) {
+		return errEmptyDirInPath
+	}
 	return nil
 }
 
@@ -305,7 +338,7 @@ func (tfs *TierFS) newLocalFileRef(namespace, filename string) localFileRef {
 
 func (tfs *TierFS) objPointer(namespace, filename string) block.ObjectPointer {
 	if runtime.GOOS == "windows" {
-		filename = strings.ReplaceAll(filename, `\\'`, "/")
+		filename = strings.ReplaceAll(filename, `\\`, "/")
 	}
 
 	return block.ObjectPointer{

diff --git a/pyramid/tierFS_test.go b/pyramid/tierFS_test.go
@@ -24,11 +24,11 @@ const blockStoragePrefix = "prefix"
 const allocatedDiskBytes = 4 * 1024 * 1024
 
 func TestMain(m *testing.M) {
-	fsName := uuid.Must(uuid.NewRandom()).String()
+	fsName := uuid.New().String()
 
 	// cleanup
 	defer func() {
-		if err := os.RemoveAll(path.Join("/tmp", fsName)); err != nil {
+		if err := os.RemoveAll(path.Join(os.TempDir(), fsName)); err != nil {
 			panic(err)
 		}
 	}()
@@ -39,7 +39,7 @@ func TestMain(m *testing.M) {
 		fsName:               fsName,
 		adaptor:              adapter,
 		fsBlockStoragePrefix: blockStoragePrefix,
-		localBaseDir:         "/tmp",
+		localBaseDir:         os.TempDir(),
 		allocatedDiskBytes:   allocatedDiskBytes,
 	})
 	if err != nil {
@@ -52,7 +52,7 @@ func TestMain(m *testing.M) {
 }
 
 func TestSimpleWriteRead(t *testing.T) {
-	namespace := uuid.Must(uuid.NewRandom()).String()
+	namespace := uuid.New().String()
 	filename := "1/2/file1.txt"
 
 	content := "hello world!"
@@ -61,7 +61,7 @@ func TestSimpleWriteRead(t *testing.T) {
 }
 
 func TestReadFailDuringWrite(t *testing.T) {
-	namespace := uuid.Must(uuid.NewRandom()).String()
+	namespace := uuid.New().String()
 	filename := "file1"
 	f, err := fs.Create(namespace)
 	require.NoError(t, err)
@@ -81,27 +81,27 @@ func TestReadFailDuringWrite(t *testing.T) {
 }
 
 func TestEvictionSingleNamespace(t *testing.T) {
-	testEviction(t, uuid.Must(uuid.NewRandom()).String())
+	testEviction(t, uuid.New().String())
 }
 
 func TestEvictionMultipleNamespaces(t *testing.T) {
-	testEviction(t, uuid.Must(uuid.NewRandom()).String(),
-		uuid.Must(uuid.NewRandom()).String(),
-		uuid.Must(uuid.NewRandom()).String())
+	testEviction(t, uuid.New().String(),
+		uuid.New().String(),
+		uuid.New().String())
 }
 
 func TestStartup(t *testing.T) {
-	fsName := uuid.Must(uuid.NewRandom()).String()
-	namespace := uuid.Must(uuid.NewRandom()).String()
+	fsName := uuid.New().String()
+	namespace := uuid.New().String()
 
 	// cleanup
 	defer func() {
-		if err := os.RemoveAll(path.Join("/tmp", fsName)); err != nil {
+		if err := os.RemoveAll(path.Join(os.TempDir(), fsName)); err != nil {
 			panic(err)
 		}
 	}()
 
-	namespacePath := path.Join("/tmp", fsName, namespace)
+	namespacePath := path.Join(os.TempDir(), fsName, namespace)
 	workspacePath := path.Join(namespacePath, workspaceDir)
 	if err := os.MkdirAll(workspacePath, os.ModePerm); err != nil {
 		panic(err)
@@ -120,7 +120,7 @@ func TestStartup(t *testing.T) {
 		fsName:               fsName,
 		adaptor:              mem.New(),
 		fsBlockStoragePrefix: blockStoragePrefix,
-		localBaseDir:         "/tmp",
+		localBaseDir:         os.TempDir(),
 		allocatedDiskBytes:   allocatedDiskBytes,
 	})
 	if err != nil {
-Original file line number
+Diff line change
@@ Expand Up @@
     }
     func (f *ROFile) Stat() (os.FileInfo, error) {
+    	f.eviction.touch(f.rPath)
     	return f.fh.Stat()
     }
@@ Expand Down @@