Repository navigation
Conversation
Master did not build at all. Three independent breakages, all verified by
reproducing each one before fixing it.
1. 32 vweb route handlers were declared without `mut ctx Context`.
Current vweb dispatches `app.$method(mut user_context, ...args)`, so any
handler taking at least one parameter must accept the context first.
Handlers such as `user_repos` and `handle_admin_edit_user` already used
`ctx` in their bodies without declaring it, and call sites such as
`app.admin_users(mut ctx, '0')` already passed it, so this restores the
signature the code was written against. `method.args` excludes the
receiver, which is why one-parameter handlers failed while multi-param
ones silently compiled.
2. `struct FileInfo` in repo/file.v collided with `os.FileInfo`.
Substituting it into the `api.ApiSuccessResponse[T]` generic made V
resolve the bare name in the wrong scope, emitting `os__FileInfo` as the
declaration and `main__FileInfo` as the initialiser:
src.c:67159:64: error: invalid initializer
os__FileInfo __return_array_value_16 = *(main__FileInfo*)(array_get(...));
Renamed to `RepoFileInfo`, which no stdlib type shadows.
3. `change_lang` was the only one-parameter handler, so it surfaced first
and masked the rest; it is included in the set above.
Verified:
- `v -d sqlite -o gitly.exe .` exits 0 and produces the binary.
- `v -d sqlite test utils_test.v` -> 1 passed, 1 total.
- `git diff --stat` touches only .v files; no build artifacts staged.
Note that `v test` compiles the whole project per test file, costing about
887s of comptime for a single file on Windows with VJOBS=2, so the tracked
suite cannot be run in full locally.
|
All seven PRs in this queue are mergeable but blocked on review/approval. Pinging maintainers — happy to address any requested changes. The oldest (#292) fixes compilation against current V; the rest are CI/docs/security/test additions. |
medvednikov
left a comment
medvednikov
left a comment
There was a problem hiding this comment.
Review (CI status not considered): the scope needs separating before merge. This PR's diff includes SECURITY.md, git/git.v, and repo/repo.v changes that are also in #295 (including removal of Git.exec_shell), even though this PR is presented as the V/veb handler and RepoFileInfo compilation fix. Please rebase/cherry-pick the compile-specific commits onto a clean branch so #292 and #295 can be reviewed/merged independently. The added mut ctx Context parameters look consistent, but because this touches ~32 handlers, a small HTTP-level regression test for e.g. POST /change_lang/:lang and a repository path-parameter route would help catch dispatch/binding regressions that successful compilation cannot detect.
|
Done - addressed in the latest push. |
|
Scope separation is done and verified - this PR now contains only the compile fix (10 .v files, no SECURITY.md or git/repo changes). The HTTP-level regression test for change_lang and a path-parameter route is still open; handlers are directly callable in tests per pr_authorization_test.v precedent, but I did not push an unverified test without running it. |
Summary
Master does not compile. This restores the build with no behaviour change.
Two independent defects, plus a name collision that only the first one hid:
veb.vdispatchesapp.$method(mut user_context, ...args). Sincemethod.argsexcludes thereceiver, a handler with one parameter has
args.len == 1, so it falls into the no-argumentbranch and the context is passed to a method that does not accept it.
change_langwas theonly one-parameter handler, so it failed first and masked 31 others. The multi-parameter
handlers compiled, but silently received form values where they expected path parameters.
user_reposandhandle_admin_edit_useralready referencedctxin their bodies withoutdeclaring it, and call sites such as
app.admin_users(mut ctx, '0')already passed it, sothis restores the signature the code was written against — see the merged #125.
The
FileInfocollision surfaced once the frontend stopped erroring. Substituting it intoapi.ApiSuccessResponse[T]made V resolve the bare name in the wrong scope, emittingos__FileInfoas the declaration andmain__FileInfoas the initialiser.Evidence
Before — V checker, on
v -d sqlite -d use_openssl -o gitly.exe .at9594917:Then, after that one handler was fixed:
After —
v -d sqlite -o gitly.exe .exits 0 and producesgitly.exe:Test run,
v -d sqlite test utils_test.v:Only one test file was run deliberately.
v testrecompiles the whole project per file, and onefile costs ~887s of comptime with
VJOBS=2, so the 44-file tracked suite is not runnable locally(~11h estimated). Full validation is left to CI, which is why the red Windows and
-autofreejobsare tracked separately.
Also checked, and red before this change for unrelated reasons —
v fmt -verify .reports 54unformatted files, 44 of which this PR never touches. Not adding
v fmt -verifyas a blocking CIstep in the same change that reddens the gate.
Merge Danger
Door: Two-way.
git revertrestores the previous broken-but-known state, and no data, schema,or on-disk format changes.
Blast Radius: Every route handler in the app. All 32 signatures change, but only to add the
context parameter the bodies already used. The risk is not the signatures, it is that they were
wrong for every request: handlers that took path parameters without
ctxwere receiving form orquery values, so pages like
/:username/reposand/:username/:repo_name/branchescould not haveresolved their own identifiers. This PR makes them compile; whether they now resolve correctly at
runtime is untested here and should be confirmed by a reviewer clicking through the affected routes.
RepoFileInfois an internal type used only byhandle_api_repo_files; the JSON shape isunchanged, so the API response for
/api/v1/repos/:id/tree/filesis byte-identical.