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.
Close linked issues when a change request lands
183d48e55e14b0355db4268275893274433ce8ee · Keenan Nemetz · 13d ago · parent 097b2110
Close linked issues when a change request lands A change request can now name the issues it closes. Landing writes closed onto each of them in the same commit that marks the request merged, so the merge and the closure cannot be separated: a request cannot be merged with its issues still open, and an issue cannot be closed by a merge that did not happen. The link lives in request.md as a list of full issue ids, which is what makes it reviewable: it is part of the file a reviewer reads, not a reference buried in prose. Ids are validated when the request is written, including that the issue exists, so a typo fails while the author is still looking at the command instead of at the landing they have waited for. `vole change new --closes <id>` (repeatable, comma lists accepted) sets it, `vole change merge` reports what it closed, and the browser form takes the same field. The request page links each issue, and the merge answer names the ones it closed. An issue that is already closed, or that is no longer there, is left alone: the promise is about what a merge does, not permission to overrule somebody who closed it deliberately. Re-marking an already-merged request closes nothing, so a retried landing is harmless. Closes #01M5XG8WYZ1566KZFC73WQWGX3.
metadata/request_write.go +77−8
| @@ -53,6 +53,9 @@ func validateRequestForWrite(r *Request) error { | |||
| 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 | if _, problems := parseCloses(r.Closes, "request"); len(problems) > 0 { | ||
| 57 | return fmt.Errorf("%s", strings.Join(problems, "; ")) | ||
| 58 | } | ||
| 56 | 59 | return nil | |
| 57 | 60 | } | |
| 58 | 61 | ||
| @@ -65,6 +68,7 @@ func renderRequest(r *Request) ([]byte, error) { | |||
| 65 | 68 | d.Word("author", r.Author) | |
| 66 | 69 | d.Word("base", r.Base) | |
| 67 | 70 | d.Word("head", r.Head) | |
| 71 | d.List("closes", r.Closes) | ||
| 68 | 72 | d.Word("created", r.Created.UTC().Format(time.RFC3339)) | |
| 69 | 73 | if r.State == "merged" { | |
| 70 | 74 | d.Word("merged_head", r.MergedHead) | |
| @@ -88,6 +92,24 @@ func CreateRequest(g *git.Repo, r *Request, actor string) error { | |||
| 88 | 92 | if err := validateRequestForWrite(r); err != nil { | |
| 89 | 93 | return err | |
| 90 | 94 | } | |
| 95 | // A link is a promise that a merge closes the issue, so it has to name one. Doing | ||
| 96 | // this here rather than at the merge means a typo is refused while the author is | ||
| 97 | // still looking at it, instead of at the landing of a request they waited on. | ||
| 98 | if len(r.Closes) > 0 { | ||
| 99 | issues, err := LoadIssues(g, Ref) | ||
| 100 | if err != nil { | ||
| 101 | return err | ||
| 102 | } | ||
| 103 | known := make(map[string]bool, len(issues)) | ||
| 104 | for _, iss := range issues { | ||
| 105 | known[iss.ID] = true | ||
| 106 | } | ||
| 107 | for _, id := range r.Closes { | ||
| 108 | if !known[id] { | ||
| 109 | return fmt.Errorf("requests: closes %s, and there is no such issue", id) | ||
| 110 | } | ||
| 111 | } | ||
| 112 | } | ||
| 91 | 113 | raw, err := renderRequest(r) | |
| 92 | 114 | if err != nil { | |
| 93 | 115 | return err | |
| @@ -212,13 +234,13 @@ func SetRequestState(g *git.Repo, id, state, actor string) (*Request, error) { | |||
| 212 | 234 | // | |
| 213 | 235 | // A request that is already merged with the same commit is left alone and returned | |
| 214 | 236 | // unchanged, which makes a retry after a half-finished landing harmless. | |
| 215 | func MarkMerged(g *git.Repo, id, landedHead, actor string, at time.Time) (*Request, bool, error) { | ||
| 237 | func MarkMerged(g *git.Repo, id, landedHead, actor string, at time.Time) (*Request, []string, error) { | ||
| 216 | 238 | if !shaRE.MatchString(landedHead) { | |
| 217 | return nil, false, fmt.Errorf("requests: merged head must be a full commit id, got %q", landedHead) | ||
| 239 | return nil, nil, fmt.Errorf("requests: merged head must be a full commit id, got %q", landedHead) | ||
| 218 | 240 | } | |
| 219 | 241 | var ( | |
| 220 | out *Request | ||
| 221 | changed bool | ||
| 242 | out *Request | ||
| 243 | closed []string | ||
| 222 | 244 | ) | |
| 223 | 245 | base := fmt.Sprintf("%s/%s/", RequestsDir, id) | |
| 224 | 246 | _, err := git.CommitFiles(g, Ref, func(files map[string][]byte) error { | |
| @@ -239,17 +261,64 @@ func MarkMerged(g *git.Repo, id, landedHead, actor string, at time.Time) (*Reque | |||
| 239 | 261 | return err | |
| 240 | 262 | } | |
| 241 | 263 | files[base+requestName] = raw | |
| 264 | // The issues this request promised to close are closed in the same commit as | ||
| 265 | // the mark, so the merge and the closure cannot be separated by a crash. An | ||
| 266 | // issue that is already closed, or that is no longer there, is left alone: the | ||
| 267 | // promise is about what the merge did, not about overriding a later decision. | ||
| 268 | ids, err := closeIssues(files, r.Closes) | ||
| 269 | if err != nil { | ||
| 270 | return err | ||
| 271 | } | ||
| 272 | closed = ids | ||
| 242 | 273 | out = r | |
| 243 | changed = true | ||
| 244 | 274 | return nil | |
| 245 | 275 | }, fmt.Sprintf("request: %s merged %s", id, landedHead), actor) | |
| 246 | 276 | if errors.Is(err, ErrNoChange) { | |
| 247 | return out, false, nil | ||
| 277 | return out, nil, nil | ||
| 248 | 278 | } | |
| 249 | 279 | if err != nil { | |
| 250 | return nil, false, err | ||
| 280 | return nil, nil, err | ||
| 281 | } | ||
| 282 | return out, closed, nil | ||
| 283 | } | ||
| 284 | |||
| 285 | // closeIssues rewrites the named issues as closed inside an open metadata commit, | ||
| 286 | // and returns the ones it actually changed. Unknown and already-closed issues are | ||
| 287 | // skipped: a request may outlive the issue it was filed against, and a merge must not | ||
| 288 | // reopen-and-close something somebody closed on purpose. | ||
| 289 | func closeIssues(files map[string][]byte, ids []string) ([]string, error) { | ||
| 290 | if len(ids) == 0 { | ||
| 291 | return nil, nil | ||
| 292 | } | ||
| 293 | contents := make(map[string]string, len(files)) | ||
| 294 | for path, raw := range files { | ||
| 295 | if strings.HasPrefix(path, IssuesDir+"/") { | ||
| 296 | contents[path] = string(raw) | ||
| 297 | } | ||
| 298 | } | ||
| 299 | issues, err := ParseIssues(contents) | ||
| 300 | if err != nil { | ||
| 301 | return nil, fmt.Errorf("requests: cannot read the issues this request closes: %w", err) | ||
| 302 | } | ||
| 303 | byID := make(map[string]*Issue, len(issues)) | ||
| 304 | for _, iss := range issues { | ||
| 305 | byID[iss.ID] = iss | ||
| 306 | } | ||
| 307 | var closed []string | ||
| 308 | for _, id := range ids { | ||
| 309 | iss, ok := byID[id] | ||
| 310 | if !ok || iss.State == "closed" { | ||
| 311 | continue | ||
| 312 | } | ||
| 313 | iss.State = "closed" | ||
| 314 | raw, err := renderIssue(iss) | ||
| 315 | if err != nil { | ||
| 316 | return nil, err | ||
| 317 | } | ||
| 318 | files[fmt.Sprintf("%s/%s/%s", IssuesDir, id, issueName)] = raw | ||
| 319 | closed = append(closed, id) | ||
| 251 | 320 | } | |
| 252 | return out, changed, nil | ||
| 321 | return closed, nil | ||
| 253 | 322 | } | |
| 254 | 323 | ||
| 255 | 324 | // NewComment builds a comment on a request with a fresh id. | |