Record generated ref commits for automatic rebase merges

Automatic rebase before merge never records the rewritten commit SHAs as belonging to the merge request, because CreateRefService only writes generated_ref_commits rows for merge trains. This extends that gate to the automatic-rebase path too, so rebased commits resolve back to their merge request on the commit page, the commits API, and in changelog generation.

The change is CE-only but takes effect in both editions, since the EE override is additive over the CE method. It ships disabled by default behind the feature flag generated_ref_commits_for_automatic_rebase.

Closes #627601. Raised by a customer via https://gitlab.com/gitlab-com/request-for-help/-/work_items/5340.

Detailed context for AI agents

Root cause

The only writer of the p_generated_ref_commits table is MergeRequests::CreateRefService#store_generated_ref_commits, gated behind should_store_generated_ref_commits?. The CE implementation returns false unconditionally, with the comment "only available in ee for merge trains for now". The EE override returns super || (target_project.can_create_new_ref_commits? && merge_request.merge_train_car.present?), so rows are written only when a merge train car exists. The automatic-rebase path (MergeRequests::MergeStrategies::FromSourceBranch#use_create_ref_service?) calls the same CreateRefService but has no train car, so nothing is recorded.

Nothing else covers the rewritten SHAs. The merge request diff still holds the pre-rebase SHAs, unlike the manual Rebase button, which rewrites the source branch and refreshes the diff. merged_commit_sha and merge_commit_sha only cover the final tip commit.

CreateRefService is only reachable from the merge-train path and the automatic-rebase path, and both already require merge_method != :merge, which is exactly what can_create_new_ref_commits? checks. The CE gate therefore keys off whether maybe_rebase! actually ran, recorded as rebase_performed?. That is the same predicate expressed at the point the decision is made rather than re-derived from the merge method, and it does not affect projects that merge with a merge commit.

Replication matrix

Local, real Gitaly, two-commit merge request with the source branch behind the target so a rebase is required:

Configuration Commits landed on target Resolve back to the MR
Semi-linear (rebase_merge) + automatic rebase 2 rebased + merge commit merge commit only
Fast-forward (ff) + automatic rebase 2 rebased tip only
Plain merge commit, no rebase 2 + merge commit all

The fast-forward case only resolves the tip because merged_commit_sha happens to equal it. The original report framed this bug as specific to semi-linear history and believed fast-forward retained the association. That is not accurate: fast-forward is broken too for every commit except the tip, and only looks correct on single-commit merge requests. The real boundary is merge trains versus everything else.

Edition and licence

Verified broken in both CE (FOSS_ONLY=1) and EE, with and without licensed features stubbed. The EE override never consults the licence, only the presence of a merge train car. Fix is CE-only and applies to both editions because the EE override is super || (...).

Changelog attribution

Merge request A merges feature into main with automatic rebase, then merge request B promotes main into a stable release branch, so B's diff carries the same rebased SHAs. Asking which merge request the changelog credits for one of those commits:

Merge method Credited
Plain merge commit, no rebase A (correct)
Semi-linear with manual rebase then merge A (correct)
Semi-linear with automatic rebase B (wrong)

The fix makes the third case return A, matching the other two. This is not a behaviour change needing product sign-off. MergeRequests::OldestPerCommitFinder's docstring says it returns "the oldest merge request that introduced that commit", and a comment inside it already anticipates this case: "It is possible a newer merge request includes the commit, but in that case we still want the oldest merge request." Automatic rebase returned B only because A's rows were missing, so B won by default.

The list-style surfaces go from [B] to [A, B], which is what plain merge and manual rebase already return. No new behaviour.

Affected surfaces

  1. Commits API GET /projects/:id/repository/commits/:sha/merge_requests returns nothing for a rebased SHA.
  2. The commit page's related merge requests line (app/controllers/projects/commit_controller.rb, rendered by app/assets/javascripts/commit_merge_requests.js) shows "No related merge requests found".
  3. Changelog generation (Repositories::ChangelogService via MergeRequests::OldestPerCommitFinder) attributes the commit to the wrong merge request or none.

Feature flag and rollout

generated_ref_commits_for_automatic_rebase, type gitlab_com_derisk, default disabled, project actor. Rollout issue: #627604.

