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.

Phase 3: a review comment points at a line, and stays glued to it

9cc2f08757a36a49c7baa3ce502f5934341e7a5b · Keenan Nemetz · 17d ago · parent 7c6e52e7

Phase 3: a review comment points at a line, and stays glued to it

A review thread can now say where it applies. A comment may carry an anchor — path,
side, line and blob — on the same file it already lives in, written by the same
reader and writer as before (four extra frontmatter lines, refused as a half-written
set if any is missing).

The interesting part is what it anchors on. A line number is not an identity: the
file moves and the number points at different code. A commit id is too coarse: any
unrelated change to the branch would invalidate every anchor. The **blob** of the
content reviewed is content-addressed, so a comment is glued to exactly what was
read: it stays current for as long as that file is unchanged — the same file at a
later head has the same blob id — and becomes stale the moment it changes, rather
than sliding to a line that now says something else. That is what "survive a
rewrite" has to mean, and the page enforces it by resolving the blob at the head (or
at the merge base, for `side: old`) and comparing.

Which is also why nothing is verified at the push boundary. An anchor is a display
hint, not an authorization: an anchor that lies can only make itself stale, and a
rule in pre-receive would have to re-check every old anchor against a head that has
since moved — rejecting pushes for comments nobody touched.

`vole request comment <id> -m … --path f --line n [--side old]` computes the blob
from the clone, refuses a line past the end of the file, and says `git fetch` rather
than writing an anchor it cannot compute. On the page, a current anchor puts a marker
under the line that links to the comment, and the card says `path:line (side)`; a
stale one keeps its place in the thread with "the file has changed since this
comment" and loses the marker. `side: old` covers reviewing a deletion, since that
line exists only at the merge base.

Verified: TestAnchorRoundTrips and TestAnchorMustBeWhole (blob is full-length, side
is new or old, line is 1 or more, path stays inside the repository, and half an
anchor is a refusal naming the file); and end to end through the binary,
TestReviewCommentAnchorsALine — a new-side comment whose blob matches
`rev-parse <head>:code.txt` on the server, the marker and the labelled card on the
page, an old-side comment whose blob matches the merge base, a line past the end
refused with the branch unmoved, and then a new head that changes the file, after
which the new-side comment is labelled stale and loses its marker while the old-side
one does not. scripts/ci.sh green (gofmt, vet, build, markdown links, go test -race
./...).

internal/server/integration_test.go +115

