From f8add2ad22ca06214ac3c0275a6db5c5e02cc124 Mon Sep 17 00:00:00 2001 From: Spencer Brower Date: Fri, 12 Jun 2026 07:14:03 -0400 Subject: [PATCH] refactor(odin): Fixed AI mistakes. --- .tokeignore | 1 + TODOS.md | 49 ++++++++++++++++++++++ cmd_list.odin | 30 +++----------- cmd_list_test.odin | 26 ++++++++++++ db.odin | 5 +-- db_test.odin | 19 +++++++++ table.odin | 37 ++++++++++------- table_test.odin | 101 +++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 226 insertions(+), 42 deletions(-) create mode 100644 .tokeignore create mode 100644 TODOS.md create mode 100644 cmd_list_test.odin create mode 100644 db_test.odin create mode 100644 table_test.odin diff --git a/.tokeignore b/.tokeignore new file mode 100644 index 0000000..98b6c79 --- /dev/null +++ b/.tokeignore @@ -0,0 +1 @@ +**/*_test.{odin,go} diff --git a/TODOS.md b/TODOS.md new file mode 100644 index 0000000..56576e8 --- /dev/null +++ b/TODOS.md @@ -0,0 +1,49 @@ +# TODO + +Note: These todos can wait until all the subcommands have been ported. + +## HIGH + +1. [x] **table.odin:74-89** — Hand-rolled JSON output doesn't escape `"`, `\`, newlines. Reimplements `json.marshal` which is already imported in `cmd_list.odin`. Replace with `json.marshal`. + +2. **db.odin:380-383, 405, 446** — `sqlite.bind_text` return values overwritten but never checked. A failed bind means `sqlite.step` operates on unbound params. + +3. **config.odin:52-54** — `os.user_home_dir` error silently ignored. If it fails, `home` is `""` and all paths become relative (`".envr"` instead of `"~/.envr"`). + +## MEDIUM + +4. **db.odin:29-35** — `make_temp_path` never calls `strings.builder_destroy`. Leaks builder buffer every call. + +5. **db.odin:324-327** — Map iteration (`remote_set`) is non-deterministic. Same file can produce different JSON on each backup, causing spurious DB diffs. Sort remotes before storing. + +6. **db.odin:470-473** — `string_to_cstring` allocates via `strings.clone_to_cstring` and never frees. Called dozens of times across db operations. + +7. **db.odin:470, 462** — Both `string_to_cstring` and `cstring_to_string` ignore allocation errors. A nil cstring gets passed to SQLite (UB). + +8. **db.odin:135, 250** — String interpolation into SQL (`VACUUM INTO '%s'`, `ATTACH DATABASE '%s'`). Currently safe because input is controlled, but fragile. + +9. **features.odin:30-41** — `find_binary` uses `strings.join` instead of `filepath.join`, uses `os.stat` instead of checking executability, hardcodes `:` as PATH separator (wrong on Windows). + +10. **cmd_restore.odin:20-30 & cmd_remove.odin:19-29** — Identical path-resolution block copy-pasted. `is_abs` guard is redundant since `filepath.abs` is a no-op on absolute paths. Extract a helper. + +11. **cmd_restore.odin:44** — `os.mkdir_all` error silently discarded. Subsequent write failure will be confusing. + +12. **cmd_edit_config.odin:27** — `$EDITOR` used as single binary name. Breaks for multi-word values like `"code -w"`. Needs `strings.fields()`. + +13. [x] **cmd_list.odin:31-35, 58-61** — Uses a `strings.Builder` (never destroyed) for what is just `row.Dir + "/"`. Also `filepath.rel` used where `filepath.base` would suffice since dir is always the parent. + +## LOW + +14. [x] **db.odin:338-341** — Unnecessary `strings.clone` before `filepath.dir` (which already returns a slice into the input). + +15. **db.odin:115** — `json.unmarshal_string` error not checked. Malformed JSON silently produces empty/partial data. + +16. **db.odin:352-353** — `hex.encode` error ignored. `string(hex_bytes)` aliases the byte slice. + +18. **config.odin:51-60** — `envr_dir` recomputes home dir on every call. Could cache. + +19. **main.odin:42-46** — Dynamic array in `fallback_to_go` never deleted. Harmless since process exits. + +## REFACTOR + +20. **cmd_list.odin** — Non-TTY branch builds `ListEntry` structs and marshals JSON separately. Now that `render_json_rows` (issue 1) accepts an `io.Writer` and uses `json.marshal`, unify both branches to use it. Note: will change JSON keys from `"directory"/"path"` to `"Directory"/"Path"`. diff --git a/cmd_list.odin b/cmd_list.odin index 7bd8177..604ca9d 100644 --- a/cmd_list.odin +++ b/cmd_list.odin @@ -28,21 +28,11 @@ cmd_list :: proc(cmd: ^Command) { table_rows := make([dynamic][]string, 0, len(rows)) for row in rows { - b: strings.Builder - strings.builder_init(&b) - strings.write_string(&b, row.Dir) - strings.write_string(&b, "/") - dir_str, _ := strings.clone(strings.to_string(b)) - - rel, rel_err := filepath.rel(row.Dir, row.Path) - if rel_err != nil { - fmt.printf("Error getting relative path: %v\n", rel_err) - return - } - cloned_rel, _ := strings.clone(rel) + dir_str := strings.concatenate({row.Dir, "/"}) + filename := filepath.base(row.Path) row_slice := make([]string, 2) row_slice[0] = dir_str - row_slice[1] = cloned_rel + row_slice[1] = filename append(&table_rows, row_slice) } @@ -50,18 +40,10 @@ cmd_list :: proc(cmd: ^Command) { } else { entries: [dynamic]ListEntry for row in rows { - rel, rel_err := filepath.rel(row.Dir, row.Path) - if rel_err != nil { - fmt.printf("Error getting relative path: %v\n", rel_err) - return - } - b: strings.Builder - strings.builder_init(&b) - strings.write_string(&b, row.Dir) - strings.write_string(&b, "/") + filename := filepath.base(row.Path) append(&entries, ListEntry{ - Directory = strings.to_string(b), - Path = rel, + Directory = strings.concatenate({row.Dir, "/"}), + Path = filename, }) } diff --git a/cmd_list_test.odin b/cmd_list_test.odin new file mode 100644 index 0000000..b1458e9 --- /dev/null +++ b/cmd_list_test.odin @@ -0,0 +1,26 @@ +package main + +import "core:path/filepath" +import "core:testing" + +@(test) +test_filepath_base_equals_rel :: proc(t: ^testing.T) { + cases := []string{ + "/home/user/.env", + "/home/user/project/.envrc", + "/tmp/foo", + "/a/b/c/d.txt", + } + + for path in cases { + dir := filepath.dir(path) + rel, rel_err := filepath.rel(dir, path) + testing.expect(t, rel_err == nil, "filepath.rel returned an error") + base := filepath.base(path) + testing.expect( + t, + rel == base, + "filepath.rel(dir, path) should equal filepath.base(path)", + ) + } +} diff --git a/db.odin b/db.odin index 1dabc1f..8b44c51 100644 --- a/db.odin +++ b/db.odin @@ -338,9 +338,8 @@ new_env_file :: proc(path: string) -> (EnvFile, bool) { cloned_path, _ := strings.clone(abs_path) dir := filepath.dir(cloned_path) - cloned_dir, _ := strings.clone(dir) - remotes := get_git_remotes(cloned_dir) + remotes := get_git_remotes(dir) data, read_err := os.read_entire_file_from_path(cloned_path, context.allocator) if read_err != nil { @@ -354,7 +353,7 @@ new_env_file :: proc(path: string) -> (EnvFile, bool) { return EnvFile{ Path = cloned_path, - Dir = cloned_dir, + Dir = dir, Remotes = remotes, Sha256 = sha_str, contents = string(data), diff --git a/db_test.odin b/db_test.odin new file mode 100644 index 0000000..8565b7c --- /dev/null +++ b/db_test.odin @@ -0,0 +1,19 @@ +package main + +import "core:path/filepath" +import "core:strings" +import "core:testing" + +@(test) +test_dir_slice_owns_parent :: proc(t: ^testing.T) { + abs_path := "/home/user/project/.env" + cloned_path, _ := strings.clone(abs_path) + + dir := filepath.dir(cloned_path) + + testing.expect(t, dir == "/home/user/project", "filepath.dir should return parent directory") + testing.expect(t, len(dir) > 0, "dir should not be empty") + + cloned_dir, _ := strings.clone(dir) + testing.expect(t, cloned_dir == dir, "clone of dir should equal dir") +} diff --git a/table.odin b/table.odin index 3647f09..bafddde 100644 --- a/table.odin +++ b/table.odin @@ -1,11 +1,16 @@ package main +import "core:encoding/json" import "core:fmt" +import "core:io" +import "core:os" import "core:strings" render_table :: proc(headers: []string, rows: [][]string) { if !is_tty() { - render_json_rows(headers, rows) + w := io.to_writer(os.to_writer(os.stdout)) + render_json_rows(w, headers, rows) + io.write_string(w, "\n") return } @@ -71,20 +76,22 @@ render_table :: proc(headers: []string, rows: [][]string) { hline(&b, "\u2514", "\u2534", "\u2518", col_widths) } -render_json_rows :: proc(headers: []string, rows: [][]string) { - fmt.print("[") - for i in 0.. 0 { - fmt.print(",") +render_json_rows :: proc(w: io.Writer, headers: []string, rows: [][]string) { + entries := make([dynamic]map[string]string, 0, len(rows)) + defer delete(entries) + + for row in rows { + entry: map[string]string + for i in 0.. 0 { - fmt.print(",") - } - fmt.printf("\"%s\":\"%s\"", headers[j], rows[i][j]) - } - fmt.print("}") + append(&entries, entry) } - fmt.println("]") + + data, err := json.marshal(entries[:]) + if err != nil { + fmt.eprintf("Error marshaling JSON: %v\n", err) + return + } + io.write_string(w, string(data)) } diff --git a/table_test.odin b/table_test.odin new file mode 100644 index 0000000..c884412 --- /dev/null +++ b/table_test.odin @@ -0,0 +1,101 @@ +package main + +import "core:encoding/json" +import "core:fmt" +import "core:io" +import "core:strings" +import "core:testing" + +@(test) +test_render_json_rows_normal :: proc(t: ^testing.T) { + b: strings.Builder + strings.builder_init(&b) + defer strings.builder_destroy(&b) + + headers := []string{"name", "path"} + rows := [][]string{{"foo", "/home/user/.env"}, {"bar", "/home/user/project/.env"}} + + w := strings.to_writer(&b) + render_json_rows(w, headers, rows) + + output := strings.to_string(b) + + result: []map[string]string + unmarshal_err := json.unmarshal_string(output, &result) + testing.expect( + t, + unmarshal_err == nil, + fmt.aprintf("json unmarshal failed: %v\noutput was: %q", unmarshal_err, output), + ) + testing.expect(t, len(result) == 2, fmt.aprintf("expected 2 rows, got %d", len(result))) + testing.expect( + t, + result[0]["name"] == "foo", + fmt.aprintf("expected name=foo, got %q", result[0]["name"]), + ) + testing.expect(t, result[0]["path"] == "/home/user/.env") + testing.expect(t, result[1]["name"] == "bar") + testing.expect(t, result[1]["path"] == "/home/user/project/.env") +} + +@(test) +test_render_json_rows_special_chars :: proc(t: ^testing.T) { + b: strings.Builder + strings.builder_init(&b) + defer strings.builder_destroy(&b) + + headers := []string{"key", "value"} + rows := [][]string { + {"quote", `has "double quotes"`}, + {"backslash", `path\to\file`}, + {"newline", "line1\nline2"}, + {"mixed", `a "b" c\nd`}, + } + + w := strings.to_writer(&b) + render_json_rows(w, headers, rows) + + output := strings.to_string(b) + + result: []map[string]string + unmarshal_err := json.unmarshal(transmute([]byte)output, &result) + testing.expect( + t, + unmarshal_err == nil, + fmt.aprintf("json unmarshal failed: %v\noutput was: %q", unmarshal_err, output), + ) + testing.expect(t, len(result) == 4) + testing.expect( + t, + result[0]["value"] == `has "double quotes"`, + fmt.aprintf("got %q", result[0]["value"]), + ) + testing.expect(t, result[1]["value"] == `path\to\file`) + testing.expect(t, result[2]["value"] == "line1\nline2") + testing.expect(t, result[3]["value"] == `a "b" c\nd`) +} + +@(test) +test_render_json_rows_empty :: proc(t: ^testing.T) { + b: strings.Builder + strings.builder_init(&b) + defer strings.builder_destroy(&b) + + headers := []string{"name"} + rows: [][]string + + w := strings.to_writer(&b) + render_json_rows(w, headers, rows) + + output := strings.to_string(b) + + result: []map[string]string + unmarshal_err := json.unmarshal_string(output, &result) + testing.expect( + t, + unmarshal_err == nil, + fmt.aprintf("json unmarshal failed: %v\noutput was: %q", unmarshal_err, output), + ) + testing.expect(t, len(result) == 0) +} +