Commit 93b38a063b
Verified · cmc ci/build: success
cmd/gitbay/main.go +1
| @@ -395,6 +395,7 @@ func mrCmd() *cobra.Command { | ||
| 395 | 395 | pass("close", "close without merging", passOpts{server: []string{"mr", "close"}, needsRepo: true}), |
| 396 | 396 | pass("edit", "edit title or body: <n> [--title <t>] [--body <b>|--file -]", passOpts{server: []string{"mr", "edit"}, needsRepo: true, stdinOK: true}), |
| 397 | 397 | pass("milestone", "set or clear the milestone: <n> <title|none>", passOpts{server: []string{"mr", "milestone"}, needsRepo: true}), |
| 398 | pass("retarget", "retarget onto another branch: <n> <branch>", passOpts{server: []string{"mr", "retarget"}, needsRepo: true}), | |
| 398 | 399 | ) |
| 399 | 400 | } |
| 400 | 401 | |
e2e/mr_test.go +6 −5
| @@ -13,11 +13,12 @@ import ( | ||
| 13 | 13 | ) |
| 14 | 14 | |
| 15 | 15 | type mrShow struct { |
| 16 | Number int64 `json:"number"` | |
| 17 | State string `json:"state"` | |
| 18 | Source string `json:"source"` | |
| 19 | HeadSHA string `json:"head_sha"` | |
| 20 | Reviews []struct { | |
| 16 | Number int64 `json:"number"` | |
| 17 | State string `json:"state"` | |
| 18 | Source string `json:"source"` | |
| 19 | TargetRef string `json:"target_ref"` | |
| 20 | HeadSHA string `json:"head_sha"` | |
| 21 | Reviews []struct { | |
| 21 | 22 | Reviewer string `json:"reviewer"` |
| 22 | 23 | Verdict string `json:"verdict"` |
| 23 | 24 | Stale bool `json:"stale"` |
e2e/mrretarget_test.go added +127
| @@ -0,0 +1,127 @@ | ||
| 1 | package e2e | |
| 2 | ||
| 3 | import ( | |
| 4 | "net/url" | |
| 5 | "os" | |
| 6 | "path/filepath" | |
| 7 | "strings" | |
| 8 | "testing" | |
| 9 | ) | |
| 10 | ||
| 11 | // TestMRRetarget moves an open merge request onto another branch, over | |
| 12 | // SSH and from the browser. Retargeting changes which diff a review was | |
| 13 | // of, so the existing approvals have to go stale with it. | |
| 14 | func TestMRRetarget(t *testing.T) { | |
| 15 | inst := startInstanceWith(t, "[web]\nmode = \"accounts\"\n") | |
| 16 | aliceKey := inst.newKey(t, "alice") | |
| 17 | bobKey := inst.newKey(t, "bob") | |
| 18 | inst.admin(t, "admin", "user", "create", "alice", | |
| 19 | "--key", aliceKey+".pub", "--email", "alice@example.test", "--verified") | |
| 20 | inst.admin(t, "admin", "user", "create", "bob", | |
| 21 | "--key", bobKey+".pub", "--email", "bob@example.test", "--verified") | |
| 22 | ||
| 23 | if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/lib"); code != 0 { | |
| 24 | t.Fatalf("repo create: %s", errOut) | |
| 25 | } | |
| 26 | if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "access", "grant", "alice/lib", "bob", "write"); code != 0 { | |
| 27 | t.Fatalf("grant: %s", errOut) | |
| 28 | } | |
| 29 | ||
| 30 | env := inst.gitEnv(aliceKey) | |
| 31 | work := t.TempDir() | |
| 32 | mustGit(t, work, env, "clone", inst.sshURL("alice/lib"), "w") | |
| 33 | dir := filepath.Join(work, "w") | |
| 34 | os.WriteFile(filepath.Join(dir, "lib.txt"), []byte("v1\n"), 0o644) | |
| 35 | mustGit(t, dir, env, "checkout", "-q", "-b", "main") | |
| 36 | mustGit(t, dir, env, "add", ".") | |
| 37 | mustGit(t, dir, env, "commit", "-q", "-m", "base") | |
| 38 | mustGit(t, dir, env, "push", "-q", "origin", "main") | |
| 39 | // A second long-lived branch to retarget onto. | |
| 40 | mustGit(t, dir, env, "checkout", "-q", "-b", "release") | |
| 41 | os.WriteFile(filepath.Join(dir, "release.txt"), []byte("1.0\n"), 0o644) | |
| 42 | mustGit(t, dir, env, "add", ".") | |
| 43 | mustGit(t, dir, env, "commit", "-q", "-m", "cut release") | |
| 44 | mustGit(t, dir, env, "push", "-q", "origin", "release") | |
| 45 | mustGit(t, dir, env, "checkout", "-q", "-b", "feat", "main") | |
| 46 | os.WriteFile(filepath.Join(dir, "feat.txt"), []byte("work\n"), 0o644) | |
| 47 | mustGit(t, dir, env, "add", ".") | |
| 48 | mustGit(t, dir, env, "commit", "-q", "-m", "add feat") | |
| 49 | mustGit(t, dir, env, "push", "-q", "origin", "feat") | |
| 50 | ||
| 51 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/lib", | |
| 52 | "--source", "feat", "--target", "main", "--title", "'feature'"); code != 0 { | |
| 53 | t.Fatalf("mr create: %s", errOut) | |
| 54 | } | |
| 55 | if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "review", "alice/lib", "1", "--approve"); code != 0 { | |
| 56 | t.Fatalf("review: %s", errOut) | |
| 57 | } | |
| 58 | ||
| 59 | // Refusals first: an unknown branch, the branch it already targets, | |
| 60 | // and its own source. | |
| 61 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "retarget", "alice/lib", "1", "nope"); code != 3 || | |
| 62 | !strings.Contains(errOut, "not found") { | |
| 63 | t.Fatalf("unknown branch: exit %d, %s", code, errOut) | |
| 64 | } | |
| 65 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "retarget", "alice/lib", "1", "main"); code != 2 || | |
| 66 | !strings.Contains(errOut, "already targets") { | |
| 67 | t.Fatalf("same branch: exit %d, %s", code, errOut) | |
| 68 | } | |
| 69 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "retarget", "alice/lib", "1", "feat"); code != 2 || | |
| 70 | !strings.Contains(errOut, "source branch") { | |
| 71 | t.Fatalf("source branch: exit %d, %s", code, errOut) | |
| 72 | } | |
| 73 | // Nobody outside the repo can move it. | |
| 74 | eveKey := inst.newKey(t, "eve") | |
| 75 | inst.admin(t, "admin", "user", "create", "eve", "--key", eveKey+".pub") | |
| 76 | if _, _, code := inst.ssh(t, eveKey, "", "mr", "retarget", "alice/lib", "1", "release"); code == 0 { | |
| 77 | t.Fatal("a stranger retargeted the merge request") | |
| 78 | } | |
| 79 | ||
| 80 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "retarget", "alice/lib", "1", "release"); code != 0 { | |
| 81 | t.Fatalf("retarget: %s", errOut) | |
| 82 | } | |
| 83 | show := inst.mrShow(t, aliceKey, "alice/lib", "1") | |
| 84 | if show.TargetRef != "release" { | |
| 85 | t.Fatalf("target not moved: %q", show.TargetRef) | |
| 86 | } | |
| 87 | if len(show.Reviews) != 1 || !show.Reviews[0].Stale { | |
| 88 | t.Fatalf("approval survived the retarget: %+v", show.Reviews) | |
| 89 | } | |
| 90 | // The diff follows the new base: release.txt is on the target now, so | |
| 91 | // it is no longer part of the change. | |
| 92 | out, errOut, code := inst.ssh(t, aliceKey, "", "mr", "diff", "alice/lib", "1") | |
| 93 | if code != 0 { | |
| 94 | t.Fatalf("mr diff: %s", errOut) | |
| 95 | } | |
| 96 | if !strings.Contains(out, "feat.txt") || strings.Contains(out, "release.txt") { | |
| 97 | t.Fatalf("diff not rebased on the new target:\n%s", out) | |
| 98 | } | |
| 99 | ||
| 100 | // The move is recorded on the conversation. | |
| 101 | if out, _, _ := inst.ssh(t, aliceKey, "", "mr", "show", "alice/lib", "1"); !strings.Contains(out, "retargeted from main to release") { | |
| 102 | t.Fatalf("no system comment for the move:\n%s", out) | |
| 103 | } | |
| 104 | ||
| 105 | // And the browser can do it, through the same command. | |
| 106 | mrURL := inst.base() + "/alice/lib/mrs/1" | |
| 107 | alice := inst.login(t, aliceKey) | |
| 108 | _, body := browserGet(t, alice, mrURL) | |
| 109 | if !strings.Contains(body, `action="/alice/lib/mrs/1/retarget"`) { | |
| 110 | t.Fatalf("no retarget control on the MR page:\n%s", body) | |
| 111 | } | |
| 112 | if status, _ := browserPost(t, alice, mrURL+"/retarget", url.Values{"target": {"main"}}); status != 200 { | |
| 113 | t.Fatalf("retarget post: %d", status) | |
| 114 | } | |
| 115 | if got := inst.mrShow(t, aliceKey, "alice/lib", "1").TargetRef; got != "main" { | |
| 116 | t.Fatalf("web retarget did not land: %q", got) | |
| 117 | } | |
| 118 | ||
| 119 | // A merged merge request is settled. | |
| 120 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/lib", "1"); code != 0 { | |
| 121 | t.Fatalf("merge: %s", errOut) | |
| 122 | } | |
| 123 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "retarget", "alice/lib", "1", "release"); code != 2 || | |
| 124 | !strings.Contains(errOut, "only an open merge request") { | |
| 125 | t.Fatalf("merged MR retargeted: exit %d, %s", code, errOut) | |
| 126 | } | |
| 127 | } | |
internal/control/mr.go +59
| @@ -38,6 +38,8 @@ func init() { | ||
| 38 | 38 | register(Command{Path: []string{"mr", "edit"}, |
| 39 | 39 | Summary: "edit title or body: mr edit <owner/name> <n> [--title <t>] [--body <b> | --file -] [--format md|org]", |
| 40 | 40 | ReadsStdin: true, Run: runMREdit}) |
| 41 | register(Command{Path: []string{"mr", "retarget"}, | |
| 42 | Summary: "retarget onto another branch: mr retarget <owner/name> <n> <branch>", Run: runMRRetarget}) | |
| 41 | 43 | register(Command{Path: []string{"mr", "comment"}, |
| 42 | 44 | Summary: "comment: mr comment <owner/name> <n> [--message <m> | --file -] [--format md|org]", |
| 43 | 45 | ReadsStdin: true, Run: runMRComment}) |
| @@ -550,6 +552,63 @@ func runMREdit(c *Ctx, args []string) int { | ||
| 550 | 552 | }) |
| 551 | 553 | } |
| 552 | 554 | |
| 555 | // runMRRetarget moves an open merge request onto another branch of the | |
| 556 | // same repository. | |
| 557 | func runMRRetarget(c *Ctx, args []string) int { | |
| 558 | if len(args) != 3 { | |
| 559 | return c.fail(protocol.ExitUsage, "usage: mr retarget <owner/name> <n> <branch>") | |
| 560 | } | |
| 561 | repo, mr, code := mrRef(c, args[:2], policy.CanRead) | |
| 562 | if code >= 0 { | |
| 563 | return code | |
| 564 | } | |
| 565 | if code := refuseArchived(c, repo); code >= 0 { | |
| 566 | return code | |
| 567 | } | |
| 568 | grant, err := c.Store.AccessRole(repo.ID, c.User.ID) | |
| 569 | if err != nil { | |
| 570 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 571 | } | |
| 572 | if mr.Author != c.User.Username && !policy.CanWrite(c.User, repo, grant) { | |
| 573 | return c.fail(protocol.ExitDenied, "only the author or users with write access can retarget this merge request") | |
| 574 | } | |
| 575 | if mr.State == "merged" || mr.State == "closed" { | |
| 576 | return c.fail(protocol.ExitUsage, "!%d is %s; only an open merge request can be retargeted", mr.Number, mr.State) | |
| 577 | } | |
| 578 | target := args[2] | |
| 579 | if target == mr.TargetRef { | |
| 580 | return c.fail(protocol.ExitUsage, "!%d already targets %s", mr.Number, target) | |
| 581 | } | |
| 582 | if mr.SourceRepoID == repo.ID && target == mr.SourceRef { | |
| 583 | return c.fail(protocol.ExitUsage, "%s is the source branch of !%d", target, mr.Number) | |
| 584 | } | |
| 585 | dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name) | |
| 586 | if _, err := gitutil.ResolveRef(dir, "refs/heads/"+target); err != nil { | |
| 587 | return c.fail(protocol.ExitNotFound, "branch %s not found in %s", target, repo.Path()) | |
| 588 | } | |
| 589 | // The diff, the commit list and the merge gates all derive their base | |
| 590 | // from the target on every read, so the only thing to check here is | |
| 591 | // that a base exists at all: without one there is nothing to show and | |
| 592 | // nothing to merge. | |
| 593 | base, err := gitutil.MergeBase(dir, "refs/heads/"+target, mrHeadRef(mr.Number)) | |
| 594 | if err != nil || base == "" { | |
| 595 | return c.fail(protocol.ExitUsage, "%s shares no history with the head of !%d", target, mr.Number) | |
| 596 | } | |
| 597 | old := mr.TargetRef | |
| 598 | if err := c.Store.SetMRTarget(mr.ID, target); err != nil { | |
| 599 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 600 | } | |
| 601 | c.Store.AddMRSystemComment(mr.ID, c.User.ID, fmt.Sprintf("retargeted from %s to %s", old, target)) | |
| 602 | if parts, err := c.Store.MRParticipants(mr.ID); err == nil { | |
| 603 | notifyUsers(c, parts, mrSubject(repo, mr.Number, mr.Title), | |
| 604 | notifyBody(c, fmt.Sprintf("retargeted !%d from %s to %s", mr.Number, old, target), "", | |
| 605 | fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number))) | |
| 606 | } | |
| 607 | return c.emit(map[string]any{"number": mr.Number, "target_ref": target, "merge_base": base}, func(w io.Writer) { | |
| 608 | fmt.Fprintf(w, "retargeted %s!%d from %s to %s (base %.10s)\n", repo.Path(), mr.Number, old, target, base) | |
| 609 | }) | |
| 610 | } | |
| 611 | ||
| 553 | 612 | func runMRComment(c *Ctx, args []string) int { |
| 554 | 613 | var rest []string |
| 555 | 614 | var message, file, format string |
internal/httpd/mractions.go +14
| @@ -121,6 +121,20 @@ func (s *Server) mrDiffCommentSubmit(w http.ResponseWriter, r *http.Request, u s | ||
| 121 | 121 | s.mrDiffRedirect(w, r, msg) |
| 122 | 122 | } |
| 123 | 123 | |
| 124 | // mrRetargetSubmit moves the merge request onto another branch. | |
| 125 | func (s *Server) mrRetargetSubmit(w http.ResponseWriter, r *http.Request, u store.User) { | |
| 126 | target := strings.TrimSpace(r.FormValue("target")) | |
| 127 | if target == "" { | |
| 128 | s.mrRedirect(w, r, "pick a branch to retarget onto") | |
| 129 | return | |
| 130 | } | |
| 131 | _, msg, ok := s.runControl(u, mrArgs(r, "retarget", target)) | |
| 132 | if ok { | |
| 133 | msg = "" | |
| 134 | } | |
| 135 | s.mrRedirect(w, r, msg) | |
| 136 | } | |
| 137 | ||
| 124 | 138 | // mrThreadSubmit resolves or reopens one review thread. |
| 125 | 139 | func (s *Server) mrThreadSubmit(w http.ResponseWriter, r *http.Request, u store.User) { |
| 126 | 140 | verb := "resolve" |
internal/httpd/routes.go +2
| @@ -149,6 +149,8 @@ func (s *Server) Routes() []Route { | ||
| 149 | 149 | Handler: s.checkOrigin(s.requireUser(s.mrMergeSubmit))}, |
| 150 | 150 | Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/close", Mutating: true, |
| 151 | 151 | Handler: s.checkOrigin(s.requireUser(s.mrCloseSubmit))}, |
| 152 | Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/retarget", Mutating: true, | |
| 153 | Handler: s.checkOrigin(s.requireUser(s.mrRetargetSubmit))}, | |
| 152 | 154 | Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/thread", Mutating: true, |
| 153 | 155 | Handler: s.checkOrigin(s.requireUser(s.mrThreadSubmit))}, |
| 154 | 156 | Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/diff-comment", Mutating: true, |
internal/httpd/web.go +3 −1
| @@ -1656,6 +1656,7 @@ func (s *Server) mr(w http.ResponseWriter, r *http.Request) { | ||
| 1656 | 1656 | // its own view rather than a fold at the foot of the conversation. |
| 1657 | 1657 | // A query parameter keeps this working without JavaScript. |
| 1658 | 1658 | unresolved, _ := s.st.UnresolvedThreadCount(m.ID) |
| 1659 | branches, _ := gitutil.Refs(p.Dir, "heads") | |
| 1659 | 1660 | view := r.URL.Query().Get("view") |
| 1660 | 1661 | if view != "commits" && view != "diff" { |
| 1661 | 1662 | view = "conversation" |
| @@ -1672,13 +1673,14 @@ func (s *Server) mr(w http.ResponseWriter, r *http.Request) { | ||
| 1672 | 1673 | DiffFiles []diffFile |
| 1673 | 1674 | Stat diffStat |
| 1674 | 1675 | Commits []commitRow |
| 1676 | Branches []gitutil.Ref | |
| 1675 | 1677 | CanEdit bool |
| 1676 | 1678 | CanWrite bool |
| 1677 | 1679 | Unresolved int |
| 1678 | 1680 | Notice string |
| 1679 | 1681 | DetachedThreads []diffThread |
| 1680 | 1682 | }{p, m, view, md(m.Body, m.BodyFormat), checks, store.CombinedStatus(checks), renderComments(comments, md), |
| 1681 | reviews, files, stat, commits, s.canEditItem(r, p.Repo, m.Author), | |
| 1683 | reviews, files, stat, commits, branches, s.canEditItem(r, p.Repo, m.Author), | |
| 1682 | 1684 | canWrite, unresolved, r.URL.Query().Get("e"), detachedThreads}) |
| 1683 | 1685 | } |
| 1684 | 1686 | |
internal/store/mrs.go +25
| @@ -183,6 +183,31 @@ func (s *Store) UpdateMRHead(mrID int64, headSHA string) error { | ||
| 183 | 183 | return tx.Commit() |
| 184 | 184 | } |
| 185 | 185 | |
| 186 | // SetMRTarget retargets a merge request and marks every existing review | |
| 187 | // stale, in one transaction. The base of the diff is derived from the | |
| 188 | // target on every read, so nothing else has to move; an approval, | |
| 189 | // though, was of the diff against the old branch. | |
| 190 | func (s *Store) SetMRTarget(mrID int64, targetRef string) error { | |
| 191 | tx, err := s.DB.Begin() | |
| 192 | if err != nil { | |
| 193 | return err | |
| 194 | } | |
| 195 | defer tx.Rollback() | |
| 196 | res, err := tx.Exec( | |
| 197 | "UPDATE merge_requests SET target_ref = ?, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ','now') WHERE id = ?", | |
| 198 | targetRef, mrID) | |
| 199 | if err != nil { | |
| 200 | return err | |
| 201 | } | |
| 202 | if n, _ := res.RowsAffected(); n == 0 { | |
| 203 | return ErrNotFound | |
| 204 | } | |
| 205 | if _, err := tx.Exec("UPDATE mr_reviews SET stale = 1 WHERE mr_id = ?", mrID); err != nil { | |
| 206 | return err | |
| 207 | } | |
| 208 | return tx.Commit() | |
| 209 | } | |
| 210 | ||
| 186 | 211 | // MarkSourceGoneForRepo flags every open MR sourced from the repo; called |
| 187 | 212 | // when a fork is deleted. Head refs in the target repos are retained. |
| 188 | 213 | func (s *Store) MarkSourceGoneForRepo(sourceRepoID int64) error { |
internal/web/templates/mr.html +13
| @@ -94,6 +94,19 @@ | ||
| 94 | 94 | </form> |
| 95 | 95 | </div> |
| 96 | 96 | {{end}} |
| 97 | {{if and .CanEdit (or (eq .MR.State "open") (eq .MR.State "source_gone"))}} | |
| 98 | <div class="grp"> | |
| 99 | <h2>Target</h2> | |
| 100 | <form method="post" action="{{$base}}/retarget" class="actions"> | |
| 101 | <label class="none" for="target">Branch</label> | |
| 102 | <select id="target" name="target"> | |
| 103 | {{range .Branches}}<option value="{{.Name}}"{{if eq .Name $.MR.TargetRef}} selected{{end}}>{{.Name}}</option>{{end}} | |
| 104 | </select> | |
| 105 | <button type="submit">Retarget</button> | |
| 106 | </form> | |
| 107 | <p class="row none">Retargeting stales existing reviews.</p> | |
| 108 | </div> | |
| 109 | {{end}} | |
| 97 | 110 | <div class="grp"> |
| 98 | 111 | <h2>Reviews</h2> |
| 99 | 112 | {{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}}</p> |