pkg/gitinterface: remove os.Chdir; set cmd.Dir on the executor instead

LoadRepository did os.Chdir(repositoryPath) to run 'git rev-parse
--git-dir', and Status/RestoreWorktree/tree-restore did the same to run
worktree-relative commands. os.Chdir is process-global; concurrent
LoadRepository calls for different paths could each resolve the wrong
gitDirPath.

Adds executor.withDir(dir) which sets cmd.Dir. LoadRepository now runs
'git rev-parse --absolute-git-dir' with cmd.Dir set to the repository
path (and EvalSymlinks the result to match prior behaviour). The
worktree commands use withDir(worktree).

TestLoadRepositoryConcurrent runs 20 pairs of LoadRepository for two
distinct repos in parallel under -race and asserts each gets its own
gitDirPath.

Signed-off-by: Andrew Nesbitt <andrewnez@gmail.com>

pkg/gitinterface/repository.go

@@ -64,18 +64,9 @@ func LoadRepository(repositoryPath string) (*Repository, error) {
 	}
 
 	repo := &Repository{clock: clockwork.NewRealClock()}
-	currentDir, err := os.Getwd()
-	if err != nil {
-		return nil, err
-	}
-
-	if err = os.Chdir(repositoryPath); err != nil {
-		return nil, err
-	}
-	defer os.Chdir(currentDir) //nolint:errcheck
 
 	slog.Debug("Identifying git directory for repository...")
