| Comment: | checkout: Update returns UpdateResult so conflicts are visible (#61)
* checkout: Update returns UpdateResult so conflicts are visible The internal 3-way merge already tracked which files were written, removed, and left with conflict markers, but Checkout.Update discarded all of it and returned a bare error. That collapsed a clean update and one that silently wrote <<<<<<< markers into working-tree files into the same outcome, with no way for a caller to tell them apart. Update now returns (UpdateResult, error). UpdateResult.Conflicted lists paths that were merged but not cleanly -- this is still a successful update (err == nil), not an error, since the working tree is usable and the caller just needs to know which files to show a user. A genuine failure still returns a non-nil error paired with a zero-value UpdateResult. FilesWritten/FilesRemoved are populated the same way, as paths rather than counts. Threading the paths up required changing the internal update's processFileUpdates from returning bare int counts to returning the actual []string paths, since the count was all that existed before. Checkout.Extract is untouched -- it has no conflict concept. Breaking change, clean break at v0.7.0 (pre-1.0). * checkout: sort Update result paths, document FilesWritten/Conflicted overlap Update's result slices were built by ranging over maps.allNames, a map[string]bool, so their order was whatever Go's randomized map iteration happened to produce that run. A caller diffing result.FilesWritten against a golden file, or asserting on it with reflect.DeepEqual, would see it fail intermittently for no reason related to their code. Sort all three slices (sort.Strings) right before Update returns. Added a test that touches four files -- two conflicting, two clean additions, named so map iteration order is very unlikely to hand them back sorted by luck -- and asserts the exact sorted slice on both FilesWritten and Conflicted. Verified it fails reliably (5/5 runs) with the sort calls temporarily removed, then restored the fix. Also stated outright, in both the internal and public UpdateResult doc comments, what "clean or not" was only implying: every path in Conflicted also appears in FilesWritten, since the conflict markers are what got written there. |
|---|---|
| Timelines: | family | ancestors | descendants | both | trunk |
| Files: | files | file ages | folders |
| SHA3-256: |
75f3521e54145123b72ca542dd226950 |
| User & Date: | [email protected] on 2026-07-19 16:31:18.000 |
| Other Links: | manifest | tags |
|
2026-07-19
| ||
| 16:50 |
fix: populate mlink parent columns on the Crosslink check-in path (#63)
* fix: populate mlink parent columns on the Crosslink check-in path insertCheckinMlinks (the xfer/Crosslink ingestion path) wrote mlink rows with only (mid, fid, fnid) set, leaving pmid/pid NULL. Fossil's compute_fileage filters mlink.fid != mlink.pid, and in SQLite <int> != NULL evaluates to NULL rather than true, so every row written by this path was silently dropped from ancestry-walk queries - `fossil ls -R <repo> -r <rev>` returned empty for any commit that arrived over the wire, even though `fossil open` worked fine (it reads the manifest blob directly and never consults mlink). Root cause: the two check-in paths that write mlink both needed to implement the same three-case pid convention (src/manifest.c:1668-1679 in canonical Fossil - carried-over/added/added-by-merge) and had drifted. insertMlinks (the direct check-in path) only handled two of the three cases and ignored merge parents entirely; insertCheckinMlinks didn't populate the parent columns at all. Factor the convention into one place (internal/manifest/mlink.go): resolveMlinkParent implements the pid/pmid rule, insertMlinkRow wraps it and also fills pfnid (rename tracking) and mperm (permission encoding), which neither path wrote before. Both insertMlinks and insertCheckinMlinks now call insertMlinkRow, so the two write paths produce indistinguishable rows and cannot drift apart again. insertCheckinMlinks also stopped skipping F-cards with an empty UUID (the delta-manifest deletion marker) - it now writes a row with fid=0, matching canonical's "deleted files have mlink.fid=0" and letting deletions participate in ancestry queries the same way Fossil's own writer does. Un-gates ls_branch_tip_repro_test.go (previously untracked, guarded behind LIBFOSSIL_LS_BRANCH_TIP_REPRO) as a permanent regression test, and adds explicit coverage in internal/manifest/mlink_test.go for all three pid cases (normal add, merge add, delete) on both write paths. * test: pin the deleted-file pid to the parent's exact file rid Strengthens the deleted_file_gets_fid_zero subtest added for #29: it was only checking pid != 0, which would pass even if resolveMlinkParent picked the wrong nonzero value. Compare against the primary parent's actual mlink.fid for the same filename instead. * fix: review findings on #29 — substring perm match, test isolation Blocking: permToMperm switched on the whole perm string, matching only exact "x"/"l". Canonical manifest_file_mperm (src/manifest.c:1482-1492) does a substring test (strstr), and F-card perm fields can carry more than one character — internal/deck/parse.go:194 assigns the field verbatim from remote xfer input, and Fossil itself emits multi-char perm strings (the " w" rename placeholder, see #51). An exact match on e.g. "wx" would silently drop the executable bit, the exact invariant PR #48 landed to protect. Switch to strings.Contains, x checked before l to match canonical's order (the same idiom already used at internal/checkout/vfile.go:52). Added TestPermToMperm_SubstringMatch, including a "wx" case pinned to mperm=1. Test isolation: rowFor in both mlink_test.go table-driven tests closed over the parent *testing.T, so a Fatalf in one subtest's row lookup aborted every subtest after it via runtime.Goexit unwinding the shared parent goroutine. Pass each subtest's own t into rowFor instead. Proven on the reviewer's revert run: after the deletion subtest's Fatalf, the merge-add and normal-add subtests never even started. Tightened the carried-over-file assertion in TestInsertMlinks_MergeParentsGetPidNegativeOne from "pid != 0" (would pass for -1 too) to pin the exact parent fid, matching how the crosslink-path twin test already asserts it. Documented (comment only, no behavior change) a known divergence from canonical in resolveMlinkParent: a file present in a merge parent with different content (a conflict resolved during the merge) should stay pid=0 in canonical Fossil, but this function returns pid=-1 for any file present in a merge parent regardless of content match. Low blast radius — every mlink consumer in this package filters pid != fid, which holds either way — but worth flagging so it isn't rediscovered as a surprise. check-in: 24c35dad87 user: [email protected] tags: trunk | |
| 16:31 |
checkout: Update returns UpdateResult so conflicts are visible (#61)
* checkout: Update returns UpdateResult so conflicts are visible The internal 3-way merge already tracked which files were written, removed, and left with conflict markers, but Checkout.Update discarded all of it and returned a bare error. That collapsed a clean update and one that silently wrote <<<<<<< markers into working-tree files into the same outcome, with no way for a caller to tell them apart. Update now returns (UpdateResult, error). UpdateResult.Conflicted lists paths that were merged but not cleanly -- this is still a successful update (err == nil), not an error, since the working tree is usable and the caller just needs to know which files to show a user. A genuine failure still returns a non-nil error paired with a zero-value UpdateResult. FilesWritten/FilesRemoved are populated the same way, as paths rather than counts. Threading the paths up required changing the internal update's processFileUpdates from returning bare int counts to returning the actual []string paths, since the count was all that existed before. Checkout.Extract is untouched -- it has no conflict concept. Breaking change, clean break at v0.7.0 (pre-1.0). * checkout: sort Update result paths, document FilesWritten/Conflicted overlap Update's result slices were built by ranging over maps.allNames, a map[string]bool, so their order was whatever Go's randomized map iteration happened to produce that run. A caller diffing result.FilesWritten against a golden file, or asserting on it with reflect.DeepEqual, would see it fail intermittently for no reason related to their code. Sort all three slices (sort.Strings) right before Update returns. Added a test that touches four files -- two conflicting, two clean additions, named so map iteration order is very unlikely to hand them back sorted by luck -- and asserts the exact sorted slice on both FilesWritten and Conflicted. Verified it fails reliably (5/5 runs) with the sort calls temporarily removed, then restored the fix. Also stated outright, in both the internal and public UpdateResult doc comments, what "clean or not" was only implying: every path in Conflicted also appears in FilesWritten, since the conflict markers are what got written there. check-in: 75f3521e54 user: [email protected] tags: trunk | |
| 16:30 |
fix: make Timeline pagination cursor an exact total order (#62)
* fix: make Timeline pagination cursor an exact total order #54 (merged as 95cbbb7) split Repo.Timeline into a repository-wide enumeration and Repo.Ancestry, the original first-parent walk under an honest name. Its pagination cursor predicate used a tolerance window around the caller-supplied Before/After pair to work around a lossy julian<->time.Time round trip: mtime < (C - eps) OR (mtime <= (C + eps) AND rid < After) This reintroduces exactly the defect the composite cursor existed to prevent, and makes it worse: rows within eps of the boundary that are not real ties get filtered by rid instead of tie-broken by it, which can skip a row, duplicate another, or fail to advance the cursor at all — unlike canonical fossil's own bare-timestamp slop, which repeats rows but still advances. A pagination test using only exact-tie and one-hour-apart fixtures could not exercise the 0 < delta < eps range where the defect lives, so it stayed green. Fix: make the predicate an exact total order, with rid as a true tie-break only at exact mtime equality: mtime < C OR (mtime = C AND rid < After) and stop deriving C from a time.Time altogether. TimelineOpts.After is now an opaque Cursor (internal/fsltype.Cursor, re-exported as libfossil.Cursor): obtained only from a LogEntry the library already produced, carrying the exact float64 mtime read off that row rather than one reconstructed from a rounded timestamp. A cursor built from parts can silently drift from the row it claims to follow; a cursor that can only ever be handed back cannot. That exactness depends on the row's scanned mtime being faithful to begin with, which surfaced a second bug: under the ncruces/WASM sqlite driver, event.mtime scans as a time.Time via the driver's own internal REAL conversion, carrying ~13us of noise that db.ScanJulianDay's time.Time branch then truncated (via UnixMilli()) into a full missed millisecond — independent of and in addition to the JulianToTime truncation bug #54 already fixed on the read-back side. Fixed db.timeToJulian to round to the nearest millisecond, the same class of fix, at the point where the driver-specific noise actually enters. Also tightened the regression coverage: - Pagination tests now use a fixed loop bound of exactly ceil(n/pageSize) pages rather than an open "while non-empty" loop, so a non-advancing cursor fails the test immediately instead of hanging it. - Added a sub-millisecond-boundary fixture (events 3ms apart, well inside the old epsilon window) with rid order deliberately decoupled from mtime order — a monotonic fixture cannot expose this class of bug, since rid and mtime need not move together in general (backdated commits, imports). - TestJulianRoundTrip now asserts exact equality for values originating from TimeToJulian instead of a *1ms tolerance, which would have passed under both the old truncating JulianToTime and the current rounding one and so was not a regression guard for that fix. No change to Ancestry, event-type filtering, NULL tolerance, or either loop bound; all were already correct. * fix: assert pagination termination; mark Cursor change breaking Both pagination tests (the pre-existing ones and the sub-millisecond regression) ran exactly ceil(n/pageSize) pages and asserted each event appeared once, but neither ever called Timeline one more time to confirm the next page comes back empty. The headline symptom of the epsilon-cursor defect these tests exist to catch was non-termination — a caller paginating until empty never stops — and that specific failure mode was going unguarded: a cursor that enumerates every row correctly but never signals exhaustion would have passed all four tests as they stood. Added one more Timeline call after the last expected page in all four tests, asserting it returns zero entries. Verified this is not vacuous: reverted the tie-break to <= (non-strict) and confirmed the existing per-event count checks catch it; separately, made Timeline phantom-return one row only on an already-exhausted call (isolated from the real per-page counts, which stayed correct in that experiment) and confirmed only the new final-page assertion fails — proving it exercises a real, non-redundant path, not one already covered by the count checks. Both experiments reverted before commit. Also corrected the CHANGELOG bullet describing the Cursor redesign: it described the new Cursor type but never stated that TimelineOpts.Before (time.Time) and TimelineOpts.After (FslID) were deleted outright, not deprecated. A reader diffing releases would have read it as an addition rather than a second compile-breaking change on top of #54. check-in: 168e20fc91 user: [email protected] tags: trunk | |