commit 8f323544e21085b7879909fd01c1a44cea630708
parent cdc53fe8abd7ef32d5f46b3522875c2db7ee0ce7
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Wed, 23 Sep 2026 21:02:42 -0300
odin: read the archive and bound a run without tar or timeout programs
tree unpacks a range's end with jfm:tar from git archive's output read
in memory, and the analysers and providers bound a child's run with the
collection's exec timeout, so nothing under odin/ runs the tar or timeout
programs any more and both seams close on Windows.
The first live run against the Go tool, sonnet through claude, replayed
nothing from the shared cache: the rendered subjects differed. Both tools
now order symbols, tests and comments by file then line, where the Go
tool had read its frontends in map order and so missed its own cache on
a multi-language change; and the Odin git wrapper keeps the raw diff,
stat and message as the Go tool does. The subjects both tools render are
byte for byte the same.
Diffstat:
8 files changed, 132 insertions(+), 28 deletions(-)
diff --git a/frontend.go b/frontend.go
@@ -1,6 +1,7 @@
package main
import (
+ "cmp"
"fmt"
"maps"
"os"
@@ -163,4 +164,33 @@ func (c *Change) read(root, rev string, frontends []Frontend) {
missing(covers, f, FeatTests, "tests")
missing(covers, f, FeatComments, "claims")
}
+ c.order()
+}
+
+// order puts what the frontends read into the diff's own order — file as
+// git lists it, then line — so that a subject renders the same on every
+// run whichever frontend answered first, and the answer cache is hit.
+func (c *Change) order() {
+ position := map[string]int{}
+ for i, f := range c.Files {
+ position[f] = i
+ }
+ slices.SortStableFunc(c.Symbols, func(a, b Symbol) int {
+ if d := cmp.Compare(position[a.File], position[b.File]); d != 0 {
+ return d
+ }
+ return cmp.Compare(a.Line, b.Line)
+ })
+ slices.SortStableFunc(c.Tests, func(a, b Function) int {
+ if d := cmp.Compare(position[a.File], position[b.File]); d != 0 {
+ return d
+ }
+ return cmp.Compare(a.Line, b.Line)
+ })
+ slices.SortStableFunc(c.Comments, func(a, b Located) int {
+ if d := cmp.Compare(position[a.File], position[b.File]); d != 0 {
+ return d
+ }
+ return cmp.Compare(a.Line, b.Line)
+ })
}
diff --git a/odin/analyser/analyser.odin b/odin/analyser/analyser.odin
@@ -15,6 +15,7 @@ import "core:path/filepath"
import "core:strconv"
import "core:strings"
import "core:text/regex"
+import "core:time"
import "jfm:sh"
import "../change"
@@ -26,6 +27,10 @@ import "../tree"
// rather than holding the review.
timeout_seconds :: 300
+// timeout_seconds_override is the bound a test lowers; zero means the
+// constant above.
+timeout_seconds_override: int
+
// Diagnostic is one thing an analyser said, located in the tree. A fault
// is a compile error — the tree does not build — rather than an
// analyser's opinion about code that does.
@@ -187,17 +192,15 @@ execute_both :: proc(
err: string,
) {
argv := make([dynamic]string, context.temp_allocator)
- if _, found := sh.which("timeout", context.temp_allocator); found {
- append(&argv, "timeout", fmt.tprintf("%d", timeout_seconds))
- }
append(&argv, name)
append(&argv, ..args)
- r := sh.exec(argv[:], {dir = dir}, allocator)
+ bound := timeout_seconds if timeout_seconds_override == 0 else timeout_seconds_override
+ r := sh.exec(argv[:], {dir = dir, timeout = time.Duration(bound) * time.Second}, allocator)
if r.err != nil {
return "", "", fmt.aprintf("%s: %s", name, os.error_string(r.err), allocator = allocator)
}
- if r.code == 124 && len(argv) > 2 && argv[0] == "timeout" {
- return r.stdout, r.stderr, fmt.aprintf("a run past %d seconds is not waited for", timeout_seconds, allocator = allocator)
+ if r.timed_out {
+ return r.stdout, r.stderr, fmt.aprintf("a run past %d seconds is not waited for", bound, allocator = allocator)
}
if !r.ok {
detail := strings.trim_space(r.stderr)
diff --git a/odin/analyser/analyser_test.odin b/odin/analyser/analyser_test.odin
@@ -3,6 +3,7 @@ package analyser
import "core:fmt"
import "core:os"
import "core:path/filepath"
+import "core:strings"
import "core:testing"
import "jfm:sh"
@@ -179,6 +180,18 @@ parse_tsc_ruff_mypy_cargo_semgrep :: proc(t: ^testing.T) {
}
@(test)
+a_run_past_the_timeout_is_not_waited_for :: proc(t: ^testing.T) {
+ saved := timeout_seconds_override
+ timeout_seconds_override = 1
+ defer timeout_seconds_override = saved
+ _, _, err := execute_both(".", "sleep", {"3"}, context.temp_allocator)
+ testing.expect(t, strings.contains(err, "not waited for"), err)
+ out, _, ok_err := execute_both(".", "echo", {"quick"}, context.temp_allocator)
+ testing.expect_value(t, ok_err, "")
+ testing.expect_value(t, out, "quick\n")
+}
+
+@(test)
nearest_walks_up :: proc(t: ^testing.T) {
temp := os.temp_directory(context.temp_allocator) or_else ""
root, err := os.make_directory_temp(temp, "review-analyser-*", context.temp_allocator)
diff --git a/odin/change/change.odin b/odin/change/change.odin
@@ -148,15 +148,15 @@ gather :: proc(rev, root: string, allocator := context.allocator) -> (c: Change,
name_args = {"diff", rev, "--name-only"}
stat_args = {"diff", rev, "--stat"}
}
- c.diff = git.run(root, diff_args) or_return
+ c.diff = git.raw(root, diff_args) or_return
if len(c.diff) > max_diff {
c.diff = strings.concatenate({c.diff[:max_diff], "\n… diff truncated\n"})
c.truncated = true
}
c.files = git.lines(root, name_args) or_return
- c.stat = git.run(root, stat_args) or_return
+ c.stat = git.raw(root, stat_args) or_return
if rev != "" {
- c.message, _ = git.run(root, {"log", "-1", "--format=%B", strings.trim_suffix(rev, "^")})
+ c.message, _ = git.raw(root, {"log", "-1", "--format=%B", strings.trim_suffix(rev, "^")})
}
// The whitespace-only lines are what the diff loses when git is asked
// to ignore whitespace; the difference is the formatting mixed in.
@@ -497,9 +497,48 @@ read :: proc(c: ^Change, t: tree.Tree, allocator := context.allocator) -> bool {
}
}
annotate(c, t)
+ order(c)
return ok
}
+// order puts what the sidecars read into the diff's own order — file as
+// git lists it, then line — so that a subject renders the same on every
+// run whichever sidecar answered first, and the answer cache is hit.
+order :: proc(c: ^Change) {
+ position := make(map[string]int, context.temp_allocator)
+ for f, i in c.files {
+ position[f] = i
+ }
+ positions = &position
+ defer positions = nil
+ slice.stable_sort_by_cmp(c.symbols[:], proc(a, b: Symbol) -> slice.Ordering {
+ return before(a.file, a.line, b.file, b.line)
+ })
+ slice.stable_sort_by_cmp(c.tests[:], proc(a, b: Function) -> slice.Ordering {
+ return before(a.file, a.line, b.file, b.line)
+ })
+ slice.stable_sort_by_cmp(c.comments[:], proc(a, b: Located) -> slice.Ordering {
+ return before(a.file, a.line, b.file, b.line)
+ })
+}
+
+// positions is the file order under sort, reachable from the comparators,
+// which cannot capture it.
+@(private)
+positions: ^map[string]int
+
+@(private)
+before :: proc(file_a: string, line_a: int, file_b: string, line_b: int) -> slice.Ordering {
+ pa, pb := positions[file_a], positions[file_b]
+ if pa != pb {
+ return .Less if pa < pb else .Greater
+ }
+ if line_a != line_b {
+ return .Less if line_a < line_b else .Greater
+ }
+ return .Equal
+}
+
// annotate adds to what the sidecars read the parts every language
// shares: the code below each comment, and the line a test skips itself
// on. Both are read by shape from the source.
diff --git a/odin/change/change_test.odin b/odin/change/change_test.odin
@@ -3,6 +3,7 @@ package change
import "core:fmt"
import "core:os"
import "core:path/filepath"
+import "core:strings"
import "core:testing"
import "jfm:sh"
@@ -106,7 +107,7 @@ gather_reads_a_range_through_the_sidecar :: proc(t: ^testing.T) {
c, ok := gather("HEAD^..HEAD", root, context.temp_allocator)
testing.expect(t, ok, "gather")
testing.expect_value(t, len(c.files), 2)
- testing.expect_value(t, c.message, "a: add added")
+ testing.expect_value(t, strings.trim_space(c.message), "a: add added")
testing.expect_value(t, c.truncated, false)
tr, at_ok := tree.at(root, "HEAD^..HEAD", context.temp_allocator)
diff --git a/odin/git/git.odin b/odin/git/git.odin
@@ -29,6 +29,27 @@ run :: proc(
return strings.trim_right_space(r.stdout), true
}
+// raw asks git and returns what it printed as it printed it, trailing
+// newlines kept: a diff, a stat or a message rendered into a prompt has
+// to be byte for byte what the Go tool renders.
+raw :: proc(
+ root: string,
+ args: []string,
+ allocator := context.allocator,
+) -> (
+ out: string,
+ ok: bool,
+) {
+ argv := make([]string, len(args) + 1, context.temp_allocator)
+ argv[0] = "git"
+ copy(argv[1:], args)
+ r := sh.exec(argv, {dir = root}, allocator)
+ if !r.ok {
+ return r.stderr, false
+ }
+ return r.stdout, true
+}
+
// lines is run, split into lines, empty ones dropped.
lines :: proc(
root: string,
diff --git a/odin/provider/provider.odin b/odin/provider/provider.odin
@@ -11,6 +11,7 @@ import "core:encoding/json"
import "core:fmt"
import "core:os"
import "core:strings"
+import "core:time"
import "jfm:sh"
// Answer is what a provider returned, with whatever it could say about
@@ -355,18 +356,15 @@ shell :: proc(
out: string,
err: string,
) {
- full := make([dynamic]string, context.temp_allocator)
- timed := false
- if _, found := sh.which("timeout", context.temp_allocator); found {
- append(&full, "timeout", fmt.tprintf("%d", ask_timeout_seconds()))
- timed = true
- }
- append(&full, ..argv)
- r := sh.exec(full[:], {stdin = prompt}, allocator)
+ r := sh.exec(
+ argv,
+ {stdin = prompt, timeout = time.Duration(ask_timeout_seconds()) * time.Second},
+ allocator,
+ )
if r.err != nil {
return "", fmt.aprintf("%s: %s", argv[0], os.error_string(r.err), allocator = allocator)
}
- if timed && r.code == 124 {
+ if r.timed_out {
return "", fmt.aprintf(
"%s: no answer within %d seconds",
argv[0],
diff --git a/odin/tree/tree.odin b/odin/tree/tree.odin
@@ -7,12 +7,12 @@ Every reader after that reads files, which is what a sidecar can be handed.
*/
package tree
-import "core:fmt"
import "core:os"
import "core:path/filepath"
import "core:slice"
import "core:strings"
import "jfm:sh"
+import "jfm:tar"
import "../git"
@@ -46,24 +46,23 @@ close :: proc(t: Tree) {
}
// materialise writes the revision's files to a scratch directory, out of
-// git's own archive of it: one ask, however many files. The archive is
-// unpacked by the tar on the path, which every platform now ships.
+// git's own archive of it: one ask, however many files, unpacked here
+// rather than by a tar program the platform may not have.
materialise :: proc(root, rev: string, allocator := context.allocator) -> (dir: string, ok: bool) {
temp := os.temp_directory(context.temp_allocator) or_else ""
scratch, err := os.make_directory_temp(temp, "review-tree-*", allocator)
if err != nil {
return "", false
}
- cmd := fmt.tprintf(
- "git archive --format=tar %s | tar -x -C %s",
- sh.quote(rev, context.temp_allocator),
- sh.quote(scratch, context.temp_allocator),
- )
- r := sh.capture(cmd, {dir = root}, context.temp_allocator)
+ r := sh.exec({"git", "archive", "--format=tar", rev}, {dir = root}, context.temp_allocator)
if !r.ok {
os.remove_all(scratch)
return "", false
}
+ if _, unpacked := tar.extract(transmute([]byte)r.stdout, scratch); unpacked != .None {
+ os.remove_all(scratch)
+ return "", false
+ }
return scratch, true
}
