Fix: Git package error handling (#8345)

cmd/lakectl/cmd/local_init.go

@@ -12,6 +12,7 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/treeverse/lakefs/pkg/api/apigen"
 	"github.com/treeverse/lakefs/pkg/git"
+	giterror "github.com/treeverse/lakefs/pkg/git/errors"
 	"github.com/treeverse/lakefs/pkg/local"
 	"github.com/treeverse/lakefs/pkg/uri"
 )
@@ -50,7 +51,7 @@ func localInit(ctx context.Context, dir string, remote *uri.URI, force, updateIg
 			gitDir, err := git.GetRepositoryPath(dir)
 			if err != nil {
 				// report an error only if it's not a git repository
-				if !errors.Is(err, git.ErrNotARepository) && !errors.Is(err, git.ErrNoGit) {
+				if !errors.Is(err, giterror.ErrNotARepository) && !errors.Is(err, giterror.ErrNoGit) {
 					return "", err
 				}
 			} else if gitDir == dir {
@@ -79,7 +80,7 @@ func localInit(ctx context.Context, dir string, remote *uri.URI, force, updateIg
 		ignoreFile, err := git.Ignore(dir, []string{dir}, []string{filepath.Join(dir, local.IndexFileName)}, local.IgnoreMarker)
 		if err == nil {
 			fmt.Println("Location added to", ignoreFile)
-		} else if !(errors.Is(err, git.ErrNotARepository) || errors.Is(err, git.ErrNoGit)) {
+		} else if !(errors.Is(err, giterror.ErrNotARepository) || errors.Is(err, giterror.ErrNoGit)) {
 			return "", err
 		}
 	}

cmd/lakectl/cmd/root.go

@@ -21,6 +21,7 @@ import (
 	"github.com/treeverse/lakefs/pkg/api/apiutil"
 	lakefsconfig "github.com/treeverse/lakefs/pkg/config"
 	"github.com/treeverse/lakefs/pkg/git"
+	giterror "github.com/treeverse/lakefs/pkg/git/errors"
 	"github.com/treeverse/lakefs/pkg/local"
 	"github.com/treeverse/lakefs/pkg/logging"
 	"github.com/treeverse/lakefs/pkg/osinfo"
@@ -282,7 +283,7 @@ func getSyncArgs(args []string, requireRemote bool, considerGitRoot bool) (remot
 		gitRoot, err := git.GetRepositoryPath(localPath)
 		if err == nil {
 			localPath = gitRoot
-		} else if !(errors.Is(err, git.ErrNotARepository) || errors.Is(err, git.ErrNoGit)) { // allow support in environments with no git
+		} else if !(errors.Is(err, giterror.ErrNotARepository) || errors.Is(err, giterror.ErrNoGit)) { // allow support in environments with no git
 			DieErr(err)
 		}
 	}

pkg/git/errors.go → pkg/git/errors/errors.go

@@ -1,4 +1,4 @@
-package git
+package errors
 
 import (
 	"errors"

pkg/git/git.go

@@ -12,6 +12,8 @@ import (
 	"text/template"
 
 	"github.com/treeverse/lakefs/pkg/fileutil"
+	giterror "github.com/treeverse/lakefs/pkg/git/errors"
+	"github.com/treeverse/lakefs/pkg/git/internal"
 	"golang.org/x/exp/slices"
 )
 
@@ -39,7 +41,7 @@ type URL struct {
 func git(dir string, args ...string) (string, int, error) {
 	_, err := exec.LookPath("git") // assume git is in the path, otherwise consider as not having git support
 	if err != nil {
-		return "", 0, ErrNoGit
+		return "", 0, giterror.ErrNoGit
 	}
 	cmd := exec.Command("git", args...)
 	cmd.Dir = dir
@@ -65,7 +67,7 @@ func IsRepository(dir string) bool {
 // GetRepositoryPath Returns the git repository root path if dir is a directory inside a git repository, otherwise returns error
 func GetRepositoryPath(dir string) (string, error) {
 	out, _, err := git(dir, "rev-parse", "--show-toplevel")
-	return handleOutput(out, err)
+	return internal.HandleOutput(out, err)
 }
 
 func createEntriesForIgnore(dir string, paths []string, exclude bool) ([]string, error) {
@@ -191,14 +193,14 @@ func Ignore(dir string, ignorePaths, excludePaths []string, marker string) (stri
 
 func CurrentCommit(path string) (string, error) {
 	out, _, err := git(path, "rev-parse", "--short", "HEAD")
-	return handleOutput(out, err)
+	return internal.HandleOutput(out, err)
 }
 
 func MetadataFor(path, ref string) (map[string]string, error) {
 	kv := make(map[string]string)
 	kv["git_commit_id"] = ref
 	originURL, err := Origin(path)
-	if errors.Is(err, ErrRemoteNotFound) {
+	if errors.Is(err, giterror.ErrRemoteNotFound) {
 		return kv, nil // no additional data to add
 	} else if err != nil {
 		return kv, err
@@ -231,7 +233,7 @@ func Origin(path string) (string, error) {
 		//	the exit status is 2"
 		return "", nil
 	}
-	return handleOutput(out, err)
+	return internal.HandleOutput(out, err)
 }
 
 func ParseURL(raw string) *URL {
@@ -245,16 +247,3 @@ func ParseURL(raw string) *URL {
 		Project: matches[RemoteRegex.SubexpIndex("project")],
 	}
 }
-
-func handleOutput(out string, err error) (string, error) {
-	switch {
-	case err == nil:
-		return strings.TrimSpace(out), nil
-	case strings.Contains(out, "not a git repository"):
-		return "", ErrNotARepository
-	case strings.Contains(out, "remote not found"):
-		return "", ErrRemoteNotFound
-	default:
-		return "", fmt.Errorf("%s: %w", out, err)
-	}
-}

pkg/git/git_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"github.com/treeverse/lakefs/pkg/git"
+	giterror "github.com/treeverse/lakefs/pkg/git/errors"
 )
 
 func TestIsRepository(t *testing.T) {
@@ -57,7 +58,7 @@ func TestGetRepositoryPath(t *testing.T) {
 	}()
 
 	_, err = git.GetRepositoryPath(tmpdir)
-	require.ErrorIs(t, err, git.ErrNotARepository)
+	require.ErrorIs(t, err, giterror.ErrNotARepository)
 	_, err = git.GetRepositoryPath(tmpFile.Name())
 	require.Error(t, err)
 
@@ -90,7 +91,7 @@ func TestIgnore(t *testing.T) {
 
 	// Test we can't ignore if not a git repo
 	_, err = git.Ignore(tmpdir, []string{}, []string{}, marker)
-	require.ErrorIs(t, err, git.ErrNotARepository)
+	require.ErrorIs(t, err, giterror.ErrNotARepository)
 	_, err = git.Ignore(tmpFile.Name(), []string{}, []string{}, marker)
 	require.Error(t, err)
 

pkg/git/internal/shared.go

@@ -0,0 +1,22 @@
+package internal
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/treeverse/lakefs/pkg/git/errors"
+)
+
+func HandleOutput(out string, err error) (string, error) {
+	lowerOut := strings.ToLower(out)
+	switch {
+	case err == nil:
+		return strings.TrimSpace(out), nil
+	case strings.Contains(lowerOut, "not a git repository"):
+		return "", errors.ErrNotARepository
+	case strings.Contains(lowerOut, "remote not found"):
+		return "", errors.ErrRemoteNotFound
+	default:
+		return "", fmt.Errorf("%s: %w", out, err)
+	}
+}

pkg/git/internal/shared_test.go

@@ -0,0 +1,63 @@
+package internal_test
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	giterror "github.com/treeverse/lakefs/pkg/git/errors"
+	"github.com/treeverse/lakefs/pkg/git/internal"
+)
+
+var testErr = errors.New("this is a test generated error")
+
+func TestHandleOutput(t *testing.T) {
+	testCases := []struct {
+		Name          string
+		Output        string
+		Error         error
+		ExpectedError error
+	}{
+		{
+			Name:          "not git repository",
+			Output:        "fatal: not a git repository (or any of the parent directories): .git",
+			Error:         testErr,
+			ExpectedError: giterror.ErrNotARepository,
+		},
+		{
+			Name:          "not a git repository - Mount",
+			Output:        "fatal: Not a git repository (or any parent up to mount point /home/my_home)\nStopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).",
+			Error:         testErr,
+			ExpectedError: giterror.ErrNotARepository,
+		},
+		{
+			Name:          "other error",
+			Output:        "Some other error happened",
+			Error:         testErr,
+			ExpectedError: testErr,
+		},
+		{
+			Name:          "remote not found",
+			Output:        "prefix, Remote nOt founD, suffix",
+			Error:         testErr,
+			ExpectedError: giterror.ErrRemoteNotFound,
+		},
+		{
+			Name:   "no error",
+			Output: "This is a valid response",
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.Name, func(t *testing.T) {
+			str, err := internal.HandleOutput(tt.Output, tt.Error)
+			require.ErrorIs(t, err, tt.ExpectedError)
+			if tt.ExpectedError != nil {
+				require.Equal(t, "", str)
+			} else {
+				require.Equal(t, tt.Output, str)
+			}
+
+		})
+	}
+}