refactor git command function

.github/workflows/test.yaml

@@ -30,4 +30,4 @@ jobs:
         run: |
           go test -v -cover -race -count 1 -timeout 20s --tags deadlock_test -run Test_mirror_detect_race_clone ./internal/integration_test/...
           go test -v -cover -race -count 1 -timeout 60s --tags deadlock_test -run Test_mirror_detect_race_repo_pool ./internal/integration_test/...
-          # go test -v -cover -race -count 1 -timeout 240s --tags deadlock_test -run Test_mirror_detect_race_slow_fetch ./internal/integration_test/...
+          go test -v -cover -race -count 1 -timeout 240s --tags deadlock_test -run Test_mirror_detect_race_slow_fetch ./internal/integration_test/...

cleanup.go

@@ -1,19 +1,18 @@
 package main
 
 import (
+	"context"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"slices"
 	"strconv"
 	"strings"
 
+	"github.com/utilitywarehouse/git-mirror/internal/utils"
 	"github.com/utilitywarehouse/git-mirror/repopool"
 	"github.com/utilitywarehouse/git-mirror/repository"
 )
 
-var gitExecutablePath = exec.Command("git").String()
-
 // cleanupOrphanedRepos deletes directory of the repos from the default root
 // which are no longer referenced in config and it was removed while app was down.
 // Any removal while app is running is already handled by ensureConfig() hence
