CR Fixes

cmd/lakectl/cmd/local.go

@@ -5,11 +5,14 @@ import (
 )
 
 var localCmd = &cobra.Command{
-	Use:   "local",
-	Short: "sync local directories with remote lakeFS locations",
+	Use: "local",
+	// TODO: Remove BETA when feature complete
+	Short: "BETA: sync local directories with remote lakeFS locations",
 }
 
 //nolint:gochecknoinits
 func init() {
+	// TODO: Remove line when feature complete
+	localCmd.Hidden = true
 	rootCmd.AddCommand(localCmd)
 }

cmd/lakectl/cmd/local_init.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/treeverse/lakefs/pkg/git"
-	"github.com/treeverse/lakefs/pkg/uri"
 )
 
 const (
@@ -17,44 +16,28 @@ const (
 )
 
 var localInitCmd = &cobra.Command{
-	Use:   "init [directory] <lakeFS path URI>",
-	Short: "set a local directory to sync with a lakeFS ref and path",
+	Use:   "init <path uri> [directory]",
+	Short: "set a local directory to sync with a lakeFS path",
 	Args:  cobra.RangeArgs(initMinArgs, initMaxArgs),
 	Run: func(cmd *cobra.Command, args []string) {
-		var remote *uri.URI
+		remote := MustParsePathURI("path", args[0])
 		dir := "."
 		if len(args) == initMaxArgs {
-			dir = args[0]
-			remote = MustParsePathURI("remote ref", args[1])
-		} else {
-			remote = MustParsePathURI("remote ref", args[0])
+			dir = args[1]
 		}
+		flagSet := cmd.Flags()
+		force := MustBool(flagSet.GetBool("force"))
 
 		localPath, err := filepath.Abs(dir)
 		if err != nil {
 			DieErr(err)
 		}
 
-		stat, err := os.Stat(localPath)
-		switch {
-		case errors.Is(err, os.ErrNotExist):
-			if confirmation, err := Confirm(cmd.Flags(), fmt.Sprintf("Directory '%s' doesn't exist, create", localPath)); err != nil || !confirmation {
-				Die("local init aborted", 1)
-			}
-			if err := os.MkdirAll(dir, os.ModePerm); err != nil {
-				DieFmt("Failed to create dir: %s", localPath)
-			}
-		case err != nil:
+		if err := os.MkdirAll(dir, os.ModePerm); err != nil {
 			DieErr(err)
-		case !stat.IsDir():
-			DieErr(fmt.Errorf("not a directory: %w", os.ErrInvalid))
 		}
-
-		if IndexExists(localPath) {
-			if confirmation, err := Confirm(cmd.Flags(),
-				fmt.Sprintf("Directory '%s' already linked to a lakefs path, do you wish to override the configuration", localPath)); err != nil || !confirmation {
-				Die("local init aborted", 1)
-			}
+		if IndexExists(localPath) && !force {
+			Die(fmt.Sprintf("directory '%s' already linked to a lakefs path, run command with --force to overwrite", localPath), 1)
 		}
 
 		// dereference
@@ -64,20 +47,20 @@ var localInitCmd = &cobra.Command{
 			DieErr(err)
 		}
 
-		if git.IsGitRepository(localPath) {
-			ignoreFile, err := git.Ignore(localPath, []string{localPath, IndexFileName}, []string{IndexFileName}, IgnoreMarker)
-			if err != nil {
-				DieErr(err)
-			}
-			Fmt("location added to %s\n", ignoreFile)
+		ignoreFile, err := git.Ignore(localPath, []string{localPath, IndexFileName}, []string{IndexFileName}, IgnoreMarker)
+		if err != nil && !errors.Is(err, git.ErrNotARepository) {
+			DieErr(err)
+		} else if err == nil {
+			fmt.Println("location added to", ignoreFile)
 		}
 
-		Fmt("Successfully linked local directory '%s' with remote '%s'\n", localPath, remote.String())
+		fmt.Printf("Successfully linked local directory '%s' with remote '%s'\n", localPath, remote)
 	},
 }
 
 //nolint:gochecknoinits
 func init() {
-	localCmd.AddCommand(localInitCmd)
 	AssignAutoConfirmFlag(localInitCmd.Flags())
+	localInitCmd.Flags().Bool("force", false, "Overwrites if directory already linked to a lakeFS path")
+	localCmd.AddCommand(localInitCmd)
 }

esti/golden/lakectl_help.golden

@@ -21,7 +21,6 @@ Available Commands:
   gc              Manage the garbage collection policy
   help            Help about any command
   import          Import data from external source to a destination branch
-  local           sync local directories with remote lakeFS locations
   log             Show log of commits
   merge           Merge & commit changes from source branch into destination branch
   metastore       Manage metastore commands

pkg/ioutils/io.go → pkg/fileutil/io.go

@@ -1,12 +1,12 @@
-package ioutils
+package fileutil
 
 import "os"
 
 // IsDir Returns true if p is a directory, otherwise false
-func IsDir(p string) bool {
+func IsDir(p string) (bool, error) {
 	stat, err := os.Stat(p)
 	if err != nil {
-		return false
+		return false, err
 	}
-	return stat.IsDir()
+	return stat.IsDir(), nil
 }

pkg/git/errors.go

@@ -2,10 +2,9 @@ package git
 
 import (
 	"errors"
-	"fmt"
 )
 
 var (
 	ErrGitError       = errors.New("git error")
-	ErrNotARepository = fmt.Errorf("not a git repository")
+	ErrNotARepository = errors.New("not a git repository")
 )

pkg/git/git.go

@@ -2,14 +2,16 @@ package git
 
 import (
 	"bufio"
+	"bytes"
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"strings"
 
-	"github.com/treeverse/lakefs/pkg/ioutils"
+	"github.com/treeverse/lakefs/pkg/fileutil"
 	"golang.org/x/exp/slices"
 )
 
@@ -18,130 +20,127 @@ const (
 	IgnoreDefaultMode = 0644
 )
 
-func git(dir string, args ...string) (int, string) {
-	stdout := new(strings.Builder)
-	stderr := new(strings.Builder)
+func git(dir string, args ...string) (string, error) {
 	cmd := exec.Command("git", args...)
 	cmd.Dir = dir
-	cmd.Stdout = stdout
-	cmd.Stderr = stderr
-	err := cmd.Run()
-	if err != nil {
-		var exitError *exec.ExitError
-		if errors.As(err, &exitError) {
-			return exitError.ExitCode(), stderr.String()
-		}
-		return -1, err.Error()
-	}
-	return 0, stdout.String()
+	out, err := cmd.CombinedOutput()
+	return string(out), err
 }
 
-// IsGitRepository Return true if dir is a path to a directory in a git repository, false otherwise
-func IsGitRepository(dir string) bool {
-	rc, _ := git(dir, "rev-parse", "--is-inside-work-tree")
-	return rc == 0
+// IsRepository Return true if dir is a path to a directory in a git repository, false otherwise
+func IsRepository(dir string) bool {
+	_, err := git(dir, "rev-parse", "--is-inside-work-tree")
+	return err == nil
 }
 
-// GetGitRepositoryPath Returns the git repository root path if dir is a directory inside a git repository, otherwise returns error
-func GetGitRepositoryPath(dir string) (string, error) {
-	rc, out := git(dir, "rev-parse", "--show-toplevel")
-	if rc == 0 {
+// 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")
+	if err == nil {
 		return strings.TrimSpace(out), nil
 	}
-	if strings.Contains(out, ErrNotARepository.Error()) {
+	if strings.Contains(out, "not a git repository") {
 		return "", ErrNotARepository
 	}
-	return "", fmt.Errorf("%w: exit code %d: %s", ErrGitError, rc, out)
+	return "", fmt.Errorf("%s: %w", out, ErrGitError)
 }
 
-// Ignore Modify .ignore file to include a section headed by the marker string and contains the provided paths
-// If section exists, it will append paths to the given section, otherwise writes the sections at the end of the file.
-// File paths must be absolute.
-// Creates the .ignore file if it doesn't exist.
-func Ignore(dir string, ignorePaths, excludePaths []string, marker string) (string, error) {
-	gitDir, err := GetGitRepositoryPath(dir)
-	if err != nil {
-		return "", err
-	}
-
-	var ignoreEntries []string
-
-	for _, p := range ignorePaths {
-		pathInRepo, err := filepath.Rel(gitDir, p)
+func createEntriesForIgnore(dir string, paths []string, exclude bool) ([]string, error) {
+	var entries []string
+	for _, p := range paths {
+		pathInRepo, err := filepath.Rel(dir, p)
 		if err != nil {
-			return "", err
+			return nil, fmt.Errorf("%s :%w", p, err)
+		}
+		isDir, err := fileutil.IsDir(p)
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return nil, fmt.Errorf("%s :%w", p, err)
 		}
-		if ioutils.IsDir(p) {
+		if isDir {
 			pathInRepo = filepath.Join(pathInRepo, "*")
 		}
-		ignoreEntries = append(ignoreEntries, pathInRepo)
-	}
-	for _, p := range excludePaths {
-		pathInRepo, err := filepath.Rel(gitDir, p)
-		if err != nil {
-			return "", err
+		if exclude {
+			pathInRepo = "!" + pathInRepo
 		}
-		if ioutils.IsDir(p) {
-			pathInRepo = filepath.Join(pathInRepo, "*")
+		entries = append(entries, pathInRepo)
+	}
+	return entries, nil
+}
+
+func updateIgnoreFileSection(contents []byte, marker string, entries []string) []byte {
+	var newContent []byte
+	scanner := bufio.NewScanner(bytes.NewReader(contents))
+	for scanner.Scan() {
+		line := strings.TrimSpace(scanner.Text())
+		newContent = append(newContent, []byte(line+"\n")...)
+		if line == marker {
+			for scanner.Scan() {
+				line = strings.TrimSpace(scanner.Text())
+				if line == "" {
+					break
+				}
+				if !slices.Contains(entries, line) {
+					newContent = append(newContent, []byte(line+"\n")...)
+				}
+			}
+			buffer := strings.Join(entries, "\n") + "\n"
+			newContent = append(newContent, buffer...)
 		}
-		ignoreEntries = append(ignoreEntries, "!"+pathInRepo)
 	}
 
-	ignoreFilePath := filepath.Join(gitDir, IgnoreFile)
+	return newContent
+}
+
+// Ignore modify/create .ignore file to include a section headed by the marker string and contains the provided ignore and exclude paths.
+// If section exists, it will append paths to the given section, otherwise writes the section at the end of the file.
+// All file paths must be absolute.
+// dir is a path in the git repository, if a .gitignore file is not found, a new file will be created in the repository root
+func Ignore(dir string, ignorePaths, excludePaths []string, marker string) (string, error) {
+	gitDir, err := GetRepositoryPath(dir)
+	if err != nil {
+		return "", err
+	}
+
+	ignoreEntries, err := createEntriesForIgnore(gitDir, ignorePaths, false)
+	if err != nil {
+		return "", err
+	}
+	excludeEntries, err := createEntriesForIgnore(gitDir, excludePaths, true)
+	if err != nil {
+		return "", err
+	}
+	ignoreEntries = append(ignoreEntries, excludeEntries...)
 
-	found := false
 	var (
-		mode          os.FileMode = IgnoreDefaultMode
-		ignoreContent []string
+		mode       os.FileMode = IgnoreDefaultMode
+		ignoreFile []byte
 	)
+	ignoreFilePath := filepath.Join(gitDir, IgnoreFile)
 	markerLine := "# " + marker
 	info, err := os.Stat(ignoreFilePath)
 	switch {
 	case err == nil: // ignore file exists
 		mode = info.Mode()
-		ignoreFile, err := os.Open(ignoreFilePath)
+		ignoreFile, err = os.ReadFile(ignoreFilePath)
 		if err != nil {
 			return "", err
 		}
-		fileScanner := bufio.NewScanner(ignoreFile)
-		for fileScanner.Scan() {
-			line := strings.TrimSpace(fileScanner.Text())
-			ignoreContent = append(ignoreContent, line)
-			if line == markerLine {
-				found = true
-				for fileScanner.Scan() {
-					line = strings.TrimSpace(fileScanner.Text())
-					if line == "" {
-						ignoreContent = append(ignoreContent, "")
-						break
-					}
-					if !slices.Contains(ignoreEntries, line) {
-						ignoreContent = append(ignoreContent, line)
-					}
-				}
-				ignoreContent = append(ignoreContent, ignoreEntries...)
-			}
-		}
-
-		if !found { // Add the marker and ignore list to the end of the file
-			ignoreContent = append(ignoreContent, "")
-			ignoreContent = append(ignoreContent, markerLine)
-			ignoreContent = append(ignoreContent, ignoreEntries...)
+		idx := bytes.Index(ignoreFile, []byte(markerLine))
+		if idx == -1 {
+			section := markerLine + "\n" + strings.Join(ignoreEntries, "\n") + "\n"
+			ignoreFile = append(ignoreFile, section...)
+		} else { // Update section
+			ignoreFile = updateIgnoreFileSection(ignoreFile, markerLine, ignoreEntries)
 		}
 
-		err = ignoreFile.Close()
-		if err != nil {
-			return "", err
-		}
 	case !os.IsNotExist(err):
 		return "", err
 	default: // File doesn't exist
-		ignoreContent = append(ignoreContent, markerLine)
-		ignoreContent = append(ignoreContent, ignoreEntries...)
+		section := markerLine + "\n" + strings.Join(ignoreEntries, "\n") + "\n"
+		ignoreFile = append(ignoreFile, []byte(section)...)
 	}
 
-	buffer := strings.Join(ignoreContent, "\n") + "\n"
-	if err = os.WriteFile(ignoreFilePath, []byte(buffer), mode); err != nil {
+	if err = os.WriteFile(ignoreFilePath, ignoreFile, mode); err != nil {
 		return "", err
 	}