From e32f0ea6d2febe2b33c36f0ddba4a9c4c36a7a34 Mon Sep 17 00:00:00 2001 From: Spencer Brower Date: Fri, 12 Jun 2026 17:16:51 -0400 Subject: [PATCH] refactor: Fixed logic bug in db. --- TODOS.md | 2 -- cmd_sync.odin | 18 ++++++++---------- db.odin | 49 ++++++++++++++++++++++++------------------------- db_test.odin | 13 ++++++------- 4 files changed, 38 insertions(+), 44 deletions(-) diff --git a/TODOS.md b/TODOS.md index 4d40013..4fcb184 100644 --- a/TODOS.md +++ b/TODOS.md @@ -10,8 +10,6 @@ Note: These todos can wait until all the subcommands have been ported. 30. **cmd_sync.odin:46-50, 64-68** — Double `db_insert` when `BackedUp`: first insert on line 48, then `db_update_required` is also true for `BackedUp` so second insert runs on line 65. Redundant and wasteful. -31. **db.odin:626 & env_file.go:183** — `BackedUp` discards `DirUpdated`. When `TrustFilesystem` is used and the hash differs, the result is just `BackedUp` (not `BackedUp | DirUpdated`). If a file's directory was moved AND its contents changed, the old DB entry won't be deleted because the `DirUpdated` check at `cmd_sync.odin:59` never fires. Bug exists in both Go and Odin. - ## MEDIUM 4. **db.odin:29-35** — `make_temp_path` never calls `strings.builder_destroy`. Leaks builder buffer every call. diff --git a/cmd_sync.odin b/cmd_sync.odin index ad138f9..d1acc22 100644 --- a/cmd_sync.odin +++ b/cmd_sync.odin @@ -11,6 +11,7 @@ SyncEntry :: struct { Status: string `json:"status"`, } +// TODO: Check for quiet failures. cmd_sync :: proc(cmd: ^Command) { db, db_ok := db_open() if !db_ok { @@ -33,28 +34,25 @@ cmd_sync :: proc(cmd: ^Command) { result, err_msg := db_sync(&db, &file) status: string - s := i32(result) - is_error := (s & i32(SyncResult.Error)) != 0 - is_backed := (s & i32(SyncResult.BackedUp)) != 0 - is_restored := (s & i32(SyncResult.Restored)) != 0 - is_dir_updated := (s & i32(SyncResult.DirUpdated)) != 0 + is_dir_updated := .DirUpdated in result - if is_error { + switch { + case .Error in result: if len(err_msg) > 0 { status = err_msg } else { status = "error" } - } else if is_backed { + case .BackedUp in result: status = "Backed Up" if !db_insert(&db, file) { return } - } else if is_restored { + case .Restored in result: status = "Restored" - } else if is_dir_updated && !is_restored { + case .DirUpdated in result: status = "Moved" - } else { + case: status = "OK" } diff --git a/db.odin b/db.odin index c806dcb..31a8c63 100644 --- a/db.odin +++ b/db.odin @@ -12,14 +12,16 @@ import "core:time" import "sqlite" -SyncResult :: enum i32 { - Noop = 0, - DirUpdated = 1, - Restored = 1 << 1, - BackedUp = 1 << 2, - Error = 1 << 3, +SyncFlagEnum :: enum { + Noop, + DirUpdated, + Restored, + BackedUp, + Error, } +SyncFlag :: bit_set[SyncFlagEnum] + SyncDirection :: enum { TrustDatabase, TrustFilesystem, @@ -449,9 +451,8 @@ string_to_cstring :: proc(s: string) -> cstring { return cs } -db_update_required :: proc(status: SyncResult) -> bool { - s := i32(status) - return (s & (i32(SyncResult.BackedUp) | i32(SyncResult.DirUpdated))) != 0 +db_update_required :: proc(status: SyncFlag) -> bool { + return .BackedUp in status || .DirUpdated in status } shares_remote :: proc(f: ^EnvFile, remotes: []string) -> bool { @@ -510,8 +511,8 @@ env_file_backup :: proc(f: ^EnvFile) -> bool { return true } -env_file_sync :: proc(f: ^EnvFile, dir: SyncDirection, d: ^Db) -> (SyncResult, string) { - result: SyncResult = .Noop +env_file_sync :: proc(f: ^EnvFile, dir: SyncDirection, d: ^Db) -> (SyncFlag, string) { + result: SyncFlag = {} err_msg: string _, stat_err := os.stat(f.Dir, context.allocator) @@ -521,18 +522,18 @@ env_file_sync :: proc(f: ^EnvFile, dir: SyncDirection, d: ^Db) -> (SyncResult, s if d != nil { dirs, dirs_ok := find_moved_dirs(d, f) if !dirs_ok { - return .Error, "failed to find moved dirs" + return {.Error}, "failed to find moved dirs" } moved_dirs = dirs } if len(moved_dirs) == 0 { - return .Error, "directory missing" + return {.Error}, "directory missing" } else if len(moved_dirs) == 1 { update_dir(f, moved_dirs[0]) - result = .DirUpdated + result = {.DirUpdated} } else { - return .Error, "multiple directories found" + return {.Error}, "multiple directories found" } } @@ -541,11 +542,10 @@ env_file_sync :: proc(f: ^EnvFile, dir: SyncDirection, d: ^Db) -> (SyncResult, s write_err := os.write_entire_file(f.Path, f.contents) if write_err != nil { msg, _ := strings.concatenate({"failed to write file: ", fmt.tprintf("%v", write_err)}) - return .Error, msg + return {.Error}, msg } - s := i32(result) | i32(SyncResult.Restored) - return SyncResult(s), "" + return result + {.Restored}, "" } data, read_err := os.read_entire_file_from_path(f.Path, context.allocator) @@ -553,7 +553,7 @@ env_file_sync :: proc(f: ^EnvFile, dir: SyncDirection, d: ^Db) -> (SyncResult, s msg, _ := strings.concatenate( {"failed to read file for SHA comparison: ", fmt.tprintf("%v", read_err)}, ) - return .Error, msg + return {.Error}, msg } digest := hash.hash_bytes(hash.Algorithm.SHA256, data) @@ -569,21 +569,20 @@ env_file_sync :: proc(f: ^EnvFile, dir: SyncDirection, d: ^Db) -> (SyncResult, s write_err := os.write_entire_file(f.Path, f.contents) if write_err != nil { msg, _ := strings.concatenate({"failed to write file: ", fmt.tprintf("%v", write_err)}) - return .Error, msg + return {.Error}, msg } - s := i32(result) | i32(SyncResult.Restored) - return SyncResult(s), "" + return result + {.Restored}, "" case .TrustFilesystem: if !env_file_backup(f) { - return .Error, "failed to backup file" + return {.Error}, "failed to backup file" } - return .BackedUp, "" + return result + {.BackedUp}, "" } return result, "" } -db_sync :: proc(d: ^Db, f: ^EnvFile) -> (SyncResult, string) { +db_sync :: proc(d: ^Db, f: ^EnvFile) -> (SyncFlag, string) { return env_file_sync(f, .TrustFilesystem, d) } diff --git a/db_test.odin b/db_test.odin index 6390e17..a1d2c70 100644 --- a/db_test.odin +++ b/db_test.odin @@ -4,33 +4,32 @@ import "core:testing" @(test) test_db_update_required_noop :: proc(t: ^testing.T) { - testing.expect(t, !db_update_required(.Noop), "Noop should not require update") + testing.expect(t, !db_update_required({}), "Noop should not require update") } @(test) test_db_update_required_backed_up :: proc(t: ^testing.T) { - testing.expect(t, db_update_required(.BackedUp), "BackedUp should require update") + testing.expect(t, db_update_required({.BackedUp}), "BackedUp should require update") } @(test) test_db_update_required_dir_updated :: proc(t: ^testing.T) { - testing.expect(t, db_update_required(.DirUpdated), "DirUpdated should require update") + testing.expect(t, db_update_required({.DirUpdated}), "DirUpdated should require update") } @(test) test_db_update_required_restored :: proc(t: ^testing.T) { - testing.expect(t, !db_update_required(.Restored), "Restored alone should not require update") + testing.expect(t, !db_update_required({.Restored}), "Restored alone should not require update") } @(test) test_db_update_required_error :: proc(t: ^testing.T) { - testing.expect(t, !db_update_required(.Error), "Error alone should not require update") + testing.expect(t, !db_update_required({.Error}), "Error alone should not require update") } @(test) test_db_update_required_combined :: proc(t: ^testing.T) { - s := i32(SyncResult.DirUpdated) | i32(SyncResult.Restored) - combined := SyncResult(s) + combined := SyncFlag{.DirUpdated, .Restored} testing.expect(t, db_update_required(combined), "DirUpdated|Restored should require update") }