fix(mr): read squash-merged requests whose commits were pruned and check branches under the merge lock

commit: 5cb495bb00891ca123b2e2888314681782f47886
author: brice · 11d ago · 2026-10-11 05:53 UTC
parent: 4ea27ba
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +60 -9

M internal/app/mergerequests.go +20 -9


@@ -188,21 +188,21 @@ func (m *MergeRequests) Create(ctx context.Context, p domain.Principal, name dom
188	188		if err != nil {
189	189		v.AddErr("description", err)
190	190		}
191		-	source, target, err := m.checkBranches(ctx, name, params, &v)
	191	+	// Under the lock, so that two requests for the same pair cannot both pass
	192	+	// the check for an existing one, and a merge that deletes its source
	193	+	// cannot remove a branch between its check here and the creation.
	194	+	unlock, err := m.lockRepo(ctx, name)
192	195		if err != nil {
193	196		return domain.MergeRequest{}, err
194	197		}
195		-	if err := v.Err(); err != nil {
	198	+	defer unlock()
	199	+	source, target, err := m.checkBranches(ctx, name, params, &v)
	200	+	if err != nil {
196	201		return domain.MergeRequest{}, err
197	202		}
198		-
199		-	// Under the lock, so that two requests for the same pair cannot both pass
200		-	// the check for an existing one.
201		-	unlock, err := m.lockRepo(ctx, name)
202		-	if err != nil {
	203	+	if err := v.Err(); err != nil {
203	204		return domain.MergeRequest{}, err
204	205		}
205		-	defer unlock()
206	206		existing, err := m.store.List(ctx, name)
207	207		if err != nil {
208	208		return domain.MergeRequest{}, fmt.Errorf("list merge requests of %q: %w", name, err)

@@ -414,6 +414,16 @@ func (m *MergeRequests) Get(ctx context.Context, p domain.Principal, name domain
414	414		if head.IsZero() {
415	415		return d, nil // the source branch is gone: no commits, no diff
416	416		}
	417	+	if !mr.IsOpen() {
	418	+	// After a squash the reviewed commits are only reachable from the
	419	+	// source branch; once it is deleted, gc may prune them.
	420	+	switch ok, err := m.git.HasCommit(ctx, name, head); {
	421	+	case err != nil:
	422	+	return MergeRequestDetail{}, err
	423	+	case !ok:
	424	+	return d, nil // the reviewed commits are gone: no commits, no diff
	425	+	}
	426	+	}
417	427		if d.Commits, d.CommitsTruncated, err = m.git.ListRange(ctx, name, exclude, head, MaxMergeRequestCommits); err != nil {
418	428		return MergeRequestDetail{}, err
419	429		}

@@ -860,7 +870,8 @@ func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name doma
860	870
861	871		// checkDeletable refuses to delete the source branch of mr when it is the
862	872		// default branch or the source or target of another open request. It runs
863		-	// under the merge lock, which also guards the creation of requests.
	873	+	// under the merge lock, which Create also holds while it checks its branches
	874	+	// and stores the request.
864	875		func (m *MergeRequests) checkDeletable(ctx context.Context, name domain.RepoName, mr domain.MergeRequest) error {
865	876		def, _, err := m.repos.DefaultBranch(ctx, name)
866	877		if err != nil {


@@ -188,21 +188,21 @@ func (m *MergeRequests) Create(ctx context.Context, p domain.Principal, name dom
188		if err != nil {	188		if err != nil {
189		v.AddErr("description", err)	189		v.AddErr("description", err)
190		}	190		}
191	-	source, target, err := m.checkBranches(ctx, name, params, &v)	191	+	// Under the lock, so that two requests for the same pair cannot both pass
			192	+	// the check for an existing one, and a merge that deletes its source
			193	+	// cannot remove a branch between its check here and the creation.
			194	+	unlock, err := m.lockRepo(ctx, name)
192		if err != nil {	195		if err != nil {
193		return domain.MergeRequest{}, err	196		return domain.MergeRequest{}, err
194		}	197		}
195	-	if err := v.Err(); err != nil {	198	+	defer unlock()
			199	+	source, target, err := m.checkBranches(ctx, name, params, &v)
			200	+	if err != nil {
196		return domain.MergeRequest{}, err	201		return domain.MergeRequest{}, err
197		}	202		}
198	-		203	+	if err := v.Err(); err != nil {
199	-	// Under the lock, so that two requests for the same pair cannot both pass
200	-	// the check for an existing one.
201	-	unlock, err := m.lockRepo(ctx, name)
202	-	if err != nil {
203		return domain.MergeRequest{}, err	204		return domain.MergeRequest{}, err
204		}	205		}
205	-	defer unlock()
206		existing, err := m.store.List(ctx, name)	206		existing, err := m.store.List(ctx, name)
207		if err != nil {	207		if err != nil {
208		return domain.MergeRequest{}, fmt.Errorf("list merge requests of %q: %w", name, err)	208		return domain.MergeRequest{}, fmt.Errorf("list merge requests of %q: %w", name, err)

@@ -414,6 +414,16 @@ func (m *MergeRequests) Get(ctx context.Context, p domain.Principal, name domain
414		if head.IsZero() {	414		if head.IsZero() {
415		return d, nil // the source branch is gone: no commits, no diff	415		return d, nil // the source branch is gone: no commits, no diff
416		}	416		}
			417	+	if !mr.IsOpen() {
			418	+	// After a squash the reviewed commits are only reachable from the
			419	+	// source branch; once it is deleted, gc may prune them.
			420	+	switch ok, err := m.git.HasCommit(ctx, name, head); {
			421	+	case err != nil:
			422	+	return MergeRequestDetail{}, err
			423	+	case !ok:
			424	+	return d, nil // the reviewed commits are gone: no commits, no diff
			425	+	}
			426	+	}
417		if d.Commits, d.CommitsTruncated, err = m.git.ListRange(ctx, name, exclude, head, MaxMergeRequestCommits); err != nil {	427		if d.Commits, d.CommitsTruncated, err = m.git.ListRange(ctx, name, exclude, head, MaxMergeRequestCommits); err != nil {
418		return MergeRequestDetail{}, err	428		return MergeRequestDetail{}, err
419		}	429		}

@@ -860,7 +870,8 @@ func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name doma
860			870
861		// checkDeletable refuses to delete the source branch of mr when it is the	871		// checkDeletable refuses to delete the source branch of mr when it is the
862		// default branch or the source or target of another open request. It runs	872		// default branch or the source or target of another open request. It runs
863	-	// under the merge lock, which also guards the creation of requests.	873	+	// under the merge lock, which Create also holds while it checks its branches
			874	+	// and stores the request.
864		func (m *MergeRequests) checkDeletable(ctx context.Context, name domain.RepoName, mr domain.MergeRequest) error {	875		func (m *MergeRequests) checkDeletable(ctx context.Context, name domain.RepoName, mr domain.MergeRequest) error {
865		def, _, err := m.repos.DefaultBranch(ctx, name)	876		def, _, err := m.repos.DefaultBranch(ctx, name)
866		if err != nil {	877		if err != nil {

M internal/app/mergerequests_delete_test.go +40 -0


@@ -112,6 +112,46 @@ func TestMergeRefusesToDeleteANeededBranch(t *testing.T) {
112	112		})
113	113		}
114	114
	115	+	// After a squash with deletion, gc may prune the reviewed commits: the merged
	116	+	// request still reads, without commits or diff.
	117	+	func TestSquashMergedRequestReadsAfterItsCommitsArePruned(t *testing.T) {
	118	+	r := newMRRig(t)
	119	+	r.seed()
	120	+	head := r.feature("feat")
	121	+	mr := r.open("feat")
	122	+	res, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{Mode: domain.MergeModeSquash, HeadID: head, DeleteSource: true})
	123	+	require.NoError(t, err)
	124	+	require.True(t, res.SourceDeleted)
	125	+
	126	+	r.run(r.bare(), r.env(author), "reflog", "expire", "--expire=now", "--all")
	127	+	r.run(r.bare(), r.env(author), "gc", "-q", "--prune=now")
	128	+	ok, err := r.git.HasCommit(r.ctx, r.repo, head)
	129	+	require.NoError(t, err)
	130	+	require.False(t, ok, "the reviewed commits are pruned")
	131	+
	132	+	d, err := r.mrs.Get(r.ctx, reader, r.repo, mr.Number, false)
	133	+	require.NoError(t, err)
	134	+	assert.Equal(t, domain.MRStateMerged, d.MergeRequest.State)
	135	+	assert.Equal(t, res.MergedCommitID, d.MergeRequest.MergedCommitID)
	136	+	assert.Empty(t, d.Commits)
	137	+	assert.Empty(t, d.Diff.Files)
	138	+	assert.True(t, d.DiffBaseID.IsZero())
	139	+	}
	140	+
	141	+	// A request opened on a branch a merge is deleting waits for the merge lock
	142	+	// and then sees the branch is gone.
	143	+	func TestCreateAfterAMergeDeletedTheBranch(t *testing.T) {
	144	+	r := newMRRig(t)
	145	+	r.seed()
	146	+	head := r.feature("feat")
	147	+	mr := r.open("feat")
	148	+	_, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true})
	149	+	require.NoError(t, err)
	150	+	_, err = r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "late", Source: "feat"})
	151	+	require.ErrorIs(t, err, domain.ErrInvalid)
	152	+	assert.Contains(t, fieldsOf(t, err), "source_branch")
	153	+	}
	154	+
115	155		// movingGit pushes to the source branch between the merge and the deletion.
116	156		type movingGit struct {
117	157		*gitcli.Git


@@ -112,6 +112,46 @@ func TestMergeRefusesToDeleteANeededBranch(t *testing.T) {
112	})	112		})
113	}	113		}
114		114
		115	+	// After a squash with deletion, gc may prune the reviewed commits: the merged
		116	+	// request still reads, without commits or diff.
		117	+	func TestSquashMergedRequestReadsAfterItsCommitsArePruned(t *testing.T) {
		118	+	r := newMRRig(t)
		119	+	r.seed()
		120	+	head := r.feature("feat")
		121	+	mr := r.open("feat")
		122	+	res, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{Mode: domain.MergeModeSquash, HeadID: head, DeleteSource: true})
		123	+	require.NoError(t, err)
		124	+	require.True(t, res.SourceDeleted)
		125	+
		126	+	r.run(r.bare(), r.env(author), "reflog", "expire", "--expire=now", "--all")
		127	+	r.run(r.bare(), r.env(author), "gc", "-q", "--prune=now")
		128	+	ok, err := r.git.HasCommit(r.ctx, r.repo, head)
		129	+	require.NoError(t, err)
		130	+	require.False(t, ok, "the reviewed commits are pruned")
		131	+
		132	+	d, err := r.mrs.Get(r.ctx, reader, r.repo, mr.Number, false)
		133	+	require.NoError(t, err)
		134	+	assert.Equal(t, domain.MRStateMerged, d.MergeRequest.State)
		135	+	assert.Equal(t, res.MergedCommitID, d.MergeRequest.MergedCommitID)
		136	+	assert.Empty(t, d.Commits)
		137	+	assert.Empty(t, d.Diff.Files)
		138	+	assert.True(t, d.DiffBaseID.IsZero())
		139	+	}
		140	+
		141	+	// A request opened on a branch a merge is deleting waits for the merge lock
		142	+	// and then sees the branch is gone.
		143	+	func TestCreateAfterAMergeDeletedTheBranch(t *testing.T) {
		144	+	r := newMRRig(t)
		145	+	r.seed()
		146	+	head := r.feature("feat")
		147	+	mr := r.open("feat")
		148	+	_, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true})
		149	+	require.NoError(t, err)
		150	+	_, err = r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "late", Source: "feat"})
		151	+	require.ErrorIs(t, err, domain.ErrInvalid)
		152	+	assert.Contains(t, fieldsOf(t, err), "source_branch")
		153	+	}
		154	+
115	// movingGit pushes to the source branch between the merge and the deletion.	155		// movingGit pushes to the source branch between the merge and the deletion.
116	type movingGit struct {	156		type movingGit struct {
117	*gitcli.Git	157		*gitcli.Git