Skip to content
go-libfossil Timeline
Login

Timeline

Parents and children of check-in [f2fa2cc751]

2026-07-19
23:16
fix: remove StatusOpts, MergeOpts, CheckoutOpts — no call sites (#97)

* fix: remove StatusOpts, MergeOpts, CheckoutOpts — no call sites

No function in the package accepts any of these three types, and nothing constructs one anywhere: Checkout.Status() takes zero arguments, Repo.Merge takes positional string arguments, and CheckoutOpts had no construction site at all. A public type with no call site documents a capability that does not exist -- a consumer reading MergeOpts{Strategy: ...} in the docs could reasonably conclude a strategy-selecting merge API exists; it never did.

Repo.Merge and Checkout.Status keep their current signatures. An options-struct refactor for Merge was considered and declined.

CheckoutOpts.Force was also a third same-named Force field in this package, alongside the already-removed UpdateOpts.Force (#58) and the real, fully-wired ExtractOpts.Force -- removing it also removes that readability hazard. ExtractOpts.Force and its plumbing (checkout.go Extract -> internal/checkout.ExtractOpts -> extract.go's Force check) are untouched.

* docs: match em dash style in CHANGELOG breaking-change bullet

The new StatusOpts/MergeOpts/CheckoutOpts entry used ASCII double hyphens where the adjacent CloneOpts and UpdateOpts.Force bullets in the same file use a real em dash. check-in: 1a33f22119 user: [email protected] tags: trunk

22:42
fix: clone_seqno is server-to-client only (#96)

* fix: clone_seqno is server-to-client only (#74)

The clone client transmitted a `clone_seqno` card to carry its pagination cursor. That card travels the other way: canonical's page_xfer() only ever writes it (src/xfer.c:1571) and has no parser for it, so a real fossil server answered `bad command: clone_seqno N` and the clone died on round two. Repositories that fit in a single batch never reached the pagination path, which is why wapp and pikchr cloned while fossil and sqlite did not.

The request cursor belongs on the clone card itself — `clone VERSION SEQNO`, read from token 2 at src/xfer.c:1553 — and the response cursor belongs on `clone_seqno`, which the client only consumes. Give each direction a single owner:

- The client sends its cursor on the clone card and never emits a clone_seqno card. It already consumed the server's card correctly; it now also rejects a negative value, matching src/xfer.c:2769. - The server reads its cursor from the clone card and the CloneSeqNoCard parser is gone, so there is exactly one way to set the cursor rather than two.

Aligning with canonical also corrects the cursor's meaning. Ours was the last rid sent, queried with `rid > cursor`; canonical's is the rid of the next blob to send, looped inclusively with `while( seqno<=max )`. Now that the value crosses to a real server it has to mean what canonical means, so emitCloneBatch queries `rid >= cursor` and reports the first rid it did not send, or 0 once the snapshot is exhausted. That collapses the old more/lastSentRID pair into a single nextRID and keeps the truncate hook honest by re-owing the dropped rid instead of skipping it.

TestCloneMultiBatchAgainstCanonicalServer drives the client against a stand-in server that implements the canonical contract and rejects a client-sent clone_seqno the way fossil does; it reproduces `bad command: clone_seqno` against the old client. TestHandleCloneCursorFromCloneCard pins the server half, including the inclusive cursor.

Verified against fossil 2.28 over a loopback server: fossil and sqlite now transfer every artifact (67,499 and 130,994 blobs), and wapp and pikchr still clone in 3 rounds.

* fix: conform clone cursor validation to the spec in both directions (#74)

Review caught two rules applied to the wrong side of the wire — the same axis error the original bug was.

The client rejected a negative clone_seqno as fatal, citing src/xfer.c:2769. That line is the server's rule for the SEQNO on a clone REQUEST. The reply parser is governed by §3.2 and §8.2: NEXT is recorded only when it is decimal, and a visible but non-decimal NEXT is a defined receiver-tolerance exception that leaves the recorded sequence unchanged and does not stop parsing later reply cards. Aborting the clone was the one thing the spec rules out. The rule belongs in the decoder, where a non-decimal NEXT now decodes to an unknown card: the token is preserved, no consumer records it, and following cards still parse. strconv.Atoi accepted "-1" off the wire, so this was reachable, not theoretical.

Conversely the server clamped `clone 3 0` to a cursor of 1 where §8.1 mandates a fatal: clear accumulated output, roll back, emit `error invalid clone sequence number`, and parse no later request card. Nothing is written before processDataCards, so returning early is what the rollback amounts to. Seeing this case at all needs a presence flag, because `clone` and `clone 3 0` both parsed to SeqNo 0 and only the former is exempt. §8.1 also keys on a digit-only SEQNO and withholds the fatal from wrong arity, a VERSION below 2, and a SEQNO that fails digit-only recognition, so the flag records digit-only presence rather than merely arity.

Both card rules now turn on the spec's `decimal = 1*DIGIT` production via a shared predicate instead of on whatever strconv.Atoi tolerates.

Also promotes emitCloneBatch's `count > 1` truncate guard to a documented liveness constraint. Under the old exclusive `rid > cursor` semantics `count > 0` was safe; under the inclusive cursor, dropping the only card in a batch reports a cursor equal to the one we entered with and the client re-requests forever. A check before the reply card now names a stalled round at its source instead of letting it surface as an opaque MaxRounds failure, and the cursor precondition became an error return rather than a panic — it is enforced ~350 lines away in a network-facing handler.

Records the deliberate asymmetry that our server ignores an inbound clone_seqno where canonical answers `bad command`, and renames TestHandleCloneSeqNo, which no longer sends that card.

* fix: name the clone SEQNO flag for the shape it records (#74)

HasSeqNo described presence, but the field records whether the SEQNO token is digit-only: `clone 3 -1` carries a SEQNO and sets it false. §8.1 keys its fatal on that shape rather than on the parsed value, which is why a plain int cannot stand in for it. SeqNoIsDecimal says so.

Reading it as presence would also collapse `clone 0 0` — a distinct wire form that parses to the same Version and SeqNo as a bare `clone` — back into `clone` on re-encode. encodeClone already guarded against that; a round-trip test over every clone wire form now pins it, and fails on exactly that input when the guard is removed.

Also records why the §8.1 rollback is currently vacuous: no card handled before processDataCards writes anything, so returning early leaves nothing to undo. Nothing enforces that ordering, and §3.5's ci-lock would break it, so the dependency is written down next to the code that relies on it.

Corrects a test comment that claimed a continuation property it did not exercise — the malformed card sits in the trailer, so nothing follows it. That half of §8.2 is covered by the follow-on card in TestDecode_CloneSeqNoNonDecimal. check-in: f2fa2cc751 user: [email protected] tags: trunk

22:27
Enforce card occurrence classes and intra-run ordering (#94)

* Enforce card occurrence classes and intra-run ordering

The deck parser enforced card-type ordering and nothing else. Each of the 14 single-occurrence card types was handled by a case that assigned, so a second occurrence silently overwrote the first, and the repeatable types were appended with no sort check at all. draft-fossil-artifact-format-00 §4.5.1 and §4.5.2 define both rules; implement them.

Duplicate rejection covers A, B, C, D, E, G, H, I, K, L, N, R, U and W. The guard is separate from the card-type check because that check permits equal consecutive letters. F, J and M gain a strictly-ascending check against the preceding entry, T the two-level (name, target hash) rule with the full sign-prefixed name as primary key. P and Q deliberately get neither: P order is semantic — primary parent first — and Q is unordered-repeatable. P also now appends across cards instead of replacing, which is what §4.5.1 means by repeatable. Z belongs to neither class, so it is accepted with no duplicate or last-card rule; the authoritative checksum is the trailing 35 bytes VerifyZ already verified.

All four ordering checks route through one comparator, deck.Compare, which is §4.5.3: unsigned octets, locale-independent, no normalization, no case folding. §4.5.3 exists because the same ordering underlies the delta-manifest merge walk and baseline binary search of §8.2, so a second copy would resolve a delta manifest wrongly rather than merely fail validation. The F check runs on the decoded filename, per §4.5.2.

Malformed input is reported through the error return, never a panic -- these bytes arrive from peers.

Two writer-side defects surfaced, both of which let this package emit artifacts its own parser now rejects. Marshal emitted J cards in caller-supplied order while already sorting F, M and T; it now sorts them. Marshal also sorted T by the concatenation name+target, which lets the target's leading hex digit compete with the tail of a longer name and reorders tags whose names are prefixes of one another; it now uses the two-level key. The F and M sorts and the R-card file enumeration route through deck.Compare as well, the latter per §6.3.

Closes #70

* Order T cards by decoded name and type the Z payload

Review found two ways the first pass diverged from canonical fossil in the accept/reject direction.

The T ordering key used the raw wire name. §4.7.16 says the name is escape-decoded and that the primary ordering key is the full name token including its leading sign character; manifest.c defossilizes the name before the ordering compare. Because "\s" begins 0x5C and a space is 0x20, the two forms order oppositely, so a run like

T *sym-a\sb aaaa... T *sym-a-c bbbb...

is valid, fossil accepts it, and we rejected it. Branch names may contain spaces, so this is an ordinary tag, not a corner case -- the same post-decode trap already handled for F. tagOrderKey now builds the decoded key and both the parser check and the marshaller's sort call it, so the two cannot drift apart.

The Z card accepted any payload. Accepting a Z line inside the body is right -- §4.5.1 puts Z in neither occurrence class and treats an earlier Z as content covered by the final checksum -- but §4.7.19 still fixes the payload at exactly 32 hexadecimal characters. Validating nothing left an arbitrary-byte channel in an otherwise fully typed grammar and would have stored and forwarded artifacts canonical refuses. Upper-case digits are accepted, matching fossil's validate16.

Also: the T sign prefix is now checked against {+,-,*} per §4.7.16, both because canonical checks it and because an unvalidated sign byte >= 0x80 would UTF-8-expand to two bytes when the ordering key is assembled and silently corrupt the comparison. Two stale §4.7.20 citations corrected to §4.7.16, and the two-level T test gained the prefix-name case that actually distinguishes a two-level comparison from a concatenated one.

* Cite the normative grammar for the case-insensitive Z token

The Z payload's case-insensitivity was justified by inferring it from the word "hexadecimal" in §4.7.19. §6.1 states it normatively: md5-token = 32hexdig-ci, a parse-accept grammar admitting both 0-9a-f and A-F. Cite that instead, and record the split it implies -- the structural check is case-insensitive while §6.2 keeps verification byte-exact against the lowercase form.

Also soften deck.Compare's claim that the delta-manifest merge walk relies on F ordering to the future tense parse.go already uses: the merge walk is a map overlay today with no comparator to share. check-in: 5036cae392 user: [email protected] tags: trunk