git integration cleanup (#1194)

 * Delete a lot of unused git integration code
 * Don't crash in git integration code, return an error instead (remove `log.Fatalf` instances)
 * Provide a more helpful error message when the user supplies a non-existent commit

Fixes #1157

cli/internal/run/run.go

@@ -216,7 +216,7 @@ func (c *RunCommand) Run(args []string) int {
 	}
 	filteredPkgs, err := scope.ResolvePackages(runOptions.scopeOpts(), scmInstance, ctx, c.Ui, c.Config.Logger)
 	if err != nil {
-		c.logError(c.Config.Logger, "", fmt.Errorf("failed resolve packages to run %v", err))
+		c.logError(c.Config.Logger, "", fmt.Errorf("failed to resolve packages to run: %v", err))
 	}
 	c.Config.Logger.Debug("global hash", "value", ctx.GlobalHash)
 	c.Config.Logger.Debug("local cache folder", "path", runOptions.cacheFolder)

cli/internal/scm/git.go

@@ -1,68 +1,27 @@
 // Package scm abstracts operations on various tools like git
 // Currently, only git is supported.
-
+//
 // Adapted from https://github.com/thought-machine/please/tree/master/src/scm
 // Copyright Thought Machine, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 package scm
 
 import (
-	"bufio"
 	"fmt"
-	"log"
-	"os"
 	"os/exec"
 	"path/filepath"
-	"strconv"
 	"strings"
-	"time"
 
-	"github.com/sourcegraph/go-diff/diff"
+	"github.com/pkg/errors"
 )
 
 // git implements operations on a git repository.
 type git struct {
 	repoRoot string
 }
 
-// DescribeIdentifier returns the string that is a "human-readable" identifier of the given revision.
-func (g *git) DescribeIdentifier(revision string) string {
-	out, err := exec.Command("git", "describe", "--always", revision).CombinedOutput()
-	if err != nil {
-		log.Fatalf("Failed to read %s: %s", revision, err)
-	}
-	return strings.TrimSpace(string(out))
-}
-
-// CurrentRevIdentifier returns the string that specifies what the current revision is.
-func (g *git) CurrentRevIdentifier() string {
-	out, err := exec.Command("git", "rev-parse", "HEAD").CombinedOutput()
-	if err != nil {
-		log.Fatalf("Failed to read HEAD: %s", err)
-	}
-	return strings.TrimSpace(string(out))
-}
-
-// ChangesIn returns a list of modified files in the given diffSpec.
-func (g *git) ChangesIn(diffSpec string, relativeTo string) []string {
-	if relativeTo == "" {
-		relativeTo = g.repoRoot
-	}
-	files := make([]string, 0)
-	command := []string{"diff-tree", "--no-commit-id", "--name-only", "-r", diffSpec}
-	out, err := exec.Command("git", command...).CombinedOutput()
-	if err != nil {
-		log.Fatalf("unable to determine changes: %s", err)
-	}
-	output := strings.Split(string(out), "\n")
-	for _, o := range output {
-		files = append(files, g.fixGitRelativePath(strings.TrimSpace(o), relativeTo))
-	}
-	return files
-}
-
 // ChangedFiles returns a list of modified files since the given commit, optionally including untracked files.
-func (g *git) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) []string {
+func (g *git) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) ([]string, error) {
 	if relativeTo == "" {
 		relativeTo = g.repoRoot
 	}
@@ -71,7 +30,7 @@ func (g *git) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo
 
 	out, err := exec.Command("git", append(command, relSuffix...)...).CombinedOutput()
 	if err != nil {
-		log.Fatalf("unable to find changes: %s", err)
+		return nil, errors.Wrapf(err, "finding changes relative to %v", relativeTo)
 	}
 	files := strings.Split(string(out), "\n")
 
@@ -81,7 +40,13 @@ func (g *git) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo
 		command = []string{"diff", "--name-only", fromCommit + "...HEAD"}
 		out, err = exec.Command("git", append(command, relSuffix...)...).CombinedOutput()
 		if err != nil {
-			log.Fatalf("unable to check diff vs. %s: %s", fromCommit, err)
+			// Check if we can provide a better error message for non-existent commits.
+			// If we error on the check or can't find it, fall back to whatever error git
+			// reported.
+			if exists, err := commitExists(fromCommit); err == nil && !exists {
+				return nil, fmt.Errorf("commit %v does not exist", fromCommit)
+			}
+			return nil, errors.Wrapf(err, "git comparing with %v", fromCommit)
 		}
 		committedChanges := strings.Split(string(out), "\n")
 		files = append(files, committedChanges...)
@@ -90,142 +55,39 @@ func (g *git) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo
 		command = []string{"ls-files", "--other", "--exclude-standard"}
 		out, err = exec.Command("git", append(command, relSuffix...)...).CombinedOutput()
 		if err != nil {
-			log.Fatalf("unable to determine untracked files: %s", err)
+			return nil, errors.Wrap(err, "finding untracked files")
 		}
 		untracked := strings.Split(string(out), "\n")
 		files = append(files, untracked...)
 	}
 	// git will report changed files relative to the worktree: re-relativize to relativeTo
 	normalized := make([]string, 0)
 	for _, f := range files {
-		normalized = append(normalized, g.fixGitRelativePath(strings.TrimSpace(f), relativeTo))
-	}
-	return normalized
-}
-
-func (g *git) fixGitRelativePath(worktreePath, relativeTo string) string {
-	p, err := filepath.Rel(relativeTo, filepath.Join(g.repoRoot, worktreePath))
-	if err != nil {
-		log.Fatalf("unable to determine relative path for %s and %s", g.repoRoot, relativeTo)
-	}
-	return p
-}
-
-const pleaseDoNotEdit = "# Entries below this point are managed by Turbo (DO NOT EDIT)"
-
-var defaultIgnoredFiles = []string{"plz-out", ".plzconfig.local"}
-
-func readUserEntries(file string) ([]string, error) {
-	f, err := os.Open(file)
-	if err != nil && !os.IsNotExist(err) {
-		return nil, err
-	}
-	defer f.Close()
-
-	scanner := bufio.NewScanner(f)
-
-	var userEntires []string
-	for scanner.Scan() {
-		line := scanner.Text()
-		if strings.TrimSpace(line) == pleaseDoNotEdit {
-			return userEntires, nil
-		}
-		userEntires = append(userEntires, line)
-	}
-	return userEntires, nil
-}
-
-func (g *git) IgnoreFiles(gitignore string, files []string) error {
-	// If we're generating the ignore in the root of the project, we should ignore some Please stuff too
-	if gitignore == ".gitignore" {
-		files = append(defaultIgnoredFiles, files...)
-	}
-
-	p := filepath.Join(g.repoRoot, gitignore)
-
-	userEntries, err := readUserEntries(p)
-	if err != nil {
-		return err
-	}
-
-	lines := userEntries
-	if len(lines) != 0 && lines[len(lines)-1] != "" {
-		lines = append(lines, "")
-	}
-	lines = append(lines, pleaseDoNotEdit)
-	lines = append(lines, files...)
-
-	if err := os.RemoveAll(p); err != nil && err != os.ErrNotExist {
-		return err
-	}
-
-	file, err := os.Create(p)
-	if err != nil {
-		return err
-	}
-	defer file.Close()
-
-	for _, line := range lines {
-		if _, err := fmt.Fprintln(file, line); err != nil {
-			return err
+		normalizedFile, err := g.fixGitRelativePath(strings.TrimSpace(f), relativeTo)
+		if err != nil {
+			return nil, err
 		}
+		normalized = append(normalized, normalizedFile)
 	}
-	return nil
+	return normalized, nil
 }
 
-func (g *git) Remove(names []string) error {
-	cmd := exec.Command("git", append([]string{"rm", "-q"}, names...)...)
-	out, err := cmd.CombinedOutput()
+func commitExists(commit string) (bool, error) {
+	err := exec.Command("git", "cat-file", "-t", commit).Run()
 	if err != nil {
-		return fmt.Errorf("git rm failed: %w %s", err, out)
-	}
-	return nil
-}
-
-func (g *git) ChangedLines() (map[string][]int, error) {
-	cmd := exec.Command("git", "diff", "origin/master", "--unified=0", "--no-color", "--no-ext-diff")
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		return nil, fmt.Errorf("git diff failed: %w", err)
-	}
-	return g.parseChangedLines(out)
-}
-
-func (g *git) parseChangedLines(input []byte) (map[string][]int, error) {
-	m := map[string][]int{}
-	fds, err := diff.ParseMultiFileDiff(input)
-	for _, fd := range fds {
-		m[strings.TrimPrefix(fd.NewName, "b/")] = g.parseHunks(fd.Hunks)
-	}
-	return m, err
-}
-
-func (g *git) parseHunks(hunks []*diff.Hunk) []int {
-	ret := []int{}
-	for _, hunk := range hunks {
-		for i := 0; i < int(hunk.NewLines); i++ {
-			ret = append(ret, int(hunk.NewStartLine)+i)
+		exitErr := &exec.ExitError{}
+		if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 {
+			return false, nil
 		}
+		return false, err
 	}
-	return ret
-}
-
-func (g *git) Checkout(revision string) error {
-	if out, err := exec.Command("git", "checkout", revision).CombinedOutput(); err != nil {
-		return fmt.Errorf("git checkout of %s failed: %w\n%s", revision, err, out)
-	}
-	return nil
+	return true, nil
 }
 
-func (g *git) CurrentRevDate(format string) string {
-	out, err := exec.Command("git", "show", "-s", "--format=%ct").CombinedOutput()
-	if err != nil {
-		return "Unknown"
-	}
-	timestamp, err := strconv.ParseInt(strings.TrimSpace(string(out)), 10, 64)
+func (g *git) fixGitRelativePath(worktreePath, relativeTo string) (string, error) {
+	p, err := filepath.Rel(relativeTo, filepath.Join(g.repoRoot, worktreePath))
 	if err != nil {
-		return err.Error()
+		return "", errors.Wrapf(err, "unable to determine relative path for %s and %s", g.repoRoot, relativeTo)
 	}
-	t := time.Unix(timestamp, 0)
-	return t.Format(format)
+	return p, nil
 }

cli/internal/scm/scm.go

@@ -1,6 +1,6 @@
 // Package scm abstracts operations on various tools like git
 // Currently, only git is supported.
-
+//
 // Adapted from https://github.com/thought-machine/please/tree/master/src/scm
 // Copyright Thought Machine, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
@@ -19,32 +19,35 @@ var ErrFallback = errors.New("cannot find a .git folder. Falling back to manual
 // An SCM represents an SCM implementation that we can ask for various things.
 type SCM interface {
 	// ChangedFiles returns a list of modified files since the given commit, optionally including untracked files.*/
-	ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) []string
+	ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) ([]string, error)
 }
 
-// New returns a new SCM instance for this repo root.
+// newGitSCM returns a new SCM instance for this repo root.
 // It returns nil if there is no known implementation there.
-func New(repoRoot string) SCM {
+func newGitSCM(repoRoot string) SCM {
 	if fs.PathExists(filepath.Join(repoRoot, ".git")) {
 		return &git{repoRoot: repoRoot}
 	}
 	return nil
 }
 
-// NewFallback returns a new SCM instance for this repo root.
+// newFallback returns a new SCM instance for this repo root.
 // If there is no known implementation it returns a stub.
-func NewFallback(repoRoot string) (SCM, error) {
-	if scm := New(repoRoot); scm != nil {
+func newFallback(repoRoot string) (SCM, error) {
+	if scm := newGitSCM(repoRoot); scm != nil {
 		return scm, nil
 	}
 
 	return &stub{}, ErrFallback
 }
 
-func FromInRepo(cwd string) (SCM, error) {
-	dotGitDir, err := fs.FindupFrom(".git", cwd)
+// FromInRepo produces an SCM instance, given a path within a
+// repository. It does not need to be a git repository, and if
+// it is not, the given path is assumed to be the root.
+func FromInRepo(repoRoot string) (SCM, error) {
+	dotGitDir, err := fs.FindupFrom(".git", repoRoot)
 	if err != nil {
 		return nil, err
 	}
-	return NewFallback(filepath.Dir(dotGitDir))
+	return newFallback(filepath.Dir(dotGitDir))
 }

cli/internal/scm/stub.go

@@ -19,8 +19,8 @@ func (s *stub) ChangesIn(diffSpec string, relativeTo string) []string {
 	return nil
 }
 
-func (s *stub) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) []string {
-	return nil
+func (s *stub) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) ([]string, error) {
+	return nil, nil
 }
 
 func (s *stub) IgnoreFiles(string, []string) error {

cli/internal/scope/scope.go

@@ -92,9 +92,13 @@ func (o *Opts) getPackageChangeFunc(scm scm.SCM, packageInfos map[interface{}]*f
 		// that the changes we're interested in are scoped, but we need to handle
 		// global dependencies changing as well. A future optimization might be to
 		// scope changed files more deeply if we know there are no global dependencies.
-		changedFiles := []string{}
+		var changedFiles []string
 		if since != "" {
-			changedFiles = scm.ChangedFiles(since, true, o.Cwd)
+			scmChangedFiles, err := scm.ChangedFiles(since, true, o.Cwd)
+			if err != nil {
+				return nil, err
+			}
+			changedFiles = scmChangedFiles
 		}
 		if hasRepoGlobalFileChanged, err := repoGlobalFileHasChanged(o, changedFiles); err != nil {
 			return nil, err

cli/internal/scope/scope_test.go

@@ -18,14 +18,14 @@ type mockSCM struct {
 	changed []string
 }
 
-func (m *mockSCM) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) []string {
+func (m *mockSCM) ChangedFiles(fromCommit string, includeUntracked bool, relativeTo string) ([]string, error) {
 	changed := []string{}
 	for _, change := range m.changed {
 		if strings.HasPrefix(change, relativeTo) {
 			changed = append(changed, change)
 		}
 	}
-	return changed
+	return changed, nil
 }
 
 func TestResolvePackages(t *testing.T) {