@@ -88,10 +87,6 @@ func isBareRepo(cwd string) (bool, error) {
 
 // runGitCommand runs git command with given arguments on given CWD
 func runGitCommand(cwd string, args ...string) (string, error) {
-	cmd := exec.Command(gitExecutablePath, args...)
-	if cwd != "" {
-		cmd.Dir = cwd
-	}
-	output, err := cmd.CombinedOutput()
-	return strings.TrimSpace(string(output)), err
+	output, err := utils.RunCommand(context.TODO(), logger, nil, cwd, gitExecutablePath, args...)
+	return strings.TrimSpace(output), err
 }

config_test.go

@@ -74,7 +74,7 @@ func Test_diffRepositories(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			applyGitDefaults(tt.initialConfig)
-			repoPool, err := repopool.New(t.Context(), *tt.initialConfig, nil, nil)
+			repoPool, err := repopool.New(t.Context(), *tt.initialConfig, nil, "", nil)
 			if err != nil {
 				t.Fatalf("could not create git mirror pool err:%v", err)
 			}
@@ -203,7 +203,7 @@ func Test_diffWorktrees(t *testing.T) {
 				t.Fatalf("failed to create repo error = %v", err)
 			}
 
-			repo, err := repository.New(*tt.initialRepoConf, nil, slog.Default())
+			repo, err := repository.New(*tt.initialRepoConf, "", nil, slog.Default())
 			if err != nil {
 				t.Fatalf("failed to create repo error = %v", err)
 			}

internal/integration_test/e2e_race_test.go

@@ -113,11 +113,6 @@ func Test_mirror_detect_race_clone(t *testing.T) {
 }
 
 func Test_mirror_detect_race_slow_fetch(t *testing.T) {
-	// replace global git path with slower git wrapper script
-	cwd, _ := os.Getwd()
-
-	os.Setenv("GIT_MIRROR_GIT_EXEC", exec.Command(path.Join(cwd, "git_slow_fetch.sh")).String())
-
 	testTmpDir := mustTmpDir(t)
 	defer os.RemoveAll(testTmpDir)
 
@@ -132,16 +127,28 @@ func Test_mirror_detect_race_slow_fetch(t *testing.T) {
 	t.Log("TEST-1: init upstream")
 	fileSHA1 := mustInitRepo(t, upstream, "file", testName+"-1")
 
-	repo := mustCreateRepoAndMirror(t, upstream, root, "", "")
-	// repo.mirrorTimeout = 2 * time.Minute
+	rc := repository.Config{
+		Remote:        "file://" + upstream,
+		Root:          root,
+		Interval:      testInterval,
+		MirrorTimeout: 2 * time.Minute, // testing slow fetch
+		GitGC:         "always",
+	}
+
+	// replace global git path with slower git wrapper script
+	cwd, _ := os.Getwd()
+	repo, err := repository.New(rc, exec.Command(path.Join(cwd, "git_slow_fetch.sh")).String(), testENVs, testLog)
+	if err != nil {
+		t.Fatalf("unable to create new repo error: %v", err)
+	}
+
+	if err := repo.Mirror(txtCtx); err != nil {
+		t.Fatalf("unable to mirror error: %v", err)
+	}
 
 	// verify checkout files
 	assertCommitLog(t, repo, "HEAD", "", fileSHA1, testName+"-1", []string{"file"})
 
-	// start mirror loop
-	go repo.StartLoop(ctx)
-	defer repo.StopLoop()
-
 	t.Run("slow-fetch-without-timeout", func(t *testing.T) {
 
 		// all following assertions will always be true
@@ -267,7 +274,7 @@ func Test_mirror_detect_race_repo_pool(t *testing.T) {
 		},
 	}
 
-	rp, err := repopool.New(t.Context(), rpc, testLog, testENVs)
+	rp, err := repopool.New(t.Context(), rpc, testLog, "", testENVs)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

internal/integration_test/e2e_test.go

@@ -6,7 +6,6 @@ import (
 	"io/fs"
 	"log/slog"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"slices"
 	"strings"
@@ -190,7 +189,7 @@ func Test_mirror_head_and_main(t *testing.T) {
 
 	repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
 	// add worktree for HEAD
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link2, ref2, []string{}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref2, Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// mirror again for 2nd worktree
@@ -254,7 +253,7 @@ func Test_mirror_bad_ref(t *testing.T) {
 		GitGC:         "always",
 		Worktrees:     []repository.WorktreeConfig{{Link: link, Ref: ref}},
 	}
-	repo, err := repository.New(rc, testENVs, testLog)
+	repo, err := repository.New(rc, "", testENVs, testLog)
 	if err != nil {
 		t.Fatalf("unable to create new repo error: %v", err)
 	}
@@ -288,7 +287,7 @@ func Test_mirror_other_branch(t *testing.T) {
 
 	repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
 	// add 2nd worktree
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link2, ref2, []string{}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref2, Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// mirror again for 2nd worktree
@@ -378,10 +377,10 @@ func Test_mirror_with_pathspec(t *testing.T) {
 	mustCommit(t, upstream, filepath.Join("dir2", "file"), t.Name()+"-main-2")
 
 	// add worktree for HEAD on dir2
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link2, ref2, []string{pathSpec2}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref2, Pathspecs: []string{pathSpec2}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link4, ref2, []string{pathSpec2}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link4, Ref: ref2, Pathspecs: []string{pathSpec2}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 
@@ -408,14 +407,14 @@ func Test_mirror_with_pathspec(t *testing.T) {
 	mustCommit(t, upstream, filepath.Join("dir3", "file"), t.Name()+"-main-3")
 
 	// add worktree for HEAD on dir3
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link3, ref3, []string{pathSpec3}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link3, Ref: ref3, Pathspecs: []string{pathSpec3}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// update worktree link4
 	if err := repo.RemoveWorktreeLink(link4); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link4, ref2, []string{pathSpec3, pathSpec2}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link4, Ref: ref2, Pathspecs: []string{pathSpec3, pathSpec2}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// mirror new commits
@@ -489,7 +488,7 @@ func Test_mirror_switch_branch_after_restart(t *testing.T) {
 
 	repo1 := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
 	// add 2nd worktree
-	if err := repo1.AddWorktreeLink(repository.WorktreeConfig{link2, ref2, []string{}}); err != nil {
+	if err := repo1.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref2, Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// mirror again for 2nd worktree
@@ -505,7 +504,7 @@ func Test_mirror_switch_branch_after_restart(t *testing.T) {
 
 	repo2 := mustCreateRepoAndMirror(t, upstream, root, link1, ref2)
 	// add 2nd worktree
-	if err := repo2.AddWorktreeLink(repository.WorktreeConfig{link2, ref1, []string{}}); err != nil {
+	if err := repo2.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref1, Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// mirror again for 2nd worktree
@@ -565,7 +564,7 @@ func Test_mirror_tag_sha(t *testing.T) {
 
 	repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
 	// add 2nd worktree
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link2, ref2, []string{}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref2, Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 	// mirror again for 2nd worktree
@@ -1292,7 +1291,7 @@ func Test_mirror_loop(t *testing.T) {
 
 	repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
 	// add worktree for HEAD
-	if err := repo.AddWorktreeLink(repository.WorktreeConfig{link2, ref2, []string{}}); err != nil {
+	if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link2, Ref: ref2, Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unable to add worktree error: %v", err)
 	}
 
@@ -1375,14 +1374,14 @@ func Test_RepoPool_Success(t *testing.T) {
 		},
 	}
 
-	rp, err := repopool.New(t.Context(), rpc, testLog, testENVs)
+	rp, err := repopool.New(t.Context(), rpc, testLog, "", testENVs)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
 	// add worktree
 	// we will verify this worktree in next mirror loop
-	if err := rp.AddWorktreeLink(remote1, repository.WorktreeConfig{"link3", "", []string{}}); err != nil {
+	if err := rp.AddWorktreeLink(remote1, repository.WorktreeConfig{Link: "link3", Ref: "", Pathspecs: []string{}}); err != nil {
 		t.Fatalf("unexpected err:%s", err)
 	}
 
@@ -1571,7 +1570,7 @@ func Test_RepoPool_Error(t *testing.T) {
 		},
 	}
 
-	rp, err := repopool.New(t.Context(), rpc, testLog, testENVs)
+	rp, err := repopool.New(t.Context(), rpc, testLog, "", testENVs)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -1648,12 +1647,12 @@ func mustCreateRepoAndMirror(t *testing.T, upstream, root, link, ref string) *re
 		MirrorTimeout: testTimeout,
 		GitGC:         "always",
 	}
-	repo, err := repository.New(rc, testENVs, testLog)
+	repo, err := repository.New(rc, "", testENVs, testLog)
 	if err != nil {
 		t.Fatalf("unable to create new repo error: %v", err)
 	}
 	if link != "" {
-		if err := repo.AddWorktreeLink(repository.WorktreeConfig{link, ref, []string{}}); err != nil {
+		if err := repo.AddWorktreeLink(repository.WorktreeConfig{Link: link, Ref: ref, Pathspecs: []string{}}); err != nil {
 			t.Fatalf("unable to add worktree error: %v", err)
 		}
 	}
@@ -1792,19 +1791,12 @@ func assertCommitLog(t *testing.T, repo *repository.Repository,
 	}
 }
 
-func mustExec(t *testing.T, cwd string, name string, arg ...string) string {
+func mustExec(t *testing.T, cwd string, command string, arg ...string) string {
 	t.Helper()
 
-	cmd := exec.Command(name, arg...)
-	if cwd != "" {
-		cmd.Dir = cwd
-	}
-
-	cmd.Env = testENVs
-
-	stdoutStderr, err := cmd.CombinedOutput()
+	out, err := utils.RunCommand(context.TODO(), slog.Default(), testENVs, cwd, command, arg...)
 	if err != nil {
-		t.Fatalf("err:%v run(%s): { stdoutStderr %q }", cmd.String(), err, stdoutStderr)
+		t.Fatal(err)
 	}
-	return strings.TrimSpace(string(stdoutStderr))
+	return strings.TrimSpace(out)
 }

internal/utils/utils.go

@@ -1,11 +1,16 @@
 package utils
 
 import (
+	"bytes"
+	"context"
 	"fmt"
 	"io/fs"
+	"log/slog"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
+	"time"
 )
 
 const defaultDirMode fs.FileMode = os.FileMode(0755) // 'rwxr-xr-x'
@@ -69,3 +74,44 @@ func AbsLink(root, link string) string {
 
 	return linkAbs
 }
+
+// RunCommand runs given command with given arguments on given CWD
+func RunCommand(ctx context.Context, log *slog.Logger, envs []string, cwd string, command string, args ...string) (string, error) {
+
+	cmdStr := command + " " + strings.Join(args, " ")
+	log.Log(ctx, -8, "running command", "cwd", cwd, "cmd", cmdStr)
+
+	cmd := exec.CommandContext(ctx, command, args...)
+	// force kill git & child process 5 seconds after sending it sigterm (when ctx is cancelled/timed out)
+	cmd.WaitDelay = 5 * time.Second
+	if cwd != "" {
+		cmd.Dir = cwd
+	}
+	outbuf := bytes.NewBuffer(nil)
+	errbuf := bytes.NewBuffer(nil)
+	cmd.Stdout = outbuf
+	cmd.Stderr = errbuf
+
+	// If Env is nil, the new process uses the current process's environment.
+	cmd.Env = []string{}
+
+	if len(envs) > 0 {
+		cmd.Env = append(cmd.Env, envs...)
+	}
+
+	start := time.Now()
+	err := cmd.Run()
+	runTime := time.Since(start)
+
+	stdout := strings.TrimSpace(outbuf.String())
+	stderr := strings.TrimSpace(errbuf.String())
+	if ctx.Err() == context.DeadlineExceeded {
+		err = ctx.Err()
+	}
+	if err != nil {
+		return "", fmt.Errorf("Run(%s): err:%w { stdout: %q, stderr: %q }", cmdStr, err, stdout, stderr)
+	}
+	log.Log(ctx, -8, "command result", "stdout", stdout, "stderr", stderr, "time", runTime)
+
+	return stdout, nil
+}

main.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/pprof"
 	"os"
+	"os/exec"
 	"os/signal"
 	"runtime/debug"
 	"strconv"
@@ -33,6 +34,8 @@ var (
 		"warn":  slog.LevelWarn,
 		"error": slog.LevelError,
 	}
+
+	gitExecutablePath = exec.Command("git").String()
 )
 
 func init() {
@@ -133,10 +136,7 @@ func main() {
 
 	applyGitDefaults(conf)
 
-	// path to resolve git
-	gitENV := []string{fmt.Sprintf("PATH=%s", os.Getenv("PATH"))}
-
-	repoPool, err := repopool.New(ctx, *conf, logger.With("logger", "git-mirror"), gitENV)
+	repoPool, err := repopool.New(ctx, *conf, logger.With("logger", "git-mirror"), gitExecutablePath, nil)
 	if err != nil {
 		logger.Error("could not create git mirror pool", "err", err)
 		os.Exit(1)

repopool/example_noworktree_test.go

@@ -41,7 +41,7 @@ repositories:
 	}
 	conf.Defaults.Root = tmpRoot
 
-	repos, err := repopool.New(ctx, conf, slog.Default(), nil)
+	repos, err := repopool.New(ctx, conf, slog.Default(), "", nil)
 	if err != nil {
 		panic(err)
 	}

repopool/example_worktree_test.go

@@ -44,7 +44,7 @@ repositories:
 
 	conf.Defaults.Root = tmpRoot
 
-	repos, err := repopool.New(ctx, conf, slog.Default(), nil)
+	repos, err := repopool.New(ctx, conf, slog.Default(), "", nil)
 	if err != nil {
 		panic(err)
 	}
@@ -71,7 +71,7 @@ repositories:
 	fmt.Println("last commit msg at main", "msg", msg)
 
 	// make sure file exists in the tree
-	_, err = os.Stat(tmpRoot + "/main/pkg/mirror/repository.go")
+	_, err = os.Stat(tmpRoot + "/main/repository/repository.go")
 	if err != nil {
 		panic(err)
 	}