From 6136d224bba3d43668e3ae446b7456ca80aae85a Mon Sep 17 00:00:00 2001 From: Dinesh Pandiyan Date: Wed, 21 Oct 2026 18:03:55 +1100 Subject: [PATCH] fix: compare branches against nearest default baseline Auto mode merge-based against local default only, so branches cut from a fresher origin/main included upstream commits. Pick the nearer of local and remote-tracking default merge bases; ties and divergence keep local. --- README.md | 4 +- docs/plugins.md | 6 +- internal/diffsource/comparison.go | 98 ++++++++++++++-- internal/diffsource/comparison_test.go | 148 ++++++++++++++++++++++--- 4 files changed, 227 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 1b04064b..97ec27ba 100644 --- a/README.md +++ b/README.md @@ -88,8 +88,8 @@ and plugins require a remote URL and bearer token; they never bootstrap a local server. `diffx remote status` checks the destination, credential and protocol, and reports agent-hook deliveries still waiting. -The all scope includes branch changes from the local default branch's merge base -plus working changes. Staged/unstaged retain HEAD/index semantics. +The all scope includes branch changes from the default branch's nearest merge base +(local or remote-tracking) plus working changes. Staged/unstaged retain HEAD/index semantics. `--base HEAD` selects working changes only. Collection never fetches Git refs. Matching captures reuse review state. Retention defaults to seven days after the last fresh submission; retries do not extend it. diff --git a/docs/plugins.md b/docs/plugins.md index d25c7f45..184e9b68 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -163,8 +163,10 @@ default unless an explicit config file, environment or `--listen` overrides it. ## Synchronization The worker captures committed feature-branch changes and dirty files against the -merge base with the local default branch (`origin/HEAD`, `main`, then `master`). -Staged/unstaged scopes retain their HEAD/index meanings, including untracked +merge base with the default branch (`origin/HEAD`, `main`, then `master`). It +compares the local branch and its remote-tracking upstream (else `origin`'s +counterpart), using the remote merge base only when it descends from the local +one; ties and divergence keep local. Refs are never fetched. Staged/unstaged scopes retain their HEAD/index meanings, including untracked files. If no default branch exists, live collection records explicit working-tree/HEAD fallback; object-only recovery requires an available baseline. It retains patches and bounded full before/after contents for future diff --git a/internal/diffsource/comparison.go b/internal/diffsource/comparison.go index 42c86860..6c2ad931 100644 --- a/internal/diffsource/comparison.go +++ b/internal/diffsource/comparison.go @@ -10,8 +10,8 @@ import ( "github.com/flexdinesh/diffx/internal/review" ) -// ErrNoDefaultBranch means no existing local default branch can be resolved. -var ErrNoDefaultBranch = errors.New("no local default branch available for comparison") +// ErrNoDefaultBranch means no existing default branch ref can be resolved. +var ErrNoDefaultBranch = errors.New("no default branch available for comparison") // ErrComparisonUnavailable permits replaying an already captured comparison // after its local refs disappear. It never masks process or cancellation errors. @@ -41,25 +41,93 @@ func ComparisonInfo(source Source) (Comparison, bool) { return git.comparison.Comparison, true } -// DefaultBranch resolves existing local refs only. It never fetches or consults -// a feature branch's upstream, which may simply track that same feature branch. -func DefaultBranch(ctx context.Context, root string) (string, error) { +// DefaultBaselines resolves existing refs for the first default branch name +// (`origin/HEAD`, `main`, then `master`) with a local or remote-tracking ref: +// the local branch first, then its remote-tracking upstream or origin +// counterpart. It never fetches or consults a feature branch's upstream, which +// may simply track that same feature branch. +func DefaultBaselines(ctx context.Context, root string) ([]string, error) { remote, err := runGit(ctx, root, 16<<10, "symbolic-ref", "--quiet", "refs/remotes/origin/HEAD") if err != nil && !comparisonGitExit(err, 1) { - return "", err + return nil, err } candidates := []string{"main", "master"} if branch, ok := strings.CutPrefix(strings.TrimSpace(remote), "refs/remotes/origin/"); ok { candidates = append([]string{branch}, candidates...) } for _, branch := range candidates { - if _, err := resolveCommit(ctx, root, "refs/heads/"+branch); err == nil { - return branch, nil - } else if !comparisonGitExit(err, 128) { + local := "refs/heads/" + branch + var baselines []string + exists, err := refExists(ctx, root, local) + if err != nil { + return nil, err + } + tracking := "refs/remotes/origin/" + branch + if exists { + baselines = append(baselines, local) + upstream, err := runGit(ctx, root, 16<<10, "for-each-ref", "--format=%(upstream)", local) + if err != nil { + return nil, err + } + if upstream = strings.TrimSpace(upstream); strings.HasPrefix(upstream, "refs/remotes/") { + if exists, err := refExists(ctx, root, upstream); err != nil { + return nil, err + } else if exists { + tracking = upstream + } + } + } + if exists, err := refExists(ctx, root, tracking); err != nil { + return nil, err + } else if exists { + baselines = append(baselines, tracking) + } + if len(baselines) > 0 { + return baselines, nil + } + } + return nil, ErrNoDefaultBranch +} + +func refExists(ctx context.Context, root, ref string) (bool, error) { + if _, err := resolveCommit(ctx, root, ref); err == nil { + return true, nil + } else if !comparisonGitExit(err, 128) { + return false, err + } + return false, nil +} + +// nearestBaseline keeps the first baseline unless a later one's merge base +// strictly descends from it; ties and divergence keep the earlier baseline. +// Baselines with unrelated histories are skipped; if none relate, the first +// remains so the caller reports its usual fallback or failure. +func nearestBaseline(ctx context.Context, root, head string, baselines []string) (string, error) { + chosen, chosenBase := baselines[0], "" + for _, baseline := range baselines { + raw, err := runGit(ctx, root, 1024, "merge-base", baseline, head) + if comparisonGitExit(err, 1) { + continue + } + if err != nil { + return "", err + } + mergeBase := strings.TrimSpace(raw) + if chosenBase == "" { + chosen, chosenBase = baseline, mergeBase + continue + } + if mergeBase == chosenBase { + continue + } + _, err = runGit(ctx, root, 1024, "merge-base", "--is-ancestor", chosenBase, mergeBase) + if err == nil { + chosen, chosenBase = baseline, mergeBase + } else if !comparisonGitExit(err, 1) { return "", err } } - return "", ErrNoDefaultBranch + return chosen, nil } func resolveCommit(ctx context.Context, root, ref string) (string, error) { @@ -83,8 +151,9 @@ func OpenComparison(ctx context.Context, directory, base, branch string) (Source if (base == "" || base == "HEAD") && branch == "" { return source, nil } + var baselines []string if auto { - base, err = DefaultBranch(ctx, source.root) + baselines, err = DefaultBaselines(ctx, source.root) if errors.Is(err, ErrNoDefaultBranch) && branch == "" { return source, nil } @@ -94,7 +163,7 @@ func OpenComparison(ctx context.Context, directory, base, branch string) (Source } return nil, err } - base = "refs/heads/" + base + base = baselines[0] } target := "HEAD" if branch != "" { @@ -107,6 +176,11 @@ func OpenComparison(ctx context.Context, directory, base, branch string) (Source } return nil, comparisonFailure(fmt.Sprintf("resolve comparison target %q", target), err, 128) } + if len(baselines) > 1 { + if base, err = nearestBaseline(ctx, source.root, head, baselines); err != nil { + return nil, fmt.Errorf("find comparison merge base: %w", err) + } + } baseTip, err := resolveCommit(ctx, source.root, base) if err != nil { return nil, comparisonFailure(fmt.Sprintf("resolve comparison baseline %q", base), err, 128) diff --git a/internal/diffsource/comparison_test.go b/internal/diffsource/comparison_test.go index 930106bf..0b1176a2 100644 --- a/internal/diffsource/comparison_test.go +++ b/internal/diffsource/comparison_test.go @@ -27,31 +27,153 @@ func comparisonRepository(t *testing.T) string { return root } -func TestDefaultBranchUsesLocalRemoteHeadCounterpartThenConventions(t *testing.T) { +func TestDefaultBaselinesUseRemoteHeadThenConventions(t *testing.T) { root := comparisonRepository(t) - if name, err := DefaultBranch(t.Context(), root); err != nil || name != "main" { - t.Fatalf("main fallback: %q %v", name, err) + assertBaselines := func(label string, want ...string) { + t.Helper() + got, err := DefaultBaselines(t.Context(), root) + if err != nil || strings.Join(got, " ") != strings.Join(want, " ") { + t.Fatalf("%s: %q %v", label, got, err) + } } + assertBaselines("main fallback", "refs/heads/main") testGit(t, root, "branch", "trunk") testGit(t, root, "update-ref", "refs/remotes/origin/trunk", "HEAD") testGit(t, root, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/trunk") - if name, err := DefaultBranch(t.Context(), root); err != nil || name != "trunk" { - t.Fatalf("remote HEAD local counterpart: %q %v", name, err) - } + assertBaselines("remote HEAD with local branch", "refs/heads/trunk", "refs/remotes/origin/trunk") testGit(t, root, "branch", "-D", "trunk") - if name, err := DefaultBranch(t.Context(), root); err != nil || name != "main" { - t.Fatalf("missing local counterpart: %q %v", name, err) - } + assertBaselines("remote HEAD without local branch", "refs/remotes/origin/trunk") + testGit(t, root, "symbolic-ref", "--delete", "refs/remotes/origin/HEAD") + testGit(t, root, "update-ref", "-d", "refs/remotes/origin/trunk") testGit(t, root, "branch", "-m", "main", "master") - if name, err := DefaultBranch(t.Context(), root); err != nil || name != "master" { - t.Fatalf("master fallback: %q %v", name, err) - } + assertBaselines("master fallback", "refs/heads/master") testGit(t, root, "branch", "-m", "master", "custom") - if _, err := DefaultBranch(t.Context(), root); !errors.Is(err, ErrNoDefaultBranch) { + if _, err := DefaultBaselines(t.Context(), root); !errors.Is(err, ErrNoDefaultBranch) { t.Fatalf("missing default: %v", err) } } +func TestDefaultBaselinesPreferDefaultBranchUpstream(t *testing.T) { + root := comparisonRepository(t) + testGit(t, root, "remote", "add", "origin", "https://example.com/origin.git") + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "remote", "add", "upstream", "https://example.com/upstream.git") + testGit(t, root, "update-ref", "refs/remotes/upstream/main", "HEAD") + testGit(t, root, "branch", "--set-upstream-to=upstream/main", "main") + testGit(t, root, "switch", "-c", "feature") + testGit(t, root, "branch", "--set-upstream-to=origin/main", "feature") + got, err := DefaultBaselines(t.Context(), root) + if err != nil || strings.Join(got, " ") != "refs/heads/main refs/remotes/upstream/main" { + t.Fatalf("default branch upstream ignored: %q %v", got, err) + } +} + +func TestAutoComparisonUsesNearestDefaultBaseline(t *testing.T) { + cases := []struct { + name, want string + arrange func(t *testing.T, root string) + }{ + {"branch from fetched remote", "refs/remotes/origin/main", func(t *testing.T, root string) { + testGit(t, root, "switch", "-c", "remote") + testGit(t, root, "commit", "--allow-empty", "-m", "upstream") + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "switch", "-c", "feature") + testGit(t, root, "branch", "-D", "remote") + }}, + {"branch from unpushed local", "refs/heads/main", func(t *testing.T, root string) { + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "commit", "--allow-empty", "-m", "unpushed") + testGit(t, root, "switch", "-c", "feature") + }}, + {"stale remote", "refs/heads/main", func(t *testing.T, root string) { + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "commit", "--allow-empty", "-m", "fetched then pulled") + testGit(t, root, "switch", "-c", "feature") + }}, + {"equal merge bases", "refs/heads/main", func(t *testing.T, root string) { + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "switch", "-c", "feature") + }}, + {"diverged defaults, branch from remote", "refs/remotes/origin/main", func(t *testing.T, root string) { + divergeDefaults(t, root) + testGit(t, root, "switch", "-c", "feature", "refs/remotes/origin/main") + }}, + {"diverged defaults, branch from local", "refs/heads/main", func(t *testing.T, root string) { + divergeDefaults(t, root) + testGit(t, root, "switch", "-c", "feature", "main") + }}, + {"unrelated remote", "refs/heads/main", func(t *testing.T, root string) { + testGit(t, root, "switch", "--orphan", "remote") + testGit(t, root, "commit", "--allow-empty", "-m", "unrelated") + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "switch", "main") + testGit(t, root, "branch", "-D", "remote") + testGit(t, root, "switch", "-c", "feature") + }}, + } + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + root := comparisonRepository(t) + test.arrange(t, root) + testGit(t, root, "commit", "--allow-empty", "-m", "feature") + want := strings.TrimSpace(testGit(t, root, "merge-base", test.want, "HEAD")) + for _, branch := range []string{"", "feature"} { + source, err := OpenComparison(t.Context(), root, "auto", branch) + if err != nil { + t.Fatal(err) + } + comparison, ok := ComparisonInfo(source) + if !ok || comparison.BaseRef != test.want || comparison.BaseOID != want { + t.Fatalf("branch %q: %+v, want %s at %s", branch, comparison, test.want, want) + } + } + }) + } +} + +func divergeDefaults(t *testing.T, root string) { + t.Helper() + testGit(t, root, "switch", "-c", "remote") + testGit(t, root, "commit", "--allow-empty", "-m", "upstream") + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "switch", "main") + testGit(t, root, "branch", "-D", "remote") + testGit(t, root, "commit", "--allow-empty", "-m", "local") +} + +func TestAutoComparisonUsesRemoteWithoutLocalDefault(t *testing.T) { + root := comparisonRepository(t) + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "switch", "-c", "feature") + testGit(t, root, "branch", "-D", "main") + testGit(t, root, "commit", "--allow-empty", "-m", "feature") + source, err := OpenComparison(t.Context(), root, "auto", "") + if err != nil { + t.Fatal(err) + } + if comparison, ok := ComparisonInfo(source); !ok || comparison.BaseRef != "refs/remotes/origin/main" { + t.Fatalf("remote default ignored: %+v", comparison) + } +} + +func TestAutoComparisonDetectsMovedRemoteBaseline(t *testing.T) { + root := comparisonRepository(t) + testGit(t, root, "switch", "-c", "feature") + testGit(t, root, "commit", "--allow-empty", "-m", "upstream") + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + testGit(t, root, "commit", "--allow-empty", "-m", "feature") + source, err := OpenComparison(t.Context(), root, "auto", "") + if err != nil { + t.Fatal(err) + } + testGit(t, root, "update-ref", "refs/remotes/origin/main", "HEAD") + _, err = source.Snapshot(t.Context(), review.DiffAll) + var problem *RequestError + if !errors.As(err, &problem) || problem.Status != 409 { + t.Fatalf("moved remote baseline accepted: %v", err) + } +} + func TestComparisonFallbackDoesNotHideExplicitFailures(t *testing.T) { root := comparisonRepository(t) testGit(t, root, "branch", "-m", "main", "custom")