From 0d8e386e5494b3f26ac361437a8f35751c213b85 Mon Sep 17 00:00:00 2001 From: office Date: Tue, 13 Oct 2026 14:04:28 +0330 Subject: [PATCH 1/4] test: cover the ORM update helpers against variable shadowing Adds coverage for the two `update X set col = col` helpers where the local variable shadows the column name, so the compiler has to pick one. This is the still-open part of #262, which is otherwise superseded: the files it patches (src/repo.v, src/branch.v) no longer exist, but the pattern it was fixing is still on master at repo/repo.v:125 and repo/branch.v:116. fn (mut app App) update_branch(branch_id int, author string, hash string, date int) ! { sql app.db { update Branch set author = author, hash = hash, date = date where id == branch_id }! } fn (mut app App) save_repo(repo Repo) ! { ... update Repo set description = desc, views_count = views_count, is_public = is_public, ... Both are written this way, and if the right-hand side binds to the column instead of the local variable, each statement is a self-assignment and the write is silently a no-op. Nothing else in the suite would notice: the branch row is written by the push path and the repo row by save_repo, so a self-assignment would surface only as stale numbers in the UI, long after the write appeared to succeed. The fixture is modelled on branch_management_test.v, which is the existing pattern for a sqlite-backed test with a real bare repository. NOT LOCALLY VERIFIED. `v test ` does not pull in the sibling `module main` files that define `App`, `connect_db`, `User`, so the test cannot be compiled in isolation, and `v test .` compiles the whole tree, which exceeded the time available here (one test file already costs about 887s of comptime on this machine). `v -check-syntax` passes and `v fmt -verify` passes; correctness is left to the CI job, which runs the exact command this test is written for. That is also the point. Either the test passes, in which case the shadowing is harmless and #262 can be closed as cosmetic, or it fails, in which case the shadowing is a real no-op bug and the fix is the rename #262 already proposes. --- orm_update_test.v | 141 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 orm_update_test.v diff --git a/orm_update_test.v b/orm_update_test.v new file mode 100644 index 00000000..b371229a --- /dev/null +++ b/orm_update_test.v @@ -0,0 +1,141 @@ +module main + +import config +import git +import os + +// Coverage for the ORM update helpers in repo/branch.v (update_branch) and +// repo/repo.v (save_repo). Both build `update X set col = col` statements where +// the local variable shadows the column name, so the compiler has to pick one. +// A silent self-assignment would make every save a no-op without failing +// anything else. +// +// Nothing else catches this. The branch row is written by the push path and the +// repo row by save_repo, so a self-assignment would surface only as stale numbers +// in the UI, long after the write appeared to succeed. + +fn orm_fixture_git(args []string) string { + result := git.Git.exec(args) + if result.exit_code != 0 { + panic('git ${args} failed: ${result.output}') + } + return result.output.trim_space() +} + +fn orm_fixture_app(root string) !App { + work_dir := os.join_path(root, 'work') + bare_dir := os.join_path(root, 'project.git') + orm_fixture_git(['init', '-b', 'main', work_dir]) + orm_fixture_git(['-C', work_dir, 'config', 'user.name', 'Owner']) + orm_fixture_git(['-C', work_dir, 'config', 'user.email', 'owner@example.com']) + os.write_file(os.join_path(work_dir, 'README.md'), 'hello\n')! + orm_fixture_git(['-C', work_dir, 'add', 'README.md']) + orm_fixture_git(['-C', work_dir, 'commit', '-m', 'initial']) + orm_fixture_git(['clone', '--bare', work_dir, bare_dir]) + + conf := config.Config{ + repo_storage_path: root + archive_path: root + avatars_path: root + sqlite: config.SqliteConfig{ + path: os.join_path(root, 'test.sqlite') + } + } + mut app := App{ + db: connect_db(conf)! + config: conf + } + app.create_tables()! + user := User{ + id: 1 + username: 'owner' + is_registered: true + } + sql app.db { + insert user into User + }! + app.add_repo(Repo{ + id: 1 + git_dir: bare_dir + name: 'project' + user_id: 1 + user_name: 'owner' + description: 'seeded description' + is_public: true + primary_branch: 'main' + status: .done + })! + return app +} + +fn test_update_branch_writes_new_values() ! { + $if !sqlite ? { + return + } + root := os.join_path(os.temp_dir(), 'gitly_orm_update_branch_${os.getpid()}') + os.rmdir_all(root) or {} + os.mkdir_all(root)! + defer { + os.rmdir_all(root) or {} + } + mut app := orm_fixture_app(root)! + defer { + app.db.close() or {} + } + + // If `update Branch set author = author` binds the right-hand side to the + // column rather than the local variable, this is a self-assignment and the + // row keeps its seeded values forever. + seeded := Branch{ + id: 1 + repo_id: 1 + name: 'main' + author: 'old_author' + hash: 'old_hash' + date: 1000 + } + sql app.db { + insert seeded into Branch + }! + app.update_branch(1, 'new_author', 'new_hash', 2000)! + + branch := app.find_repo_branch_by_name(1, 'main') + assert branch.author == 'new_author' + assert branch.hash == 'new_hash' + assert branch.date == 2000 +} + +fn test_save_repo_writes_updated_counters() ! { + $if !sqlite ? { + return + } + root := os.join_path(os.temp_dir(), 'gitly_orm_save_repo_${os.getpid()}') + os.rmdir_all(root) or {} + os.mkdir_all(root)! + defer { + os.rmdir_all(root) or {} + } + mut app := orm_fixture_app(root)! + defer { + app.db.close() or {} + } + + // Only the mutable fields of Repo are set here. `description`, `is_public` and + // `views_count` are outside the `mut:` block, so save_repo cannot change them + // on a row read back from the database, and asserting on them would fail for + // that reason rather than because of the ORM update. + mut repo := app.find_repo_by_id(1)! + repo.nr_branches = 7 + repo.nr_tags = 11 + repo.nr_stars = 13 + repo.nr_releases = 5 + repo.webhook_secret = 'rotated-secret' + app.save_repo(repo)! + + saved := app.find_repo_by_id(1)! + assert saved.nr_branches == 7 + assert saved.nr_tags == 11 + assert saved.nr_stars == 13 + assert saved.nr_releases == 5 + assert saved.webhook_secret == 'rotated-secret' +} From a835e5aff3a6b5618932c571dedd7735229dc16a Mon Sep 17 00:00:00 2001 From: metif12 Date: Tue, 10 Nov 2026 09:56:22 +0330 Subject: [PATCH 2/4] test: strengthen save_repo coverage (shadowed fields + second row) (address review) --- orm_update_test.v | 66 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/orm_update_test.v b/orm_update_test.v index b371229a..ac926383 100644 --- a/orm_update_test.v +++ b/orm_update_test.v @@ -106,10 +106,10 @@ fn test_update_branch_writes_new_values() ! { } fn test_save_repo_writes_updated_counters() ! { - $if !sqlite ? { + !sqlite ? { return } - root := os.join_path(os.temp_dir(), 'gitly_orm_save_repo_${os.getpid()}') + root := os.join_path(os.temp_dir(), 'gitly_orm_save_repo_') os.rmdir_all(root) or {} os.mkdir_all(root)! defer { @@ -120,22 +120,64 @@ fn test_save_repo_writes_updated_counters() ! { app.db.close() or {} } - // Only the mutable fields of Repo are set here. `description`, `is_public` and - // `views_count` are outside the `mut:` block, so save_repo cannot change them - // on a row read back from the database, and asserting on them would fail for - // that reason rather than because of the ORM update. - mut repo := app.find_repo_by_id(1)! - repo.nr_branches = 7 - repo.nr_tags = 11 - repo.nr_stars = 13 - repo.nr_releases = 5 - repo.webhook_secret = 'rotated-secret' + // Insert a second repository so a tautological where id == id (which + // would update every row) is detected instead of silently passing. + second := Repo{ + id: 2 + git_dir: app.find_repo_by_id(1)!.git_dir + name: 'other' + user_id: 1 + user_name: 'owner' + description: 'second repo description' + is_public: false + primary_branch: 'main' + status: .done + nr_branches: 1 + nr_tags: 2 + nr_stars: 3 + } + sql app.db { + insert second into Repo + }! + + // Construct a brand-new Repo value (rather than mutating a retrieved one) + // so the shadowed description, iews_count and is_public assignments + // in save_repo are actually exercised. + repo := Repo{ + id: 1 + git_dir: app.find_repo_by_id(1)!.git_dir + name: 'project' + user_id: 1 + user_name: 'owner' + description: 'updated description' + is_public: false + primary_branch: 'main' + status: .done + views_count: 42 + nr_branches: 7 + nr_tags: 11 + nr_stars: 13 + nr_releases: 5 + webhook_secret: 'rotated-secret' + } app.save_repo(repo)! saved := app.find_repo_by_id(1)! + assert saved.description == 'updated description' + assert saved.is_public == false + assert saved.views_count == 42 assert saved.nr_branches == 7 assert saved.nr_tags == 11 assert saved.nr_stars == 13 assert saved.nr_releases == 5 assert saved.webhook_secret == 'rotated-secret' + + // The second repository must be untouched. + other := app.find_repo_by_id(2)! + assert other.description == 'second repo description' + assert other.is_public == false + assert other.views_count == 0 + assert other.nr_branches == 1 + assert other.nr_tags == 2 + assert other.nr_stars == 3 } From 5546e7c40bce4a458ccee5e714c3f18d4d2df724 Mon Sep 17 00:00:00 2001 From: metif12 Date: Tue, 10 Nov 2026 09:57:35 +0330 Subject: [PATCH 3/4] style: vfmt orm_update_test.v --- orm_update_test.v | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/orm_update_test.v b/orm_update_test.v index ac926383..4ed40dad 100644 --- a/orm_update_test.v +++ b/orm_update_test.v @@ -106,7 +106,8 @@ fn test_update_branch_writes_new_values() ! { } fn test_save_repo_writes_updated_counters() ! { - !sqlite ? { + !sqlite? + { return } root := os.join_path(os.temp_dir(), 'gitly_orm_save_repo_') From 3672990dc0d7d28bc44b3c2f152c95f86002adba Mon Sep 17 00:00:00 2001 From: metif12 Date: Tue, 10 Nov 2026 12:51:56 +0330 Subject: [PATCH 4/4] test: fix orm_update_test syntax, PID suffix, VT char, second Branch row (address review) --- orm_update_test.v | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/orm_update_test.v b/orm_update_test.v index 4ed40dad..569160a2 100644 --- a/orm_update_test.v +++ b/orm_update_test.v @@ -97,20 +97,34 @@ fn test_update_branch_writes_new_values() ! { sql app.db { insert seeded into Branch }! + second_branch := Branch{ + id: 2 + repo_id: 1 + name: 'other' + author: 'other_author' + hash: 'other_hash' + date: 500 + } + sql app.db { + insert second_branch into Branch + }! app.update_branch(1, 'new_author', 'new_hash', 2000)! branch := app.find_repo_branch_by_name(1, 'main') assert branch.author == 'new_author' assert branch.hash == 'new_hash' assert branch.date == 2000 + other := app.find_repo_branch_by_name(1, 'other') + assert other.author == 'other_author' + assert other.hash == 'other_hash' + assert other.date == 500 } fn test_save_repo_writes_updated_counters() ! { - !sqlite? - { + $if !sqlite ? { return } - root := os.join_path(os.temp_dir(), 'gitly_orm_save_repo_') + root := os.join_path(os.temp_dir(), 'gitly_orm_save_repo_${os.getpid()}') os.rmdir_all(root) or {} os.mkdir_all(root)! defer { @@ -125,7 +139,7 @@ fn test_save_repo_writes_updated_counters() ! { // would update every row) is detected instead of silently passing. second := Repo{ id: 2 - git_dir: app.find_repo_by_id(1)!.git_dir + git_dir: (app.find_repo_by_id(1) or { panic('repo missing') }).git_dir name: 'other' user_id: 1 user_name: 'owner' @@ -142,11 +156,11 @@ fn test_save_repo_writes_updated_counters() ! { }! // Construct a brand-new Repo value (rather than mutating a retrieved one) - // so the shadowed description, iews_count and is_public assignments + // so the shadowed `description`, `views_count` and `is_public` assignments // in save_repo are actually exercised. repo := Repo{ id: 1 - git_dir: app.find_repo_by_id(1)!.git_dir + git_dir: (app.find_repo_by_id(1) or { panic('repo missing') }).git_dir name: 'project' user_id: 1 user_name: 'owner' @@ -163,7 +177,7 @@ fn test_save_repo_writes_updated_counters() ! { } app.save_repo(repo)! - saved := app.find_repo_by_id(1)! + saved := app.find_repo_by_id(1) or { return error('repo missing') } assert saved.description == 'updated description' assert saved.is_public == false assert saved.views_count == 42 @@ -174,7 +188,7 @@ fn test_save_repo_writes_updated_counters() ! { assert saved.webhook_secret == 'rotated-secret' // The second repository must be untouched. - other := app.find_repo_by_id(2)! + other := app.find_repo_by_id(2) or { return error('repo missing') } assert other.description == 'second repo description' assert other.is_public == false assert other.views_count == 0