diff --git a/go/libraries/doltcore/sqle/database_provider.go b/go/libraries/doltcore/sqle/database_provider.go index fc5ea15fd9..6571d6323a 100644 --- a/go/libraries/doltcore/sqle/database_provider.go +++ b/go/libraries/doltcore/sqle/database_provider.go @@ -662,6 +662,7 @@ func (p *DoltDatabaseProvider) registerNewDatabase(ctx *sql.Context, name string // mutex is locked (without actually locking it and causing a deadlock) and to error out if we detect // that the mutex is NOT locked. if p.mu.TryLock() { + defer p.mu.Unlock() return fmt.Errorf("unable to register new database without database provider mutex being locked") } diff --git a/go/libraries/doltcore/sqle/dropped_databases.go b/go/libraries/doltcore/sqle/dropped_databases.go index 7c5b48fac9..b90b33c1ec 100644 --- a/go/libraries/doltcore/sqle/dropped_databases.go +++ b/go/libraries/doltcore/sqle/dropped_databases.go @@ -175,7 +175,6 @@ func (dd *droppedDatabaseManager) ListDroppedDatabases(_ *sql.Context) ([]string databaseNames := make([]string, 0, 5) callback := func(path string, size int64, isDir bool) (stop bool) { _, lastPathSegment := filepath.Split(path) - // TODO: Is there a common util we use for this somewhere? lastPathSegment = strings.ReplaceAll(lastPathSegment, "-", "_") databaseNames = append(databaseNames, lastPathSegment) return false diff --git a/go/libraries/doltcore/sqle/dsess/session_db_provider.go b/go/libraries/doltcore/sqle/dsess/session_db_provider.go index d8ef268419..ba23a979a9 100644 --- a/go/libraries/doltcore/sqle/dsess/session_db_provider.go +++ b/go/libraries/doltcore/sqle/dsess/session_db_provider.go @@ -102,7 +102,6 @@ type DoltDatabaseProvider interface { // (e.g. a root database will be restored as a regular/non-root database, // databases original stored with hyphens in their directory name will be rewritten // to underscores to match their SQL database name). - // TODO: Is this a problem for anything on hosted? // If the database is unable to be restored, an error is returned explaining why. UndropDatabase(ctx *sql.Context, dbName string) error // ListDroppedDatabases returns a list of the database names for dropped databases that are still diff --git a/go/libraries/utils/filesys/fs_test.go b/go/libraries/utils/filesys/fs_test.go index 899fd32b54..a78463e7cb 100644 --- a/go/libraries/utils/filesys/fs_test.go +++ b/go/libraries/utils/filesys/fs_test.go @@ -128,12 +128,12 @@ func TestFilesystems(t *testing.T) { err = fs.MoveDir(subdir, filepath.Join(newLocation, "subdir")) require.NoError(t, err) - // Assert that nothing exists as the old path + // Assert that nothing exists at the old path exists, isDir = fs.Exists(subdir) require.False(t, exists) require.False(t, isDir) - // Assert that our directory exists as the new path + // Assert that our directory exists at the new path exists, isDir = fs.Exists(filepath.Join(newLocation, "subdir")) require.True(t, exists) require.True(t, isDir) diff --git a/go/libraries/utils/filesys/inmemfs.go b/go/libraries/utils/filesys/inmemfs.go index 218b45aef8..a6690b6a79 100644 --- a/go/libraries/utils/filesys/inmemfs.go +++ b/go/libraries/utils/filesys/inmemfs.go @@ -518,6 +518,13 @@ func (fs *InMemFS) MoveDir(srcPath, destPath string) error { } func (fs *InMemFS) moveDirHelper(dir *memDir, destPath string) error { + // All calls to moveDirHelper should happen with the filesystem's read-write mutex locked; + // if we detect the mutex isn't locked, then return an error. + if fs.rwLock.TryLock() { + defer fs.rwLock.Unlock() + return fmt.Errorf("moveDirHelper called without first aquiring filesystem read-write lock") + } + if _, exists := fs.objs[destPath]; exists { return fmt.Errorf("destination path exists: %s", destPath) } @@ -589,6 +596,13 @@ func (fs *InMemFS) MoveFile(srcPath, destPath string) error { } func (fs *InMemFS) moveFileHelper(obj *memFile, destPath string) error { + // All calls to moveFileHelper should happen with the filesystem's read-write mutex locked; + // if we detect the mutex isn't locked, then return an error. + if fs.rwLock.TryLock() { + defer fs.rwLock.Unlock() + return fmt.Errorf("moveFileHelper called without first aquiring filesystem read-write lock") + } + destDir := filepath.Dir(destPath) destParentDir, err := fs.mkDirs(destDir) if err != nil { diff --git a/go/libraries/utils/filesys/localfs.go b/go/libraries/utils/filesys/localfs.go index 10a82de3dc..3922107dd1 100644 --- a/go/libraries/utils/filesys/localfs.go +++ b/go/libraries/utils/filesys/localfs.go @@ -288,10 +288,6 @@ func (fs *localFS) MoveFile(srcPath, destPath string) (err error) { } func (fs *localFS) MoveDir(srcPath, destPath string) (err error) { - // TODO: This is the exact same implementation as MoveFile - // Should probably at least add assertions that |srcPath| is really a dir? - // TODO: Or should we just try to make MoveFile work with dirs? It seems like the filesystem - // based implementation already does, it's just the in-memory implementation that doesn't. srcPath, err = fs.Abs(srcPath) if err != nil { return err diff --git a/integration-tests/bats/undrop.bats b/integration-tests/bats/undrop.bats index b758df10fc..2788263b20 100644 --- a/integration-tests/bats/undrop.bats +++ b/integration-tests/bats/undrop.bats @@ -40,6 +40,14 @@ teardown() { [ $status -eq 1 ] [[ $output =~ "dolt_undrop called with too many arguments" ]] || false [[ $output =~ "dolt_undrop only accepts one argument - the name of the dropped database to restore" ]] || false + + # Create and drop a database to test error messages when there is a db available to undrop + dolt sql -q "create database dropper;" + dolt sql -q "drop database dropper;" + run dolt sql -q "CALL dolt_undrop();" + [ $status -eq 1 ] + [[ $output =~ "no database name specified." ]] || false + [[ $output =~ " available databases that can be undropped: dropper" ]] || false } @test "undrop: purge error messages" {