@@ -1260,6 +1260,121 @@ func TestRequestDiscussionIsPushedAndOwned(t *testing.T) {
1260 1260 }
1261 1261 }
1262 1262
1263 // TestReviewCommentAnchorsALine: a review comment can name the line it is about,
1264 // on either side of the diff, and it is glued to the *content* it saw — the blob
1265 // id — so it stays current while that file is untouched and is marked stale the
1266 // moment the file changes, instead of sliding to a line that now says something
1267 // else.
1268 func TestReviewCommentAnchorsALine(t *testing.T) {
1269 f := newFixture(t)
1270 f.publish(f.repo)
1271
1272 base := workRepo(t, map[string]string{"code.txt": "a\nb\nc\n"})
1273 if _, errs, err := f.git(base, "push", f.sshURL(f.repo), "main:refs/heads/main"); err != nil {
1274 t.Fatalf("main push: %s", errs)
1275 }
1276 feature, head := f.cloneWithChange(t, map[string]string{"code.txt": "a\nFEATURE\nc\n"}, "feature")
1277 if _, errs, err := f.git(feature, "push", f.sshURL(f.repo), "HEAD:refs/heads/feature"); err != nil {
1278 t.Fatalf("feature push: %s", errs)
1279 }
1280 meta := f.newMetaRepo(t)
1281 if err := meta.apply(map[string]string{
1282 "requests/" + reqID + "/request.md": requestFile("main", head, "keenan", f.ownerID, "rationale\n"),
1283 }, "request"); err != nil {
1284 t.Fatalf("request push: %v", err)
1285 }
1286 clone := filepath.Join(t.TempDir(), "clone")
1287 if _, errs, err := f.git("", "clone", "-q", f.sshURL(f.repo), clone); err != nil {
1288 t.Fatalf("clone: %s", errs)
1289 }
1290
1291 // New side: the line in the head's copy of the file.
1292 out, errs, err := f.cli(clone, "request", "comment", reqID, "-m", "why FEATURE?",
1293 "--path", "code.txt", "--line", "2")
1294 if err != nil {
1295 t.Fatalf("anchored comment: %v\n%s", err, errs)
1296 }
1297 newCm := strings.TrimSpace(out)
1298 serverFile := func(path string) string {
1299 text, _, _ := runGit(t, filepath.Join(f.cfg.ReposDir(), f.repo+".git"), "show", "refs/heads/vole:"+path)
1300 return text
1301 }
1302 wantBlob, _, _ := runGit(t, filepath.Join(f.cfg.ReposDir(), f.repo+".git"), "rev-parse", head+":code.txt")
1303 wantBlob = strings.TrimSpace(wantBlob)
1304 got := serverFile("requests/" + reqID + "/comments/" + newCm + ".md")
1305 for _, want := range []string{"path: code.txt", "side: new", "line: 2", "blob: " + wantBlob} {
1306 if !strings.Contains(got, want) {
1307 t.Fatalf("the anchor is missing %q:\n%s", want, got)
1308 }
1309 }
1310 // The page puts a marker on that line and labels the card with where it points.
1311 _, body := f.get("/r/" + f.repo + "/requests/" + reqID)
1312 if !strings.Contains(body, "keenan commented") || !strings.Contains(body, "#c-"+newCm) {
1313 t.Fatalf("the diff has no marker for the comment:\n%s", body)
1314 }
1315 if !strings.Contains(body, "code.txt:2") || !strings.Contains(body, "(new side)") {
1316 t.Fatalf("the comment card does not name its line:\n%s", body)
1317 }
1318
1319 // Old side: a line of the file as it stood at the merge base.
1320 out, errs, err = f.cli(clone, "request", "comment", reqID, "-m", "the old middle line",
1321 "--path", "code.txt", "--line", "2", "--side", "old")
1322 if err != nil {
1323 t.Fatalf("anchored comment (old side): %v\n%s", err, errs)
1324 }
1325 oldCm := strings.TrimSpace(out)
1326 mergeBase, _, _ := runGit(t, filepath.Join(f.cfg.ReposDir(), f.repo+".git"), "merge-base", "refs/heads/main", head)
1327 oldBlob, _, _ := runGit(t, filepath.Join(f.cfg.ReposDir(), f.repo+".git"),
1328 "rev-parse", strings.TrimSpace(mergeBase)+":code.txt")
1329 if got := serverFile("requests/" + reqID + "/comments/" + oldCm + ".md"); !strings.Contains(got, "blob: "+strings.TrimSpace(oldBlob)) {
1330 t.Fatalf("the old side anchor does not name the merge-base blob:\n%s", got)
1331 }
1332
1333 // A line that is not in the file is refused before anything is pushed.
1334 before := f.serverIssues(t)
1335 if _, errs, err := f.cli(clone, "request", "comment", reqID, "-m", "past the end",
1336 "--path", "code.txt", "--line", "99"); err == nil {
1337 t.Fatal("a line past the end of the file was accepted")
1338 } else if !strings.Contains(errs, "past the end") {
1339 t.Fatalf("unhelpful refusal: %s", errs)
1340 }
1341 if f.serverIssues(t) != before {
1342 t.Fatal("a refused anchor moved the branch")
1343 }
1344
1345 // A new head changes that file, so the new-side comment is stale while the
1346 // old-side one still points at content that has not moved.
1347 next, nextHead := func() (string, string) {
1348 if err := os.WriteFile(filepath.Join(feature, "code.txt"), []byte("a\nFEATURE v2\nc\n"), 0o644); err != nil {
1349 t.Fatal(err)
1350 }
1351 if _, errs, err := runGit(t, feature, "commit", "-qam", "v2"); err != nil {
1352 t.Fatalf("commit: %s", errs)
1353 }
1354 if _, errs, err := f.git(feature, "push", f.sshURL(f.repo), "HEAD:refs/heads/feature"); err != nil {
1355 t.Fatalf("push: %s", errs)
1356 }
1357 out, _, _ := runGit(t, feature, "rev-parse", "HEAD")
1358 return feature, strings.TrimSpace(out)
1359 }()
1360 // The new commit reached the clone by pushing it from elsewhere, so fetch before
1361 // naming it.
1362 if _, errs, err := f.git(clone, "fetch", "-q", "origin", "feature"); err != nil {
1363 t.Fatalf("fetch: %s", errs)
1364 }
1365 if _, errs, err := f.cli(clone, "request", "update", reqID, "--head", nextHead); err != nil {
1366 t.Fatalf("request update: %v\n%s", err, errs)
1367 }
1368 _, body = f.get("/r/" + f.repo + "/requests/" + reqID)
1369 if !strings.Contains(body, "the file has changed since this comment") {
1370 t.Fatalf("a moved file did not make the anchored comment stale:\n%s", body)
1371 }
1372 if strings.Contains(body, "#c-"+newCm+`">keenan commented`) {
1373 t.Fatalf("the stale comment kept its marker:\n%s", body)
1374 }
1375 _ = next
1376 }
1377
1263 1378 func (f *fixture) serverIssues(t *testing.T) string {
1264 1379 t.Helper()
1265 1380 out, _, _ := runGit(t, filepath.Join(f.cfg.ReposDir(), f.repo+".git"),