Commit e99285743d
Verified · cmc ci/build: success ci/test: success ci/vuln: success
Layout: unified · split
cmd/gitbay/main.go +2
| @@ -507,6 +507,8 @@ func mrCmd() *cobra.Command { | |||
| 507 | pass("review", "submit a review: --approve|--request-changes|--comment, or --discard a pending batch", passOpts{server: []string{"mr", "review"}, needsRepo: true}), | 507 | pass("review", "submit a review: --approve|--request-changes|--comment, or --discard a pending batch", passOpts{server: []string{"mr", "review"}, needsRepo: true}), |
| 508 | pass("merge", "merge: [--strategy ff|merge|squash|rebase]", passOpts{server: []string{"mr", "merge"}, needsRepo: true}), | 508 | pass("merge", "merge: [--strategy ff|merge|squash|rebase]", passOpts{server: []string{"mr", "merge"}, needsRepo: true}), |
| 509 | pass("close", "close without merging", passOpts{server: []string{"mr", "close"}, needsRepo: true}), | 509 | pass("close", "close without merging", passOpts{server: []string{"mr", "close"}, needsRepo: true}), |
| 510 | pass("revisions", "the heads this merge request has had", passOpts{server: []string{"mr", "revisions"}, needsRepo: true}), | ||
| 511 | pass("range-diff", "what changed between two revisions: [--from <sha>] [--to <sha>]", passOpts{server: []string{"mr", "range-diff"}, needsRepo: true}), | ||
| 510 | pass("draft", "mark as work in progress", passOpts{server: []string{"mr", "draft"}, needsRepo: true}), | 512 | pass("draft", "mark as work in progress", passOpts{server: []string{"mr", "draft"}, needsRepo: true}), |
| 511 | pass("ready", "take the draft mark off, so it can merge", passOpts{server: []string{"mr", "ready"}, needsRepo: true}), | 513 | pass("ready", "take the draft mark off, so it can merge", passOpts{server: []string{"mr", "ready"}, needsRepo: true}), |
| 512 | pass("edit", "edit title or body: <n> [--title <t>] [--body <b>|--file -]", passOpts{server: []string{"mr", "edit"}, needsRepo: true, stdinOK: true}), | 514 | pass("edit", "edit title or body: <n> [--title <t>] [--body <b>|--file -]", passOpts{server: []string{"mr", "edit"}, needsRepo: true, stdinOK: true}), |
e2e/rangediff_test.go added +124
| @@ -0,0 +1,124 @@ | |||
| 1 | package e2e | ||
| 2 | |||
| 3 | import ( | ||
| 4 | "encoding/json" | ||
| 5 | "os" | ||
| 6 | "path/filepath" | ||
| 7 | "strings" | ||
| 8 | "testing" | ||
| 9 | ) | ||
| 10 | |||
| 11 | // TestMRRangeDiff is #111's last stage: a push stales every review and | ||
| 12 | // nothing said what had changed between the two heads. A plain diff of | ||
| 13 | // the heads cannot answer that — it shows the whole branch again. | ||
| 14 | func TestMRRangeDiff(t *testing.T) { | ||
| 15 | inst := startInstance(t) | ||
| 16 | key := inst.newKey(t, "alice") | ||
| 17 | inst.admin(t, "admin", "user", "create", "alice", "--key", key+".pub") | ||
| 18 | if _, errOut, code := inst.ssh(t, key, "", "repo", "create", "alice/app"); code != 0 { | ||
| 19 | t.Fatalf("repo create: %s", errOut) | ||
| 20 | } | ||
| 21 | env := inst.gitEnv(key) | ||
| 22 | work := t.TempDir() | ||
| 23 | mustGit(t, work, env, "clone", inst.sshURL("alice/app"), "w") | ||
| 24 | dir := filepath.Join(work, "w") | ||
| 25 | os.WriteFile(filepath.Join(dir, "a.txt"), []byte("one\n"), 0o644) | ||
| 26 | mustGit(t, dir, env, "checkout", "-q", "-b", "main") | ||
| 27 | mustGit(t, dir, env, "add", ".") | ||
| 28 | mustGit(t, dir, env, "commit", "-q", "-m", "base") | ||
| 29 | mustGit(t, dir, env, "push", "-q", "origin", "main") | ||
| 30 | |||
| 31 | mustGit(t, dir, env, "checkout", "-q", "-b", "feat") | ||
| 32 | os.WriteFile(filepath.Join(dir, "b.txt"), []byte("alpha\nbeta\ngamma\n"), 0o644) | ||
| 33 | mustGit(t, dir, env, "add", ".") | ||
| 34 | mustGit(t, dir, env, "commit", "-q", "-m", "add b") | ||
| 35 | mustGit(t, dir, env, "push", "-q", "origin", "feat") | ||
| 36 | if _, errOut, code := inst.ssh(t, key, "", "mr", "create", "alice/app", | ||
| 37 | "--source", "feat", "--target", "main", "--title", "'add b'"); code != 0 { | ||
| 38 | t.Fatalf("mr create: %s", errOut) | ||
| 39 | } | ||
| 40 | |||
| 41 | // One revision: nothing to compare, and saying so is not a failure. | ||
| 42 | out, errOut, code := inst.ssh(t, key, "", "mr", "range-diff", "alice/app", "1") | ||
| 43 | if code != 0 || !strings.Contains(errOut, "one revision") { | ||
| 44 | t.Fatalf("single-revision range-diff: exit %d, %s, %s", code, errOut, out) | ||
| 45 | } | ||
| 46 | |||
| 47 | // Amend and force-push: the same commit with one line changed, which | ||
| 48 | // is what addressing review feedback looks like. A diff of the two | ||
| 49 | // heads cannot describe this — it shows the whole branch again. | ||
| 50 | os.WriteFile(filepath.Join(dir, "b.txt"), []byte("alpha\nbeta revised\ngamma\n"), 0o644) | ||
| 51 | mustGit(t, dir, env, "add", ".") | ||
| 52 | mustGit(t, dir, env, "commit", "-q", "--amend", "--no-edit") | ||
| 53 | mustGit(t, dir, env, "push", "-q", "--force", "origin", "feat") | ||
| 54 | |||
| 55 | revs := revisions(t, inst, key) | ||
| 56 | if len(revs) != 2 { | ||
| 57 | t.Fatalf("revisions = %d, want 2 after a force-push: %+v", len(revs), revs) | ||
| 58 | } | ||
| 59 | if !revs[1].Current { | ||
| 60 | t.Fatalf("the newest revision is not marked current: %+v", revs) | ||
| 61 | } | ||
| 62 | |||
| 63 | out, errOut, code = inst.ssh(t, key, "", "mr", "range-diff", "alice/app", "1") | ||
| 64 | if code != 0 { | ||
| 65 | t.Fatalf("range-diff: %s", errOut) | ||
| 66 | } | ||
| 67 | // The two versions of the one commit are paired — "1: <old> ! 1: | ||
| 68 | // <new>" — and the interdiff shows the one line that moved, not the | ||
| 69 | // whole branch. | ||
| 70 | if !strings.Contains(out, "add b") { | ||
| 71 | t.Fatalf("range-diff does not mention the commit:\n%s", out) | ||
| 72 | } | ||
| 73 | if !strings.Contains(out, "!") { | ||
| 74 | t.Fatalf("range-diff did not pair the amended commit:\n%s", out) | ||
| 75 | } | ||
| 76 | if !strings.Contains(out, "beta revised") { | ||
| 77 | t.Fatalf("range-diff does not show the changed line:\n%s", out) | ||
| 78 | } | ||
| 79 | // One commit on each side, paired: a plain diff of the two heads | ||
| 80 | // would instead show b.txt created from nothing all over again. | ||
| 81 | if strings.Count(out, "add b") != 1 { | ||
| 82 | t.Fatalf("range-diff lists the commit more than once:\n%s", out) | ||
| 83 | } | ||
| 84 | |||
| 85 | // Naming revisions explicitly, and refusing one that is not a | ||
| 86 | // revision of this merge request. | ||
| 87 | if _, _, code := inst.ssh(t, key, "", "mr", "range-diff", "alice/app", "1", | ||
| 88 | "--from", revs[0].SHA, "--to", revs[1].SHA); code != 0 { | ||
| 89 | t.Fatal("explicit --from/--to failed") | ||
| 90 | } | ||
| 91 | if _, errOut, code := inst.ssh(t, key, "", "mr", "range-diff", "alice/app", "1", | ||
| 92 | "--from", "0123456789ab"); code != 3 || !strings.Contains(errOut, "not a revision") { | ||
| 93 | t.Fatalf("unknown revision: exit %d, %s", code, errOut) | ||
| 94 | } | ||
| 95 | if _, _, code := inst.ssh(t, key, "", "mr", "range-diff", "alice/app", "1", | ||
| 96 | "--from", revs[1].SHA, "--to", revs[1].SHA); code != 2 { | ||
| 97 | t.Fatal("comparing a revision with itself was accepted") | ||
| 98 | } | ||
| 99 | |||
| 100 | // A push that changes nothing does not add a revision. | ||
| 101 | mustGit(t, dir, env, "push", "-q", "--force", "origin", "feat") | ||
| 102 | if got := revisions(t, inst, key); len(got) != 2 { | ||
| 103 | t.Fatalf("a no-op push added a revision: %d", len(got)) | ||
| 104 | } | ||
| 105 | } | ||
| 106 | |||
| 107 | type revision struct { | ||
| 108 | N int `json:"n"` | ||
| 109 | SHA string `json:"sha"` | ||
| 110 | Current bool `json:"current"` | ||
| 111 | } | ||
| 112 | |||
| 113 | func revisions(t *testing.T, inst *instance, key string) []revision { | ||
| 114 | t.Helper() | ||
| 115 | out, errOut, code := inst.ssh(t, key, "", "mr", "revisions", "alice/app", "1", "--json") | ||
| 116 | if code != 0 { | ||
| 117 | t.Fatalf("mr revisions: %s", errOut) | ||
| 118 | } | ||
| 119 | var env struct { | ||
| 120 | Data []revision `json:"data"` | ||
| 121 | } | ||
| 122 | json.Unmarshal([]byte(out), &env) | ||
| 123 | return env.Data | ||
| 124 | } | ||
e2e/readonly_test.go +2
| @@ -144,6 +144,8 @@ func TestReadOnlyCommandsWriteNothing(t *testing.T) { | |||
| 144 | "release asset get": {"alice/app", "v1", "a.txt"}, | 144 | "release asset get": {"alice/app", "v1", "a.txt"}, |
| 145 | "notifications list": nil, | 145 | "notifications list": nil, |
| 146 | "search": {"app"}, | 146 | "search": {"app"}, |
| 147 | "mr revisions": {"alice/app", "1"}, | ||
| 148 | "mr range-diff": {"alice/app", "1"}, | ||
| 147 | "webhook list": {"alice/app"}, | 149 | "webhook list": {"alice/app"}, |
| 148 | "webhook deliveries": {"alice/app"}, | 150 | "webhook deliveries": {"alice/app"}, |
| 149 | "wiki list": {"alice/app"}, | 151 | "wiki list": {"alice/app"}, |
internal/control/mr.go +136
| @@ -39,6 +39,14 @@ func init() { | |||
| 39 | Summary: "open a merge request", | 39 | Summary: "open a merge request", |
| 40 | Usage: "mr create <target owner/name> --source [owner/name:]<branch> --target <branch> --title <t> [--body <b> | --file -] [--format md|org] [--draft]", | 40 | Usage: "mr create <target owner/name> --source [owner/name:]<branch> --target <branch> --title <t> [--body <b> | --file -] [--format md|org] [--draft]", |
| 41 | ReadsStdin: true, Run: runMRCreate}) | 41 | ReadsStdin: true, Run: runMRCreate}) |
| 42 | register(Command{Path: []string{"mr", "range-diff"}, | ||
| 43 | Summary: "what changed between two revisions of a merge request", | ||
| 44 | Usage: "mr range-diff <owner/name> <n> [--from <sha>] [--to <sha>]", | ||
| 45 | ReadOnly: true, Run: runMRRangeDiff}) | ||
| 46 | register(Command{Path: []string{"mr", "revisions"}, | ||
| 47 | Summary: "the heads a merge request has had", | ||
| 48 | Usage: "mr revisions <owner/name> <n>", | ||
| 49 | ReadOnly: true, Run: runMRRevisions}) | ||
| 42 | register(Command{Path: []string{"mr", "draft"}, | 50 | register(Command{Path: []string{"mr", "draft"}, |
| 43 | Summary: "mark a merge request as work in progress", | 51 | Summary: "mark a merge request as work in progress", |
| 44 | Usage: "mr draft <owner/name> <n>", Run: runMRDraft}) | 52 | Usage: "mr draft <owner/name> <n>", Run: runMRDraft}) |
| @@ -1306,3 +1314,131 @@ func reviewAction(number int64, verdict string, published int64) string { | |||
| 1306 | } | 1314 | } |
| 1307 | return fmt.Sprintf("reviewed !%d: %s", number, verdict) | 1315 | return fmt.Sprintf("reviewed !%d: %s", number, verdict) |
| 1308 | } | 1316 | } |
| 1317 | |||
| 1318 | // RevisionOut is one head a merge request has had. | ||
| 1319 | type RevisionOut struct { | ||
| 1320 | N int `json:"n"` // 1 is the first push | ||
| 1321 | SHA string `json:"sha"` | ||
| 1322 | BaseSHA string `json:"base_sha,omitempty"` | ||
| 1323 | CreatedAt string `json:"created_at"` | ||
| 1324 | Current bool `json:"current,omitempty"` | ||
| 1325 | } | ||
| 1326 | |||
| 1327 | func mrRevisions(c *Ctx, mr store.MR) ([]RevisionOut, error) { | ||
| 1328 | heads, err := c.Store.MRHeads(mr.ID) | ||
| 1329 | if err != nil { | ||
| 1330 | return nil, err | ||
| 1331 | } | ||
| 1332 | out := make([]RevisionOut, 0, len(heads)) | ||
| 1333 | for i, h := range heads { | ||
| 1334 | out = append(out, RevisionOut{N: i + 1, SHA: h.SHA, BaseSHA: h.BaseSHA, | ||
| 1335 | CreatedAt: h.CreatedAt, Current: h.SHA == mr.HeadSHA}) | ||
| 1336 | } | ||
| 1337 | return out, nil | ||
| 1338 | } | ||
| 1339 | |||
| 1340 | func runMRRevisions(c *Ctx, args []string) int { | ||
| 1341 | repo, mr, code := mrRef(c, args, policy.CanRead) | ||
| 1342 | if code >= 0 { | ||
| 1343 | return code | ||
| 1344 | } | ||
| 1345 | if len(args) != 2 { | ||
| 1346 | return c.fail(protocol.ExitUsage, "usage: mr revisions <owner/name> <n>") | ||
| 1347 | } | ||
| 1348 | revs, err := mrRevisions(c, mr) | ||
| 1349 | if err != nil { | ||
| 1350 | return c.fail(protocol.ExitFailure, "%v", err) | ||
| 1351 | } | ||
| 1352 | return c.emit(revs, func(w io.Writer) { | ||
| 1353 | for _, r := range revs { | ||
| 1354 | mark := " " | ||
| 1355 | if r.Current { | ||
| 1356 | mark = "*" | ||
| 1357 | } | ||
| 1358 | fmt.Fprintf(w, "%s v%d\t%.10s\t%s\n", mark, r.N, r.SHA, r.CreatedAt) | ||
| 1359 | } | ||
| 1360 | if len(revs) < 2 { | ||
| 1361 | fmt.Fprintf(w, "\nonly one revision; %s!%d has not been pushed to since it was opened\n", | ||
| 1362 | repo.Path(), mr.Number) | ||
| 1363 | } | ||
| 1364 | }) | ||
| 1365 | } | ||
| 1366 | |||
| 1367 | func runMRRangeDiff(c *Ctx, args []string) int { | ||
| 1368 | const usage = "mr range-diff <owner/name> <n> [--from <sha>] [--to <sha>]" | ||
| 1369 | f, err := parseFlags(args, flagSpec{Values: []string{"--from", "--to"}, MaxPos: 2, Usage: usage}) | ||
| 1370 | if err != nil { | ||
| 1371 | return c.fail(protocol.ExitUsage, "%v", err) | ||
| 1372 | } | ||
| 1373 | repo, mr, code := mrRef(c, f.Pos, policy.CanRead) | ||
| 1374 | if code >= 0 { | ||
| 1375 | return code | ||
| 1376 | } | ||
| 1377 | if len(f.Pos) != 2 { | ||
| 1378 | return c.fail(protocol.ExitUsage, "usage: %s", usage) | ||
| 1379 | } | ||
| 1380 | revs, err := mrRevisions(c, mr) | ||
| 1381 | if err != nil { | ||
| 1382 | return c.fail(protocol.ExitFailure, "%v", err) | ||
| 1383 | } | ||
| 1384 | // One revision is a merge request nobody has pushed to since it was | ||
| 1385 | // opened. The argv was fine and the answer is "nothing changed", so | ||
| 1386 | // this succeeds with an empty patch rather than failing. | ||
| 1387 | if len(revs) < 2 { | ||
| 1388 | fmt.Fprintf(c.Stderr, "%s!%d has one revision; nothing to compare it against\n", | ||
| 1389 | repo.Path(), mr.Number) | ||
| 1390 | return protocol.ExitOK | ||
| 1391 | } | ||
| 1392 | // Default to the two most recent, which is "what changed since the | ||
| 1393 | // last push" — the question a stale review asks. | ||
| 1394 | from, to := revs[len(revs)-2], revs[len(revs)-1] | ||
| 1395 | pick := func(sha string) (RevisionOut, bool) { | ||
| 1396 | for _, r := range revs { | ||
| 1397 | if strings.HasPrefix(r.SHA, sha) { | ||
| 1398 | return r, true | ||
| 1399 | } | ||
| 1400 | } | ||
| 1401 | return RevisionOut{}, false | ||
| 1402 | } | ||
| 1403 | if v := f.Value("--from"); v != "" { | ||
| 1404 | r, ok := pick(v) | ||
| 1405 | if !ok { | ||
| 1406 | return c.fail(protocol.ExitNotFound, "%.12s is not a revision of !%d; see `mr revisions`", v, mr.Number) | ||
| 1407 | } | ||
| 1408 | from = r | ||
| 1409 | } | ||
| 1410 | if v := f.Value("--to"); v != "" { | ||
| 1411 | r, ok := pick(v) | ||
| 1412 | if !ok { | ||
| 1413 | return c.fail(protocol.ExitNotFound, "%.12s is not a revision of !%d; see `mr revisions`", v, mr.Number) | ||
| 1414 | } | ||
| 1415 | to = r | ||
| 1416 | } | ||
| 1417 | if from.SHA == to.SHA { | ||
| 1418 | return c.fail(protocol.ExitUsage, "--from and --to are the same revision") | ||
| 1419 | } | ||
| 1420 | |||
| 1421 | dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name) | ||
| 1422 | // A revision recorded before its base could be worked out, or by a | ||
| 1423 | // migration backfill, falls back to the target's merge base. | ||
| 1424 | baseOf := func(r RevisionOut) string { | ||
| 1425 | if r.BaseSHA != "" { | ||
| 1426 | return r.BaseSHA | ||
| 1427 | } | ||
| 1428 | b, err := gitutil.MergeBase(dir, "refs/heads/"+mr.TargetRef, r.SHA) | ||
| 1429 | if err != nil { | ||
| 1430 | return r.SHA + "^" | ||
| 1431 | } | ||
| 1432 | return b | ||
| 1433 | } | ||
| 1434 | patch, truncated, err := gitutil.RangeDiff(dir, baseOf(from), from.SHA, baseOf(to), to.SHA, 4<<20) | ||
| 1435 | if err != nil { | ||
| 1436 | return c.fail(protocol.ExitFailure, | ||
| 1437 | "%v (the objects for an older revision may have been garbage-collected)", err) | ||
| 1438 | } | ||
| 1439 | fmt.Fprint(c.Stdout, patch) | ||
| 1440 | if truncated { | ||
| 1441 | fmt.Fprintln(c.Stderr, "range-diff truncated at 4 MiB") | ||
| 1442 | } | ||
| 1443 | return protocol.ExitOK | ||
| 1444 | } | ||
internal/gitutil/merge.go +29
| @@ -284,3 +284,32 @@ func DiffFiles(dir, old, new string) ([]string, error) { | |||
| 284 | } | 284 | } |
| 285 | return files, nil | 285 | return files, nil |
| 286 | } | 286 | } |
| 287 | |||
| 288 | // RangeDiff compares two revisions of the same work: what the commits | ||
| 289 | // between oldBase and oldHead became between newBase and newHead. This is | ||
| 290 | // what answers "what changed since I reviewed this", which a plain diff | ||
| 291 | // of the two heads cannot — that shows the whole branch again, rebases | ||
| 292 | // and all. | ||
| 293 | // | ||
| 294 | // Each side carries its own base, because the target moves: comparing | ||
| 295 | // both revisions against today's base would attribute every commit that | ||
| 296 | // landed on the target in between to the author of this merge request. | ||
| 297 | // | ||
| 298 | // --creation-factor is raised from git's default of 60. That default is | ||
| 299 | // tuned for comparing two independently developed patch series, where | ||
| 300 | // refusing to pair is the safe answer. Here the two sides are known to be | ||
| 301 | // revisions of one branch, and the commonest revision of all — a commit | ||
| 302 | // that adds a file, with one line inside it changed — is not paired at | ||
| 303 | // 60: git reports the commit as deleted and a different one added, which | ||
| 304 | // tells a reviewer nothing. It pairs at 80, and two genuinely unrelated | ||
| 305 | // commits are still left unpaired there; both measured. | ||
| 306 | func RangeDiff(dir, oldBase, oldHead, newBase, newHead string, limit int64) (patch string, truncated bool, err error) { | ||
| 307 | cmd := exec.Command("git", "-C", dir, "range-diff", "--creation-factor=80", "--end-of-options", | ||
| 308 | oldBase+".."+oldHead, newBase+".."+newHead) | ||
| 309 | out, err := cmd.Output() | ||
| 310 | if err != nil { | ||
| 311 | return "", false, fmt.Errorf("range-diff: %w", err) | ||
| 312 | } | ||
| 313 | out, truncated = cutAtLine(out, limit) | ||
| 314 | return string(out), truncated, nil | ||
| 315 | } | ||
internal/hookd/hookd.go +9 −1
| @@ -247,7 +247,15 @@ func (s *Server) postReceive(req Request) { | |||
| 247 | slog.Error("post-receive: refreshing MR head", "mr", mr.Number, "err", err) | 247 | slog.Error("post-receive: refreshing MR head", "mr", mr.Number, "err", err) |
| 248 | continue | 248 | continue |
| 249 | } | 249 | } |
| 250 | if err := s.st.UpdateMRHead(mr.ID, u.New); err != nil { | 250 | // The merge base as it stands now, so a later range-diff |
| 251 | // compares each revision against the target it was written | ||
| 252 | // on rather than against today's. Best-effort: a base that | ||
| 253 | // cannot be worked out costs precision, not the record. | ||
| 254 | base, err := gitutil.MergeBase(dstDir, "refs/heads/"+mr.TargetRef, headRef) | ||
| 255 | if err != nil { | ||
| 256 | base = "" | ||
| 257 | } | ||
| 258 | if err := s.st.UpdateMRHead(mr.ID, u.New, base); err != nil { | ||
| 251 | slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err) | 259 | slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err) |
| 252 | } | 260 | } |
| 253 | if srcRepo.ID != target.ID { | 261 | if srcRepo.ID != target.ID { |
internal/httpd/mrpage_test.go +1
| @@ -28,6 +28,7 @@ type mrPageData struct { | |||
| 28 | CanEdit bool | 28 | CanEdit bool |
| 29 | CanWrite bool | 29 | CanWrite bool |
| 30 | Unresolved int | 30 | Unresolved int |
| 31 | Revisions []store.MRHead | ||
| 31 | Notice string | 32 | Notice string |
| 32 | DetachedThreads []diffThread | 33 | DetachedThreads []diffThread |
| 33 | } | 34 | } |
internal/httpd/web.go +6 −1
| @@ -1805,6 +1805,10 @@ func (s *Server) mr(w http.ResponseWriter, r *http.Request) { | |||
| 1805 | // its own view rather than a fold at the foot of the conversation. | 1805 | // its own view rather than a fold at the foot of the conversation. |
| 1806 | // A query parameter keeps this working without JavaScript. | 1806 | // A query parameter keeps this working without JavaScript. |
| 1807 | unresolved, _ := s.st.UnresolvedThreadCount(m.ID) | 1807 | unresolved, _ := s.st.UnresolvedThreadCount(m.ID) |
| 1808 | // The revisions this merge request has had. A stale review is the | ||
| 1809 | // moment someone wants to know what moved, so the link to the | ||
| 1810 | // range-diff belongs next to it. | ||
| 1811 | revisions, _ := s.st.MRHeads(m.ID) | ||
| 1808 | branches, _ := gitutil.Refs(p.Dir, "heads") | 1812 | branches, _ := gitutil.Refs(p.Dir, "heads") |
| 1809 | view := r.URL.Query().Get("view") | 1813 | view := r.URL.Query().Get("view") |
| 1810 | if view != "commits" && view != "diff" { | 1814 | if view != "commits" && view != "diff" { |
| @@ -1839,13 +1843,14 @@ func (s *Server) mr(w http.ResponseWriter, r *http.Request) { | |||
| 1839 | CanEdit bool | 1843 | CanEdit bool |
| 1840 | CanWrite bool | 1844 | CanWrite bool |
| 1841 | Unresolved int | 1845 | Unresolved int |
| 1846 | Revisions []store.MRHead | ||
| 1842 | Notice string | 1847 | Notice string |
| 1843 | DetachedThreads []diffThread | 1848 | DetachedThreads []diffThread |
| 1844 | StackedOn *store.MR | 1849 | StackedOn *store.MR |
| 1845 | Stacked []store.MR | 1850 | Stacked []store.MR |
| 1846 | }{p, m, view, md(m.Body, m.BodyFormat), checks, combined, renderComments(comments, md), | 1851 | }{p, m, view, md(m.Body, m.BodyFormat), checks, combined, renderComments(comments, md), |
| 1847 | reviews, files, diffTruncated, stat, commits, commitsTotal, branches, s.canEditItem(r, p.Repo, m.Author), | 1852 | reviews, files, diffTruncated, stat, commits, commitsTotal, branches, s.canEditItem(r, p.Repo, m.Author), |
| 1848 | canWrite, unresolved, s.takeFlash(w, r), detachedThreads, stackedOn, stacked}) | 1853 | canWrite, unresolved, revisions, s.takeFlash(w, r), detachedThreads, stackedOn, stacked}) |
| 1849 | } | 1854 | } |
| 1850 | 1855 | ||
| 1851 | func (s *Server) refs(w http.ResponseWriter, r *http.Request) { | 1856 | func (s *Server) refs(w http.ResponseWriter, r *http.Request) { |
internal/store/migrations/0039_mr_head_history.down.sql added +1
| @@ -0,0 +1 @@ | |||
| 1 | DROP TABLE mr_heads; | ||
internal/store/migrations/0039_mr_head_history.up.sql added +22
| @@ -0,0 +1,22 @@ | |||
| 1 | -- Every push to a merge request's source stales its reviews, and nothing | ||
| 2 | -- said what had changed between the two heads. Answering that needs the | ||
| 3 | -- head it used to be, which nothing kept: merge_requests.head_sha is | ||
| 4 | -- overwritten in place (#111). | ||
| 5 | -- | ||
| 6 | -- One row per head a merge request has had, oldest first by id. The base | ||
| 7 | -- recorded alongside is the merge base at that moment, so a range-diff | ||
| 8 | -- compares like with like even when the target moved underneath. | ||
| 9 | CREATE TABLE mr_heads ( | ||
| 10 | id INTEGER PRIMARY KEY, | ||
| 11 | mr_id INTEGER NOT NULL REFERENCES merge_requests(id) ON DELETE CASCADE, | ||
| 12 | sha TEXT NOT NULL, | ||
| 13 | base_sha TEXT NOT NULL DEFAULT '', | ||
| 14 | created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')) | ||
| 15 | ); | ||
| 16 | CREATE INDEX mr_heads_mr ON mr_heads(mr_id, id); | ||
| 17 | |||
| 18 | -- The current head of every existing merge request, so one that has never | ||
| 19 | -- been force-pushed since this migration still has a first entry to | ||
| 20 | -- measure from. | ||
| 21 | INSERT INTO mr_heads (mr_id, sha, base_sha) | ||
| 22 | SELECT id, head_sha, COALESCE(merged_base, '') FROM merge_requests WHERE head_sha <> ''; | ||
internal/store/mrs.go +51 −1
| @@ -60,6 +60,16 @@ func (s *Store) CreateMR(repoID, authorID, sourceRepoID int64, sourceRef, target | |||
| 60 | repoID, n, authorID, sourceRepoID, sourceRef, targetRef, title, body, headSHA, format, draft); err != nil { | 60 | repoID, n, authorID, sourceRepoID, sourceRef, targetRef, title, body, headSHA, format, draft); err != nil { |
| 61 | return 0, err | 61 | return 0, err |
| 62 | } | 62 | } |
| 63 | if headSHA != "" { | ||
| 64 | var mrID int64 | ||
| 65 | if err := tx.QueryRow("SELECT id FROM merge_requests WHERE repo_id = ? AND number = ?", | ||
| 66 | repoID, n).Scan(&mrID); err != nil { | ||
| 67 | return 0, err | ||
| 68 | } | ||
| 69 | if _, err := tx.Exec("INSERT INTO mr_heads (mr_id, sha) VALUES (?, ?)", mrID, headSHA); err != nil { | ||
| 70 | return 0, err | ||
| 71 | } | ||
| 72 | } | ||
| 63 | return n, tx.Commit() | 73 | return n, tx.Commit() |
| 64 | } | 74 | } |
| 65 | 75 | ||
| @@ -241,7 +251,11 @@ func (s *Store) SetMRState(mrID int64, state string) error { | |||
| 241 | 251 | ||
| 242 | // UpdateMRHead records a new head and marks every review at another head | 252 | // UpdateMRHead records a new head and marks every review at another head |
| 243 | // stale, in one transaction. | 253 | // stale, in one transaction. |
| 244 | func (s *Store) UpdateMRHead(mrID int64, headSHA string) error { | 254 | // UpdateMRHead moves a merge request onto a new head, stales the reviews |
| 255 | // of the old one, and records the head in the history a range-diff reads. | ||
| 256 | // baseSHA is the merge base at this moment; "" when the caller could not | ||
| 257 | // work it out, which only costs the range-diff its precision. | ||
| 258 | func (s *Store) UpdateMRHead(mrID int64, headSHA, baseSHA string) error { | ||
| 245 | tx, err := s.DB.Begin() | 259 | tx, err := s.DB.Begin() |
| 246 | if err != nil { | 260 | if err != nil { |
| 247 | return err | 261 | return err |
| @@ -256,9 +270,45 @@ func (s *Store) UpdateMRHead(mrID int64, headSHA string) error { | |||
| 256 | "UPDATE mr_reviews SET stale = 1 WHERE mr_id = ? AND head_sha <> ?", mrID, headSHA); err != nil { | 270 | "UPDATE mr_reviews SET stale = 1 WHERE mr_id = ? AND head_sha <> ?", mrID, headSHA); err != nil { |
| 257 | return err | 271 | return err |
| 258 | } | 272 | } |
| 273 | // Same head twice is a push that changed nothing about this merge | ||
| 274 | // request; it should not add a revision to compare against. | ||
| 275 | var last string | ||
| 276 | tx.QueryRow("SELECT sha FROM mr_heads WHERE mr_id = ? ORDER BY id DESC LIMIT 1", mrID).Scan(&last) | ||
| 277 | if last != headSHA { | ||
| 278 | if _, err := tx.Exec( | ||
| 279 | "INSERT INTO mr_heads (mr_id, sha, base_sha) VALUES (?, ?, ?)", mrID, headSHA, baseSHA); err != nil { | ||
| 280 | return err | ||
| 281 | } | ||
| 282 | } | ||
| 259 | return tx.Commit() | 283 | return tx.Commit() |
| 260 | } | 284 | } |
| 261 | 285 | ||
| 286 | // MRHead is one revision a merge request has had. | ||
| 287 | type MRHead struct { | ||
| 288 | SHA string | ||
| 289 | BaseSHA string | ||
| 290 | CreatedAt string | ||
| 291 | } | ||
| 292 | |||
| 293 | // MRHeads returns a merge request's revisions, oldest first. | ||
| 294 | func (s *Store) MRHeads(mrID int64) ([]MRHead, error) { | ||
| 295 | rows, err := s.DB.Query( | ||
| 296 | "SELECT sha, base_sha, created_at FROM mr_heads WHERE mr_id = ? ORDER BY id", mrID) | ||
| 297 | if err != nil { | ||
| 298 | return nil, err | ||
| 299 | } | ||
| 300 | defer rows.Close() | ||
| 301 | var out []MRHead | ||
| 302 | for rows.Next() { | ||
| 303 | var h MRHead | ||
| 304 | if err := rows.Scan(&h.SHA, &h.BaseSHA, &h.CreatedAt); err != nil { | ||
| 305 | return nil, err | ||
| 306 | } | ||
| 307 | out = append(out, h) | ||
| 308 | } | ||
| 309 | return out, rows.Err() | ||
| 310 | } | ||
| 311 | |||
| 262 | // SetMRTarget retargets a merge request and marks every existing review | 312 | // SetMRTarget retargets a merge request and marks every existing review |
| 263 | // stale, in one transaction. The base of the diff is derived from the | 313 | // stale, in one transaction. The base of the diff is derived from the |
| 264 | // target on every read, so nothing else has to move; an approval, | 314 | // target on every read, so nothing else has to move; an approval, |
internal/web/templates/mr.html +2
| @@ -130,6 +130,8 @@ | |||
| 130 | <h2>Reviews</h2> | 130 | <h2>Reviews</h2> |
| 131 | {{range .Reviews}}<p class="row"><span class="dot {{if eq .Verdict "approve"}}ok{{else}}pend{{end}}"></span><a href="/{{.Reviewer}}">{{.Reviewer}}</a> {{.Verdict}}{{if .Stale}} <span class="chip chip-stale">stale</span>{{end}}<span class="sub">{{when .CreatedAt}}</span></p> | 131 | {{range .Reviews}}<p class="row"><span class="dot {{if eq .Verdict "approve"}}ok{{else}}pend{{end}}"></span><a href="/{{.Reviewer}}">{{.Reviewer}}</a> {{.Verdict}}{{if .Stale}} <span class="chip chip-stale">stale</span>{{end}}<span class="sub">{{when .CreatedAt}}</span></p> |
| 132 | {{else}}<p class="none">No reviews yet</p>{{end}} | 132 | {{else}}<p class="none">No reviews yet</p>{{end}} |
| 133 | {{if gt (len .Revisions) 1}}<p class="row none">{{len .Revisions}} revisions pushed. What changed between the last two: | ||
| 134 | <code>gitbay mr range-diff {{.Repo.OwnerName}}/{{.Repo.Name}} {{.MR.Number}}</code></p>{{end}} | ||
| 133 | </div> | 135 | </div> |
| 134 | <div class="grp"> | 136 | <div class="grp"> |
| 135 | <h2>Checks</h2> | 137 | <h2>Checks</h2> |