Commit `a5ee6a83a1`

a5ee6a83a19d495b6c752d27440fae61174e971a

parent: 88fc476788

Verified · cmc ci/build: success ci/test: success

cmc <hello@cleberg.net> · 2026-09-07 22:36 UTC

gitutil, store, hookd, wiki: a rebase that keeps the diff keeps the approvals

Reviews keyed on the head sha, so a rebase onto a target that moved on
staled every approval although the diff against the merge base was the
one reviewed. On an ff-only repository every merge request that waited
behind another paid that on each catch-up.

post-receive now compares the patch-id of the old head against its
merge base with the new head against its own; when they match, the
fresh reviews of the old head move to the new one. Reviews already
stale stay stale, and a push that changes the diff stales as before.
The existing stale-approval test force-pushed a message-only amend,
which is now the case that keeps the approval; it changes content.

Closes #198

.gitbay/wiki/Stacked-MRs.org +3 −2

		@@ -53,8 +53,9 @@ below it.
53	53
54	54	Changing a lower layer is a rebase you do yourself. Amend =feat-a=,
55	55	then =git rebase feat-a= on =feat-b= and each layer above, and
56		force-push them. A force-push stales the reviews on that layer, the
57		same as on any merge request. The server never rewrites your commits:
	56	force-push them. A force-push stales the reviews on that layer when it
	57	changes the layer's diff, the same as on any merge request; a rebase
	58	that carries the same change keeps them. The server never rewrites your commits:
58	59	it holds no signing key, and a rebase it performed would land commits
59	60	nobody signed.
60	61

.gitbay/wiki/Users.org +4 −1

		@@ -378,7 +378,10 @@ Semantics worth knowing:
378	378	=refs/merge-requests/N/head= (fetchable by any reader), so an MR
379	379	survives deletion of its source branch or fork.
380	380	- force-pushing the source updates the MR and marks existing reviews
381		stale.
	381	stale — unless the diff is the one they reviewed. A rebase onto a
	382	target that moved on changes every sha and nothing about the change,
	383	so fresh reviews follow it to the new head; a push that changes the
	384	diff stales them.
382	385	- default strategy: fast-forward when possible, else a merge commit.
383	386	Squash makes one commit authored by the MR author, committed by the
384	387	merger. Rebase replays a linear range preserving authors; it refuses

e2e/approvals_test.go +4 −2

		@@ -118,7 +118,8 @@ func TestMergeRequirements(t *testing.T) {
118	118	t.Fatalf("fully gated merge: %s", errOut)
119	119	}
120	120
121		// Stale approvals never count: new MR, approve, force-push, refused.
	121	// Stale approvals never count: new MR, approve, force-push a changed
	122	// diff, refused. (A force-push carrying the same diff keeps them, #198.)
122	123	mustGit(t, dir, env, "fetch", "-q", "origin")
123	124	mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "origin/main")
124	125	os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("n\n"), 0o644)
		@@ -132,7 +133,8 @@ func TestMergeRequirements(t *testing.T) {
132	133	if _, _, code = inst.ssh(t, bobKey, "", "mr", "review", "alice/svc", "2", "--approve"); code != 0 {
133	134	t.Fatal("bob approve 2 failed")
134	135	}
135		mustGit(t, dir, env, "commit", "-q", "--amend", "-m", "notes v2")
	136	os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("n2\n"), 0o644)
	137	mustGit(t, dir, env, "commit", "-q", "-a", "--amend", "-m", "notes v2")