-	stdOut, stdErr, err := repo.executor("rev-parse", "--git-dir").withoutGitDir().execute()
+	stdOut, stdErr, err := repo.executor("rev-parse", "--absolute-git-dir").withoutGitDir().withDir(repositoryPath).execute()
 	if err != nil {
 		errContents, newErr := io.ReadAll(stdErr)
 		if newErr != nil {
@@ -89,9 +80,7 @@ func LoadRepository(repositoryPath string) (*Repository, error) {
 		return nil, fmt.Errorf("unable to identify git directory for repository: %w", err)
 	}
 
-	// git rev-parse --git-dir returns a local path, so filepath.Abs gives us
-	// the final path _including_ symlink follows.
-	absPath, err := filepath.Abs(strings.TrimSpace(string(stdOutContents)))
+	absPath, err := filepath.EvalSymlinks(strings.TrimSpace(string(stdOutContents)))
 	if err != nil {
 		return nil, err
 	}
@@ -109,6 +98,7 @@ type executor struct {
 	args        []string
 	env         []string
 	stdIn       io.Reader
+	dir         string
 	unsetGitDir bool
 }
 
@@ -132,6 +122,14 @@ func (e *executor) withoutGitDir() *executor {
 	return e
 }
 
+// withDir runs the command with the given working directory instead of the
+// process's. Use this for worktree-relative commands (status, restore) so the
+// process-global os.Chdir is never touched.
+func (e *executor) withDir(dir string) *executor {
+	e.dir = dir
+	return e
+}
+
 // withStdIn sets the contents of stdin to be passed in to the command.
 func (e *executor) withStdIn(stdIn *bytes.Buffer) *executor {
 	e.stdIn = stdIn
@@ -170,6 +168,9 @@ func (e *executor) execute() (io.Reader, io.Reader, error) {
 	cmd := exec.Command(binary, e.args...) //nolint:gosec
 	cmd.Env = e.env
 	cmd.Env = append(cmd.Env, "LC_ALL=C") // force git to the C (and thus english) locale
+	if e.dir != "" {
+		cmd.Dir = e.dir
+	}
 
 	var (
 		stdOut bytes.Buffer

pkg/gitinterface/repository_test.go

@@ -5,6 +5,7 @@ package gitinterface
 
 import (
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -65,3 +66,38 @@ func TestRepository(t *testing.T) {
 		assert.ErrorContains(t, err, "unable to identify git directory for repository")
 	})
 }
+
+func TestLoadRepositoryConcurrent(t *testing.T) {
+	// LoadRepository must not change the process working directory; concurrent
+	// callers for different paths must each resolve their own gitDirPath.
+	dirA := t.TempDir()
+	dirB := t.TempDir()
+	_ = CreateTestGitRepository(t, dirA, true)
+	_ = CreateTestGitRepository(t, dirB, false)
+
+	wantA, err := filepath.EvalSymlinks(dirA)
+	require.NoError(t, err)
+	wantB, err := filepath.EvalSymlinks(filepath.Join(dirB, ".git"))
+	require.NoError(t, err)
+
+	const iterations = 20
+	var wg sync.WaitGroup
+	for i := 0; i < iterations; i++ {
+		wg.Add(2)
+		go func() {
+			defer wg.Done()
+			r, err := LoadRepository(dirA)
+			if assert.NoError(t, err) {
+				assert.Equal(t, wantA, r.GetGitDir())
+			}
+		}()
+		go func() {
+			defer wg.Done()
+			r, err := LoadRepository(dirB)
+			if assert.NoError(t, err) {
+				assert.Equal(t, wantB, r.GetGitDir())
+			}
+		}()
+	}
+	wg.Wait()
+}

pkg/gitinterface/status.go

@@ -6,7 +6,6 @@ package gitinterface
 import (
 	"errors"
 	"fmt"
-	"os"
 	"strings"
 )
 
@@ -100,16 +99,8 @@ func (r *Repository) Status() (map[string]FileStatus, error) {
 	if !r.IsBare() {
 		worktree = strings.TrimSuffix(worktree, ".git") // TODO: this doesn't support detached git dir
 	}
-	cwd, err := os.Getwd()
-	if err != nil {
-		return nil, err
-	}
-	if err := os.Chdir(worktree); err != nil {
-		return nil, err
-	}
-	defer os.Chdir(cwd) //nolint:errcheck
 
-	output, err := r.executor("status", "--porcelain=1", "-z", "--untracked-files=all", "--ignored").executeString()
+	output, err := r.executor("status", "--porcelain=1", "-z", "--untracked-files=all", "--ignored").withDir(worktree).executeString()
 	if err != nil {
 		return nil, fmt.Errorf("unable to check status of repository: %w", err)
 	}

pkg/gitinterface/tree.go

@@ -303,19 +303,11 @@ func (r *Repository) CreateSubtreeFromUpstreamRepository(upstream *Repository, u
 		}
 		if head == localRef {
 			worktree := strings.TrimSuffix(r.gitDirPath, ".git") // TODO: this doesn't support detached git dir
-			cwd, err := os.Getwd()
-			if err != nil {
-				return nil, err
-			}
-			if err := os.Chdir(worktree); err != nil {
-				return nil, err
-			}
-			defer os.Chdir(cwd) //nolint:errcheck
 
-			if _, err := r.executor("restore", "--staged", localPath).executeString(); err != nil {
+			if _, err := r.executor("restore", "--staged", localPath).withDir(worktree).executeString(); err != nil {
 				return nil, err
 			}
-			if _, err := r.executor("restore", localPath).executeString(); err != nil {
+			if _, err := r.executor("restore", localPath).withDir(worktree).executeString(); err != nil {
 				return nil, err
 			}
 		}

pkg/gitinterface/utils.go

@@ -5,7 +5,6 @@ package gitinterface
 
 import (
 	"fmt"
-	"os"
 	"path"
 	"strings"
 	"testing"
@@ -51,20 +50,12 @@ func (r *Repository) RestoreWorktree(t *testing.T) {
 	if !r.IsBare() {
 		worktree = strings.TrimSuffix(worktree, ".git") // TODO: this doesn't support detached git dir
 	}
-	cwd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if err := os.Chdir(worktree); err != nil {
-		t.Fatal(err)
-	}
-	defer os.Chdir(cwd) //nolint:errcheck
 
-	if _, err := r.executor("restore", "--staged", ".").executeString(); err != nil {
+	if _, err := r.executor("restore", "--staged", ".").withDir(worktree).executeString(); err != nil {
 		t.Fatal(err)
 	}
 
-	if _, err := r.executor("restore", ".").executeString(); err != nil {
+	if _, err := r.executor("restore", ".").withDir(worktree).executeString(); err != nil {
 		t.Fatal(err)
 	}
 }