When db_url is omitted, pixa derives it from state_dir, and it put the directory name into a file: URL as it was. The SQLite driver takes everything after the first ? as its own parameters, and SQLite ends the path at a ? or # and decodes each %HH. A state_dir holding one of those characters therefore opened a different file and lost the journal mode parameter.
The derived URL is now built with net/url in a new function defaultDBURL in internal/config. String() percent-encodes the directory name and SQLite decodes it back. For an ordinary path the URL comes out as before: OmitHost keeps the file:/var/lib/pixa/state.sqlite3?... form, and a relative state_dir stays a path instead of being read as a host name. When the path written into the URL, state_dir followed by /state.sqlite3, starts with //, as it does for a state_dir of / or one starting with //, the empty host is kept: the URL starts file://// and SQLite does not read the first name in the path as a host. The directory is not cleaned, so .. resolves the way the operating system resolves it.
Each new test is in its own commit, before its fix; no existing test changed. README.md now says state_dir is percent-encoded in the derived db_url, and that for a state_dir of / or one starting with // it starts file:////.
Judgement call: the URL building sits in defaultDBURL rather than inline because newFromSmartConfig went over the linter's function length limit.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/pixa/issues/290.
When `db_url` is omitted, pixa derives it from `state_dir`, and it put the directory name into a `file:` URL as it was. The SQLite driver takes everything after the first `?` as its own parameters, and SQLite ends the path at a `?` or `#` and decodes each `%HH`. A `state_dir` holding one of those characters therefore opened a different file and lost the journal mode parameter.
The derived URL is now built with `net/url` in a new function `defaultDBURL` in `internal/config`. `String()` percent-encodes the directory name and SQLite decodes it back. For an ordinary path the URL comes out as before: `OmitHost` keeps the `file:/var/lib/pixa/state.sqlite3?...` form, and a relative `state_dir` stays a path instead of being read as a host name. When the path written into the URL, `state_dir` followed by `/state.sqlite3`, starts with `//`, as it does for a `state_dir` of `/` or one starting with `//`, the empty host is kept: the URL starts `file:////` and SQLite does not read the first name in the path as a host. The directory is not cleaned, so `..` resolves the way the operating system resolves it.
Each new test is in its own commit, before its fix; no existing test changed. `README.md` now says `state_dir` is percent-encoded in the derived `db_url`, and that for a `state_dir` of `/` or one starting with `//` it starts `file:////`.
Judgement call: the URL building sits in `defaultDBURL` rather than inline because `newFromSmartConfig` went over the linter's function length limit.
Model: opus-5-5
internal/config/config.go, defaultDBURL: a state_dir that starts with //, such as //var/lib/pixa, still gives a db_url beginning file://var/. SQLite reads var as a host name there, so pixa fails to start. Item 1 of the definition of done in #290 asks for the derived db_url to name state.sqlite3 in the state directory whatever characters the directory holds, and the doc comment on defaultDBURL says the URL names exactly that file. Acceptable looks like this: such a state_dir opens state.sqlite3 in that directory in WAL mode, an ordinary path still gives file:/var/lib/pixa/state.sqlite3?_pragma=journal_mode(WAL), and a test for the // case is committed before the fix. One way is to leave OmitHost off when the directory starts with //. The URL then starts file:////, which SQLite reads as an empty host followed by the path. Once that is fixed, remove the "Unchanged" line from the PR body.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/config/config.go`, `defaultDBURL`: a `state_dir` that starts with `//`, such as `//var/lib/pixa`, still gives a `db_url` beginning `file://var/`. SQLite reads `var` as a host name there, so pixa fails to start. Item 1 of the definition of done in https://git.eeqj.de/sneak/pixa/issues/290 asks for the derived `db_url` to name `state.sqlite3` in the state directory whatever characters the directory holds, and the doc comment on `defaultDBURL` says the URL names exactly that file. Acceptable looks like this: such a `state_dir` opens `state.sqlite3` in that directory in WAL mode, an ordinary path still gives `file:/var/lib/pixa/state.sqlite3?_pragma=journal_mode(WAL)`, and a test for the `//` case is committed before the fix. One way is to leave `OmitHost` off when the directory starts with `//`. The URL then starts `file:////`, which SQLite reads as an empty host followed by the path. Once that is fixed, remove the "Unchanged" line from the PR body.
Model: opus-5-5
defaultDBURL leaves OmitHost off when state_dir starts with //, so the URL starts file:////; an ordinary path gives the same URL as before. A new test, committed before the fix, opens the derived db_url for such a directory and checks WAL mode and state.sqlite3 in that directory. README.md and the TODO.md entry say how such a state_dir is written, and the "Unchanged" line is gone from the PR body.
Judgement call: the open-and-check steps of the earlier new test moved into a helper both new tests call, because a third copy of the wal check failed the linter (goconst). The test that is on next is untouched.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/292#issuecomment-144455.
1. `defaultDBURL` leaves `OmitHost` off when `state_dir` starts with `//`, so the URL starts `file:////`; an ordinary path gives the same URL as before. A new test, committed before the fix, opens the derived `db_url` for such a directory and checks WAL mode and `state.sqlite3` in that directory. `README.md` and the `TODO.md` entry say how such a `state_dir` is written, and the "Unchanged" line is gone from the PR body.
Judgement call: the open-and-check steps of the earlier new test moved into a helper both new tests call, because a third copy of the `wal` check failed the linter (goconst). The test that is on `next` is untouched.
Model: opus-5-5
internal/config/config.go, defaultDBURL: the URL starts file://// only when state_dir itself starts with //. But SQLite reads the path written after file:, which is state_dir followed by /state.sqlite3. For a state_dir of / that path is //state.sqlite3, so the derived db_url is file://state.sqlite3?_pragma=journal_mode(WAL). SQLite reads state.sqlite3 as a host and refuses to open it, so pixa fails to start, even though a state_dir of / passes the startup checks when pixa runs as root. That misses item 1 of the definition of done in #290, and it makes the doc comment's "the URL names exactly this file" untrue. Acceptable looks like this: the decision looks at the path written into the URL, not at state_dir, so a state_dir of / gives a db_url that opens /state.sqlite3. A test for that case is committed before the fix; checking the derived db_url is enough, since a test cannot write to /. An ordinary path still gives file:/var/lib/pixa/state.sqlite3?_pragma=journal_mode(WAL). The doc comment, the README.md sentence about file:////, the TODO.md entry and the PR body name the state_dir values that get a URL starting file:////.
Judgement call: a state_dir of / gives the same broken db_url on next; I counted it under item 1 of the definition of done, as the earlier review did for //.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/config/config.go`, `defaultDBURL`: the URL starts `file:////` only when `state_dir` itself starts with `//`. But SQLite reads the path written after `file:`, which is `state_dir` followed by `/state.sqlite3`. For a `state_dir` of `/` that path is `//state.sqlite3`, so the derived `db_url` is `file://state.sqlite3?_pragma=journal_mode(WAL)`. SQLite reads `state.sqlite3` as a host and refuses to open it, so pixa fails to start, even though a `state_dir` of `/` passes the startup checks when pixa runs as root. That misses item 1 of the definition of done in https://git.eeqj.de/sneak/pixa/issues/290, and it makes the doc comment's "the URL names exactly this file" untrue. Acceptable looks like this: the decision looks at the path written into the URL, not at `state_dir`, so a `state_dir` of `/` gives a `db_url` that opens `/state.sqlite3`. A test for that case is committed before the fix; checking the derived `db_url` is enough, since a test cannot write to `/`. An ordinary path still gives `file:/var/lib/pixa/state.sqlite3?_pragma=journal_mode(WAL)`. The doc comment, the `README.md` sentence about `file:////`, the `TODO.md` entry and the PR body name the `state_dir` values that get a URL starting `file:////`.
Judgement call: a `state_dir` of `/` gives the same broken `db_url` on `next`; I counted it under item 1 of the definition of done, as the earlier review did for `//`.
Model: opus-5-5
defaultDBURL now keeps the empty host when the path written into the URL starts with //, so a state_dir of / gives file:////state.sqlite3?_pragma=journal_mode(WAL); an ordinary path gives the same URL as before. A new test, committed before the fix, checks the derived db_url for a state_dir of /. The doc comment, README.md, the TODO.md entry (moved to the top of Completed Steps after the rebase) and the PR body name a state_dir of / or one starting with //.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/292#issuecomment-144510.
1. `defaultDBURL` now keeps the empty host when the path written into the URL starts with `//`, so a `state_dir` of `/` gives `file:////state.sqlite3?_pragma=journal_mode(WAL)`; an ordinary path gives the same URL as before. A new test, committed before the fix, checks the derived `db_url` for a `state_dir` of `/`. The doc comment, `README.md`, the `TODO.md` entry (moved to the top of Completed Steps after the rebase) and the PR body name a `state_dir` of `/` or one starting with `//`.
Model: opus-5-5
The test sets state_dir to a directory named a%41b?c#d, opens the
derived db_url with the SQLite driver, and checks that the database is
state.sqlite3 in that directory, in WAL mode. It fails until the
directory is escaped in the URL.
Model: opus-5-5
The derived db_url put state_dir into a file: URL as it was, so SQLite
decoded a %HH in the name and ended the path at a ? or #, and the
journal mode parameter was lost. It is now built with net/url, which
percent-encodes the directory name; SQLite decodes it back and opens
state.sqlite3 in that directory. OmitHost keeps the usual form
file:/var/lib/pixa/state.sqlite3 and a relative state_dir a path.
Model: opus-5-5
The test sets state_dir to a temporary directory with a / put in front,
opens the derived db_url with the SQLite driver, and checks that the
database is state.sqlite3 in that directory, in WAL mode. It fails while
the URL starts file://, where SQLite reads the first directory name as
a host. The open-and-check part of the test before it moves into a
helper both tests call.
Model: opus-5-5
With OmitHost set, a state_dir starting with // gave a db_url starting
file://, and SQLite read the first directory name as a host, so pixa
failed to start. OmitHost is now left off for such a directory: the URL
starts file://// and SQLite reads an empty host followed by the path.
Every other state_dir gives the same URL as before.
Model: opus-5-5
The path written into the URL is then //state.sqlite3, so the URL
starts file:// and SQLite reads state.sqlite3 as a host. The test
checks that the derived db_url starts file://// instead; it only
checks the URL, since a test cannot write to /.
Model: opus-5-5
defaultDBURL kept the empty host only when state_dir started with //,
but SQLite reads the path written into the URL, state_dir followed by
/state.sqlite3. For a state_dir of / that path is //state.sqlite3, the
URL started file:// and SQLite read state.sqlite3 as a host, so pixa
failed to start. The decision now looks at that path: a state_dir of
/ gives file:////state.sqlite3. Every other state_dir gives the same
URL as before.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #290.
When
db_urlis omitted, pixa derives it fromstate_dir, and it put the directory name into afile:URL as it was. The SQLite driver takes everything after the first?as its own parameters, and SQLite ends the path at a?or#and decodes each%HH. Astate_dirholding one of those characters therefore opened a different file and lost the journal mode parameter.The derived URL is now built with
net/urlin a new functiondefaultDBURLininternal/config.String()percent-encodes the directory name and SQLite decodes it back. For an ordinary path the URL comes out as before:OmitHostkeeps thefile:/var/lib/pixa/state.sqlite3?...form, and a relativestate_dirstays a path instead of being read as a host name. When the path written into the URL,state_dirfollowed by/state.sqlite3, starts with//, as it does for astate_dirof/or one starting with//, the empty host is kept: the URL startsfile:////and SQLite does not read the first name in the path as a host. The directory is not cleaned, so..resolves the way the operating system resolves it.Each new test is in its own commit, before its fix; no existing test changed.
README.mdnow saysstate_diris percent-encoded in the deriveddb_url, and that for astate_dirof/or one starting with//it startsfile:////.Judgement call: the URL building sits in
defaultDBURLrather than inline becausenewFromSmartConfigwent over the linter's function length limit.Model: opus-5-5
Review failed: needs-rework.
internal/config/config.go,defaultDBURL: astate_dirthat starts with//, such as//var/lib/pixa, still gives adb_urlbeginningfile://var/. SQLite readsvaras a host name there, so pixa fails to start. Item 1 of the definition of done in #290 asks for the deriveddb_urlto namestate.sqlite3in the state directory whatever characters the directory holds, and the doc comment ondefaultDBURLsays the URL names exactly that file. Acceptable looks like this: such astate_diropensstate.sqlite3in that directory in WAL mode, an ordinary path still givesfile:/var/lib/pixa/state.sqlite3?_pragma=journal_mode(WAL), and a test for the//case is committed before the fix. One way is to leaveOmitHostoff when the directory starts with//. The URL then startsfile:////, which SQLite reads as an empty host followed by the path. Once that is fixed, remove the "Unchanged" line from the PR body.Model: opus-5-5
Rework for #292 (comment).
defaultDBURLleavesOmitHostoff whenstate_dirstarts with//, so the URL startsfile:////; an ordinary path gives the same URL as before. A new test, committed before the fix, opens the deriveddb_urlfor such a directory and checks WAL mode andstate.sqlite3in that directory.README.mdand theTODO.mdentry say how such astate_diris written, and the "Unchanged" line is gone from the PR body.Judgement call: the open-and-check steps of the earlier new test moved into a helper both new tests call, because a third copy of the
walcheck failed the linter (goconst). The test that is onnextis untouched.Model: opus-5-5
Review failed: needs-rework.
internal/config/config.go,defaultDBURL: the URL startsfile:////only whenstate_diritself starts with//. But SQLite reads the path written afterfile:, which isstate_dirfollowed by/state.sqlite3. For astate_dirof/that path is//state.sqlite3, so the deriveddb_urlisfile://state.sqlite3?_pragma=journal_mode(WAL). SQLite readsstate.sqlite3as a host and refuses to open it, so pixa fails to start, even though astate_dirof/passes the startup checks when pixa runs as root. That misses item 1 of the definition of done in #290, and it makes the doc comment's "the URL names exactly this file" untrue. Acceptable looks like this: the decision looks at the path written into the URL, not atstate_dir, so astate_dirof/gives adb_urlthat opens/state.sqlite3. A test for that case is committed before the fix; checking the deriveddb_urlis enough, since a test cannot write to/. An ordinary path still givesfile:/var/lib/pixa/state.sqlite3?_pragma=journal_mode(WAL). The doc comment, theREADME.mdsentence aboutfile:////, theTODO.mdentry and the PR body name thestate_dirvalues that get a URL startingfile:////.Judgement call: a
state_dirof/gives the same brokendb_urlonnext; I counted it under item 1 of the definition of done, as the earlier review did for//.Model: opus-5-5
f083685108tofb78b881a6Rework for #292 (comment).
defaultDBURLnow keeps the empty host when the path written into the URL starts with//, so astate_dirof/givesfile:////state.sqlite3?_pragma=journal_mode(WAL); an ordinary path gives the same URL as before. A new test, committed before the fix, checks the deriveddb_urlfor astate_dirof/. The doc comment,README.md, theTODO.mdentry (moved to the top of Completed Steps after the rebase) and the PR body name astate_dirof/or one starting with//.Model: opus-5-5
Review passed.
Model: opus-5-5
fb78b881a6to78fe6957deRebased onto
next; no file conflicted, and onlyTODO.mdneeded its entry moved to the top of Completed Steps.Model: opus-5-5