136	138	mustGit(t, dir, env, "push", "-q", "--force", "origin", "feat2")
137	139	_, errOut, code = inst.ssh(t, aliceKey, "", "mr", "merge", "alice/svc", "2")
138	140	if code != 4 \|\| !strings.Contains(errOut, "requires 1 fresh approval") {

e2e/mr_test.go +4 −3

		@@ -108,9 +108,10 @@ func TestMergeRequests(t *testing.T) {
108	108	}
109	109	firstHead := show.HeadSHA
110	110
111		// Bob force-pushes the source branch: the MR head updates and the
112		// review goes stale.
113		mustGit(t, bobDir, bobEnv, "commit", "-q", "--amend", "-m", "add feature (amended)")
	111	// Bob force-pushes a changed diff: the MR head updates and the review
	112	// goes stale. (A force-push carrying the same diff keeps it, #198.)
	113	os.WriteFile(filepath.Join(bobDir, "feature.txt"), []byte("bob's work, amended\n"), 0o644)
	114	mustGit(t, bobDir, bobEnv, "commit", "-q", "-a", "--amend", "-m", "add feature (amended)")
114	115	mustGit(t, bobDir, bobEnv, "push", "-q", "--force", "origin", "feature")
115	116	show = inst.mrShow(t, aliceKey, "alice/lib", "1")
116	117	if show.HeadSHA == firstHead {

e2e/reviewcarry_test.go added +82

		@@ -0,0 +1,82 @@
	1	package e2e
	2
	3	import (
	4	"os"
	5	"path/filepath"
	6	"strings"
	7	"testing"
	8	)
	9
	10	// A rebase that leaves the merge request's diff unchanged keeps its
	11	// fresh approvals; a push that changes the diff stales them (#198).
	12	func TestApprovalsSurviveSameDiffRebase(t *testing.T) {
	13	inst := startInstance(t)
	14	aliceKey := inst.newKey(t, "alice")
	15	bobKey := inst.newKey(t, "bob")
	16	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
	17	inst.admin(t, "admin", "user", "create", "bob", "--key", bobKey+".pub")
	18	for _, args := range [][]string{
	19	{"repo", "create", "alice/app"},
	20	{"repo", "access", "grant", "alice/app", "bob", "write"},
	21	{"repo", "settings", "require-approvals", "alice/app", "1"},
	22	} {
	23	if _, errOut, code := inst.ssh(t, aliceKey, "", args...); code != 0 {
	24	t.Fatalf("%v: %s", args, errOut)
	25	}
	26	}
	27	env := inst.gitEnv(aliceKey)
	28	work := t.TempDir()
	29	mustGit(t, work, env, "clone", "-q", inst.sshURL("alice/app"), "w")
	30	dir := filepath.Join(work, "w")
	31	write := func(name, content string) {
	32	t.Helper()
	33	if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil {
	34	t.Fatal(err)
	35	}
	36	mustGit(t, dir, env, "add", name)
	37	mustGit(t, dir, env, "commit", "-q", "-m", name)
	38	}
	39	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	40	write("a.txt", "a\n")
	41	mustGit(t, dir, env, "push", "-q", "origin", "main")
	42
	43	mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
	44	write("b.txt", "b\n")
	45	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	46	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/app",
	47	"--source", "feat", "--target", "main", "--title", "'feat'"); code != 0 {
	48	t.Fatalf("mr create: %s", errOut)
	49	}
	50	if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "review", "alice/app", "1", "--approve"); code != 0 {
	51	t.Fatalf("approve: %s", errOut)
	52	}
	53
	54	// main moves on; the author rebases and force-pushes. Same diff.
	55	mustGit(t, dir, env, "checkout", "-q", "main")
	56	write("c.txt", "c\n")
	57	mustGit(t, dir, env, "push", "-q", "origin", "main")
	58	mustGit(t, dir, env, "checkout", "-q", "feat")
	59	mustGit(t, dir, env, "rebase", "-q", "main")
	60	mustGit(t, dir, env, "push", "-q", "--force", "origin", "feat")
	61	out, _, _ := inst.ssh(t, aliceKey, "", "mr", "show", "alice/app", "1", "--json")
	62	if !strings.Contains(out, `"reviewer":"bob","verdict":"approve","stale":false`) {
	63	t.Fatalf("approval went stale on a same-diff rebase:\n%s", out)
	64	}
	65
	66	// A push that changes the diff stales it, and the merge waits.
	67	write("b.txt", "b2\n")
	68	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	69	out, _, _ = inst.ssh(t, aliceKey, "", "mr", "show", "alice/app", "1", "--json")
	70	if !strings.Contains(out, `"reviewer":"bob","verdict":"approve","stale":true`) {
	71	t.Fatalf("approval survived a changed diff:\n%s", out)
	72	}
	73	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/app", "1"); code != 4 \|\| !strings.Contains(errOut, "fresh approval") {
	74	t.Fatalf("merge on a stale approval: %d %s", code, errOut)
	75	}
	76	if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "review", "alice/app", "1", "--approve"); code != 0 {
	77	t.Fatalf("second approve: %s", errOut)
	78	}
	79	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/app", "1", "--strategy", "ff"); code != 0 {
	80	t.Fatalf("merge after fresh approval: %s", errOut)
	81	}
	82	}

internal/gitutil/merge.go +29

		@@ -122,6 +122,35 @@ func cutAtLine(out []byte, limit int64) ([]byte, bool) {
122	122	return cut, true
123	123	}
124	124
	125	// PatchID identifies the change between old and new independently of the
	126	// commits carrying it: git patch-id --stable over the whole-range diff.
	127	// Two revisions with the same PatchID propose the same change, whatever
	128	// was rebased underneath. "" when the range has no diff.
	129	func PatchID(dir, old, new string) (string, error) {
	130	diff := exec.Command(toolpath.Look("git"), "-C", dir, "diff", "--end-of-options", old, new)
	131	pid := exec.Command(toolpath.Look("git"), "-C", dir, "patch-id", "--stable")
	132	pipe, err := diff.StdoutPipe()
	133	if err != nil {
	134	return "", err
	135	}
	136	pid.Stdin = pipe
	137	if err := diff.Start(); err != nil {
	138	return "", fmt.Errorf("diff: %w", err)
	139	}
	140	out, err := pid.Output()
	141	if werr := diff.Wait(); werr != nil {
	142	return "", fmt.Errorf("diff: %w", werr)
	143	}
	144	if err != nil {
	145	return "", fmt.Errorf("patch-id: %w", err)
	146	}
	147	fields := strings.Fields(string(out))
	148	if len(fields) == 0 {
	149	return "", nil
	150	}
	151	return fields[0], nil
	152	}
	153
