dolt_checkout, dolt_revert and dolt_cherry_pick mask SQLITE_READONLY as SQLITE_ERROR
- Dominant language
- C
- Stars
- 268
- Forks
- 18
- Avg merge
- 2h 27m
- Merged PRs (30d)
- 447
Description
Found in a full-repo review at `0ba280f06f`.
On a read-only connection most version-control functions correctly return `SQLITE_READONLY`, but three return a bare `SQLITE_ERROR` with a message that does not say why. An application retrying on `SQLITE_READONLY` or `SQLITE_BUSY` cannot tell "this database is read-only" from "the revert genuinely failed".
Measured on a `SQLITE_OPEN_READONLY` connection (and identically under `PRAGMA query_only=1`):
```
INSERT INTO t VALUES(50,'x') rc=8 attempt to write a readonly database
dolt_add('-A') rc=8 attempt to write a readonly database
dolt_branch('nb') rc=8 attempt to write a readonly database
dolt_branch('-D','feat') rc=8 attempt to write a readonly database
dolt_tag('nt') rc=8 attempt to write a readonly database
dolt_merge('feat') rc=8 attempt to write a readonly database
dolt_reset('--hard') rc=8 attempt to write a readonly database
dolt_clean() rc=8 attempt to write a readonly database
dolt_gc() rc=8 attempt to write a readonly database
dolt_remote('add','r','file://./x') rc=8 attempt to write a readonly database
dolt_checkout('feat') rc=1 checkout failed
dolt_revert('HEAD') rc=1 revert of "HEAD" failed
dolt_cherry_pick('HEAD') rc=1 cherry-pick of HEAD failed
```
All three succeed on a writable connection with the same arguments, so the `rc=1` is the masked read-only refusal rather than a different failure.
## Cause
`doltliteVcResultError` (`src/doltlite_core.c:1049`) sets the message and never the code:
```c
void doltliteVcResultError(sqlite3_context *ctx, sqlite3 *db, const char *zMsg){
(void)doltliteVcSealSavepointError(db);
sqlite3_result_error(ctx, zMsg, -1);
}
```
The three catch-all sites that swallow a propagated `rc` through it are `src/doltlite_checkout.c:1358` and `:1403`, `src/doltlite_revert.c:260` and `:270`, and `src/doltlite_cherry_pick.c:468` and `:485`.
Most of the ~80 other callers of this helper are genuine usage/validation errors where `SQLITE_ERROR` is right, so the helper itself is not wrong everywhere — only these catch-alls are.
## Fix
At the sites that are relaying a real `rc`, follow the pattern already used at 124 other places in the tree: `sqlite3_result_error()` for the message, then `sqlite3_result_error_code()` for the code (that order preserves the message). This is the residue of #2794; the same sweep that fixed the ref-result path did not reach these three.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start at doltliteVcResultError in src/doltlite_core.c, then inspect the catch-all sites in src/doltlite_checkout.c, src/doltlite_revert.c, and src/doltlite_cherry_pick.c, comparing existing error-code call sites. Done when read-only connections return SQLITE_READONLY with the existing messages for checkout, revert, and cherry-pick, while genuine validation errors remain SQLITE_ERROR.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, sqlite
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- Half a day
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 88/100