Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.
vole
Clone

Read only, no credentials:

https://rodent.software/git/vole.git

Write over SSH with a registered key:

git clone ssh://git@0.0.0.0:2222/vole.git

A free and open-source forge that lives in your Git repo.

Draw the merge control from the landing plan

097b21105b70f57fbf1c119c87ae7a75b3a44ca6 · Keenan Nemetz · 13d ago · parent b14a7819 

Draw the merge control from the landing plan

The merge button was drawn from the approval and check gate, which never
looks at the tree, so a request that conflicted with its base still offered
"Merge" while the "What would land" block two sections above said it did
not merge cleanly. Clicking it answered with a bare 409 and one sentence:
the page, the diff and the conflict paths were all replaced by text.

The decision now lives in one place. landing.Prepare works out what a
landing would do — fast-forward, merge commit, already landed, or the
paths that conflict — and refuses the same things Land refuses. Land
prepares and carries out; the page prepares to decide whether to draw the
control; and the refusal path re-renders the request page with the message
beside the conflict block instead of answering with plain text.

A conflict is reported in the plan rather than as an error, because it is
an outcome the page can style: the paths are already listed by "What would
land", so the sidebar adds nothing and the button stays away.

Prepare writes one unreferenced commit object for a non-fast-forward
landing, because the approval matches a commit whose parents are the base
tip and the approved head, and that commit has to exist to be matched.
Nothing visible changes and git collects it.

forge/landing/land.go +106−94

