diff --git a/README.md b/README.md index f639c279..f7b8a384 100644 --- a/README.md +++ b/README.md @@ -180,6 +180,28 @@ ocr review --from main --to feature-branch ocr review --commit abc123 ``` +#### Reviewing a fork against upstream + +When you work on a fork and want to review what your current branch changed +relative to the original repository, use `--upstream`: + +```bash +# Against a configured remote (auto-fetches its default branch): +ocr review --upstream upstream + +# Against the original repo by URL, without configuring a remote: +ocr review --upstream https://github.com/orig/repo + +# Pick a specific upstream branch; default is HEAD vs upstream's default branch: +ocr review --upstream upstream --upstream-branch main + +# Offline: use the already-fetched remote-tracking branch, no network: +ocr review --upstream upstream --no-fetch +``` + +`--upstream` reviews `merge-base(upstream, HEAD)..HEAD` — i.e. exactly the +commits your branch adds on top of upstream. Override the target with `--to`. + ### Integrate with Coding Agents OCR can be seamlessly integrated into AI coding agents as a slash command, enabling code review directly within your agent workflow. @@ -307,6 +329,9 @@ See the [`examples/`](./examples/) directory for integration examples: | `--from` | — | — | Source ref (e.g., `main`) | | `--to` | — | — | Target ref (e.g., `feature-branch`) | | `--commit` | `-c` | — | Single commit to review | +| `--upstream` | — | — | Upstream remote name or git URL to review the current branch against (fork workflow); reviews `merge-base(upstream, HEAD)..HEAD` | +| `--upstream-branch` | — | upstream default | Branch on `--upstream` to compare against | +| `--no-fetch` | — | `false` | With `--upstream`: skip network fetch, use the local remote-tracking ref | | `--preview` | `-p` | `false` | Preview which files will be reviewed without running the LLM | | `--format` | `-f` | `text` | Output format: `text` or `json` | | `--concurrency` | — | `8` | Max concurrent file reviews | diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 3e13bb58..7d8b1330 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -101,6 +101,9 @@ type reviewOptions struct { from string to string commit string + upstream string // --upstream: remote name or git URL to review against (fork workflow) + upstreamBranch string // --upstream-branch: branch on --upstream (default: its default branch) + noFetch bool // --no-fetch: with --upstream, skip network fetch and use local ref outputFormat string audience string // --audience: "human" (default) or "agent" background string // --background: optional requirement context @@ -131,6 +134,9 @@ func parseReviewFlags(args []string) (reviewOptions, error) { a.IntVar(&opts.maxTools, "max-tools", 0, "max tool call rounds per file (0 = template default; min 10)") a.IntVar(&opts.maxGitProcs, "max-git-procs", 16, "max concurrent git subprocesses") a.BoolVarP(&opts.preview, "preview", "p", false, "preview which files will be reviewed without running the LLM") + a.StringVar(&opts.upstream, "upstream", "", "upstream remote name or git URL to review the current branch against (fork workflow)") + a.StringVar(&opts.upstreamBranch, "upstream-branch", "", "branch on --upstream to compare against (default: upstream's default branch)") + a.BoolVar(&opts.noFetch, "no-fetch", false, "with --upstream: skip network fetch and use the local remote-tracking ref") if err := a.Parse(args); err != nil { return opts, fmt.Errorf("parse flags: %w", err) @@ -152,6 +158,18 @@ func parseReviewFlags(args []string) (reviewOptions, error) { if modeCount > 1 { return opts, fmt.Errorf("only one review mode allowed (--from/--to or --commit)") } + if opts.upstream != "" { + if opts.from != "" || opts.commit != "" { + return opts, fmt.Errorf("--upstream cannot be combined with --from or --commit") + } + } else { + if opts.upstreamBranch != "" { + return opts, fmt.Errorf("--upstream-branch requires --upstream") + } + if opts.noFetch { + return opts, fmt.Errorf("--no-fetch requires --upstream") + } + } if opts.from != "" && opts.to == "" { return opts, fmt.Errorf("--to is required when --from is specified") } @@ -196,6 +214,10 @@ Examples: ocr review --commit abc123 ocr review -c abc123 + # Review current branch against an upstream repo (fork workflow) + ocr review --upstream upstream + ocr review --upstream https://github.com/orig/repo --upstream-branch main + # Output JSON format ocr review --format json ocr review -f json @@ -221,7 +243,10 @@ Flags: --rule string path to JSON file with system review rules --timeout int concurrent task timeout in minutes (default 10) --to string target ref to end diff at (e.g., 'feature-branch') - --tools string path to JSON tools config file (default: embedded)`) + --tools string path to JSON tools config file (default: embedded) + --upstream string upstream remote name or git URL to review the current branch against + --upstream-branch string branch on --upstream to compare against (default: its default branch) + --no-fetch with --upstream: skip network fetch, use local remote-tracking ref`) } // --- config subcommand --- diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go new file mode 100644 index 00000000..f5d0b06c --- /dev/null +++ b/cmd/opencodereview/flags_test.go @@ -0,0 +1,44 @@ +package main + +import ( + "strings" + "testing" +) + +func TestParseReviewFlagsUpstreamConflictsWithFrom(t *testing.T) { + _, err := parseReviewFlags([]string{"--upstream", "upstream", "--from", "main"}) + if err == nil || !strings.Contains(err.Error(), "--upstream cannot be combined") { + t.Fatalf("expected conflict error, got: %v", err) + } +} + +func TestParseReviewFlagsUpstreamConflictsWithCommit(t *testing.T) { + _, err := parseReviewFlags([]string{"--upstream", "upstream", "--commit", "abc123"}) + if err == nil || !strings.Contains(err.Error(), "--upstream cannot be combined") { + t.Fatalf("expected conflict error, got: %v", err) + } +} + +func TestParseReviewFlagsUpstreamBranchRequiresUpstream(t *testing.T) { + _, err := parseReviewFlags([]string{"--upstream-branch", "develop"}) + if err == nil || !strings.Contains(err.Error(), "--upstream-branch requires --upstream") { + t.Fatalf("expected requires-upstream error, got: %v", err) + } +} + +func TestParseReviewFlagsNoFetchRequiresUpstream(t *testing.T) { + _, err := parseReviewFlags([]string{"--no-fetch"}) + if err == nil || !strings.Contains(err.Error(), "--no-fetch requires --upstream") { + t.Fatalf("expected requires-upstream error, got: %v", err) + } +} + +func TestParseReviewFlagsUpstreamOK(t *testing.T) { + opts, err := parseReviewFlags([]string{"--upstream", "upstream", "--to", "feature"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.upstream != "upstream" || opts.to != "feature" { + t.Fatalf("unexpected opts: %+v", opts) + } +} diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index bf9f3c5a..a94dd784 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -49,6 +49,14 @@ func runReview(args []string) error { if err != nil { return fmt.Errorf("resolve repo: %w", err) } + if opts.upstream != "" { + from, to, uerr := resolveUpstream(repoDir, opts) + if uerr != nil { + return uerr + } + opts.from = from + opts.to = to + } if err := validateReviewRefs(repoDir, opts); err != nil { return err } diff --git a/cmd/opencodereview/upstream.go b/cmd/opencodereview/upstream.go new file mode 100644 index 00000000..adae69f5 --- /dev/null +++ b/cmd/opencodereview/upstream.go @@ -0,0 +1,140 @@ +package main + +import ( + "fmt" + "os" + "strings" +) + +// isUpstreamURL reports whether spec looks like a git URL or local path rather +// than the name of a configured remote. +func isUpstreamURL(spec string) bool { + if strings.Contains(spec, "://") { + return true + } + if strings.HasPrefix(spec, "/") || strings.HasPrefix(spec, "./") || + strings.HasPrefix(spec, "../") || strings.HasPrefix(spec, "~") { + return true + } + // scp-like syntax: user@host:path (a ':' that precedes any '/'). + if i := strings.Index(spec, ":"); i > 0 { + slash := strings.Index(spec, "/") + if (slash == -1 || i < slash) && strings.Contains(spec[:i], "@") { + return true + } + } + return false +} + +// upstreamRemoteExists reports whether name is a configured git remote in repoDir. +func upstreamRemoteExists(repoDir, name string) bool { + out, err := runGitCmd(repoDir, "remote") + if err != nil { + return false + } + for _, line := range strings.Split(string(out), "\n") { + if strings.TrimSpace(line) == name { + return true + } + } + return false +} + +// parseSymrefDefaultBranch extracts the default branch name from the output of +// `git ls-remote --symref HEAD`, e.g. a line "ref: refs/heads/main\tHEAD". +func parseSymrefDefaultBranch(lsRemoteOut string) (string, error) { + for _, line := range strings.Split(lsRemoteOut, "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "ref:") { + continue + } + fields := strings.Fields(line) // ["ref:", "refs/heads/main", "HEAD"] + if len(fields) >= 2 && strings.HasPrefix(fields[1], "refs/heads/") { + return strings.TrimPrefix(fields[1], "refs/heads/"), nil + } + } + return "", fmt.Errorf("no default branch (HEAD symref) found in ls-remote output") +} + +// discoverDefaultBranch queries target (a remote name or URL) for its default +// branch via `git ls-remote --symref`. +func discoverDefaultBranch(repoDir, target string) (string, error) { + out, err := runGitCmd(repoDir, "ls-remote", "--symref", "--end-of-options", target, "HEAD") + if err != nil { + return "", fmt.Errorf("git ls-remote %q failed: %s", target, strings.TrimSpace(string(out))) + } + branch, perr := parseSymrefDefaultBranch(string(out)) + if perr != nil { + return "", fmt.Errorf("determine default branch for %q: %w", target, perr) + } + return branch, nil +} + +// defaultTo returns to, or "HEAD" when to is empty. +func defaultTo(to string) string { + if to == "" { + return "HEAD" + } + return to +} + +// resolveUpstream turns an --upstream spec (remote name or URL) into a concrete +// range to review: it fetches the upstream branch as needed and returns a usable +// `from` ref plus the `to` ref (defaulting to HEAD). The result feeds the normal +// ModeRange path: merge-base(from,to)..to. +func resolveUpstream(repoDir string, opts reviewOptions) (from, to string, err error) { + target := opts.upstream + isURL := isUpstreamURL(target) + + if !isURL && !upstreamRemoteExists(repoDir, target) { + return "", "", fmt.Errorf("--upstream %q: no git remote named %q; pass a git URL or run \"git remote add %s \"", target, target, target) + } + if isURL && opts.noFetch { + return "", "", fmt.Errorf("--no-fetch cannot be used with a URL upstream (%q); a URL must be fetched", target) + } + + // Determine the upstream branch. + branch := opts.upstreamBranch + if branch == "" { + if opts.noFetch { + // Local-only discovery via the remote's HEAD symref, e.g. + // "refs/remotes/origin/HEAD" -> "refs/remotes/origin/main". + out, e := runGitCmd(repoDir, "symbolic-ref", "--end-of-options", "refs/remotes/"+target+"/HEAD") + ref := strings.TrimSpace(string(out)) + prefix := "refs/remotes/" + target + "/" + if e != nil || !strings.HasPrefix(ref, prefix) { + return "", "", fmt.Errorf("--no-fetch: cannot determine default branch for %q locally; pass --upstream-branch", target) + } + branch = strings.TrimPrefix(ref, prefix) + } else { + branch, err = discoverDefaultBranch(repoDir, target) + if err != nil { + return "", "", err + } + } + } + + // Fetch unless asked not to. + if !opts.noFetch { + fmt.Fprintf(os.Stderr, "[ocr] fetching %s from %s...\n", branch, target) + if out, e := runGitCmd(repoDir, "fetch", "--end-of-options", target, branch); e != nil { + // For a configured remote, fall back to a local tracking ref if present. + if !isURL { + if _, verr := runGitCmd(repoDir, "rev-parse", "--verify", "--end-of-options", target+"/"+branch+"^{commit}"); verr == nil { + fmt.Fprintf(os.Stderr, "[ocr] fetch failed (%s); using local %s/%s\n", strings.TrimSpace(string(out)), target, branch) + return target + "/" + branch, defaultTo(opts.to), nil + } + } + return "", "", fmt.Errorf("git fetch %s %s failed: %s", target, branch, strings.TrimSpace(string(out))) + } + } + + if isURL { + out, e := runGitCmd(repoDir, "rev-parse", "--verify", "--end-of-options", "FETCH_HEAD^{commit}") + if e != nil { + return "", "", fmt.Errorf("resolve FETCH_HEAD after fetching %q: %s", target, strings.TrimSpace(string(out))) + } + return strings.TrimSpace(string(out)), defaultTo(opts.to), nil + } + return target + "/" + branch, defaultTo(opts.to), nil +} diff --git a/cmd/opencodereview/upstream_test.go b/cmd/opencodereview/upstream_test.go new file mode 100644 index 00000000..1ec62239 --- /dev/null +++ b/cmd/opencodereview/upstream_test.go @@ -0,0 +1,194 @@ +package main + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/open-code-review/open-code-review/internal/diff" + "github.com/open-code-review/open-code-review/internal/gitcmd" +) + +func TestIsUpstreamURL(t *testing.T) { + cases := []struct { + in string + want bool + }{ + {"https://github.com/o/r", true}, + {"git://example.com/r.git", true}, + {"ssh://git@example.com/r.git", true}, + {"file:///tmp/bare.git", true}, + {"git@github.com:o/r.git", true}, + {"/abs/path/bare.git", true}, + {"./rel/bare.git", true}, + {"../rel/bare.git", true}, + {"upstream", false}, + {"origin", false}, + {"my-remote", false}, + } + for _, c := range cases { + if got := isUpstreamURL(c.in); got != c.want { + t.Errorf("isUpstreamURL(%q) = %v, want %v", c.in, got, c.want) + } + } +} + +func TestParseSymrefDefaultBranch(t *testing.T) { + out := "ref: refs/heads/main\tHEAD\n" + + "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0\tHEAD\n" + branch, err := parseSymrefDefaultBranch(out) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if branch != "main" { + t.Fatalf("got %q, want main", branch) + } +} + +func TestParseSymrefDefaultBranchMaster(t *testing.T) { + out := "ref: refs/heads/master\tHEAD\n0000\tHEAD\n" + branch, err := parseSymrefDefaultBranch(out) + if err != nil || branch != "master" { + t.Fatalf("got %q, err %v; want master", branch, err) + } +} + +func TestParseSymrefDefaultBranchMissing(t *testing.T) { + if _, err := parseSymrefDefaultBranch("0000\tHEAD\n"); err == nil { + t.Fatal("expected error when no symref line present") + } +} + +// initUpstreamAndFork builds: a bare "upstream" repo (branch main, one commit), +// and a "fork" clone of it with origin -> bare and a local "feature" branch that +// modifies app.go. Returns the bare repo path and the fork working dir. +func initUpstreamAndFork(t *testing.T) (upstreamBare, fork string) { + t.Helper() + root := t.TempDir() + upstreamWork := filepath.Join(root, "upstream-work") + upstreamBare = filepath.Join(root, "upstream.git") + fork = filepath.Join(root, "fork") + + mustGit := func(dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v in %s failed: %v\n%s", args, dir, err, out) + } + } + + if err := os.MkdirAll(upstreamWork, 0o755); err != nil { + t.Fatal(err) + } + mustGit(upstreamWork, "init", "-q", "-b", "main") + mustGit(upstreamWork, "config", "user.email", "up@example.com") + mustGit(upstreamWork, "config", "user.name", "Up") + mustGit(upstreamWork, "config", "commit.gpgsign", "false") + if err := os.WriteFile(filepath.Join(upstreamWork, "app.go"), + []byte("package app\n\nfunc A() int { return 1 }\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(upstreamWork, "add", ".") + mustGit(upstreamWork, "commit", "-q", "-m", "upstream initial") + + mustGit(root, "clone", "-q", "--bare", upstreamWork, upstreamBare) + + mustGit(root, "clone", "-q", upstreamBare, fork) + mustGit(fork, "config", "user.email", "me@example.com") + mustGit(fork, "config", "user.name", "Me") + mustGit(fork, "config", "commit.gpgsign", "false") + mustGit(fork, "checkout", "-q", "-b", "feature") + if err := os.WriteFile(filepath.Join(fork, "app.go"), + []byte("package app\n\nfunc A() int { return 2 }\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(fork, "add", ".") + mustGit(fork, "commit", "-q", "-m", "my change") + + return upstreamBare, fork +} + +// assertReviewsAppChange builds a range provider for from..to and asserts the +// only changed file is app.go. +func assertReviewsAppChange(t *testing.T, repoDir, from, to string) { + t.Helper() + runner := gitcmd.New(4) + p := diff.NewProvider(repoDir, from, to, runner) + diffs, err := p.GetDiff(context.Background()) + if err != nil { + t.Fatalf("GetDiff(%s..%s): %v", from, to, err) + } + if len(diffs) != 1 { + t.Fatalf("expected 1 changed file, got %d: %+v", len(diffs), diffs) + } +} + +func TestResolveUpstreamRemoteNameFetches(t *testing.T) { + bare, fork := initUpstreamAndFork(t) + cmd := exec.Command("git", "remote", "add", "upstream", bare) + cmd.Dir = fork + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("add upstream remote: %v\n%s", err, out) + } + + from, to, err := resolveUpstream(fork, reviewOptions{upstream: "upstream"}) + if err != nil { + t.Fatalf("resolveUpstream: %v", err) + } + if from != "upstream/main" { + t.Fatalf("from = %q, want upstream/main", from) + } + if to != "HEAD" { + t.Fatalf("to = %q, want HEAD", to) + } + assertReviewsAppChange(t, fork, from, to) +} + +func TestResolveUpstreamURLFetches(t *testing.T) { + bare, fork := initUpstreamAndFork(t) + + from, to, err := resolveUpstream(fork, reviewOptions{upstream: "file://" + bare}) + if err != nil { + t.Fatalf("resolveUpstream: %v", err) + } + if !regexp.MustCompile(`^[0-9a-f]{40}$`).MatchString(from) { + t.Fatalf("from = %q, want a 40-hex SHA", from) + } + if to != "HEAD" { + t.Fatalf("to = %q, want HEAD", to) + } + assertReviewsAppChange(t, fork, from, to) +} + +func TestResolveUpstreamNoFetchUsesLocal(t *testing.T) { + _, fork := initUpstreamAndFork(t) + // origin/main + origin/HEAD already exist from the clone; no network needed. + from, to, err := resolveUpstream(fork, reviewOptions{upstream: "origin", noFetch: true}) + if err != nil { + t.Fatalf("resolveUpstream: %v", err) + } + if from != "origin/main" || to != "HEAD" { + t.Fatalf("got from=%q to=%q, want origin/main HEAD", from, to) + } +} + +func TestResolveUpstreamUnknownRemote(t *testing.T) { + _, fork := initUpstreamAndFork(t) + _, _, err := resolveUpstream(fork, reviewOptions{upstream: "nope"}) + if err == nil || !strings.Contains(err.Error(), "no git remote named") { + t.Fatalf("expected unknown-remote error, got: %v", err) + } +} + +func TestResolveUpstreamNoFetchRejectsURL(t *testing.T) { + bare, fork := initUpstreamAndFork(t) + _, _, err := resolveUpstream(fork, reviewOptions{upstream: "file://" + bare, noFetch: true}) + if err == nil || !strings.Contains(err.Error(), "--no-fetch") { + t.Fatalf("expected --no-fetch/URL error, got: %v", err) + } +} diff --git a/internal/diff/git.go b/internal/diff/git.go index 3118ebbf..e2dfb997 100644 --- a/internal/diff/git.go +++ b/internal/diff/git.go @@ -3,6 +3,7 @@ package diff import ( "bytes" "context" + "errors" "fmt" "os" "os/exec" @@ -101,7 +102,8 @@ func (p *Provider) MergeBase(ctx context.Context) string { if p.mode != ModeRange || p.mergeBase != "" { return p.mergeBase } - p.mergeBase = p.computeMergeBase(ctx, p.from, p.to) + base, _ := p.computeMergeBase(ctx, p.from, p.to) + p.mergeBase = base return p.mergeBase } @@ -113,7 +115,14 @@ func (p *Provider) GetDiff(ctx context.Context) ([]model.Diff, error) { case ModeRange: base := p.MergeBase(ctx) if base == "" { - return nil, fmt.Errorf("cannot find merge-base between %s and %s", p.from, p.to) + // Re-run to recover git's own diagnostic (merge-base can fail for + // many reasons: invalid/missing refs, unrelated histories, etc.). + _, mbErr := p.computeMergeBase(ctx, p.from, p.to) + msg := fmt.Sprintf("cannot find merge-base between %s and %s", p.from, p.to) + if mbErr != nil { + msg += ": " + mbErr.Error() + } + return nil, fmt.Errorf("%s (if this is a shallow clone, run: git fetch --unshallow)", msg) } out, err := p.runGit(ctx, "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--end-of-options", base, p.to, "--") if err != nil { @@ -256,12 +265,20 @@ func (p *Provider) filterDiffs(diffs []model.Diff) []model.Diff { // ---- Internal helpers ---- -func (p *Provider) computeMergeBase(ctx context.Context, from, to string) string { +// computeMergeBase runs `git merge-base from to`. On success it returns the +// common-ancestor commit. On failure it returns an empty base and, when git +// emitted a diagnostic, an error carrying that message. A nil error with an +// empty base means git found no common ancestor without printing anything +// (e.g. unrelated histories). +func (p *Provider) computeMergeBase(ctx context.Context, from, to string) (string, error) { out, err := p.runGit(ctx, "merge-base", "--end-of-options", from, to) if err != nil { - return "" + if detail := strings.TrimSpace(out); detail != "" { + return "", errors.New(detail) + } + return "", nil } - return strings.TrimSpace(out) + return strings.TrimSpace(out), nil } func (p *Provider) workspaceTrackedDiff(ctx context.Context) (string, error) { diff --git a/internal/diff/git_test.go b/internal/diff/git_test.go index ea96da91..db43a7b3 100644 --- a/internal/diff/git_test.go +++ b/internal/diff/git_test.go @@ -330,3 +330,68 @@ func TestRangeDiffSurvivesExternalDiffTool(t *testing.T) { "--no-ext-diff (issue #82). GIT_EXTERNAL_DIFF=%s", garbage) } } + +// TestRangeMergeBaseErrorHasUnshallowHint verifies the range-mode error names +// the offending refs and hints at shallow clones when no common ancestor exists. +func TestRangeMergeBaseErrorHasUnshallowHint(t *testing.T) { + repo := t.TempDir() + runGitTest(t, repo, "init", "-q") + runGitTest(t, repo, "config", "user.email", "test@example.com") + runGitTest(t, repo, "config", "user.name", "Test User") + runGitTest(t, repo, "config", "commit.gpgsign", "false") + + // Two unrelated root commits => no merge-base. + if err := os.WriteFile(filepath.Join(repo, "a.txt"), []byte("a\n"), 0o644); err != nil { + t.Fatal(err) + } + runGitTest(t, repo, "add", "a.txt") + runGitTest(t, repo, "commit", "-q", "-m", "first") + runGitTest(t, repo, "checkout", "-q", "--orphan", "other") + runGitTest(t, repo, "rm", "-q", "-rf", ".") + if err := os.WriteFile(filepath.Join(repo, "b.txt"), []byte("b\n"), 0o644); err != nil { + t.Fatal(err) + } + runGitTest(t, repo, "add", "b.txt") + runGitTest(t, repo, "commit", "-q", "-m", "unrelated") + + runner := gitcmd.New(4) + p := NewProvider(repo, "master", "other", runner) + _, err := p.GetDiff(context.Background()) + if err == nil { + t.Fatal("expected merge-base error for unrelated histories") + } + if !strings.Contains(err.Error(), "merge-base") || !strings.Contains(err.Error(), "unshallow") { + t.Fatalf("error missing merge-base/unshallow hint: %v", err) + } +} + +// TestRangeMergeBaseErrorIncludesGitDetail verifies that when merge-base fails +// with a real git diagnostic (e.g. a missing ref), the message surfaces it +// instead of swallowing it -- so users can tell apart "unrelated histories" +// from "bad ref" and other failure modes. +func TestRangeMergeBaseErrorIncludesGitDetail(t *testing.T) { + repo := t.TempDir() + runGitTest(t, repo, "init", "-q") + runGitTest(t, repo, "config", "user.email", "test@example.com") + runGitTest(t, repo, "config", "user.name", "Test User") + runGitTest(t, repo, "config", "commit.gpgsign", "false") + if err := os.WriteFile(filepath.Join(repo, "a.txt"), []byte("a\n"), 0o644); err != nil { + t.Fatal(err) + } + runGitTest(t, repo, "add", "a.txt") + runGitTest(t, repo, "commit", "-q", "-m", "first") + + runner := gitcmd.New(4) + p := NewProvider(repo, "HEAD", "does-not-exist", runner) + _, err := p.GetDiff(context.Background()) + if err == nil { + t.Fatal("expected error for missing ref") + } + // git's own diagnostic names the bad ref; it must not be swallowed. + if !strings.Contains(err.Error(), "does-not-exist") { + t.Fatalf("error should include git's detail naming the bad ref: %v", err) + } + if !strings.Contains(err.Error(), "merge-base") { + t.Fatalf("error should still name the merge-base failure: %v", err) + } +}