Precedent: the original merge-train fix (!195831 (merged), for #436943 (closed)) was derisked the same way behind a flag named generate_ref_commits. There is no Service Ping metric for automatic_rebase_enabled, so the affected population cannot be sized before shipping. That is the main argument for the flag.

Tests added, and the gap each closes

  • spec/services/merge_requests/merge_strategies/from_source_branch_spec.rb had zero references to generated ref commits. The entire automatic-rebase merge path had no assertion about commit-to-merge-request mapping, which is the gap that let this ship. The new test merges through the real strategy against a real repository, for both semi-linear and fast-forward, and asserts no commit landing on the target branch is left without its merge request. Confirmed to fail without the fix and pass with it.
  • spec/services/merge_requests/create_ref_service_spec.rb had a context named "when we are not on ee" asserting no rows are written. It passed either way, because the project in that context was left at the default merge method, where can_create_new_ref_commits? is false regardless of edition. It was not pinning the behaviour it appeared to pin, and the name was misleading since no FOSS_ONLY guard is involved. Replaced with explicit per-merge-method contexts covering merge, rebase_merge and ff, each with the flag on and off.
  • spec/finders/merge_requests/oldest_per_commit_finder_spec.rb already pinned "oldest wins when a newer merge request carries the commit", but only for merge commits, and covered generated ref commits only in isolation via factory. Nothing covered the two interacting. The new test adds the promotion case so the changelog answer is asserted rather than accidentally correct.

Verification

133 examples, 0 failures across spec/services/merge_requests/create_ref_service_spec.rb, ee/spec/services/ee/merge_requests/create_ref_service_spec.rb, spec/services/merge_requests/merge_strategies/from_source_branch_spec.rb, spec/services/merge_requests/merge_service_spec.rb and spec/finders/merge_requests/oldest_per_commit_finder_spec.rb. Baseline before the change was 126 examples, all of which still pass unchanged.

Follow-ups, deliberately out of scope

Detailed in #627601. In short:

  1. upsert_all(records, unique_by: [:id, :project_id]) provides no deduplication, because id is absent from the records so the primary key conflict target never fires. Rebase is not SHA-stable, so each merge attempt writes a fresh set of rows.
  2. Rows are written before the target branch moves. A fast-forward that never lands is now cleaned up here, mirroring MergeTrains::Car#cleanup_ref, but recording post-merge instead would remove the window entirely. Tracked as #571785.
  3. from_source_branch.rb passes only: [:rebase_on_merge_path] as an array where MergeRequest#refs_to_cleanup expects a symbol, so the intended immediate ref cleanup deletes nothing.
  4. MergeRequest.by_generated_ref_commit_sha(sha) takes no project argument. No leak today, since both real callers scope by project first, but adding project scoping would also let the query use the existing indexes.

Volume assessment

Rows per merge equals the number of commits plus one for semi-linear, or the number of commits for fast-forward, capped at 500 by the existing limit: 500. p_generated_ref_commits is classified table_size: small. For comparison, merge_request_diff_commits is over_limit and stores a row per commit per diff version for every merge request. No new read queries are introduced; OldestPerCommitFinder and by_related_commit_sha already read the table. The one new statement is the cleanup delete, covered below.

Cleanup query and query plan

This merge request adds one new statement, a cleanup delete that runs only when an automatic-rebase merge's fast-forward fails. It is an error-path query, not a per-merge query, and nothing extra runs on a successful merge.

DELETE FROM p_generated_ref_commits
WHERE project_id = $1
  AND merge_request_iid = $2;

Row count deleted is at most one row per commit in the merge request, capped at 500 by the existing limit: 500 on the insert side. Merge trains already run the identical statement from MergeTrains::Car#cleanup_ref, so this is not a new query shape against the table.

Plan captured on the production clone: https://console.postgres.ai/gitlab/projects/gitlab-production-main/sessions/56327/commands/160549

Partition pruning worked, only p_generated_ref_commits_1 was scanned, with Index Cond: project_id = 278964. 2 shared buffer reads, 1.590 ms execution, 1.557 ms planning.

Caveat: that plan ran against gitlab-org/gitlab, project id 278964, which has merge_method: merge. can_create_new_ref_commits? is false for such projects, so the table holds no rows for it. With zero matching rows the planner had no reason to prefer either index, and it chose p_generated_ref_commits_1_project_id_commit_sha_idx, the (project_id, commit_sha) index, leaving merge_request_iid as a Filter rather than an Index Cond.

The intended index is p_index_generated_ref_commits_on_merge_request_id, covering (project_id, merge_request_iid), the exact columns and order this delete filters on. On a project that actually holds rows the planner should use it with both columns as Index Cond. Re-running the plan against a project and merge request pair with rows is worth doing before this is considered fully reviewed: if the planner still picks the commit_sha index and filters, the delete reads every row for that project instead of just the merge request's.

Regardless, the blast radius is bounded: error path only, at most 500 rows, and the same statement already runs in production for merge trains.

Checked and unaffected

  • Fork merge requests: merge_request.project_id is the target project id, matching the foreign key (project_id, merge_request_iid) to merge_requests(target_project_id, iid).
  • Project and merge request deletion: both foreign keys are ON DELETE CASCADE.
  • Import and export: generated_ref_commits is not in project/import_export.yml, so exports are unchanged. This also means an instance migration drops these mappings.
  • Squash: runs before the rebase, and the resulting commit is captured in the same range.

Not retroactive

Already-merged merge requests stay orphaned. Backfilling would mean walking merge history per project.

Edited by Marc Shaw

Merge request reports

Loading
Loading