From 0523c09601385be5b765b257e3e5ed876f765063 Mon Sep 17 00:00:00 2001 From: Spencer Brower Date: Thu, 18 Jun 2026 11:58:33 -0400 Subject: [PATCH] refactor: Gave db its own allocator. --- TODOS.md | 2 + cmd_check.odin | 2 - cmd_edit_config.odin | 3 +- cmd_list.odin | 2 - config.odin | 21 ++++--- db.odin | 130 +++++++++++++++++++++++++------------------ db_test.odin | 103 +++++++++++++--------------------- 7 files changed, 129 insertions(+), 134 deletions(-) diff --git a/TODOS.md b/TODOS.md index 6617afe..2e11309 100644 --- a/TODOS.md +++ b/TODOS.md @@ -4,6 +4,8 @@ 27. Commands are still leaking. +28. **db.odin** — Inconsistencies in how struct vs sqlite are named. + 2. Generate md and man pages again. 3. **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. diff --git a/cmd_check.odin b/cmd_check.odin index 3d926d1..e59b6d8 100644 --- a/cmd_check.odin +++ b/cmd_check.odin @@ -54,8 +54,6 @@ cmd_check :: proc(cmd: ^Command) { if !list_ok { return } - defer delete(db_files) - defer for &file in db_files {delete_envfile(&file)} not_backed := find_unbacked(files_in_path[:], db_files[:]) diff --git a/cmd_edit_config.odin b/cmd_edit_config.odin index 32eac36..c9a7b4c 100644 --- a/cmd_edit_config.odin +++ b/cmd_edit_config.odin @@ -12,8 +12,7 @@ cmd_edit_config :: proc(cmd: ^Command) { config_path := cmd.config_path - _, stat_err := os.stat(config_path, context.allocator) - if stat_err != nil { + if !os.exists(config_path) { fmt.wprintf( cmd.err, "Config file does not exist at %s. Run 'envr init' first.\n", diff --git a/cmd_list.odin b/cmd_list.odin index 17157b2..f6c4e6f 100644 --- a/cmd_list.odin +++ b/cmd_list.odin @@ -25,8 +25,6 @@ cmd_list :: proc(cmd: ^Command) { if !list_ok { return } - defer delete(rows) - defer for &row in rows {delete_envfile(&row)} if terminal.is_terminal(os.stdout) { headers := []string{"Directory", "Path"} diff --git a/config.odin b/config.odin index b313bad..9373d83 100644 --- a/config.odin +++ b/config.odin @@ -25,17 +25,16 @@ Config :: struct { config_path: string `json:"-"`, } -load_config :: proc(config_path: string) -> (Config, bool) { - data, read_err := os.read_entire_file_from_path(config_path, context.allocator) +load_config :: proc(config_path: string, allocator := context.allocator) -> (Config, bool) { + // TODO: Should we use context.allocator + defer delete()? + data, read_err := os.read_entire_file_from_path(config_path, context.temp_allocator) if read_err != nil { fmt.println("No config file found. Please run `envr init` to generate one.") return Config{}, false } - defer delete(data) cfg: Config - // TODO: use json 5 - err := json.unmarshal(data, &cfg) + err := json.unmarshal(data, &cfg, .JSON5, allocator) if err != nil { fmt.printf("Error parsing config: %v\n", err) return Config{}, false @@ -53,22 +52,22 @@ default_config_path :: proc(home: string, allocator := context.allocator) -> str return path } -delete_config :: proc(cfg: ^Config) { +delete_config :: proc(cfg: ^Config, allocator := context.allocator) { for key in cfg.Keys { - delete(key.Private) - delete(key.Public) + delete(key.Private, allocator) + delete(key.Public, allocator) } delete(cfg.Keys) - delete(cfg.ScanConfig.Matcher) + delete(cfg.ScanConfig.Matcher, allocator) for exclude in cfg.ScanConfig.Exclude { - delete(exclude) + delete(exclude, allocator) } delete(cfg.ScanConfig.Exclude) for include in cfg.ScanConfig.Include { - delete(include) + delete(include, allocator) } delete(cfg.ScanConfig.Include) } diff --git a/db.odin b/db.odin index a92fc5c..fd3579f 100644 --- a/db.odin +++ b/db.odin @@ -5,6 +5,7 @@ import "core:encoding/hex" import "core:encoding/ini" import "core:encoding/json" import "core:fmt" +import "core:mem" import "core:os" import "core:path/filepath" import "core:strings" @@ -21,16 +22,12 @@ SyncFlagEnum :: enum { SyncFlag :: bit_set[SyncFlagEnum] -SyncDirection :: enum { - TrustDatabase, - TrustFilesystem, -} - Db :: struct { // Pointer to the sqlite db db: ^rawptr, cfg: Config, changed: bool, + arena: mem.Dynamic_Arena, } EnvFile :: struct { @@ -41,6 +38,7 @@ EnvFile :: struct { contents: string, } +@(deprecated = "call db_close to clean up EnvFiles") delete_envfile :: proc(f: ^EnvFile) { delete(f.Path) for &remote in f.Remotes { @@ -52,32 +50,15 @@ delete_envfile :: proc(f: ^EnvFile) { } db_open :: proc(cfg_path: string) -> (database: Db, ok: bool) { - database.cfg = load_config(cfg_path) or_return - - { - db: ^rawptr - rc := sqlite.db_open(":memory:", &db) - if rc != sqlite.OK { - fmt.printf("Error opening in-memory database: %s\n", sqlite.db_errmsg(db)) - return - } - - create_sql: cstring = "CREATE TABLE IF NOT EXISTS envr_env_files (path TEXT PRIMARY KEY NOT NULL, remotes TEXT, sha256 TEXT NOT NULL, contents TEXT NOT NULL)" - rc = sqlite.db_exec(db, create_sql, nil, nil, nil) - if rc != sqlite.OK { - fmt.printf("Error creating table: %s\n", sqlite.db_errmsg(db)) - sqlite.db_close(db) - return - } - database.db = db - } + database = db_init() or_return + database.cfg = load_config(cfg_path, db_allocator(&database)) or_return // TODO: Use different allocators? data_path := data_path(database.cfg.config_path, context.temp_allocator) if os.exists(data_path) { if ok = db_restore_from_encrypted(&database, data_path); !ok { sqlite.db_close(database.db) - return + return database, false } } else { // DB was created @@ -87,14 +68,42 @@ db_open :: proc(cfg_path: string) -> (database: Db, ok: bool) { return database, true } +// Creates a database an allocator and fresh, empty table, with zero encryption. +// In production, you most likely want to use `db_open`. +db_init :: proc() -> (database: Db, ok: bool) { + db: ^rawptr + rc := sqlite.db_open(":memory:", &db) + if rc != sqlite.OK { + fmt.printf("Error opening in-memory database: %s\n", sqlite.db_errmsg(db)) + return + } + + create_sql: cstring = "CREATE TABLE IF NOT EXISTS envr_env_files (path TEXT PRIMARY KEY NOT NULL, remotes TEXT, sha256 TEXT NOT NULL, contents TEXT NOT NULL)" + rc = sqlite.db_exec(db, create_sql, nil, nil, nil) + if rc != sqlite.OK { + fmt.printf("Error creating table: %s\n", sqlite.db_errmsg(db)) + sqlite.db_close(db) + return + } + database.db = db + + mem.dynamic_arena_init(&database.arena) + + return database, true +} + +db_allocator :: proc(db: ^Db) -> mem.Allocator { + return mem.dynamic_arena_allocator(&db.arena) +} + db_restore_from_encrypted :: proc(db: ^Db, data_path: string) -> bool { - encrypted_data, read_err := os.read_entire_file_from_path(data_path, context.allocator) - defer delete(encrypted_data) + encrypted_data, read_err := os.read_entire_file_from_path(data_path, context.temp_allocator) if read_err != nil { fmt.printf("Error reading encrypted database: %v\n", read_err) return false } + // TODO: Use context.temp_allocator plaintext, dec_ok := decrypt(encrypted_data, db.cfg.Keys[:]) if !dec_ok { fmt.println("Error: decryption failed") @@ -128,8 +137,15 @@ db_restore_from_encrypted :: proc(db: ^Db, data_path: string) -> bool { } db_close :: proc(d: ^Db) { - defer sqlite.db_close(d.db) - defer delete_config(&d.cfg) + allocator := db_allocator(d) + + defer { + sqlite.db_close(d.db) + + delete_config(&d.cfg, allocator) + + mem.dynamic_arena_destroy(&d.arena) + } if d.changed { rc := sqlite.db_exec(d.db, "VACUUM", nil, nil, nil) @@ -147,13 +163,14 @@ db_close :: proc(d: ^Db) { defer sqlite.free(data) sqlite_data := data[:sz] + // TODO: PAss allocator chain encrypted, enc_ok := encrypt(sqlite_data, d.cfg.Keys[:]) if !enc_ok { fmt.println("Error: encryption failed") return } - data_path := data_path(d.cfg.config_path) + data_path := data_path(d.cfg.config_path, allocator) envr_d := envr_dir(d.cfg.config_path) os.mkdir_all(envr_d) @@ -168,7 +185,8 @@ db_close :: proc(d: ^Db) { } } -db_list :: proc(d: ^Db, allocator := context.allocator) -> (results: [dynamic]EnvFile, ok: bool) { +// Results will be freed when `db_close` is called. +db_list :: proc(d: ^Db) -> ([]EnvFile, bool) { stmt: ^rawptr rc := sqlite.prepare_v2( d.db, @@ -179,10 +197,13 @@ db_list :: proc(d: ^Db, allocator := context.allocator) -> (results: [dynamic]En ) if rc != sqlite.OK { fmt.printf("Error preparing query: %s\n", sqlite.db_errmsg(d.db)) - return + return []EnvFile{}, false } defer sqlite.finalize(stmt) + allocator := db_allocator(d) + results := make([dynamic]EnvFile, 0, 10, allocator) + for { rc = sqlite.step(stmt) if rc == sqlite.DONE { @@ -190,7 +211,7 @@ db_list :: proc(d: ^Db, allocator := context.allocator) -> (results: [dynamic]En } if rc != sqlite.ROW { fmt.printf("Error stepping query: %s\n", sqlite.db_errmsg(d.db)) - return + #no_bounds_check return results[:], false } remotes_json := string(sqlite.column_text(stmt, 1)) @@ -212,17 +233,16 @@ db_list :: proc(d: ^Db, allocator := context.allocator) -> (results: [dynamic]En ) } - ok = true - return + #no_bounds_check return results[:], true } +// TODO: Should we use context.temp_allocator for proc scoped lifetimes? db_insert :: proc(d: ^Db, file: EnvFile) -> bool { - remotes_json, marshal_err := json.marshal(file.Remotes) + remotes_json, marshal_err := json.marshal(file.Remotes, allocator = context.temp_allocator) if marshal_err != nil { fmt.printf("Error marshaling remotes: %v\n", marshal_err) return false } - defer delete(remotes_json) sql: cstring = "INSERT OR REPLACE INTO " + @@ -278,7 +298,8 @@ db_insert :: proc(d: ^Db, file: EnvFile) -> bool { return true } -db_fetch :: proc(d: ^Db, path: string, allocator := context.allocator) -> (EnvFile, bool) { +// Result will be freed when `db_close` is called. +db_fetch :: proc(d: ^Db, path: string) -> (EnvFile, bool) { sql: cstring = "SELECT path, remotes, sha256, contents FROM envr_env_files WHERE path = ?" stmt: ^rawptr rc := sqlite.prepare_v2(d.db, sql, -1, &stmt, nil) @@ -288,6 +309,8 @@ db_fetch :: proc(d: ^Db, path: string, allocator := context.allocator) -> (EnvFi } defer sqlite.finalize(stmt) + allocator := db_allocator(d) + cpath := to_cstring(path, allocator) defer delete(cpath, allocator) rc = sqlite.bind_text(stmt, 1, cpath, -1, nil) @@ -311,7 +334,7 @@ db_fetch :: proc(d: ^Db, path: string, allocator := context.allocator) -> (EnvFi json.unmarshal_string(remotes_json, &remotes, allocator = allocator) } - file_path := clone_cstring(sqlite.column_text(stmt, 0)) + file_path := clone_cstring(sqlite.column_text(stmt, 0), allocator) return EnvFile { Path = file_path, @@ -466,7 +489,7 @@ find_moved_dirs :: proc(d: ^Db, f: ^EnvFile) -> ([dynamic]string, bool) { update_dir :: proc(f: ^EnvFile, new_dir: string) { f.Dir = new_dir base := filepath.base(f.Path) - new_path, _ := strings.concatenate({new_dir, "/", base}) + new_path, _ := filepath.join({new_dir, base}) f.Path = new_path f.Remotes = get_git_remotes(new_dir) } @@ -504,29 +527,30 @@ shares_remote :: proc(f: ^EnvFile, remotes: []string) -> bool { } get_git_remotes :: proc(dir: string) -> [dynamic]string { - remotes: [dynamic]string - remote_set: map[string]bool - defer delete(remote_set) - config_path, _ := filepath.join({dir, ".git", "config"}, context.temp_allocator) - m, _, ok := ini.load_map_from_path(config_path, context.allocator) + // TODO: Handle error + m, _, ok := ini.load_map_from_path(config_path, context.temp_allocator) if !ok { - return remotes + return nil } + remotes := make([dynamic]string, 0, 1) + for section_name, section in m { if strings.has_prefix(section_name, "remote ") { if url, ok := section["url"]; ok { - remote_set[url] = true + found := false + for r in remotes { + if r == url {found = true; break} + } + if !found { + cloned, _ := strings.clone(url) + append(&remotes, cloned) + } } } } - for remote in remote_set { - cloned, _ := strings.clone(remote) - append(&remotes, cloned) - } - return remotes } @@ -548,7 +572,7 @@ string_to_cstring :: proc(s: string, allocator := context.allocator) -> cstring return cs } -// Caller is responsible for freeing the result +// Unless an explicit allocator is passed, caller is responsible for freeing the result clone_cstring :: proc(c: cstring, allocator := context.allocator) -> string { str, err := strings.clone_from_cstring(c, allocator) if err != nil { diff --git a/db_test.odin b/db_test.odin index bf6a91f..27b32c5 100644 --- a/db_test.odin +++ b/db_test.odin @@ -8,23 +8,6 @@ import "core:testing" import "sqlite" -make_test_db :: proc() -> (Db, bool) { - db: ^rawptr - rc := sqlite.db_open(":memory:", &db) - if rc != sqlite.OK { - return Db{}, false - } - - create_sql: cstring = "CREATE TABLE IF NOT EXISTS envr_env_files (path TEXT PRIMARY KEY NOT NULL, remotes TEXT, sha256 TEXT NOT NULL, contents TEXT NOT NULL)" - rc = sqlite.db_exec(db, create_sql, nil, nil, nil) - if rc != sqlite.OK { - sqlite.db_close(db) - return Db{}, false - } - - return Db{db = db}, true -} - make_test_env_file :: proc(path, sha, contents: string, remotes: []string = {}) -> EnvFile { f := EnvFile { Path = path, @@ -41,10 +24,10 @@ make_test_env_file :: proc(path, sha, contents: string, remotes: []string = {}) @(test) test_db_insert_and_fetch :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) path := "/project/.env" sha := "abc123" @@ -56,7 +39,7 @@ test_db_insert_and_fetch :: proc(t: ^testing.T) { testing.expect(t, db_insert(&d, f), "insert should succeed") fetched, fetch_ok := db_fetch(&d, "/project/.env") - defer delete_envfile(&fetched) + // defer delete_envfile(&fetched) testing.expect(t, fetch_ok, "fetch should succeed") if !fetch_ok do return @@ -69,10 +52,10 @@ test_db_insert_and_fetch :: proc(t: ^testing.T) { @(test) test_db_fetch_missing :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) _, fetch_ok := db_fetch(&d, "/nonexistent/.env") testing.expect(t, !fetch_ok, "fetch missing should return false") @@ -80,10 +63,9 @@ test_db_fetch_missing :: proc(t: ^testing.T) { @(test) test_db_insert_or_replace :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() + defer db_close(&d) testing.expect(t, ok, "failed to create test db") - if !ok do return - defer sqlite.db_close(d.db) f1 := make_test_env_file("/project/.env", "sha1", "KEY=old") defer delete(f1.Remotes) @@ -95,18 +77,13 @@ test_db_insert_or_replace :: proc(t: ^testing.T) { results, list_ok := db_list(&d) testing.expect(t, list_ok, "list should succeed") - if !list_ok do return - defer delete(results) - for &result in results { - defer delete_envfile(&result) - } testing.expect(t, len(results) == 1, "should have 1 row, not 2") fetched, fetch_ok := db_fetch(&d, "/project/.env") testing.expect(t, fetch_ok, "fetch should succeed") if !fetch_ok do return - defer delete_envfile(&fetched) + // defer delete_envfile(&fetched) testing.expect_value(t, fetched.contents, "KEY=new") testing.expect_value(t, fetched.Sha256, "sha2") @@ -114,10 +91,10 @@ test_db_insert_or_replace :: proc(t: ^testing.T) { @(test) test_db_delete_existing :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) f := make_test_env_file("/project/.env", "sha", "KEY=val") defer delete(f.Remotes) @@ -131,20 +108,19 @@ test_db_delete_existing :: proc(t: ^testing.T) { @(test) test_db_delete_missing :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) testing.expect(t, !db_delete(&d, "/nonexistent/.env"), "delete missing should return false") } @(test) test_db_list_multiple :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") - if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) f1 := make_test_env_file("/proj1/.env", "sha1", "A=1", []string{"git@github.com:a/repo.git"}) defer delete(f1.Remotes) @@ -158,36 +134,27 @@ test_db_list_multiple :: proc(t: ^testing.T) { results, list_ok := db_list(&d) testing.expect(t, list_ok, "list should succeed") - if !list_ok do return - defer delete(results) - defer { - for &result in results { - delete_envfile(&result) - } - } testing.expect_value(t, len(results), 3) } @(test) test_db_list_empty :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") - if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) results, list_ok := db_list(&d) testing.expect(t, list_ok, "list should succeed on empty db") testing.expect(t, len(results) == 0, "should have 0 rows") - if list_ok do delete(results) } @(test) test_db_insert_sets_changed :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) testing.expect(t, !d.changed, "changed should start false") @@ -200,10 +167,10 @@ test_db_insert_sets_changed :: proc(t: ^testing.T) { @(test) test_db_delete_sets_changed :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) f := make_test_env_file("/project/.env", "sha", "KEY=val") defer delete(f.Remotes) @@ -216,10 +183,10 @@ test_db_delete_sets_changed :: proc(t: ^testing.T) { @(test) test_db_serialize :: proc(t: ^testing.T) { - d, ok := make_test_db() + d, ok := db_init() testing.expect(t, ok, "failed to create test db") if !ok do return - defer sqlite.db_close(d.db) + defer db_close(&d) f := make_test_env_file("/project/.env", "sha", "KEY=val") defer delete(f.Remotes) @@ -462,7 +429,7 @@ test_update_dir :: proc(t: ^testing.T) { f := EnvFile { Path = "/old/project/.env", Dir = "/old/project", - Remotes = make([dynamic]string, 0), + Remotes = make([dynamic]string, 0, context.temp_allocator), } defer delete_envfile(&f) @@ -481,13 +448,14 @@ test_closing_db_has_no_leaks :: proc(t: ^testing.T) { cfg_path, err := filepath.join([]string{base, "config.json"}, context.temp_allocator) testing.expect(t, err == nil, "cfgPath should build successfully") - cfg := new_config([]string{"fixtures/keys/insecure-test-key"}, cfg_path) - defer delete_config(&cfg) - testing.expect(t, save_config(cfg, force = true), "save should succeed") + { + cfg := new_config([]string{"fixtures/keys/insecure-test-key"}, cfg_path) + testing.expect(t, save_config(cfg, force = true), "save should succeed") + delete_config(&cfg) + } db, ok := db_open(cfg_path) testing.expect(t, ok, "db should open") - if !ok do return db_close(&db) } @@ -500,15 +468,22 @@ test_open_existing_db_has_no_leaks :: proc(t: ^testing.T) { cfg_path, err := filepath.join([]string{base, "config.json"}, context.temp_allocator) testing.expect(t, err == nil, "cfgPath should build successfully") - cfg := new_config([]string{"fixtures/keys/insecure-test-key"}, cfg_path) - defer delete_config(&cfg) - testing.expect(t, save_config(cfg, force = true), "save should succeed") + { + cfg := new_config([]string{"fixtures/keys/insecure-test-key"}, cfg_path) + testing.expect(t, save_config(cfg, force = true), "save should succeed") + delete_config(&cfg) + } // First open/close creates data.envr on disk db, ok := db_open(cfg_path) testing.expect(t, ok, "db should open") if !ok do return - f := make_test_env_file("/project/.env", "abc123", "SECRET=value", []string{"git@github.com:user/repo.git"}) + f := make_test_env_file( + "/project/.env", + "abc123", + "SECRET=value", + []string{"git@github.com:user/repo.git"}, + ) defer delete(f.Remotes) testing.expect(t, db_insert(&db, f), "insert should succeed") db_close(&db)