From 770ceb8777cef47105f381ded93c3366b8bf3390 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Mon, 31 Aug 2026 08:14:02 -0700 Subject: [PATCH] teach compare to use the git commit that added the snapshot for comparison --- docs/guides/periodic-review.md | 59 ++++++++++--- docs/reference/cli/compare.md | 2 +- src/commands/compare.zig | 41 +++++---- src/commands/contributions.zig | 6 +- src/git.zig | 153 +++++++++++++++++++++++++++++++++ 5 files changed, 232 insertions(+), 29 deletions(-) diff --git a/docs/guides/periodic-review.md b/docs/guides/periodic-review.md index dd9c425..43d78cb 100644 --- a/docs/guides/periodic-review.md +++ b/docs/guides/periodic-review.md @@ -66,7 +66,7 @@ the attribution will lie to you. One command gives you the whole "since last review" picture: ```bash -zfin compare 1W --projections --commit-before HEAD +zfin compare 1W --projections ``` - **`1W`** is the point of comparison -- the snapshot from one week ago. @@ -75,8 +75,8 @@ zfin compare 1W --projections --commit-before HEAD - **`--projections`** folds in projected-return and safe-withdrawal (SWR@99%) deltas, then vs. now. (Costs ~1-2s per endpoint for the Monte Carlo search; add `--no-events` to exclude life events.) -- **`--commit-before HEAD`** pins the contributions/gains attribution to - your latest reconciliation commit. This matters -- see +- **No `--commit-before` needed.** Attribution now anchors itself on the + commit that *recorded* the comparison snapshot. See [Attribution and commit timing](#attribution-and-commit-timing). Read off the liquid-total delta, the contributions-vs-gains split, the @@ -122,16 +122,51 @@ your backup and your audit trail. `compare` and `contributions` work out "contributions vs. gains" by walking the git history of `portfolio.srf`. The positional date (`1W`) -picks the *snapshot* whose prices you compare against; **`--commit-before`** -picks which *commit* anchors the attribution. Those two can drift apart. +picks the *snapshot* whose prices you compare against; the attribution +needs a *commit* to anchor on. If those two describe different periods, +the split is wrong -- and it is wrong silently, because gains are derived +as a residual (`delta - contributions`), so anything the attribution +window misses is reported as market performance. -If you reconcile on Saturday but don't commit until Monday, next -Saturday's `1W` snapshot lands *before* your last commit -- so a bare -`compare 1W` would attribute **two weeks** of contributions to one week. -`--commit-before HEAD` sidesteps this by pinning attribution to your -most recent reconciliation commit regardless of the snapshot date. When -the dates line up anyway, the flag is harmless -- which is why it's -worth making a habit. +**This is now handled for you.** The default anchor is the commit that +ADDED `history/-portfolio.srf` for the snapshot being compared +against. Because a review commits its snapshots together with the +reconciled `portfolio.srf`, that commit *is* that week's +reconciliation -- no naming convention, no dependence on how many times +you committed, and unaffected by later edits to the file. + +**Do not pass `--commit-before HEAD`.** It used to be the recommended +workaround for reconciling on Saturday and committing on Monday, and it +worked only while `HEAD` happened to be the *previous* week's +reconciliation commit. Commit anything during a review -- a data fix, a +metadata tweak -- and the attribution window collapses to "uncommitted +edits only" while the value window still spans the week. One observed +case reported ~$23k of vested shares as market gains. + +The flag remains for pinning a window deliberately; it is no longer a +habit worth having. + +### Re-snapshot after reconciling + +One ordering detail matters more than it looks. If a scheduled job +writes daily snapshots, the one for the last market day is captured +*before* you reconcile -- so it holds pre-reconcile share counts while +`portfolio.srf` ends up holding post-reconcile ones. The two then +disagree at the same commit, permanently, by one reconciliation: + +``` +history/2026-08-21-portfolio.srf AMZN 1569 (pre-reconcile) +portfolio.srf @ that same commit AMZN 1656 (post-reconcile) +``` + +`compare` reads the snapshot for "then" and the live portfolio for +"now", so those 87 shares appear in the value delta. Attribution walks +`portfolio.srf` across commits, where they were already present -- so no +contribution is found and the residual absorbs them. + +Fix: run `zfin snapshot --force` after reconciling and before +committing. On a non-trading day it rewrites the last trading day's +file, so it never invents a weekend snapshot. ## Relative dates diff --git a/docs/reference/cli/compare.md b/docs/reference/cli/compare.md index e882293..5501f3d 100644 --- a/docs/reference/cli/compare.md +++ b/docs/reference/cli/compare.md @@ -23,7 +23,7 @@ silently. | `--projections` | Add projected-return and 99% safe-withdrawal deltas (adds ~1-2s per endpoint). | | `--no-events` | With `--projections`, exclude life events. | | `--snapshot-before ` / `--snapshot-after ` | Override a side's snapshot (`--snapshot-after live` for the current portfolio). | -| `--commit-before ` / `--commit-after ` | Pin the git commit for the attribution block (`HEAD`, `HEAD~N`, SHA, or `working`). Useful when a review date and its commit diverge. | +| `--commit-before ` / `--commit-after ` | Pin the git commit for the attribution block (`HEAD`, `HEAD~N`, SHA, or `working`). Rarely needed: by default each side anchors on the commit that *recorded* its snapshot. Passing `HEAD` is usually wrong -- see [Attribution and commit timing](../../guides/periodic-review.md#attribution-and-commit-timing). | ## Example diff --git a/src/commands/compare.zig b/src/commands/compare.zig index 1231cb2..a8684ad 100644 --- a/src/commands/compare.zig +++ b/src/commands/compare.zig @@ -353,19 +353,19 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { // ── Snap dates to nearest-earlier snapshots ────────────── // - // The requested date may not correspond to an actual snapshot - // file. Snap to the most recent snapshot at-or-before. Record - // the original requested date so we can pass it to the - // attribution git-window independently (liquid uses the snap'd - // date; attribution uses the requested date unless --commit-* - // overrides). + // The requested date may not correspond to an actual snapshot file. Snap to the + // most recent snapshot at-or-before. + // + // The snapped dates are then used for BOTH the value window and the attribution + // window. They used to diverge - values on the snapped date, attribution on the + // requested one - which quietly guaranteed the two windows described different + // periods whenever a snap occurred, and `gains` (a residual) absorbed the + // difference. Snapping both keeps them aligned; see the `attr_before` + // construction below. var arena_state = std.heap.ArenaAllocator.init(allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); - const then_date_requested = then_date; - const now_date_requested = now_date; - const then_resolved = cli.resolveSnapshotOrExplain(io, arena, hist_dir, then_date) catch return error.SnapshotNotFound; if (!then_resolved.exact) { var stderr_buf: [256]u8 = undefined; @@ -445,18 +445,29 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { } } - // Build the CommitSpecs for attribution. Explicit --commit-* - // overrides win; otherwise fall back to the requested snapshot - // date. now_is_live implies `null` for after (= working copy - // default handling downstream). + // Build the CommitSpecs for attribution. Explicit --commit-* overrides win; + // otherwise anchor on the commit that RECORDED each side's snapshot. + // + // `snapshot_add` rather than `date_at_or_before`, and it matters. The value + // window's endpoints are snapshot files, so the attribution window has to be + // "the commit that recorded that snapshot" for the two to describe the same + // period. A date lands on whatever commit happened to be nearest, which drifts + // whenever a review is done on one day and committed on another - and `HEAD` + // (the previous workaround for that drift) collapses the window entirely as + // soon as anything is committed mid-review. See `git.commitThatAdded`. + // + // Uses the SNAPPED dates, not the requested ones: the snapped date is the + // snapshot that actually supplied the values, so it is the one whose recording + // commit bounds them. `snapshot_add` degrades to the old date behaviour when no + // snapshot for that date was committed. const attr_before: git.CommitSpec = commit_before_override orelse - .{ .date_at_or_before = then_date_requested }; + .{ .snapshot_add = then_date }; const attr_after_opt: ?git.CommitSpec = if (commit_after_override) |s| s else if (now_is_live) null else - .{ .date_at_or_before = now_date_requested }; + .{ .snapshot_add = now_date }; // Opt-in: bring the "then" snapshot into the current split basis so // a split between the two dates doesn't read as a phantom position diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index fa3a108..5e42d5b 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -744,7 +744,7 @@ fn specDisplayString(spec: ?git.CommitSpec, date_buf: *[10]u8) []const u8 { const s = spec orelse return "(unset)"; return switch (s) { .git_ref => |r| r, - .date_at_or_before => |d| std.fmt.bufPrint(date_buf, "{f}", .{d}) catch "????-??-??", + .date_at_or_before, .snapshot_add => |d| std.fmt.bufPrint(date_buf, "{f}", .{d}) catch "????-??-??", .working_copy => "working", }; } @@ -845,6 +845,10 @@ fn specLabel(arena: std.mem.Allocator, spec: ?git.CommitSpec, resolved_ref: []co return switch (s) { .git_ref => |r| arena.dupe(u8, r), .date_at_or_before => |d| std.fmt.allocPrint(arena, "commit at-or-before {f}", .{d}), + // Names the anchor, because "the commit that recorded this snapshot" is a + // materially different claim from "some commit near this date" and the + // header is where a reader checks which window they got. + .snapshot_add => |d| std.fmt.allocPrint(arena, "the commit recording {f}", .{d}), .working_copy => arena.dupe(u8, "working copy"), }; } diff --git a/src/git.zig b/src/git.zig index 8055f22..74e9960 100644 --- a/src/git.zig +++ b/src/git.zig @@ -78,6 +78,13 @@ pub const RepoInfo = struct { pub const CommitSpec = union(enum) { git_ref: []const u8, date_at_or_before: Date, + /// The commit that ADDED `history/-portfolio.srf`. + /// + /// The stable anchor for a weekly window - see `commitThatAdded` for why this + /// beats a bare date or `HEAD`. Falls back to `date_at_or_before` when no + /// snapshot for that date was ever committed, so a portfolio with no history + /// directory keeps working unchanged. + snapshot_add: Date, working_copy, }; @@ -408,6 +415,68 @@ pub fn lastCommitTimestampForPath( return std.fmt.parseInt(i64, trimmed, 10) catch return null; } +/// Return the SHA of the commit that ADDED `rel_path`, or null if the path was +/// never added (or does not exist in history). +/// +/// `--diff-filter=A` is the whole point. It matches only the commit that +/// introduced the file, ignoring every later modification - which is what makes +/// this a stable anchor where a date or `HEAD` is not. +/// +/// ## Why anchoring on a snapshot's add-commit is the right window +/// +/// The weekly review writes `history/-portfolio.srf` for each market day and +/// commits the whole batch together with the reconciled `portfolio.srf`. So the +/// commit that ADDED a given Friday's snapshot IS that week's reconciliation +/// commit, by construction. Resolving it needs no commit-message convention, is +/// unaffected by how many other commits landed that day, and survives later +/// rewrites of the file's contents. +/// +/// The alternatives all failed in practice: +/// +/// - `HEAD` assumes HEAD is the PREVIOUS week's reconcile commit. Commit anything +/// during a review - a data fix, a metadata tweak - and the attribution window +/// silently collapses to "uncommitted edits only" while the value window still +/// spans the week. Observed: five commits landed during one review, and ~$23k +/// of vested shares got reported as market gains. +/// - A date (`--until= -- portfolio.srf`) is close, but drifts whenever a +/// review is done on one day and committed on another, which is the documented +/// "committed mid-week after the review date" edge case. +/// - "Last commit touching history/" picks up retroactive restatements. One +/// commit rewrote 97 historical snapshots at once; `--diff-filter=A` ignored it +/// correctly, a plain path filter would not have. +/// +/// Caller owns the returned string. +pub fn commitThatAdded( + io: std.Io, + allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, + root: []const u8, + rel_path: []const u8, +) Error!?[]const u8 { + // `--follow` is deliberately NOT used. It would chase renames and could walk + // back into a differently-named predecessor, which is exactly the kind of + // silent window-widening this function exists to prevent. + const result = runGit(io, allocator, env, &.{ + "git", "-C", root, + "log", "--diff-filter=A", "-1", + "--format=%H", "--", rel_path, + }, .limited(64 * 1024)) catch return error.GitUnavailable; + defer allocator.free(result.stdout); + defer allocator.free(result.stderr); + + switch (result.term) { + .exited => |code| if (code != 0) return error.GitLogFailed, + else => return error.GitLogFailed, + } + + const trimmed = std.mem.trim(u8, result.stdout, " \t\r\n"); + if (trimmed.len == 0) return null; + // Same hash-shape guard as `commitAtOrBeforeDate`. + if (trimmed.len < 40) return error.GitLogFailed; + for (trimmed) |c| if (!std.ascii.isHex(c)) return error.GitLogFailed; + return try allocator.dupe(u8, trimmed); +} + /// Return the SHA of the most recent commit that touched any of /// `rel_paths` at or before `date_iso` (YYYY-MM-DD, inclusive /// end-of-day semantics via `git log --until`). @@ -651,10 +720,36 @@ fn resolveSpec(io: std.Io, arena: std.mem.Allocator, env: *const std.process.Env return error.NoCommitAtOrBefore; break :blk sha; }, + .snapshot_add => |d| blk: { + const snap_rel = try snapshotRelPath(arena, rel_paths, d); + if (try commitThatAdded(io, arena, env, repo.root, snap_rel)) |sha| break :blk sha; + // No committed snapshot for that date. Fall back to the date behaviour + // rather than erroring: a portfolio kept without a history directory is + // a legitimate configuration, and this spec should degrade to the old + // resolution instead of breaking it. + var buf: [10]u8 = undefined; + const date_str = std.fmt.bufPrint(&buf, "{f}", .{d}) catch buf[0..]; + const sha = (try commitAtOrBeforeDate(io, arena, env, repo.root, rel_paths, date_str)) orelse + return error.NoCommitAtOrBefore; + break :blk sha; + }, .working_copy => error.InvalidArg, }; } +/// `history/-portfolio.srf`, relative to the repo root. +/// +/// Derived from the portfolio's own repo-relative directory so a portfolio kept in +/// a subdirectory resolves its sibling history dir rather than one at the root. +fn snapshotRelPath(arena: std.mem.Allocator, rel_paths: []const []const u8, d: Date) Error![]const u8 { + const first = if (rel_paths.len > 0) rel_paths[0] else "portfolio.srf"; + const dir = std.fs.path.dirname(first); + return if (dir) |dd| + std.fmt.allocPrint(arena, "{s}/history/{f}-portfolio.srf", .{ dd, d }) catch error.OutOfMemory + else + std.fmt.allocPrint(arena, "history/{f}-portfolio.srf", .{d}) catch error.OutOfMemory; +} + /// Back-compat wrapper for the original `Date`-based API. Existing /// callers (legacy `zfin contributions --since / --until`) keep /// working unchanged. New callers using explicit commit refs go @@ -913,3 +1008,61 @@ test "resolveCommitRange: --since with no earlier commit -> NoCommitAtOrBefore" ); try std.testing.expectError(error.NoCommitAtOrBefore, result); } + +test "commitThatAdded finds the introducing commit and ignores later edits" { + // Uses the ambient zfin checkout: `build.zig` was added once and modified many + // times since, which is exactly the shape `--diff-filter=A` must see through. + const allocator = std.testing.allocator; + var env = gitTestEnv(allocator); + defer env.deinit(); + const info = findRepo(std.testing.io, allocator, &env, "build.zig") catch return; + defer allocator.free(info.root); + defer allocator.free(info.rel_path); + + const add_sha = (commitThatAdded(std.testing.io, allocator, &env, info.root, info.rel_path) catch return) orelse return; + defer allocator.free(add_sha); + try std.testing.expect(add_sha.len == 40 or add_sha.len == 64); + for (add_sha) |c| try std.testing.expect(std.ascii.isHex(c)); + + // The add-commit must be at-or-older than the newest commit touching the file. + // If `--diff-filter=A` were being ignored these would be equal for a file with + // any edit history, which `build.zig` certainly has. + const newest = (commitAtOrBeforeDate(std.testing.io, allocator, &env, info.root, &.{info.rel_path}, "2099-01-01") catch return) orelse return; + defer allocator.free(newest); + try std.testing.expect(!std.mem.eql(u8, add_sha, newest)); +} + +test "commitThatAdded returns null for a path never committed" { + const allocator = std.testing.allocator; + var env = gitTestEnv(allocator); + defer env.deinit(); + const info = findRepo(std.testing.io, allocator, &env, "build.zig") catch return; + defer allocator.free(info.root); + defer allocator.free(info.rel_path); + + // Null, not an error: `resolveSpec` relies on this to fall back to the date + // behaviour for a portfolio kept without a committed history directory. + const sha_opt = commitThatAdded(std.testing.io, allocator, &env, info.root, "history/1970-01-01-portfolio.srf") catch return; + try std.testing.expect(sha_opt == null); +} + +test "snapshotRelPath places history beside the portfolio, not at the repo root" { + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + const d = Date.fromYmd(2026, 8, 21); + + // Portfolio at the repo root. + const at_root = try snapshotRelPath(arena, &.{"portfolio.srf"}, d); + try std.testing.expectEqualStrings("history/2026-08-21-portfolio.srf", at_root); + + // Portfolio in a subdirectory: the history dir is its SIBLING. Resolving to a + // root-level `history/` would silently find nothing and fall back to the date + // behaviour, reintroducing the drift this spec exists to remove. + const nested = try snapshotRelPath(arena, &.{"accounts/portfolio.srf"}, d); + try std.testing.expectEqualStrings("accounts/history/2026-08-21-portfolio.srf", nested); + + // Empty path list is defensive, not expected; must not crash. + const empty = try snapshotRelPath(arena, &.{}, d); + try std.testing.expectEqualStrings("history/2026-08-21-portfolio.srf", empty); +}