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.

Land a change request on the server instead of by pushing

38b3025145d8cee8479733fd4d3b0df443c72607 · Keenan Nemetz · 13d ago · parent 536f08b9

Land a change request on the server instead of by pushing

A change request could only be landed by merging locally and pushing the
base branch: the web page was a review surface with no way to finish, and
nothing recorded that the request had landed. `state: merged` was already
accepted by the request reader and rendered by the activity feed, but
nothing ever wrote it, so a merged request and an abandoned one looked
identical.

The gate that decides a landing lives in the pre-receive hook, and the
hook only sees pushes. A merge performed inside the server moves the ref
without the hook, so a server-side merge either re-runs that rule or
becomes the easiest way to land an unapproved change. It now re-runs it:
the approval and check rule moved to forge/landing, and both the hook and
the new landing operation call the same Check.

Landing is fast-forward when the base is an ancestor of the reviewed head,
so the commit that passed CI is the commit that lands; otherwise a
two-parent merge commit, which is the only other shape the gate accepts.
Conflicts are refused with the conflicting paths, never resolved. The ref
moves by compare-and-swap, so a concurrent push loses cleanly and the
landing retries; the request is marked merged after the ref moves, which
makes a retry after a crash finish the marking instead of failing.

The request now records merged_head, merged_by and merged_at, and the
reader refuses a merged request that names no commit, and refuses those
fields on a request that is not merged. "merged" is no longer settable
through the generic state writer: it has to travel with the commit.

The browser gets a Merge button that preflights the same gate, so it only
appears when the landing would succeed, and the page shows what landed,
who landed it, and when. `vole change merge` follows in its own change:
the CLI has no access to the ACL or the run queue, so it needs a verb on
the SSH channel, and that transport is worth reviewing on its own.

Closes the landing half of #01M5XF4XWHVMMCQEYENA8X32GK.

server/hook/hook.go +16−47

@@ -17,6 +17,7 @@ import (
17 17 "github.com/rodent-software/vole/core/lfs"
18 18 "github.com/rodent-software/vole/forge/access"
19 19 "github.com/rodent-software/vole/forge/identity"
20 "github.com/rodent-software/vole/forge/landing"
20 21 "github.com/rodent-software/vole/forge/repo"
21 22 "github.com/rodent-software/vole/internal/format"
22 23 "github.com/rodent-software/vole/metadata"
@@ -280,58 +281,26 @@ func PreReceive(cfg Config) int {
280 281 }
281 282 }
282 283
284 // The gate a push meets is the same one a server-side merge meets, so it is
285 // asked here in the same function the merge operation calls
286 // (forge/landing). An unprotected ref needs nothing; an admin keeps the bypass
287 // it always had; everyone else needs an approved request naming this exact
288 // commit, with the required checks passing.
283 289 if a.IsProtected(c.ref) {
284 // Admins keep the bypass they always had. Everyone else needs an
285 // approved request that names the exact commit they are landing.
286 if role.AtLeast(access.Admin) {
287 continue
288 }
289 if a.RequiredApprovals() <= 0 {
290 problems = append(problems, fmt.Sprintf(
291 "%s is protected and this repository requires approvals_required=0, so only admins may push it (role: %s)",
292 c.ref, role))
293 continue
294 }
295 reqs, err := metadata.LoadRequests(g, metadata.Ref)
296 if err != nil {
297 problems = append(problems, fmt.Sprintf(
298 "%s: cannot read change requests, refusing the push: %v", c.ref, err))
299 continue
300 }
301 290 branch := strings.TrimPrefix(c.ref, "refs/heads/")
302 m, err := metadata.FindApprovedMatch(g, reqs, metadata.ApprovalRequirement{
303 Branch: branch,
304 NewTip: c.new,
305 OldTip: c.old,
306 Required: a.RequiredApprovals(),
307 })
308 if err != nil {
309 problems = append(problems, fmt.Sprintf("%s: cannot evaluate approvals: %v", c.ref, err))
310 continue
311 }
312 if m == nil {
313 problems = append(problems, fmt.Sprintf(
314 "%s is protected: %s needs %d approval(s) on an open change request whose head is exactly %s. "+
315 "Add requests/<ulid>/request.md with base=%q and head=%s, then have an approver commit approvals/<their-name>.md naming that same head.",
316 c.ref, actor, a.RequiredApprovals(), format.SHA(c.new), branch, format.SHA(c.new)))
291 merge, gateErr := landing.Gate{
292 Repo: g, Name: repoNameFromDir(cfg.RepoDir),
293 Runs: openRuns(cfg.RunsDB),
294 ACL: a, Actor: actor, Role: role,
295 }.Check(branch, c.new, c.old)
296 if gateErr != nil {
297 problems = append(problems, gateErr.Error())
317 298 continue
318 299 }
319 // Approvals say a person read the change; checks say it works. Both are
320 // required unless the repository turns checks off.
321 if a.RequiresChecks() {
322 gaps, err := checkGaps(g, cfg.RunsDB, repoNameFromDir(cfg.RepoDir), m.Request.Head, branch, a)
323 if err != nil {
324 problems = append(problems, fmt.Sprintf("%s: cannot verify required checks: %v", c.ref, err))
325 continue
326 }
327 if len(gaps) > 0 {
328 problems = append(problems, fmt.Sprintf("%s: checks have not passed for %s:\n %s",
329 c.ref, format.SHA(m.Request.Head), strings.Join(gaps, "\n ")))
330 continue
331 }
300 if merge != nil {
301 auditLine(cfg.AuditPath, fmt.Sprintf("merge %s via request %s (head %s) approved by %s",
302 c.ref, merge.Request.ID, format.SHA(merge.Request.Head), strings.Join(merge.Approvers, ",")))
332 303 }
333 auditLine(cfg.AuditPath, fmt.Sprintf("merge %s via request %s (head %s) approved by %s",
334 c.ref, m.Request.ID, format.SHA(m.Request.Head), strings.Join(m.Approvers, ",")))
335 304 }
336 305 // The issues branch is user-authored content, so it is validated at the
337 306 // boundary: bad data never reaches the tree the web reads.