Skip to content

feat: add MergeBase and multi-parent commit support#377

Closed
MissingRoberto wants to merge 1 commit into
mainfrom
MissingRoberto/bordeaux
Closed

feat: add MergeBase and multi-parent commit support#377
MissingRoberto wants to merge 1 commit into
mainfrom
MissingRoberto/bordeaux

Conversation

@MissingRoberto

Copy link
Copy Markdown
Contributor

Summary

Adds a merge-base (common-ancestor) primitive to the Client interface, the building block for three-dot diffs (a...b). Comparing the merge base against a ref yields only what that ref actually introduced, excluding commits that landed on the other branch after the fork point.

This is Stage 1 of the fix for the Git Sync PR-preview issue (support-escalations #23127), where the preview listed files the PR author never touched because CompareCommits does a two-dot (main..branch) snapshot diff with no common-ancestor step. nanogit had no merge-base primitive and only tracked a single commit parent, so the fix has to originate here.

Stage 2 (wiring a three-dot path into apps/provisioning's PR-preview and bumping nanogit) is a separate PR in the Grafana repo, gated on a tagged release of this change.

Changes

  • MergeBase(ctx, a, b, ...MergeBaseOption) (hash.Hash, error) on the Client interface (merge_base.go): walks the commit graph from both inputs following all parents, ordered by committer time, painting reachability flags and terminating as soon as common ancestors are found rather than descending to the root. Bounded by WithMergeBaseMaxCommits (default 2000).
  • Multi-parent commit support (correctness prerequisite for merge commits):
    • protocol: PackfileCommit gains Parents; the parser appends each parent line instead of overwriting, and Build() emits all parents. Parent still mirrors Parents[0], and writer-created single-parent commits serialize byte-for-byte as before (object hashes unchanged).
    • nanogit.Commit gains Parents, populated from the parsed object.
  • New sentinel errors: ErrNoMergeBase, ErrMergeBaseLimitExceeded.
  • Regenerated mocks/client.go.

Testing

  • Unit: multi-parent parse/build round-trips (single / merge / root commits); heap ordering, tie-break, allStale, and candidate selection.
  • Integration (Gitea via testcontainers) — reproduces the escalation: two-dot compare wrongly includes the unrelated main file while three-dot via the merge base returns only the branch's change; plus order-independence, identical/ancestor inputs, the no-common-ancestor error, and a real merge commit exposing both parents.
  • Verified locally: go build ./..., unit tests, make lint (0 issues), make lint-staticcheck (0 issues), MergeBase integration specs (8/8), and the full Commit-focused integration regression (53/53).

Notes / limitations

  • In the presence of criss-cross merges a pair of commits can have more than one merge base; MergeBase returns a single one (most recent by committer time), which is sufficient for computing a three-dot file diff. Documented in the godoc.
  • No breaking changes: Parents is additive; Parent behavior is preserved.

Checklist

  • Tests added/updated
  • Godoc for new exported APIs
  • No breaking changes

🤖 Generated with Claude Code

Add a merge-base (common-ancestor) primitive to the Client interface so
callers can compute three-dot diffs (a...b): comparing the merge base
against a ref yields only what that ref introduced, excluding commits
that landed on the other branch after the fork point. This is the
building block a PR-preview needs to stop reporting files the branch
author never touched.

MergeBase walks the commit graph from both inputs following ALL parents,
ordered by committer time, painting reachability flags and terminating
as soon as common ancestors are found rather than descending to the
root. The walk is bounded (WithMergeBaseMaxCommits, default 2000) and
returns ErrNoMergeBase for unrelated histories or
ErrMergeBaseLimitExceeded when the budget is exhausted.

Correct merge-base computation requires every parent of a merge commit,
so this also adds multi-parent support:

- protocol: PackfileCommit gains Parents; the parser appends each parent
  line instead of overwriting, and Build() emits all parents. Parent
  still mirrors Parents[0], and writer-created (single-parent) commits
  serialize byte-for-byte as before, keeping their object hashes stable.
- nanogit.Commit gains Parents, populated from the parsed object.

Tested with unit coverage for multi-parent parse/build and the walk's
ordering/selection helpers, plus integration tests against Gitea that
reproduce the escalation scenario (two-dot reports an unrelated main
file; three-dot via the merge base returns only the branch's change),
order-independence, ancestor/identical inputs, the no-common-ancestor
error, and a real merge commit exposing both parents.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.15789% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.36%. Comparing base (8859d59) to head (d59ae96).

Files with missing lines Patch % Lines
merge_base.go 71.59% 35 Missing and 13 partials ⚠️
protocol/packfile.go 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
+ Coverage   72.34%   72.36%   +0.02%     
==========================================
  Files          59       60       +1     
  Lines        7391     7574     +183     
==========================================
+ Hits         5347     5481     +134     
- Misses       1636     1671      +35     
- Partials      408      422      +14     
Files with missing lines Coverage Δ
client.go 89.83% <ø> (ø)
commit.go 80.72% <100.00%> (+0.03%) ⬆️
errors.go 100.00% <ø> (ø)
protocol/packfile.go 70.27% <82.35%> (+0.10%) ⬆️
merge_base.go 71.59% <71.59%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amalavet

amalavet commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Did some exploring and I have a proposed long term solution here:

Why not nanogit? #377

If we add a method in nanogit to derive the mergeBase, it requires complex logic and has to pull in a lot of commit history. When compared to just reading the merge base from the git provider's servers via provided REST apis, this is not optimal.

Why not use merge refs? grafana/grafana#127545

GitHub publishes refs/pull/<n>/merge, GitLab publishes refs/merge-requests/<n>/merge however bitbucket doesn't https://jira.atlassian.com/browse/BCLOUD-5814. So if we did the merge ref approach, it wouldn't be supported by all git providers. BitBucket does have a rest api to get the mergeBase though, which is why the above implementation works.

Other Considerations

Merge Requests/Pull Requests are not native to git, they're a feature offered by git providers and it makes sense to rely on their REST APIs to interface with them rather than using native git apis/nanogit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants