krz/gitbay

gitbay: a rebase that keeps the diff keeps the approvals !342

merged merged by cmc on 2026-09-08 02:35 UTC · krz/gitbay:review-survives-rebase into main

10 files changed, +221 −13

Layout: unified · split

.gitbay/wiki/Stacked-MRs.org +3 −2
@@ -53,8 +53,9 @@ below it.
53 53
54Changing a lower layer is a rebase you do yourself. Amend =feat-a=, 54Changing a lower layer is a rebase you do yourself. Amend =feat-a=,
55then =git rebase feat-a= on =feat-b= and each layer above, and 55then =git rebase feat-a= on =feat-b= and each layer above, and
56force-push them. A force-push stales the reviews on that layer, the 56force-push them. A force-push stales the reviews on that layer when it
57same as on any merge request. The server never rewrites your commits: 57changes the layer's diff, the same as on any merge request; a rebase
58that carries the same change keeps them. The server never rewrites your commits:
58it holds no signing key, and a rebase it performed would land commits 59it holds no signing key, and a rebase it performed would land commits
59nobody signed. 60nobody signed.
60 61
.gitbay/wiki/Users.org +4 −1
@@ -378,7 +378,10 @@ Semantics worth knowing:
378 =refs/merge-requests/N/head= (fetchable by any reader), so an MR 378 =refs/merge-requests/N/head= (fetchable by any reader), so an MR
379 survives deletion of its source branch or fork. 379 survives deletion of its source branch or fork.
380- force-pushing the source updates the MR and marks existing reviews 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- default strategy: fast-forward when possible, else a merge commit. 385- default strategy: fast-forward when possible, else a merge commit.
383 Squash makes one commit authored by the MR author, committed by the 386 Squash makes one commit authored by the MR author, committed by the
384 merger. Rebase replays a linear range preserving authors; it refuses 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 t.Fatalf("fully gated merge: %s", errOut) 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 mustGit(t, dir, env, "fetch", "-q", "origin") 123 mustGit(t, dir, env, "fetch", "-q", "origin")
123 mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "origin/main") 124 mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "origin/main")
124 os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("n\n"), 0o644) 125 os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("n\n"), 0o644)
@@ -132,7 +133,8 @@ func TestMergeRequirements(t *testing.T) {
132 if _, _, code = inst.ssh(t, bobKey, "", "mr", "review", "alice/svc", "2", "--approve"); code != 0 { 133 if _, _, code = inst.ssh(t, bobKey, "", "mr", "review", "alice/svc", "2", "--approve"); code != 0 {
133 t.Fatal("bob approve 2 failed") 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 mustGit(t, dir, env, "push", "-q", "--force", "origin", "feat2") 138 mustGit(t, dir, env, "push", "-q", "--force", "origin", "feat2")
137 _, errOut, code = inst.ssh(t, aliceKey, "", "mr", "merge", "alice/svc", "2") 139 _, errOut, code = inst.ssh(t, aliceKey, "", "mr", "merge", "alice/svc", "2")
138 if code != 4 || !strings.Contains(errOut, "requires 1 fresh approval") { 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 firstHead := show.HeadSHA 109 firstHead := show.HeadSHA
110 110
111 // Bob force-pushes the source branch: the MR head updates and the 111 // Bob force-pushes a changed diff: the MR head updates and the review
112 // review goes stale. 112 // goes stale. (A force-push carrying the same diff keeps it, #198.)
113 mustGit(t, bobDir, bobEnv, "commit", "-q", "--amend", "-m", "add feature (amended)") 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 mustGit(t, bobDir, bobEnv, "push", "-q", "--force", "origin", "feature") 115 mustGit(t, bobDir, bobEnv, "push", "-q", "--force", "origin", "feature")
115 show = inst.mrShow(t, aliceKey, "alice/lib", "1") 116 show = inst.mrShow(t, aliceKey, "alice/lib", "1")
116 if show.HeadSHA == firstHead { 117 if show.HeadSHA == firstHead {
e2e/reviewcarry_test.go added +82
@@ -0,0 +1,82 @@
1package e2e
2
3import (
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).
12func 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 return cut, true 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.
129func 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// MergeBase returns the best common ancestor, or an error if none exists. 154// MergeBase returns the best common ancestor, or an error if none exists.
126func MergeBase(dir, a, b string) (string, error) { 155func MergeBase(dir, a, b string) (string, error) {
127 cmd := exec.Command(toolpath.Look("git"), "-C", dir, "merge-base", "--end-of-options", a, b) 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 if err != nil { 283 if err != nil {
284 base = "" 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 slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err) 287 slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err)
288 } 288 }
289 if srcRepo.ID != target.ID { 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.
334func 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// queueBuilds queues the push jobs for a branch update. The work is 350// queueBuilds queues the push jobs for a branch update. The work is
330// shared with the merge path, which moves a ref without reaching a hook. 351// shared with the merge path, which moves a ref without reaching a hook.
331func (s *Server) queueBuilds(repo store.Repo, userID int64, branch, old, sha string) { 352func (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 return nil 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// UpdateMRHead moves a merge request onto a new head, stales the reviews 287// UpdateMRHead moves a merge request onto a new head, stales the reviews
290// of the old one, and records the head in the history a range-diff reads. 288// of the old one, and records the head in the history a range-diff reads.
291// baseSHA is the merge base at this moment; "" when the caller could not 289// baseSHA is the merge base at this moment; "" when the caller could not
292// work it out, which only costs the range-diff its precision. 290// work it out, which only costs the range-diff its precision.
293func (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.
296func (s *Store) UpdateMRHead(mrID int64, headSHA, baseSHA string, sameDiff bool) error {
294 tx, err := s.DB.Begin() 297 tx, err := s.DB.Begin()
295 if err != nil { 298 if err != nil {
296 return err 299 return err
297 } 300 }
298 defer tx.Rollback() 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 if _, err := tx.Exec( 310 if _, err := tx.Exec(
300 "UPDATE merge_requests SET head_sha = ?, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ','now') WHERE id = ?", 311 "UPDATE merge_requests SET head_sha = ?, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ','now') WHERE id = ?",
301 headSHA, mrID); err != nil { 312 headSHA, mrID); err != nil {
internal/store/reviewcarry_test.go added +58
@@ -0,0 +1,58 @@
1package store
2
3import "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).
7func 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 t.Fatalf("queue after reviewing the current head: %+v, %v", q, err) 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 t.Fatal(err) 40 t.Fatal(err)
41 } 41 }
42 if q, err := s.ReviewQueue(reviewerID); err != nil || len(q) != 1 { 42 if q, err := s.ReviewQueue(reviewerID); err != nil || len(q) != 1 {