Merge branch 'revert-3cb9fb8d' into 'master'

Revert "Merge branch 'zj-rename-namespace-creates-parent-dir' into 'master'"

See merge request gitlab-org/gitaly!1125

4 files changed, 55 insertions, 106 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1f5e7522c..372c75665 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## v1.28.0
 
+Should not be used as it [will break gitlab-rails](https://gitlab.com/gitlab-org/gitlab-ce/issues/58855).
+
 #### Changed
 - RenameNamespace RPC creates parent directories for the new location
   https://gitlab.com/gitlab-org/gitaly/merge_requests/1090
diff --git a/internal/helper/error.go b/internal/helper/error.go
index 98ef1a23e..c20a1690a 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -1,8 +1,6 @@
 package helper
 
 import (
-	"fmt"
-
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -19,22 +17,12 @@ func DecorateError(code codes.Code, err error) error {
 	return err
 }
 
-// ErrInternal wraps err with codes.Internal, unless err is already a grpc error
+// ErrInternal wrappes err with codes.Internal, unless err is already a grpc error
 func ErrInternal(err error) error { return DecorateError(codes.Internal, err) }
 
-// ErrInternalf wraps err with codes.Internal, unless err is already a grpc error
-func ErrInternalf(format string, a ...interface{}) error {
-	return DecorateError(codes.Internal, fmt.Errorf(format, a...))
-}
-
-// ErrInvalidArgument wraps err with codes.InvalidArgument, unless err is already a grpc error
+// ErrInvalidArgument wrappes err with codes.InvalidArgument, unless err is already a grpc error
 func ErrInvalidArgument(err error) error { return DecorateError(codes.InvalidArgument, err) }
 
-// ErrInvalidArgumentf wraps err with codes.InvalidArgument, unless err is already a grpc error
-func ErrInvalidArgumentf(format string, a ...interface{}) error {
-	return DecorateError(codes.InvalidArgument, fmt.Errorf(format, a...))
-}
-
 // ErrPreconditionFailed wraps error with codes.FailedPrecondition, unless err is already a grpc error
 func ErrPreconditionFailed(err error) error { return DecorateError(codes.FailedPrecondition, err) }
 
diff --git a/internal/service/namespace/namespace.go b/internal/service/namespace/namespace.go
index f1682cee8..afd4caf87 100644
--- a/internal/service/namespace/namespace.go
+++ b/internal/service/namespace/namespace.go
@@ -2,16 +2,16 @@ package namespace
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"os"
-	"path/filepath"
+	"path"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
-var noNameError = helper.ErrInvalidArgumentf("name: cannot be empty")
+var noNameError = status.Errorf(codes.InvalidArgument, "Name: cannot be empty")
 
 func (s *server) NamespaceExists(ctx context.Context, in *gitalypb.NamespaceExistsRequest) (*gitalypb.NamespaceExistsResponse, error) {
 	storagePath, err := helper.GetStorageByName(in.GetStorageName())
@@ -19,15 +19,19 @@ func (s *server) NamespaceExists(ctx context.Context, in *gitalypb.NamespaceExis
 		return nil, err
 	}
 
-	// This case should return an error, as else we'd actually say the path
-	// exists as the storage exists
+	// This case should return an error, as else we'd actually say the path exists as the
+	// storage exists
 	if in.GetName() == "" {
 		return nil, noNameError
 	}
 
-	exists, err := directoryExists(storagePath, in.GetName())
-
-	return &gitalypb.NamespaceExistsResponse{Exists: exists}, helper.ErrInternal(err)
+	if fi, err := os.Stat(namespacePath(storagePath, in.GetName())); os.IsNotExist(err) {
+		return &gitalypb.NamespaceExistsResponse{Exists: false}, nil
+	} else if err != nil {
+		return nil, status.Errorf(codes.Internal, "could not stat the directory: %v", err)
+	} else {
+		return &gitalypb.NamespaceExistsResponse{Exists: fi.IsDir()}, nil
+	}
 }
 
 func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequest) (*gitalypb.AddNamespaceResponse, error) {
@@ -37,35 +41,41 @@ func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequ
 	}
 
 	name := in.GetName()
-	if name == "" {
+	if len(name) == 0 {
 		return nil, noNameError
 	}
 
-	if err := createDirectory(storagePath, name); err != nil {
-		return nil, helper.ErrInternal(err)
+	if err = os.MkdirAll(namespacePath(storagePath, name), 0770); err != nil {
+		return nil, status.Errorf(codes.Internal, "create directory: %v", err)
 	}
 
 	return &gitalypb.AddNamespaceResponse{}, nil
 }
 
 func (s *server) RenameNamespace(ctx context.Context, in *gitalypb.RenameNamespaceRequest) (*gitalypb.RenameNamespaceResponse, error) {
-	if err := validateRenameNamespaceRequest(in); err != nil {
-		return nil, helper.ErrInvalidArgument(err)
-	}
-
 	storagePath, err := helper.GetStorageByName(in.GetStorageName())
 	if err != nil {
 		return nil, err
 	}
 
-	fromPath, toPath := in.GetFrom(), in.GetTo()
+	if in.GetFrom() == "" || in.GetTo() == "" {
+		return nil, status.Errorf(codes.InvalidArgument, "from and to cannot be empty")
+	}
 
-	if err = createDirectory(storagePath, filepath.Dir(toPath)); err != nil {
-		return nil, helper.ErrInternal(err)
+	// No need to check if the from path exists, if it doesn't, we'd later get an
+	// os.LinkError
+	toExistsCheck := &gitalypb.NamespaceExistsRequest{StorageName: in.StorageName, Name: in.GetTo()}
+	if exists, err := s.NamespaceExists(ctx, toExistsCheck); err != nil {
+		return nil, err
+	} else if exists.Exists {
+		return nil, status.Errorf(codes.InvalidArgument, "to directory %s already exists", in.GetTo())
 	}
 
-	if err = os.Rename(namespacePath(storagePath, fromPath), namespacePath(storagePath, toPath)); err != nil {
-		return nil, helper.ErrInternal(err)
+	err = os.Rename(namespacePath(storagePath, in.GetFrom()), namespacePath(storagePath, in.GetTo()))
+	if _, ok := err.(*os.LinkError); ok {
+		return nil, status.Errorf(codes.InvalidArgument, "from directory %s not found", in.GetFrom())
+	} else if err != nil {
+		return nil, status.Errorf(codes.Internal, "rename: %v", err)
 	}
 
 	return &gitalypb.RenameNamespaceResponse{}, nil
@@ -78,53 +88,18 @@ func (s *server) RemoveNamespace(ctx context.Context, in *gitalypb.RemoveNamespa
 	}
 
 	// Needed as else we might destroy the whole storage
-	name := in.GetName()
-	if name == "" {
+	if in.GetName() == "" {
 		return nil, noNameError
 	}
 
 	// os.RemoveAll is idempotent by itself
 	// No need to check if the directory exists, or not
-	if err = os.RemoveAll(namespacePath(storagePath, name)); err != nil {
-		return nil, helper.ErrInternal(err)
+	if err = os.RemoveAll(namespacePath(storagePath, in.GetName())); err != nil {
+		return nil, status.Errorf(codes.Internal, "removal: %v", err)
 	}
 	return &gitalypb.RemoveNamespaceResponse{}, nil
 }
 
-func namespacePath(storagePath, ns string) string {
-	return filepath.Join(storagePath, ns)
-}
-
-func createDirectory(storagePath, namespace string) error {
-	return os.MkdirAll(namespacePath(storagePath, namespace), 0755)
-}
-
-func directoryExists(storagePath, namespace string) (bool, error) {
-	fi, err := os.Stat(namespacePath(storagePath, namespace))
-	if os.IsNotExist(err) {
-		return false, nil
-	} else if err != nil {
-		return false, err
-	}
-
-	if !fi.IsDir() {
-		return false, fmt.Errorf("expected directory, found file %s", namespace)
-	}
-
-	return true, nil
-}
-
-func validateRenameNamespaceRequest(req *gitalypb.RenameNamespaceRequest) error {
-	if _, err := helper.GetStorageByName(req.GetStorageName()); err != nil {
-		return err
-	}
-
-	if req.GetFrom() == "" {
-		return errors.New("from field cannot be empty")
-	}
-	if req.GetTo() == "" {
-		return errors.New("to field cannot be empty")
-	}
-
-	return nil
+func namespacePath(storage, ns string) string {
+	return path.Join(storage, ns)
 }
diff --git a/internal/service/namespace/namespace_test.go b/internal/service/namespace/namespace_test.go
index 2469d8cc0..a01fe0b0a 100644
--- a/internal/service/namespace/namespace_test.go
+++ b/internal/service/namespace/namespace_test.go
@@ -1,6 +1,7 @@
 package namespace
 
 import (
+	"context"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -39,7 +40,7 @@ func TestNamespaceExists(t *testing.T) {
 	defer conn.Close()
 
 	// Create one namespace for testing it exists
-	ctx, cancel := testhelper.Context()
+	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
 	_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{StorageName: "default", Name: "existing"})
@@ -91,7 +92,7 @@ func TestNamespaceExists(t *testing.T) {
 
 	for _, tc := range queries {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
+			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 			response, err := client.NamespaceExists(ctx, tc.request)
 
@@ -152,7 +153,7 @@ func TestAddNamespace(t *testing.T) {
 
 	for _, tc := range queries {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
+			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
 			_, err := client.AddNamespace(ctx, tc.request)
@@ -177,7 +178,7 @@ func TestRemoveNamespace(t *testing.T) {
 	client, conn := newNamespaceClient(t, serverSocketPath)
 	defer conn.Close()
 
-	ctx, cancel := testhelper.Context()
+	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
 	queries := []struct {
@@ -229,7 +230,7 @@ func TestRenameNamespace(t *testing.T) {
 	client, conn := newNamespaceClient(t, serverSocketPath)
 	defer conn.Close()
 
-	ctx, cancel := testhelper.Context()
+	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
 	queries := []struct {
@@ -262,7 +263,7 @@ func TestRenameNamespace(t *testing.T) {
 				To:          "new-path",
 				StorageName: "default",
 			},
-			errorCode: codes.Internal,
+			errorCode: codes.InvalidArgument,
 		},
 		{
 			desc: "existing destination namespace",
@@ -271,37 +272,20 @@ func TestRenameNamespace(t *testing.T) {
 				To:          "existing",
 				StorageName: "default",
 			},
-			errorCode: codes.Internal,
-		},
-		{
-			desc: "non existing to parent directory",
-			request: &gitalypb.RenameNamespaceRequest{
-				From:        "existing",
-				To:          "ab/cd/non-existing",
-				StorageName: "default",
-			},
-			errorCode: codes.OK,
+			errorCode: codes.InvalidArgument,
 		},
 	}
 
+	_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{
+		StorageName: "default",
+		Name:        "existing",
+	})
+	require.NoError(t, err)
+
 	for _, tc := range queries {
 		t.Run(tc.desc, func(t *testing.T) {
-			_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{
-				StorageName: "default",
-				Name:        "existing",
-			})
-			require.NoError(t, err)
+			_, err := client.RenameNamespace(ctx, tc.request)
 
-			defer client.RemoveNamespace(ctx, &gitalypb.RemoveNamespaceRequest{
-				StorageName: tc.request.StorageName,
-				Name:        tc.request.To,
-			})
-
-			_, err = client.RenameNamespace(ctx, tc.request)
-
-			if tc.errorCode != helper.GrpcCode(err) {
-				t.Fatal(err)
-			}
 			require.Equal(t, tc.errorCode, helper.GrpcCode(err))
 
 			if tc.errorCode == codes.OK {