@@ -10,29 +10,41 @@ import (
10 10 "github.com/rodent-software/vole/metadata"
11 11 )
12 12
13 // Result describes one landing.
14 type Result struct {
13 // Plan is what a landing would do, worked out without moving anything.
14 //
15 // It exists so that the code which draws a merge control and the code which performs
16 // a merge cannot disagree: a page that offers "Merge" for a change that conflicts is
17 // the same bug as a button that hides a landing that would have worked, and both come
18 // from asking a different question in each place. Plan is the answer to the one
19 // question, and Land is the act of carrying it out.
20 //
21 // It is not entirely free of side effects: a non-fast-forward landing needs a merge
22 // commit before the gate can be asked (the approval matches a commit whose parents
23 // are the base tip and the approved head), so Plan writes one unreferenced commit
24 // object. Nothing visible changes, and an object nobody points at is collected.
25 type Plan struct {
15 26 Request *metadata.Request
16 // Base is the branch written, NewTip is the commit now on it and OldTip is what
17 // it replaced. They differ from the request's head when the landing needed a
18 // merge commit.
27 // Base is the branch that would move, OldTip what it is now, and Head the commit
28 // under review.
19 29 Base string
20 30 OldTip string
21 NewTip string
22 // Approval is the request and approvers that authorised the landing. It is nil
23 // when the landing needed no approval (an unprotected branch, or an admin).
24 Approval *metadata.Merge
25 // FastForward is true when NewTip is the reviewed head itself, which is the
26 // usual case: nothing new was written, so the commit that passed CI is the
27 // commit that landed.
31 Head string
32 // NewTip is the commit that would land: Head for a fast-forward, a merge commit
33 // otherwise. Empty when the plan cannot land.
34 NewTip string
28 35 FastForward bool
29 // MergeCommit is true when a two-parent commit of the base tip and the reviewed
30 // head was written, which is the only other shape the gate accepts.
31 36 MergeCommit bool
32 // AlreadyLanded is true when the head was already in the base branch: a retry of
33 // a landing whose metadata write failed, or a request that was landed by pushing
34 // the branch before this command existed.
37 // AlreadyLanded is true when Head is already in OldTip: there is no ref to move,
38 // only the record to write. It is what a retry finds after a landing whose
39 // metadata write failed, and what a request landed by a push looks like.
35 40 AlreadyLanded bool
41 // Conflicts names the paths that conflict. A non-empty slice means the plan cannot
42 // land, and NewTip is empty. It is a reported outcome rather than an error so a
43 // page can style the paths instead of printing a sentence.
44 Conflicts []string
45 // Approval is the request and approvers that authorise the landing, nil when the
46 // branch needs no approval (unprotected, or the actor is an admin).
47 Approval *metadata.Merge
36 48 }
37 49
38 50 // ErrLandedUnmarked is returned when the branch moved but the request could not be
@@ -56,140 +68,140 @@ func (e *ErrLandedUnmarked) Unwrap() error { return e.Err }
56 68 const attempts = 3
57 69
58 70 // Land merges an open change request into its base branch and records that it
59 // happened.
71 // happened: Plan, then the ref move, then the mark.
60 72 //
61 // It moves the ref itself, so it does not run the pre-receive hook; g.Check is the
62 // same gate the hook would have applied, asked in the same terms and answered in the
63 // same words. A landing that the hook would refuse is refused here.
73 // It moves the ref itself, so it does not run the pre-receive hook; Plan is the same
74 // gate the hook would have applied, asked in the same terms and answered in the same
75 // words. A landing the hook would refuse is refused here.
64 76 //
65 // The ref moves before the request is marked merged. If the process dies between
66 // the two, the branch is correct and the request still looks open; running the same
77 // The ref moves before the request is marked merged. If the process dies between the
78 // two, the branch is correct and the request still looks open; running the same
67 79 // command again finds the head already in the base and only writes the mark.
68 func Land(g Gate, id, actor string, now time.Time) (*Result, error) {
80 func Land(g Gate, id, actor string, now time.Time) (*Plan, error) {
69 81 var lastRace error
70 82 for attempt := 0; attempt < attempts; attempt++ {
71 res, done, err := landOnce(g, id, actor, now)
83 plan, err := Prepare(g, id, actor, now)
72 84 if err != nil {
73 return res, err
85 return plan, err
86 }
87 if len(plan.Conflicts) > 0 {
88 return plan, fmt.Errorf(
89 "%s does not merge into %s: %s conflict. Resolve it on the branch and push again",
90 format.SHA(plan.Head), plan.Base, strings.Join(plan.Conflicts, ", "))
91 }
92 if plan.AlreadyLanded {
93 if plan.Request.State == "merged" && plan.Request.MergedHead != "" {
94 return plan, nil
95 }
96 if err := marked(g, id, plan.Head, actor, now); err != nil {
97 return plan, &ErrLandedUnmarked{NewTip: plan.OldTip, Err: err}
98 }
99 return plan, nil
100 }
101 ok, err := g.Repo.UpdateRefCAS(g.Ref(plan.Base), plan.NewTip, plan.OldTip)
102 if err != nil {
103 return plan, err
104 }
105 if !ok {
106 lastRace = fmt.Errorf("%s moved while this landing was being prepared; try again", plan.Base)
107 continue // another writer won; recompute against the new tip
74 108 }
75 if done {
76 return res, nil
109 if g.Audit != nil {
110 if plan.Approval != nil {
111 g.Audit(fmt.Sprintf("merge %s via request %s (head %s) approved by %s",
112 g.Ref(plan.Base), plan.Approval.Request.ID,
113 format.SHA(plan.Approval.Request.Head), strings.Join(plan.Approval.Approvers, ",")))
114 } else {
115 g.Audit(fmt.Sprintf("merge %s by %s (head %s)", g.Ref(plan.Base), actor, format.SHA(plan.NewTip)))
116 }
77 117 }
78 lastRace = fmt.Errorf("%s moved while this landing was being prepared; try again", res.Base)
118 if err := marked(g, id, plan.NewTip, actor, now); err != nil {
119 return plan, &ErrLandedUnmarked{NewTip: plan.NewTip, Err: err}
120 }
121 return plan, nil
79 122 }
80 123 return nil, lastRace
81 124 }
82 125
83 // landOnce is one attempt. done is false only when the compare-and-swap lost a race
84 // with another writer.
85 func landOnce(g Gate, id, actor string, now time.Time) (*Result, bool, error) {
126 // Prepare works out what landing this request would do, and refuses the same things
127 // Land refuses. It is what a page asks before it offers a merge control, and what Land
128 // asks before it moves anything, so the two cannot disagree.
129 func Prepare(g Gate, id, actor string, now time.Time) (*Plan, error) {
86 130 if g.Repo == nil {
87 return nil, false, errors.New("no repository to land in")
131 return nil, errors.New("no repository to land in")
88 132 }
89 133 req, err := metadata.GetRequest(g.Repo, id)
90 134 if err != nil {
91 return nil, false, err
135 return nil, err
92 136 }
93 137 branch := req.Base
94 ref := g.Ref(branch)
95 oldTip, err := g.Repo.RevParse(ref)
138 oldTip, err := g.Repo.RevParse(g.Ref(branch))
96 139 if err != nil {
97 return nil, false, err
140 return nil, err
98 141 }
99 142 if oldTip == "" {
100 return nil, false, fmt.Errorf("%s does not exist, so there is nothing to land into", ref)
143 return nil, fmt.Errorf("%s does not exist, so there is nothing to land into", g.Ref(branch))
101 144 }
102 145 if req.Head == "" {
103 return nil, false, fmt.Errorf("change request %s names no head commit", id)
146 return nil, fmt.Errorf("change request %s names no head commit", id)
104 147 }
105 148 head, err := g.Repo.RevParse(req.Head)
106 149 if err != nil {
107 return nil, false, err
150 return nil, err
108 151 }
109 152 if head == "" {
110 return nil, false, fmt.Errorf(
153 return nil, fmt.Errorf(
111 154 "change request %s names head %s, which is not in this repository; push the branch again",
112 155 id, format.SHA(req.Head))
113 156 }
114 res := &Result{Request: req, Base: branch, OldTip: oldTip, NewTip: head}
157 plan := &Plan{Request: req, Base: branch, OldTip: oldTip, Head: head, NewTip: head}
115 158 if req.State == "closed" {
116 return res, false, fmt.Errorf("change request %s is closed; reopen it before merging", id)
159 return plan, fmt.Errorf("change request %s is closed; reopen it before merging", id)
117 160 }
118 161
119 // Already in the base branch: nothing to merge, only the record to write. This is
120 // the retry path after the branch moved and the mark did not, and the path for a
121 // request whose branch was landed by a push.
162 // Already in the base branch: nothing to merge, only the record to write.
122 163 if landed, err := g.Repo.IsAncestor(head, oldTip); err != nil {
123 return res, false, err
164 return plan, err
124 165 } else if landed {
125 res.AlreadyLanded = true
126 res.NewTip = oldTip
127 if req.State == "merged" && req.MergedHead != "" {
128 return res, true, nil
129 }
130 if err := marked(g, id, head, actor, now); err != nil {
131 return res, false, &ErrLandedUnmarked{NewTip: oldTip, Err: err}
132 }
133 return res, true, nil
166 plan.AlreadyLanded = true
167 plan.NewTip = oldTip
168 return plan, nil
134 169 }
135 170
136 // Fast-forward when the base is an ancestor of the reviewed head: the commit that
137 // passed CI is then exactly the commit that lands, which is the property worth
138 // keeping. Otherwise a two-parent merge commit, the only other shape the gate
139 // accepts, so the reviewed head is still one of its parents.
140 171 ff, err := g.Repo.IsAncestor(oldTip, head)
141 172 if err != nil {
142 return res, false, err
173 return plan, err
143 174 }
144 newTip := head
145 175 if ff {
146 res.FastForward = true
176 // The commit that passed CI is the commit that lands.
177 plan.FastForward = true
147 178 } else {
148 179 tree, conflicts, err := g.Repo.MergeTree(oldTip, head)
149 180 if err != nil {
150 return res, false, err
181 return plan, err
151 182 }
152 183 if len(conflicts) > 0 {
153 return res, false, fmt.Errorf(
154 "%s does not merge into %s: %s conflict. Resolve it on the branch and push again",
155 format.SHA(head), branch, strings.Join(conflicts, ", "))
184 plan.Conflicts = conflicts
185 plan.NewTip = ""
186 return plan, nil
156 187 }
157 188 commit, err := g.Repo.CommitMergeTree(tree, []string{oldTip, head},
158 189 fmt.Sprintf("Merge change request %s: %s", req.ID, req.Title), actor)
159 190 if err != nil {
160 return res, false, err
191 return plan, err
161 192 }
162 newTip = commit
163 res.MergeCommit = true
193 plan.MergeCommit = true
194 plan.NewTip = commit
164 195 }
165 res.NewTip = newTip
166 196
167 197 // The same question the hook would have asked of a push of this commit.
168 merge, err := g.Check(branch, newTip, oldTip)
198 merge, err := g.Check(branch, plan.NewTip, oldTip)
169 199 if err != nil {
170 return res, false, err
171 }
172 res.Approval = merge
173
174 ok, err := g.Repo.UpdateRefCAS(ref, newTip, oldTip)
175 if err != nil {
176 return res, false, err
177 }
178 if !ok {
179 return res, false, nil // another writer won; the caller retries
180 }
181 if g.Audit != nil {
182 if merge != nil {
183 g.Audit(fmt.Sprintf("merge %s via request %s (head %s) approved by %s",
184 ref, merge.Request.ID, format.SHA(merge.Request.Head), strings.Join(merge.Approvers, ",")))
185 } else {
186 g.Audit(fmt.Sprintf("merge %s by %s (head %s)", ref, actor, format.SHA(newTip)))
187 }
188 }
189 if err := marked(g, id, newTip, actor, now); err != nil {
190 return res, false, &ErrLandedUnmarked{NewTip: newTip, Err: err}
200 plan.NewTip = ""
201 return plan, err
191 202 }
192 return res, true, nil
203 plan.Approval = merge
204 return plan, nil
193 205 }
194 206
195 207 // marked writes the merged state and turns a validation problem into an error, so a