Merge pull request #117 from github/improve-repository-detection

elhmn · web-flow · commit b84ee4d41db7 · 2023-12-17T18:27:10.000+01:00
Improve repository detection and deal with `safe.bareRepository=explicit`
diff --git a/git-sizer.go b/git-sizer.go
@@ -134,7 +134,7 @@ func mainImplementation(ctx context.Context, stdout, stderr io.Writer, args []st
 
 	// Try to open the repository, but it's not an error yet if this
 	// fails, because the user might only be asking for `--help`.
-	repo, repoErr := git.NewRepository(".")
+	repo, repoErr := git.NewRepositoryFromPath(".")
 
 	flags := pflag.NewFlagSet("git-sizer", pflag.ContinueOnError)
 	flags.Usage = func() {
diff --git a/git/git.go b/git/git.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -15,26 +16,31 @@ type ObjectType string
 
 // Repository represents a Git repository on disk.
 type Repository struct {
-	path string
+	// gitDir is the path to the `GIT_DIR` for this repository. It
+	// might be absolute or it might be relative to the current
+	// directory.
+	gitDir string
 
 	// gitBin is the path of the `git` executable that should be used
 	// when running commands in this repository.
 	gitBin string
 }
 
-// smartJoin returns the path that can be described as `relPath`
-// relative to `path`, given that `path` is either absolute or is
-// relative to the current directory.
+// smartJoin returns `relPath` if it is an absolute path. If not, it
+// assumes that `relPath` is relative to `path`, so it joins them
+// together and returns the result. In that case, if `path` itself is
+// relative, then the return value is also relative.
 func smartJoin(path, relPath string) string {
 	if filepath.IsAbs(relPath) {
 		return relPath
 	}
 	return filepath.Join(path, relPath)
 }
 
-// NewRepository creates a new repository object that can be used for
-// running `git` commands within that repository.
-func NewRepository(path string) (*Repository, error) {
+// NewRepositoryFromGitDir creates a new `Repository` object that can
+// be used for running `git` commands, given the value of `GIT_DIR`
+// for the repository.
+func NewRepositoryFromGitDir(gitDir string) (*Repository, error) {
 	// Find the `git` executable to be used:
 	gitBin, err := findGitBin()
 	if err != nil {
@@ -43,6 +49,34 @@ func NewRepository(path string) (*Repository, error) {
 		)
 	}
 
+	repo := Repository{
+		gitDir: gitDir,
+		gitBin: gitBin,
+	}
+
+	full, err := repo.IsFull()
+	if err != nil {
+		return nil, fmt.Errorf("determining whether the repository is a full clone: %w", err)
+	}
+	if !full {
+		return nil, errors.New("this appears to be a shallow clone; full clone required")
+	}
+
+	return &repo, nil
+}
+
+// NewRepositoryFromPath creates a new `Repository` object that can be
+// used for running `git` commands within `path`. It does so by asking
+// `git` what `GIT_DIR` to use. Git, in turn, bases its decision on
+// the path and the environment.
+func NewRepositoryFromPath(path string) (*Repository, error) {
+	gitBin, err := findGitBin()
+	if err != nil {
+		return nil, fmt.Errorf(
+			"could not find 'git' executable (is it in your PATH?): %w", err,
+		)
+	}
+
 	//nolint:gosec // `gitBin` is chosen carefully, and `path` is the
 	// path to the repository.
 	cmd := exec.Command(gitBin, "-C", path, "rev-parse", "--git-dir")
@@ -63,25 +97,28 @@ func NewRepository(path string) (*Repository, error) {
 	}
 	gitDir := smartJoin(path, string(bytes.TrimSpace(out)))
 
-	//nolint:gosec // `gitBin` is chosen carefully.
-	cmd = exec.Command(gitBin, "rev-parse", "--git-path", "shallow")
-	cmd.Dir = gitDir
-	out, err = cmd.Output()
+	return NewRepositoryFromGitDir(gitDir)
+}
+
+// IsFull returns `true` iff `repo` appears to be a full clone.
+func (repo *Repository) IsFull() (bool, error) {
+	shallow, err := repo.GitPath("shallow")
 	if err != nil {
-		return nil, fmt.Errorf(
-			"could not run 'git rev-parse --git-path shallow': %w", err,
-		)
+		return false, err
 	}
-	shallow := smartJoin(gitDir, string(bytes.TrimSpace(out)))
+
 	_, err = os.Lstat(shallow)
 	if err == nil {
-		return nil, errors.New("this appears to be a shallow clone; full clone required")
+		return false, nil
 	}
 
-	return &Repository{
-		path:   gitDir,
-		gitBin: gitBin,
-	}, nil
+	if !errors.Is(err, fs.ErrNotExist) {
+		return false, err
+	}
+
+	// The `shallow` file is absent, which is what we expect
+	// for a full clone.
+	return true, nil
 }
 
 func (repo *Repository) GitCommand(callerArgs ...string) *exec.Cmd {
@@ -103,15 +140,33 @@ func (repo *Repository) GitCommand(callerArgs ...string) *exec.Cmd {
 
 	cmd.Env = append(
 		os.Environ(),
-		"GIT_DIR="+repo.path,
+		"GIT_DIR="+repo.gitDir,
 		// Disable grafts when running our commands:
 		"GIT_GRAFT_FILE="+os.DevNull,
 	)
 
 	return cmd
 }
 
-// Path returns the path to `repo`.
-func (repo *Repository) Path() string {
-	return repo.path
+// GitDir returns the path to `repo`'s `GIT_DIR`. It might be absolute
+// or it might be relative to the current directory.
+func (repo *Repository) GitDir() string {
+	return repo.gitDir
+}
+
+// GitPath returns that path of a file within the git repository, by
+// calling `git rev-parse --git-path $relPath`. The returned path is
+// relative to the current directory.
+func (repo *Repository) GitPath(relPath string) (string, error) {
+	cmd := repo.GitCommand("rev-parse", "--git-path", relPath)
+	out, err := cmd.Output()
+	if err != nil {
+		return "", fmt.Errorf(
+			"running 'git rev-parse --git-path %s': %w", relPath, err,
+		)
+	}
+	// `git rev-parse --git-path` is documented to return the path
+	// relative to the current directory. Since we haven't changed the
+	// current directory, we can use it as-is:
+	return string(bytes.TrimSpace(out)), nil
 }
diff --git a/git/git_bin.go b/git/git_bin.go
@@ -2,26 +2,41 @@ package git
 
 import (
 	"path/filepath"
+	"sync"
 
 	"github.com/cli/safeexec"
 )
 
+// This variable will be used to memoize the result of `findGitBin()`,
+// since its return value only depends on the environment.
+var gitBinMemo struct {
+	once sync.Once
+
+	gitBin string
+	err    error
+}
+
 // findGitBin finds the `git` binary in PATH that should be used by
 // the rest of `git-sizer`. It uses `safeexec` to find the executable,
 // because on Windows, `exec.Cmd` looks not only in PATH, but also in
 // the current directory. This is a potential risk if the repository
 // being scanned is hostile and non-bare because it might possibly
 // contain an executable file named `git`.
 func findGitBin() (string, error) {
-	gitBin, err := safeexec.LookPath("git")
-	if err != nil {
-		return "", err
-	}
+	gitBinMemo.once.Do(func() {
+		p, err := safeexec.LookPath("git")
+		if err != nil {
+			gitBinMemo.err = err
+			return
+		}
 
-	gitBin, err = filepath.Abs(gitBin)
-	if err != nil {
-		return "", err
-	}
+		p, err = filepath.Abs(p)
+		if err != nil {
+			gitBinMemo.err = err
+			return
+		}
 
-	return gitBin, nil
+		gitBinMemo.gitBin = p
+	})
+	return gitBinMemo.gitBin, gitBinMemo.err
 }
diff --git a/git_sizer_test.go b/git_sizer_test.go
@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -273,7 +272,10 @@ func TestRefSelections(t *testing.T) {
 				args := []string{"--show-refs", "--no-progress", "--json", "--json-version=2"}
 				args = append(args, p.args...)
 				cmd := exec.Command(executable, args...)
-				cmd.Dir = repo.Path
+				cmd.Env = append(
+					os.Environ(),
+					"GIT_DIR="+repo.Path,
+				)
 				var stdout bytes.Buffer
 				cmd.Stdout = &stdout
 				var stderr bytes.Buffer
@@ -520,7 +522,10 @@ References (included references marked with '+'):
 
 				args := append([]string{"--show-refs", "-v", "--no-progress"}, p.args...)
 				cmd := exec.Command(executable, args...)
-				cmd.Dir = repo.Path
+				cmd.Env = append(
+					os.Environ(),
+					"GIT_DIR="+repo.Path,
+				)
 				var stdout bytes.Buffer
 				cmd.Stdout = &stdout
 				var stderr bytes.Buffer
@@ -760,7 +765,7 @@ func TestSubmodule(t *testing.T) {
 
 	ctx := context.Background()
 
-	tmp, err := ioutil.TempDir("", "submodule")
+	tmp, err := os.MkdirTemp("", "submodule")
 	require.NoError(t, err, "creating temporary directory")
 
 	defer func() {
diff --git a/internal/testutils/repoutils.go b/internal/testutils/repoutils.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -21,6 +20,7 @@ import (
 // TestRepo represents a git repository used for tests.
 type TestRepo struct {
 	Path string
+	bare bool
 }
 
 // NewTestRepo creates and initializes a test repository in a
@@ -29,7 +29,7 @@ type TestRepo struct {
 func NewTestRepo(t *testing.T, bare bool, pattern string) *TestRepo {
 	t.Helper()
 
-	path, err := ioutil.TempDir("", pattern)
+	path, err := os.MkdirTemp("", pattern)
 	require.NoError(t, err)
 
 	repo := TestRepo{Path: path}
@@ -38,6 +38,7 @@ func NewTestRepo(t *testing.T, bare bool, pattern string) *TestRepo {
 
 	return &TestRepo{
 		Path: path,
+		bare: bare,
 	}
 }
 
@@ -73,7 +74,7 @@ func (repo *TestRepo) Remove(t *testing.T) {
 func (repo *TestRepo) Clone(t *testing.T, pattern string) *TestRepo {
 	t.Helper()
 
-	path, err := ioutil.TempDir("", pattern)
+	path, err := os.MkdirTemp("", pattern)
 	require.NoError(t, err)
 
 	err = repo.GitCommand(
@@ -90,9 +91,15 @@ func (repo *TestRepo) Clone(t *testing.T, pattern string) *TestRepo {
 func (repo *TestRepo) Repository(t *testing.T) *git.Repository {
 	t.Helper()
 
-	r, err := git.NewRepository(repo.Path)
-	require.NoError(t, err)
-	return r
+	if repo.bare {
+		r, err := git.NewRepositoryFromGitDir(repo.Path)
+		require.NoError(t, err)
+		return r
+	} else {
+		r, err := git.NewRepositoryFromPath(repo.Path)
+		require.NoError(t, err)
+		return r
+	}
 }
 
 // localEnvVars is a list of the variable names that should be cleared
