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.

#20

Add `vole change merge` and a web merge action that lands a request and marks it merged

closed · opened by keenan 13d ago

keenan opened this 13d ago

Why

Landing a change request today is “merge it locally, push the base branch”. The review record does not say whether that happened: requests/<ulid>/request.md carries state: open|merged|closed, and metadata.SetRequestState and server/web/activity.go:requestStateDetail already understand merged, but nothing ever sets it. A request whose head is in main and one that was abandoned look identical on the page, and the only trace of the landing is the audit line the pre-receive hook writes.

That also makes the web UI a review surface with no way to finish: the only state-changing actions are approve, comment, and open/closed (server/routes.go:139-150). There is no merge button because there is no merge operation — the operator is expected to run git.

There is a second, sharper reason. The approval/check gate exists only in the pre-receive hook (server/hook/hook.go:284-336). Anything the server does internally — a merge button, a CLI command — moves the ref without the hook, so a new merge path either re-runs the same rule or silently becomes the easiest way to land an unapproved change.

Design

One operation, two front doors.

vole change merge <id>              # CLI
POST /r/{name}/requests/{id}/merge  # web button

Both call the same server-side MergeRequest, so the rules cannot differ by entry point.

Re-run the hook’s rule, do not copy it. Extract the two pieces the hook uses into a shared package (forge/landing), and have the hook and the merge path both call it:

  • metadata.FindApprovedMatch — the approved request whose head is exactly the commit being landed (already exists; unchanged).
  • server/hook.checkGaps — the required checks have passed for that head (moves out of server/hook so both callers reach it).

A merge is refused for the same reasons a push is, with the same wording.

Merge shape: only what the gate accepts. Fast-forward when the base tip is an ancestor of the head (the landed SHA is then exactly the tested SHA); otherwise a two-parent merge commit of base tip and head. Anything else is refused, because FindApprovedMatch deliberately does not accept “approved head is an ancestor of the new tip” — that would let unapproved commits ride along. Conflicts are refused with the conflicting paths, never auto-resolved.

Ref move, then mark. core/git.UpdateRefCAS(refs/heads/<base>, new, old) so a concurrent push loses cleanly instead of being overwritten; then write the metadata: state: merged plus merged_head, merged_by, merged_at in request.md. The two writes cannot be atomic, so the order is ref first, metadata second: a crash between them leaves the request open on an already-landed head, and a retry of the same command marks it merged instead of failing (idempotent, keyed on “is merged_head already in the base branch”).

Transport. The CLI has no access to the ACL or the run queue, so vole change merge asks the server over SSH — a new verb beside vole-runner/vole-roster/vole-admin (server/ssh/server.go:215-262), keyed by the same identity. The web route calls the same function in-process.

Permissions. maintain and above, i.e. the same accounts that may approve. The author may merge their own request once it has the required approvals — that is the point of separating “approve” from “land”. The admin bypass the hook has (role.AtLeast(access.Admin), audited) is preserved, because a repo whose approvals_required is 0 is admin-only by definition.

What the page shows. state: merged plus the landing commit:

  • “Merged into main at <sha> by <account> on <date>” in place of the merge button;
  • the merged chip in the request list and as a filter, next to open/closed;
  • the activity feed already renders “merged change request” (server/web/activity.go:266), so it starts being reachable.

The push path keeps working unchanged: runners and automation still land by pushing, and a push that satisfies the gate still lands. merge is the supported path, not the only one.

Scope

  • forge/landing: one gate function, called by the hook and by MergeRequest
  • metadata: merged_head/merged_by/merged_at on Request, written and validated on read and write, so a stale merged without a head is refused
  • MergeRequest: FF / two-parent merge / refuse conflicts, CAS ref move, then mark merged; idempotent on retry
  • server/ssh: vole-change-merge verb, maintain+ only, audited
  • vole change merge <id> in the CLI, printing the landing SHA and the URL
  • POST /r/{name}/requests/{id}/merge plus the button, shown only when it can succeed, hidden once merged
  • merged chip, filter, and the landing summary on the request page
  • vole change reopen on a merged request says what it can and cannot undo (it cannot un-land the commit)

Acceptance

  • vole change merge on an approved request lands exactly the approved head, and the request then reads state: merged with the landing SHA.
  • The same command with no approval, a failing check, or a moved head is refused with the reason, and changes nothing.
  • The web button appears only when the CLI command would succeed, and the two agree for every ACL configuration (a table test over approvals 0/1/2, checks on/off, author vs approver vs admin).
  • A fast-forward landing records the tested SHA; a non-fast-forward landing records a merge commit whose second parent is the approved head.
  • Two concurrent merges of the same request: exactly one lands, the other gets the CAS failure.
  • state: merged with no merged_head is rejected by ValidateRequests, so the file format cannot claim a landing that no commit backs.
  • The pre-receive gate and the merge path are covered by one test table, so the two implementations cannot drift.

Comments (0)

No comments yet.