125	154	// MergeBase returns the best common ancestor, or an error if none exists.
126	155	func MergeBase(dir, a, b string) (string, error) {
127	156	cmd := exec.Command(toolpath.Look("git"), "-C", dir, "merge-base", "--end-of-options", a, b)

internal/hookd/hookd.go +22 −1

		@@ -283,7 +283,7 @@ func (s *Server) postReceive(req Request) {
283	283	if err != nil {
284	284	base = ""
285	285	}
286		if err := s.st.UpdateMRHead(mr.ID, u.New, base); err != nil {
	286	if err := s.st.UpdateMRHead(mr.ID, u.New, base, sameChange(dstDir, mr, base, u.New)); err != nil {
287	287	slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err)
288	288	}
289	289	if srcRepo.ID != target.ID {
		@@ -326,6 +326,27 @@ func (s Server) adoptDefaultBranch(repo store.Repo, updates []policy.RefUpdate
326	326	}
327	327	}
328	328
	329	// sameChange reports whether the new head proposes the diff the old one
	330	// did: the patch-id of each revision against its own merge base. A
	331	// rebase onto a moved target changes every sha and nothing about the
	332	// change, and the reviews of it should not go stale for that (#198).
	333	// Any doubt answers false, which is the old behaviour.
	334	func sameChange(dir string, mr store.MR, newBase, newHead string) bool {
	335	if mr.HeadSHA == "" \|\| newBase == "" \|\| mr.HeadSHA == newHead {
	336	return false
	337	}
	338	oldBase, err := gitutil.MergeBase(dir, "refs/heads/"+mr.TargetRef, mr.HeadSHA)
	339	if err != nil {
	340	return false
	341	}
	342	oldID, err := gitutil.PatchID(dir, oldBase, mr.HeadSHA)
	343	if err != nil \|\| oldID == "" {
	344	return false
	345	}
	346	newID, err := gitutil.PatchID(dir, newBase, newHead)
	347	return err == nil && newID == oldID
	348	}
	349
329	350	// queueBuilds queues the push jobs for a branch update. The work is
330	351	// shared with the merge path, which moves a ref without reaching a hook.
331	352	func (s *Server) queueBuilds(repo store.Repo, userID int64, branch, old, sha string) {

internal/store/mrs.go +14 −3

		@@ -284,18 +284,29 @@ func (s *Store) SetMRState(mrID int64, state string) error {
284	284	return nil
285	285	}
286	286
287		// UpdateMRHead records a new head and marks every review at another head
288		// stale, in one transaction.
289	287	// UpdateMRHead moves a merge request onto a new head, stales the reviews
290	288	// of the old one, and records the head in the history a range-diff reads.
291	289	// baseSHA is the merge base at this moment; "" when the caller could not
292	290	// work it out, which only costs the range-diff its precision.
293		func (s *Store) UpdateMRHead(mrID int64, headSHA, baseSHA string) error {
	291	//
	292	// sameDiff says the new head proposes the change the old one did (a
	293	// rebase onto a moved target, or the same commits pushed again). Then the
	294	// fresh reviews of the old head are reviews of this diff and move to the
	295	// new head rather than going stale (#198). Reviews already stale stay so.
	296	func (s *Store) UpdateMRHead(mrID int64, headSHA, baseSHA string, sameDiff bool) error {
294	297	tx, err := s.DB.Begin()
295	298	if err != nil {
296	299	return err
297	300	}
298	301	defer tx.Rollback()
	302	if sameDiff {
	303	if _, err := tx.Exec(`
	304	UPDATE mr_reviews SET head_sha = ? WHERE mr_id = ? AND stale = 0
	305	AND head_sha = (SELECT head_sha FROM merge_requests WHERE id = ?)`,
	306	headSHA, mrID, mrID); err != nil {
	307	return err
	308	}
	309	}
299	310	if _, err := tx.Exec(
300	311	"UPDATE merge_requests SET head_sha = ?, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ','now') WHERE id = ?",
301	312	headSHA, mrID); err != nil {

internal/store/reviewcarry_test.go added +58

		@@ -0,0 +1,58 @@
	1	package store
	2
	3	import "testing"
	4
	5	// UpdateMRHead with sameDiff moves the fresh reviews of the old head to
	6	// the new one and leaves already-stale reviews stale (#198).
	7	func TestUpdateMRHeadSameDiffKeepsFreshReviews(t *testing.T) {
	8	s := open(t)
	9	if err := s.MigrateUp(); err != nil {
	10	t.Fatal(err)
	11	}
	12	alice, _ := s.CreateUser("alice", false)
	13	bob, _ := s.CreateUser("bob", false)
	14	carol, _ := s.CreateUser("carol", false)
	15	repo, _ := s.CreateRepo("user", alice, "app", "public")
	16	id, err := s.CreateMR(repo, alice, repo, "feat", "main", "t", "", "aaa", "md", false)
	17	if err != nil {
	18	t.Fatal(err)
	19	}
	20	// bob reviewed an earlier head and is stale; carol reviewed the
	21	// current one.
	22	if err := s.AddMRReview(id, bob, "approve", "000"); err != nil {
	23	t.Fatal(err)
	24	}
	25	if err := s.UpdateMRHead(id, "aaa", "", false); err != nil {
	26	t.Fatal(err)
	27	}
	28	if err := s.AddMRReview(id, carol, "approve", "aaa"); err != nil {
	29	t.Fatal(err)
	30	}
	31	if err := s.UpdateMRHead(id, "bbb", "", true); err != nil {
	32	t.Fatal(err)
	33	}
	34	reviews, err := s.ListMRReviews(id)
	35	if err != nil {
	36	t.Fatal(err)
	37	}
	38	got := map[string]MRReview{}
	39	for _, r := range reviews {
	40	got[r.Reviewer] = r
	41	}
	42	if r := got["carol"]; r.Stale \|\| r.HeadSHA != "bbb" {
	43	t.Errorf("fresh review did not follow the same diff: %+v", r)
	44	}
	45	if r := got["bob"]; !r.Stale \|\| r.HeadSHA != "000" {
	46	t.Errorf("stale review changed: %+v", r)
	47	}
	48	// A different diff stales everything, as before.
	49	if err := s.UpdateMRHead(id, "ccc", "", false); err != nil {
	50	t.Fatal(err)
	51	}
	52	reviews, _ = s.ListMRReviews(id)
	53	for _, r := range reviews {
	54	if !r.Stale {
	55	t.Errorf("review survived a changed diff: %+v", r)
	56	}
	57	}
	58	}

internal/store/reviewrequests_test.go +1 −1

		@@ -36,7 +36,7 @@ func TestReviewQueueRequestedReviewer(t *testing.T) {
36	36	t.Fatalf("queue after reviewing the current head: %+v, %v", q, err)
37	37	}
38	38
39		if err := s.UpdateMRHead(mr.ID, "def456", ""); err != nil {
	39	if err := s.UpdateMRHead(mr.ID, "def456", "", false); err != nil {
40	40	t.Fatal(err)
41	41	}
42	42	if q, err := s.ReviewQueue(reviewerID); err != nil \|\| len(q) != 1 {

Commit a5ee6a83a1

Commit `a5ee6a83a1`