diff --git a/README.md b/README.md index e3bcef7..66048b0 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-82.8%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-85.3%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -268,6 +268,14 @@ self_approval_via_teams = false # Optional reviewers are still invited with a CC comment. disable_review_status_comments = false +# `fetch_orphaned_approval` (default false) recovers an approval whose commit no +# longer resolves locally, which is what a rebase or a force-push leaves behind. +# Without it that approval is dismissed with no ownership check at all, because the +# diff it would be judged on cannot be computed. Off by default because it reaches +# the network: one fetch per unresolvable commit, bounded at 60s plus a short grace +# for the transport helper to exit. +fetch_orphaned_approval = false + # `enforcement` allows you to specify how the Codeowners Plus check should be enforced [enforcement] # see "Enforcement Options" below for more details diff --git a/internal/app/app.go b/internal/app/app.go index f7d364d..1d84470 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -132,7 +132,11 @@ func (a *App) Run() (*OutputData, error) { // Get the diff of the PR a.printDebug("Getting diff for %s...%s\n", diffContext.Base, diffContext.Head) - gitDiff, err := git.NewDiff(diffContext) + var diffOpts []git.DiffOption + if conf.FetchOrphanedApproval { + diffOpts = append(diffOpts, git.WithFetchOrphanedRefs()) + } + gitDiff, err := git.NewDiff(diffContext, diffOpts...) if err != nil { return &OutputData{}, fmt.Errorf("NewGitDiff Error: %v", err) } diff --git a/internal/app/orphaned_approval_test.go b/internal/app/orphaned_approval_test.go new file mode 100644 index 0000000..675f191 --- /dev/null +++ b/internal/app/orphaned_approval_test.go @@ -0,0 +1,214 @@ +package app + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/google/go-github/v89/github" + "github.com/multimediallc/codeowners-plus/internal/git" + gh "github.com/multimediallc/codeowners-plus/internal/github" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" +) + +type realCheckApprovalsClient struct { + *mockGitHubClient + real gh.Client + dismissed []*gh.CurrentApproval +} + +func (c *realCheckApprovalsClient) CheckApprovals( + fileReviewerMap map[string][]string, + approvals []*gh.CurrentApproval, + originalDiff git.Diff, +) ([]codeowners.Slug, []*gh.CurrentApproval) { + return c.real.CheckApprovals(fileReviewerMap, approvals, originalDiff) +} + +func (c *realCheckApprovalsClient) DismissStaleReviews(approvals []*gh.CurrentApproval) error { + c.dismissed = append(c.dismissed, approvals...) + return c.mockGitHubClient.DismissStaleReviews(approvals) +} + +func runGit(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s (in %s): %v\n%s", strings.Join(args, " "), dir, err, out) + } + return strings.TrimSpace(string(out)) +} + +func initRepo(t *testing.T, dir string) { + t.Helper() + runGit(t, dir, "init", "-q", "-b", "main") + runGit(t, dir, "config", "user.email", "test@example.invalid") + runGit(t, dir, "config", "user.name", "Test User") + runGit(t, dir, "config", "commit.gpgsign", "false") +} + +func writeRepoFile(t *testing.T, dir, name, content string) { + t.Helper() + path := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir for %s: %v", name, err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } +} + +func commitAll(t *testing.T, dir, message string) string { + t.Helper() + runGit(t, dir, "add", "-A") + runGit(t, dir, "commit", "-q", "-m", message) + return runGit(t, dir, "rev-parse", "HEAD") +} + +func runApp(t *testing.T, repoDir, baseSHA, headSHA, approvalSHA string) (*OutputData, []*gh.CurrentApproval, string) { + t.Helper() + + warnings := &bytes.Buffer{} + info := &bytes.Buffer{} + + realClient, err := gh.NewClient("test-owner", "test-repo", "test-token", "") + if err != nil { + t.Fatalf("failed to build the real client: %v", err) + } + realClient.SetWarningBuffer(warnings) + realClient.SetInfoBuffer(info) + + client := &realCheckApprovalsClient{ + mockGitHubClient: &mockGitHubClient{ + pr: &github.PullRequest{ + Number: github.Ptr(1), + Base: &github.PullRequestBranch{SHA: github.Ptr(baseSHA)}, + Head: &github.PullRequestBranch{SHA: github.Ptr(headSHA)}, + User: &github.User{Login: github.Ptr("author")}, + }, + currentApprovals: []*gh.CurrentApproval{{ + GHLogin: codeowners.NewSlug("@reviewer"), + ReviewID: 1, + Reviewers: []codeowners.Slug{codeowners.NewSlug("@owner")}, + CommitID: approvalSHA, + }}, + }, + real: realClient, + } + + app := &App{ + config: &Config{ + RepoDir: repoDir, + PR: 1, + Quiet: true, + InfoBuffer: info, + WarningBuffer: warnings, + }, + client: client, + } + + output, err := app.Run() + if err != nil { + t.Fatalf("app.Run failed: %v\nwarnings: %s", err, warnings) + } + return output, client.dismissed, warnings.String() +} + +const orphanBaseSource = `package service + +func Gamma() int { + return 3 +} +` + +const orphanApprovedSource = `package service + +func Gamma() int { + return 30 +} +` + +func buildOrphanedApprovalRepo(t *testing.T, configBody string) (repoDir, baseSHA, headSHA, approvalSHA string) { + t.Helper() + repoDir = t.TempDir() + initRepo(t, repoDir) + + writeRepoFile(t, repoDir, ".codeowners", "service.go @owner\n") + writeRepoFile(t, repoDir, "codeowners.toml", configBody) + writeRepoFile(t, repoDir, "service.go", orphanBaseSource) + writeRepoFile(t, repoDir, "notes.md", "first note\n") + baseSHA = commitAll(t, repoDir, "base") + + originDir := t.TempDir() + runGit(t, repoDir, "clone", "-q", repoDir, originDir) + initRepo(t, originDir) + runGit(t, originDir, "config", "uploadpack.allowAnySHA1InWant", "true") + writeRepoFile(t, originDir, "service.go", orphanApprovedSource) + approvalSHA = commitAll(t, originDir, "approved change") + + writeRepoFile(t, repoDir, "service.go", orphanApprovedSource) + writeRepoFile(t, repoDir, "notes.md", "first note\nsecond note\n") + headSHA = commitAll(t, repoDir, "approved change plus an unowned edit") + runGit(t, repoDir, "remote", "add", "origin", originDir) + + return repoDir, baseSHA, headSHA, approvalSHA +} + +func TestRunFetchesOrphanedApproval(t *testing.T) { + const fetchOff = `disable_review_status_comments = true +suppress_unowned_warning = true +` + const fetchOn = `disable_review_status_comments = true +suppress_unowned_warning = true +fetch_orphaned_approval = true +` + + tt := []struct { + name string + config string + expectDismissed bool + }{ + {name: "fetch disabled, approval dismissed", config: fetchOff, expectDismissed: true}, + {name: "fetch enabled, approval recovered", config: fetchOn, expectDismissed: false}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + repoDir, baseSHA, headSHA, approvalSHA := buildOrphanedApprovalRepo(t, tc.config) + + cmd := exec.Command("git", "cat-file", "-e", approvalSHA) + cmd.Dir = repoDir + if err := cmd.Run(); err == nil { + t.Fatalf("approval commit %s should not be present locally", approvalSHA) + } + + output, dismissed, warnings := runApp(t, repoDir, baseSHA, headSHA, approvalSHA) + + if tc.expectDismissed { + if len(dismissed) != 1 { + t.Errorf("expected the approval to be dismissed, got %d dismissals", len(dismissed)) + } + if !strings.Contains(warnings, "Error getting changes since") { + t.Errorf("expected a warning about the unresolvable ref, got %q", warnings) + } + if output.Success { + t.Error("expected the run to fail without the approval") + } + return + } + + if len(dismissed) != 0 { + t.Errorf("expected the approval to survive, got %d dismissals: %s", len(dismissed), warnings) + } + if !output.Success { + t.Errorf("expected the run to succeed, got %q (warnings: %s)", output.Message, warnings) + } + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..a0f0962 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -22,6 +22,7 @@ type Config struct { AllowSelfApproval bool `toml:"allow_self_approval"` SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` DisableReviewStatusComments bool `toml:"disable_review_status_comments"` + FetchOrphanedApproval bool `toml:"fetch_orphaned_approval"` } type Enforcement struct { diff --git a/internal/git/diff.go b/internal/git/diff.go index 95c74c0..6771239 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -3,21 +3,33 @@ package git import ( "bufio" "bytes" + "context" "crypto/sha256" + "errors" "fmt" "os/exec" "slices" "strings" + "time" "github.com/multimediallc/codeowners-plus/pkg/codeowners" "github.com/sourcegraph/go-diff/diff" ) +const fetchTimeout = 60 * time.Second + +// git spawns a transport helper which inherits the pipe and outlives the kill. +const transportHelperGrace = 2 * time.Second + // gitCommandExecutor defines the interface for executing git commands type gitCommandExecutor interface { execute(command string, args ...string) ([]byte, error) } +type timeoutExecutor interface { + executeWithTimeout(timeout time.Duration, command string, args ...string) ([]byte, error) +} + // realGitExecutor implements GitCommandExecutor using os/exec type realGitExecutor struct { dir string @@ -33,6 +45,19 @@ func (e *realGitExecutor) execute(command string, args ...string) ([]byte, error return cmd.CombinedOutput() } +func (e *realGitExecutor) executeWithTimeout(timeout time.Duration, command string, args ...string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + cmd := exec.CommandContext(ctx, command, args...) + cmd.Dir = e.dir + cmd.WaitDelay = transportHelperGrace + output, err := cmd.CombinedOutput() + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return output, fmt.Errorf("%s timed out after %s", command, timeout) + } + return output, err +} + type Diff interface { AllChanges() []codeowners.DiffFile ChangesSince(ref string) ([]codeowners.DiffFile, error) @@ -40,18 +65,29 @@ type Diff interface { } type GitDiff struct { - context DiffContext - diff []*diff.FileDiff - files []codeowners.DiffFile - executor gitCommandExecutor + context DiffContext + diff []*diff.FileDiff + files []codeowners.DiffFile + executor gitCommandExecutor + fetchOrphanedRefs bool } -func NewDiff(context DiffContext) (Diff, error) { +// DiffOption configures optional GitDiff behavior. +type DiffOption func(*GitDiff) + +// WithFetchOrphanedRefs makes ChangesSince fetch a ref git cannot resolve locally and retry once. +func WithFetchOrphanedRefs() DiffOption { + return func(gd *GitDiff) { + gd.fetchOrphanedRefs = true + } +} + +func NewDiff(context DiffContext, opts ...DiffOption) (Diff, error) { executor := newRealGitExecutor(context.Dir) - return NewDiffWithExecutor(context, executor) + return NewDiffWithExecutor(context, executor, opts...) } -func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff, error) { +func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor, opts ...DiffOption) (Diff, error) { gitDiff, err := getGitDiff(context, executor) if err != nil { return nil, err @@ -61,12 +97,16 @@ func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff return nil, err } - return &GitDiff{ + gd := &GitDiff{ context: context, diff: gitDiff, files: diffFiles, executor: executor, - }, nil + } + for _, opt := range opts { + opt(gd) + } + return gd, nil } func (gd *GitDiff) AllChanges() []codeowners.DiffFile { @@ -81,6 +121,15 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { IgnoreDirs: gd.context.IgnoreDirs, } olderDiff, err := getGitDiff(olderDiffContext, gd.executor) + if err != nil && gd.fetchOrphanedRefs && looksLikeObjectName(ref) && !gd.refResolvesLocally(ref) { + if fetchErr := gd.fetchRef(ref); fetchErr != nil { + err = fmt.Errorf("%w (fetching orphaned ref failed: %v)", err, fetchErr) + } else if retryDiff, retryErr := getGitDiff(olderDiffContext, gd.executor); retryErr != nil { + err = fmt.Errorf("%w (retry after fetching orphaned ref failed: %v)", err, retryErr) + } else { + olderDiff, err = retryDiff, nil + } + } if err != nil { return nil, fmt.Errorf("failed to get older diff: %w", err) } @@ -95,6 +144,41 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { return diffFiles, nil } +func looksLikeObjectName(ref string) bool { + if len(ref) < 7 || len(ref) > 64 { + return false + } + for i := 0; i < len(ref); i++ { + if c := ref[i]; (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + +// A diff can fail for reasons unrelated to the ref, and fetching then cannot help. +func (gd *GitDiff) refResolvesLocally(ref string) bool { + _, err := gd.executor.execute("git", "cat-file", "-e", ref+"^{commit}") + return err == nil +} + +// `git fetch` accepts --upload-pack, which names a command to run, so the ref goes after a `--`. +func (gd *GitDiff) fetchRef(ref string) error { + args := []string{"fetch", "--no-tags", "origin", "--", ref} + run := gd.executor.execute + if executor, ok := gd.executor.(timeoutExecutor); ok { + run = func(command string, args ...string) ([]byte, error) { + return executor.executeWithTimeout(fetchTimeout, command, args...) + } + } + // git puts the reason on stderr and the exit status alone says nothing useful, + // which is why getGitDiff wraps its output too. + if output, err := run("git", args...); err != nil { + return fmt.Errorf("%s\n%s", err, output) + } + return nil +} + func (gd *GitDiff) Context() DiffContext { return gd.context } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 59a2244..eb3e913 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -5,7 +5,11 @@ import ( "fmt" "io" "os" + "os/exec" + "slices" + "strings" "testing" + "time" "github.com/multimediallc/codeowners-plus/pkg/codeowners" "github.com/sourcegraph/go-diff/diff" @@ -31,6 +35,57 @@ func (e *mockGitExecutor) execute(command string, args ...string) ([]byte, error return []byte(e.output), nil } +type mockResult struct { + output string + err error +} + +type scriptedGitExecutor struct { + diffResults []mockResult + fetchResult mockResult + refResolves bool + calls [][]string +} + +func (e *scriptedGitExecutor) execute(command string, args ...string) ([]byte, error) { + e.calls = append(e.calls, append([]string{command}, args...)) + res := e.fetchResult + if len(args) > 0 && args[0] == "cat-file" { + if e.refResolves { + return nil, nil + } + return nil, errors.New("not a valid object name") + } + if len(args) > 0 && args[0] == "diff" { + if len(e.diffResults) == 0 { + return nil, errors.New("unexpected git diff call") + } + res, e.diffResults = e.diffResults[0], e.diffResults[1:] + } + // CombinedOutput returns both, and the output is where git puts the reason. + return []byte(res.output), res.err +} + +type timeoutScriptedExecutor struct { + scriptedGitExecutor + timeouts []time.Duration +} + +func (e *timeoutScriptedExecutor) executeWithTimeout(timeout time.Duration, command string, args ...string) ([]byte, error) { + e.timeouts = append(e.timeouts, timeout) + return e.execute(command, args...) +} + +func (e *scriptedGitExecutor) fetchCalls() [][]string { + fetches := make([][]string, 0, len(e.calls)) + for _, call := range e.calls { + if len(call) > 1 && call[1] == "fetch" { + fetches = append(fetches, call) + } + } + return fetches +} + func readFile(path string) ([]byte, error) { file, err := os.Open(path) if err != nil { @@ -129,7 +184,7 @@ Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`, expectedErr: false, expectedFiles: 2, expectedHunks: map[string]int{ - "file1.go": 1, + "file1.go": 1, "assets/img/offline.png": 0, }, }, @@ -354,6 +409,226 @@ index abc..def 100644 } } +func TestFetchRefSurfacesGitOutput(t *testing.T) { + executor := &scriptedGitExecutor{ + diffResults: []mockResult{{output: sampleGitDiff}, {err: errors.New("fatal: bad object deadbeef")}}, + fetchResult: mockResult{output: "fatal: remote error: upload-pack not permitted", err: errors.New("exit status 128")}, + } + diff, err := NewDiffWithExecutor(DiffContext{Base: "main", Head: "feature", Dir: "."}, executor, WithFetchOrphanedRefs()) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + + _, err = diff.ChangesSince("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef") + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "upload-pack not permitted") { + t.Errorf("expected git's own reason to reach the caller, got %v", err) + } +} + +func TestChangesSinceFetchOrphanedRef(t *testing.T) { + const olderDiff = `diff --git a/file1.go b/file1.go +index abc..def 100644 +--- a/file1.go ++++ b/file1.go +@@ -5,0 +6 @@ func Example() { ++ fmt.Println("Old change")` + + diffFailure := errors.New("fatal: bad object deadbeef") + retryFailure := errors.New("fatal: ambiguous argument 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef'") + fetchFailure := errors.New("fatal: could not read from remote") + + tt := []struct { + name string + fetchOrphanedRefs bool + refResolves bool + ref string + olderDiffResults []mockResult + fetchResult mockResult + expectedErr error // the error the caller must see as the cause + alsoInErr error // secondary error which must stay visible + expectedFetchCalls int + expectedFiles int + }{ + { + name: "older diff succeeds, no fetch attempted", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{output: olderDiff}}, + expectedFetchCalls: 0, + expectedFiles: 2, + }, + { + name: "fetch recovers the orphaned ref", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{err: diffFailure}, {output: olderDiff}}, + expectedFetchCalls: 1, + expectedFiles: 2, + }, + { + name: "fetch fails", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{err: diffFailure}}, + fetchResult: mockResult{err: fetchFailure}, + expectedErr: diffFailure, + alsoInErr: fetchFailure, + expectedFetchCalls: 1, + }, + { + name: "retry after fetch still fails", + fetchOrphanedRefs: true, + olderDiffResults: []mockResult{{err: diffFailure}, {err: retryFailure}}, + expectedErr: diffFailure, + alsoInErr: retryFailure, + expectedFetchCalls: 1, + }, + { + name: "disabled, no fetch attempted", + fetchOrphanedRefs: false, + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + { + name: "ref is not an object name, so nothing is fetched", + fetchOrphanedRefs: true, + ref: "refs/heads/main:refs/heads/injected", + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + { + name: "empty ref is never fetched", + fetchOrphanedRefs: true, + ref: "", + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + { + name: "ref resolves locally, so the diff failed for another reason", + fetchOrphanedRefs: true, + refResolves: true, + olderDiffResults: []mockResult{{err: diffFailure}}, + expectedErr: diffFailure, + expectedFetchCalls: 0, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + executor := &scriptedGitExecutor{ + diffResults: append([]mockResult{{output: sampleGitDiff}}, tc.olderDiffResults...), + fetchResult: tc.fetchResult, + refResolves: tc.refResolves, + } + + var opts []DiffOption + if tc.fetchOrphanedRefs { + opts = append(opts, WithFetchOrphanedRefs()) + } + context := DiffContext{Base: "main", Head: "feature", Dir: "."} + diff, err := NewDiffWithExecutor(context, executor, opts...) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + + ref := tc.ref + if ref == "" && tc.name != "empty ref is never fetched" { + ref = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + } + changes, err := diff.ChangesSince(ref) + + fetches := executor.fetchCalls() + if len(fetches) != tc.expectedFetchCalls { + t.Errorf("expected %d fetch calls, got %d", tc.expectedFetchCalls, len(fetches)) + } + for _, fetch := range fetches { + want := []string{"git", "fetch", "--no-tags", "origin", "--", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"} + if !slices.Equal(fetch, want) { + t.Errorf("expected fetch command %v, got %v", want, fetch) + } + } + + if tc.expectedErr != nil { + if err == nil { + t.Fatal("expected error but got none") + } + wantPrefix := "failed to get older diff: diff Error: " + tc.expectedErr.Error() + if !strings.HasPrefix(err.Error(), wantPrefix) { + t.Errorf("expected error to start with %q, got %v", wantPrefix, err) + } + if tc.alsoInErr != nil && !strings.Contains(err.Error(), tc.alsoInErr.Error()) { + t.Errorf("expected %q to stay visible, got %v", tc.alsoInErr, err) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(changes) != tc.expectedFiles { + t.Errorf("expected %d files, got %d", tc.expectedFiles, len(changes)) + } + }) + } +} + +func TestChangesSinceFetchIsBounded(t *testing.T) { + const olderDiff = `diff --git a/file1.go b/file1.go +index abc..def 100644 +--- a/file1.go ++++ b/file1.go +@@ -5,0 +6 @@ func Example() { ++ fmt.Println("Old change")` + + executor := &timeoutScriptedExecutor{ + scriptedGitExecutor: scriptedGitExecutor{ + diffResults: []mockResult{ + {output: sampleGitDiff}, + {err: errors.New("fatal: bad object deadbeef")}, + {output: olderDiff}, + }, + }, + } + + context := DiffContext{Base: "main", Head: "feature", Dir: "."} + diff, err := NewDiffWithExecutor(context, executor, WithFetchOrphanedRefs()) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + if _, err := diff.ChangesSince("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(executor.timeouts) != 1 { + t.Fatalf("expected 1 bounded command, got %d", len(executor.timeouts)) + } + if executor.timeouts[0] != fetchTimeout { + t.Errorf("expected the fetch to be bounded by %s, got %s", fetchTimeout, executor.timeouts[0]) + } +} + +func TestExecuteWithTimeout(t *testing.T) { + if _, err := exec.LookPath("sleep"); err != nil { + t.Skip("sleep not available") + } + executor := newRealGitExecutor(".") + + if _, err := executor.executeWithTimeout(time.Minute, "sleep", "0"); err != nil { + t.Errorf("unexpected error for a command within the timeout: %v", err) + } + + _, err := executor.executeWithTimeout(10*time.Millisecond, "sleep", "30") + if err == nil { + t.Fatal("expected an error when the command outlives the timeout") + } + if !strings.Contains(err.Error(), "timed out") { + t.Errorf("expected a timeout error, got %v", err) + } +} + func TestHunkHash(t *testing.T) { tt := []struct { name string