dolthub / dolthub/doltlite

dolt_checkout, dolt_revert and dolt_cherry_pick mask SQLITE_READONLY as SQLITE_ERROR

Closed Beginner friendly
#2,907 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.