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.
Make change requests follow branches and allow base retargeting
e050e14a64f6f19c1b1478c4c073d690ef3d3129 · Keenan Nemetz · 13d ago · parent 59464445
Make change requests follow branches and allow base retargeting A request opened from a branch now follows it: post-receive moves the request's head to the new tip after an accepted push, so approvals and checks that named the old tip stop counting. Approvals stay bound to an exact base and commit; the approval writers now require the base and head the approver saw, so a push or retarget cannot silently approve a target nobody reviewed. Merged requests freeze the reviewed head. Add `vole change update --base` and a Change base control for stacked requests. A new base changes the diff and therefore invalidates approvals, which is what retargeting an upper request after its lower branch lands needs. Base names may now contain slashes. Also fix a second-boundary flake in the landing test.
metadata/request_write.go +141−8
| @@ -47,14 +47,14 @@ func validateRequestForWrite(r *Request) error { | |||
| 47 | 47 | if !assigneeRE.MatchString(r.Author) { | |
| 48 | 48 | return fmt.Errorf("requests: author %q is not an account name", r.Author) | |
| 49 | 49 | } | |
| 50 | if !branchNameRE.MatchString(r.Base) { | ||
| 50 | if !validBranchName(r.Base) { | ||
| 51 | 51 | return fmt.Errorf("requests: base %q is not a branch name", r.Base) | |
| 52 | 52 | } | |
| 53 | 53 | if r.Head != "" && !shaRE.MatchString(r.Head) { | |
| 54 | 54 | return fmt.Errorf("requests: head %q is not a full commit id", r.Head) | |
| 55 | 55 | } | |
| 56 | 56 | if r.HeadRef != "" { | |
| 57 | if !validHeadRef(r.HeadRef) { | ||
| 57 | if !validBranchName(r.HeadRef) { | ||
| 58 | 58 | return fmt.Errorf("requests: head_ref %q is not a branch name", r.HeadRef) | |
| 59 | 59 | } | |
| 60 | 60 | if r.Head == "" { | |
| @@ -156,15 +156,22 @@ func GetRequest(g *git.Repo, id string) (*Request, error) { | |||
| 156 | 156 | return nil, fmt.Errorf("requests: no request %s", id) | |
| 157 | 157 | } | |
| 158 | 158 | ||
| 159 | // SetHead repoints a request at a new commit. The approvals stay in the tree but | ||
| 160 | // stop matching, because they name the commit they reviewed. | ||
| 159 | // SetRequestHead repoints an open request at a commit and, optionally, at the | ||
| 160 | // branch it should follow. A non-empty headRef makes the request follow that | ||
| 161 | // branch, so the next push moves the head again; an empty one pins it to head. | ||
| 162 | // Approvals stay in the tree but stop matching when the head moves, because they | ||
| 163 | // name the commit they reviewed. | ||
| 161 | 164 | // | |
| 162 | 165 | // Only an open request may move: a merged request's head is the record of what | |
| 163 | 166 | // landed, and a closed one is not going to land at all. Refusing here rather than | |
| 164 | 167 | // in each caller is what keeps the CLI and the browser on the same rule. | |
| 165 | func SetRequestHead(g *git.Repo, id, head, actor string) (*Request, error) { | ||
| 168 | func SetRequestHead(g *git.Repo, id, head, headRef, actor string) (*Request, error) { | ||
| 166 | 169 | var out *Request | |
| 167 | 170 | base := fmt.Sprintf("%s/%s/", RequestsDir, id) | |
| 171 | message := fmt.Sprintf("request: %s head -> %s", id, format.SHA(head)) | ||
| 172 | if headRef != "" { | ||
| 173 | message = fmt.Sprintf("request: %s follows %s at %s", id, headRef, format.SHA(head)) | ||
| 174 | } | ||
| 168 | 175 | _, err := git.CommitFiles(g, Ref, func(files map[string][]byte) error { | |
| 169 | 176 | r, err := loadFrom(files, id) | |
| 170 | 177 | if err != nil { | |
| @@ -174,6 +181,7 @@ func SetRequestHead(g *git.Repo, id, head, actor string) (*Request, error) { | |||
| 174 | 181 | return fmt.Errorf("requests: %s is %s, so its head cannot move", id, r.State) | |
| 175 | 182 | } | |
| 176 | 183 | r.Head = head | |
| 184 | r.HeadRef = headRef | ||
| 177 | 185 | if err := validateRequestForWrite(r); err != nil { | |
| 178 | 186 | return err | |
| 179 | 187 | } | |
| @@ -184,12 +192,111 @@ func SetRequestHead(g *git.Repo, id, head, actor string) (*Request, error) { | |||
| 184 | 192 | files[base+requestName] = raw | |
| 185 | 193 | out = r | |
| 186 | 194 | return nil | |
| 187 | }, fmt.Sprintf("request: %s head -> %s", id, format.SHA(head)), actor) | ||
| 195 | }, message, actor) | ||
| 196 | return out, err | ||
| 197 | } | ||
| 198 | |||
| 199 | // SetRequestBase retargets an open request at another base branch. It is how a | ||
| 200 | // stacked request is repointed when the branch below it lands: the diff is now | ||
| 201 | // against the new base, and every approval named the old base, so the review starts | ||
| 202 | // over. | ||
| 203 | // | ||
| 204 | // Only an open request may move: a merged request's base is the record of what it | ||
| 205 | // landed on, and a closed one is not going to land at all. | ||
| 206 | func SetRequestBase(g *git.Repo, id, newBase, actor string) (*Request, error) { | ||
| 207 | var out *Request | ||
| 208 | dir := fmt.Sprintf("%s/%s/", RequestsDir, id) | ||
| 209 | _, err := git.CommitFiles(g, Ref, func(files map[string][]byte) error { | ||
| 210 | r, err := loadFrom(files, id) | ||
| 211 | if err != nil { | ||
| 212 | return err | ||
| 213 | } | ||
| 214 | if r.State != "open" { | ||
| 215 | return fmt.Errorf("requests: %s is %s, so its base cannot move", id, r.State) | ||
| 216 | } | ||
| 217 | if r.Base == newBase { | ||
| 218 | out = r | ||
| 219 | return ErrNoChange | ||
| 220 | } | ||
| 221 | r.Base = newBase | ||
| 222 | if err := validateRequestForWrite(r); err != nil { | ||
| 223 | return err | ||
| 224 | } | ||
| 225 | raw, err := renderRequest(r) | ||
| 226 | if err != nil { | ||
| 227 | return err | ||
| 228 | } | ||
| 229 | files[dir+requestName] = raw | ||
| 230 | out = r | ||
| 231 | return nil | ||
| 232 | }, fmt.Sprintf("request: %s base -> %s", id, newBase), actor) | ||
| 233 | if errors.Is(err, ErrNoChange) { | ||
| 234 | return out, nil | ||
| 235 | } | ||
| 188 | 236 | return out, err | |
| 189 | 237 | } | |
| 190 | 238 | ||
| 239 | // SyncBranchHead moves every open request that follows branch to head. It is the | ||
| 240 | // server-side half of following a branch: post-receive calls it after the ref has | ||
| 241 | // moved, so the request names the tip a reviewer will see. It changes only head; | ||
| 242 | // approvals of the old tip stay where their owners wrote them and stop matching, | ||
| 243 | // and checks are keyed by commit, so the new tip starts without any. | ||
| 244 | // | ||
| 245 | // This is deliberately separate from SetRequestHead: it is the server reacting to | ||
| 246 | // a push, not an account moving a request, so it does not take an actor's | ||
| 247 | // permission to edit the request. The only field it can change is head, and only | ||
| 248 | // for a request that already named this branch. | ||
| 249 | func SyncBranchHead(g *git.Repo, branch, head, actor string) (int, error) { | ||
| 250 | if branch == "" || !shaRE.MatchString(head) { | ||
| 251 | return 0, nil | ||
| 252 | } | ||
| 253 | updated := 0 | ||
| 254 | _, err := git.CommitFiles(g, Ref, func(files map[string][]byte) error { | ||
| 255 | // CommitFiles re-runs this on a lost race, so the count is per attempt. | ||
| 256 | updated = 0 | ||
| 257 | contents := make(map[string]string) | ||
| 258 | for path, raw := range files { | ||
| 259 | if strings.HasPrefix(path, RequestsDir+"/") { | ||
| 260 | contents[path] = string(raw) | ||
| 261 | } | ||
| 262 | } | ||
| 263 | list, err := ParseRequests(contents) | ||
| 264 | if err != nil { | ||
| 265 | return fmt.Errorf("requests: cannot follow %s: %w", branch, err) | ||
| 266 | } | ||
| 267 | for _, r := range list { | ||
| 268 | if r.State != "open" || r.HeadRef != branch || r.Head == head { | ||
| 269 | continue | ||
| 270 | } | ||
| 271 | r.Head = head | ||
| 272 | raw, err := renderRequest(r) | ||
| 273 | if err != nil { | ||
| 274 | return err | ||
| 275 | } | ||
| 276 | files[fmt.Sprintf("%s/%s/%s", RequestsDir, r.ID, requestName)] = raw | ||
| 277 | updated++ | ||
| 278 | } | ||
| 279 | if updated == 0 { | ||
| 280 | return ErrNoChange | ||
| 281 | } | ||
| 282 | return nil | ||
| 283 | }, fmt.Sprintf("request: follow %s at %s", branch, format.SHA(head)), actor) | ||
| 284 | if errors.Is(err, ErrNoChange) { | ||
| 285 | return 0, nil | ||
| 286 | } | ||
| 287 | if err != nil { | ||
| 288 | return 0, err | ||
| 289 | } | ||
| 290 | return updated, nil | ||
| 291 | } | ||
| 292 | |||
| 191 | 293 | // Approve records principal's approval of the request's current head and base. | |
| 192 | func ApproveRequest(g *git.Repo, id, principal string, now time.Time, actor string) (*Request, error) { | ||
| 294 | // | ||
| 295 | // expectedBase and expectedHead are the target the approver was looking at. If a | ||
| 296 | // push moved the followed branch, or the request was retargeted at another base, | ||
| 297 | // the approval is refused rather than silently transferred to a target nobody | ||
| 298 | // reviewed, and the caller reads the request again. | ||
| 299 | func ApproveRequest(g *git.Repo, id, principal, expectedBase, expectedHead string, now time.Time, actor string) (*Request, error) { | ||
| 193 | 300 | if !assigneeRE.MatchString(principal) { | |
| 194 | 301 | return nil, fmt.Errorf("requests: %q is not an account name", principal) | |
| 195 | 302 | } | |
| @@ -200,12 +307,25 @@ func ApproveRequest(g *git.Repo, id, principal string, now time.Time, actor stri | |||
| 200 | 307 | if err != nil { | |
| 201 | 308 | return err | |
| 202 | 309 | } | |
| 310 | if r.State != "open" { | ||
| 311 | return fmt.Errorf("requests: %s is %s, so it cannot be approved", id, r.State) | ||
| 312 | } | ||
| 203 | 313 | if r.Author == principal { | |
| 204 | 314 | return fmt.Errorf("requests: the author cannot approve their own request") | |
| 205 | 315 | } | |
| 206 | 316 | if r.Head == "" { | |
| 207 | 317 | return fmt.Errorf("requests: %s has no head commit to approve", id) | |
| 208 | 318 | } | |
| 319 | if expectedBase != "" && r.Base != expectedBase { | ||
| 320 | return fmt.Errorf( | ||
| 321 | "requests: %s moved base from %s to %s before the approval was written; review the new base and approve again", | ||
| 322 | id, expectedBase, r.Base) | ||
| 323 | } | ||
| 324 | if expectedHead != "" && r.Head != expectedHead { | ||
| 325 | return fmt.Errorf( | ||
| 326 | "requests: %s moved from %s to %s before the approval was written; review the new head and approve again", | ||
| 327 | id, format.SHA(expectedHead), format.SHA(r.Head)) | ||
| 328 | } | ||
| 209 | 329 | raw, err := renderApproval(Approval{ | |
| 210 | 330 | Principal: principal, | |
| 211 | 331 | Base: r.Base, | |
| @@ -263,15 +383,22 @@ func SetRequestState(g *git.Repo, id, state, actor string) (*Request, error) { | |||
| 263 | 383 | // does not know it — a retry that finds the head already landed has no base tip of | |
| 264 | 384 | // its own to name — passes "", which leaves whatever the record already says. | |
| 265 | 385 | // | |
| 386 | // reviewedHead is the commit the approvals named. It is written to head as the | ||
| 387 | // record is marked merged, because a followed branch may have moved after the | ||
| 388 | // landing was prepared and the merged request must describe what actually landed. | ||
| 389 | // | ||
| 266 | 390 | // A request that is already merged with the same commit is left alone and returned | |
| 267 | 391 | // unchanged, which makes a retry after a half-finished landing harmless. | |
| 268 | func MarkMerged(g *git.Repo, id, landedBase, landedHead, actor string, at time.Time) (*Request, []string, error) { | ||
| 392 | func MarkMerged(g *git.Repo, id, landedBase, landedHead, reviewedHead, actor string, at time.Time) (*Request, []string, error) { | ||
| 269 | 393 | if !shaRE.MatchString(landedHead) { | |
| 270 | 394 | return nil, nil, fmt.Errorf("requests: merged head must be a full commit id, got %q", landedHead) | |
| 271 | 395 | } | |
| 272 | 396 | if landedBase != "" && !shaRE.MatchString(landedBase) { | |
| 273 | 397 | return nil, nil, fmt.Errorf("requests: merged base must be a full commit id, got %q", landedBase) | |
| 274 | 398 | } | |
| 399 | if reviewedHead != "" && !shaRE.MatchString(reviewedHead) { | ||
| 400 | return nil, nil, fmt.Errorf("requests: reviewed head must be a full commit id, got %q", reviewedHead) | ||
| 401 | } | ||
| 275 | 402 | var ( | |
| 276 | 403 | out *Request | |
| 277 | 404 | closed []string | |
| @@ -290,6 +417,12 @@ func MarkMerged(g *git.Repo, id, landedBase, landedHead, actor string, at time.T | |||
| 290 | 417 | return ErrNoChange | |
| 291 | 418 | } | |
| 292 | 419 | r.State = "merged" | |
| 420 | // Freeze the commit the review named. The branch may have moved after the | ||
| 421 | // landing was prepared; the merged record has to describe what landed, not | ||
| 422 | // where the branch went next. | ||
| 423 | if reviewedHead != "" { | ||
| 424 | r.Head = reviewedHead | ||
| 425 | } | ||
| 293 | 426 | r.MergedBase = landedBase | |
| 294 | 427 | r.MergedHead = landedHead | |
| 295 | 428 | r.MergedBy = actor | |