Audit date: 2026-10-01

This is a prioritized list of defects and improvement opportunities found by reviewing the current tree and running the Go test suite, race detector, go vet, Staticcheck, and golangci-lint. The ordinary tests and race-enabled tests pass; the static-analysis findings described below remain.

1. High priority

1.1. Stop copying values that contain mutexes

go vet -copylocks ./…​ reports thirteen diagnostics in four groups:

  • Six Blob methods have value receivers and copy Blob.opsetLock (surgeon/inner.go:1327, 1343, 1571, 1576, 1594, and 1748).

  • Repository.clone copies the entire repository, including _markToIndexLock, with newRepo := *repo (surgeon/inner.go:5499). Clearing the copied lock afterward prevents its state from being used, but does not make the original copy operation valid under Go’s mutex rules.

  • skipIterator and commitIterator use Repository value receivers (surgeon/selection.go:207 and 232). Besides copying the mutex, this needlessly copies the entire repository and makes the iterator point at that shallow copy.

  • Four svnReader helpers use value receivers and copy maplock (surgeon/svnread.go:123, 131, 150, and 183).

Use pointer receivers for the Blob, iterator, and svnReader methods. Make the repository clone explicit, or move the cache and its mutex behind a separately allocated pointer, so cloning never copies a live lock. Keep go vet -copylocks ./…​ as a required check to prevent regressions.

2. Medium priority

2.1. Add executable coverage for Unity Version Control support

The Unity support has no tests for selector parsing or command construction. unityRepositorySpec exists separately in both surgeon/vcs.go and tool/vcs.go, while the tool copy is unused. The integration depends on the exact .plastic/plastic.selector syntax and on the external cm fast-export file-writing behavior (surgeon/vcs.go:70 and 612; surgeon/inner.go:8562).

Add table-driven tests for rep and repository selectors, whitespace, missing and malformed selectors, repository specifications containing spaces, and exporter failure or a missing output file. Add an integration test using a fake cm executable on PATH to verify argument boundaries and export-file handling without requiring a Unity server. Remove the unused tool copy or put the shared parser in a small common package.

3. Lower-priority improvements

3.1. Reduce unchecked filesystem and stream operations

golangci-lint reports numerous ignored errors, especially in tool/repotool.go and cutter/repocutter.go. Not every terminal-status write needs elaborate recovery, but filesystem mutations, seeks, closes after writes, and generated file writes should be checked. Particularly useful starting points are makeStub (tool/repotool.go:336), checkout-directory creation (tool/repotool.go:998), and author-map output (mapper/repomapper.go:110).

Adopt an explicit policy: propagate data and filesystem errors; log or annotate best-effort terminal-display errors. Enable errcheck with a short, documented exclusion list rather than accepting the current broad set.

4. Verification summary

The following checks were run against the audited tree:

go test ./...
go test -race ./...
go vet ./...
golangci-lint run --timeout=5m ./...

Both test commands passed. go vet failed only on the thirteen mutex-copy diagnostics summarized above. golangci-lint produced additional style, deprecation, unused-code, and unchecked-error reports; this document selects the findings most likely to affect correctness or materially improve the code.