From 1c1c6716ee4ef1805ee2017ed1141e6bf25ad44b Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Sat, 5 Sep 2026 13:59:00 -0700 Subject: [PATCH] contributions dates trigger on snapshots for both before and after side --- src/commands/contributions.zig | 240 ++++++++++++++++++++++++++++----- src/git.zig | 38 +++++- 2 files changed, 246 insertions(+), 32 deletions(-) diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index a956852..5725059 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -246,6 +246,7 @@ const TimeRange = @import("TimeRange.zig"); const analysis = @import("../analytics/analysis.zig"); const transaction_log = @import("../models/transaction_log.zig"); const portfolio_loader = @import("../portfolio_loader.zig"); +const history = @import("../history.zig"); const test_git = @import("../testutil/git.zig"); const Money = @import("../Money.zig"); const Date = zfin.Date; @@ -280,17 +281,21 @@ pub const meta: framework.Meta = .{ \\ No flags (default): \\ dirty working tree: HEAD vs working copy \\ clean working tree: HEAD~1 vs HEAD (review last commit) - \\ --since commit-at-or-before(DATE) vs HEAD (or - \\ working copy when dirty) - \\ --since --until commit-at-or-before(D1) vs - \\ commit-at-or-before(D2) + \\ --since the commit recording DATE's snapshot vs + \\ HEAD (or working copy when dirty) + \\ --since --until the commits recording each snapshot \\ --until alone rejected; window is ambiguous \\ \\Date forms: YYYY-MM-DD or relative (1W/1M/1Q/1Y). \\ + \\A date resolves to the commit that recorded `history/DATE-portfolio.srf`, + \\which is the same anchor `zfin compare` uses - so the two agree on what a + \\given week means. With no snapshot committed for that date it falls back to + \\commit-at-or-before(DATE) and says so. + \\ \\Options: - \\ --since Earliest side (resolves to commit-at- - \\ or-before). + \\ --since Earliest side (the commit recording + \\ that date's snapshot). \\ --until Latest side. Pair with --since. \\ --commit-before Pin the before commit directly. Same \\ grammar as --commit-after, minus @@ -346,29 +351,47 @@ pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedAr // Translate Endpoint to CommitSpec. `--since` produces a date // endpoint; `--commit-before` a commit_spec endpoint. Map both // to the same CommitSpec union the existing run() expects. + // + // A date becomes `.snapshot_add`, NOT `.date_at_or_before`, so this command + // resolves a window the same way `compare` does. They used to disagree, and + // the disagreement was load-bearing in the worst way: `compare`'s uncounted + // line says "see `zfin contributions`" and REPORT_RUNBOOK sends you to + // `zfin contributions --since 1W` to explain `compare`'s own contributions + // figure - while `commitAtOrBeforeDate` could land on a commit BEFORE the + // reconcile that the snapshot-anchored side had already stepped past. The two + // then reported different money for the same requested week. + // + // Safe as a default because `.snapshot_add` degrades to exactly the old + // `commitAtOrBeforeDate` behaviour whenever no snapshot for that date was + // committed (see `git.resolveSpec`), so any date without a snapshot resolves + // bit-for-bit as before. Where a snapshot DOES exist it also inherits the + // anchor verification and its warning, which the date path never had. var parsed: ParsedArgs = .{}; if (tr_result.range.before) |ep| switch (ep) { - .date => |d| parsed.before = .{ .date_at_or_before = d }, + .date => |d| parsed.before = .{ .snapshot_add = d }, .commit_spec => |s| parsed.before = s, .live => unreachable, }; if (tr_result.range.after) |ep| switch (ep) { - .date => |d| parsed.after = .{ .date_at_or_before = d }, + .date => |d| parsed.after = .{ .snapshot_add = d }, .commit_spec => |s| parsed.after = s, .live => unreachable, }; - // Validate `--since on or before --until` ordering for the - // date-only form, matching legacy behavior. We can only check - // when BOTH sides are date_at_or_before (the commit-spec form - // is opaque until git resolves it). + // Validate `--since on or before --until` ordering for the date form, matching + // legacy behavior. Only checkable when BOTH sides carry a date - a `--commit-*` + // ref is opaque until git resolves it. + // + // Goes through `specDate` rather than testing for one variant. It used to read + // `b == .date_at_or_before`, which silently stopped rejecting inverted windows + // the moment a date started producing `.snapshot_add`. if (parsed.before) |b| if (parsed.after) |a| { - if (b == .date_at_or_before and a == .date_at_or_before) { - if (b.date_at_or_before.days > a.date_at_or_before.days) { + if (specDate(b)) |bd| if (specDate(a)) |ad| { + if (bd.days > ad.days) { cli.stderrPrint(io, "Error: --since must be on or before --until.\n"); return error.InvalidArg; } - } + }; }; return parsed; } @@ -682,7 +705,17 @@ fn resolveEndpoints( dirty: bool, verbosity: Verbosity, ) !Endpoints { - const range = git.resolveCommitRangeSpec(io, arena, env, repo, rel_paths, before, after, dirty) catch |err| { + // Snap any requested date to the snapshot that actually exists at-or-before it, + // BEFORE handing it to git. `compare` has always done this and it is the other + // half of making the two commands agree: a bare `--since 1W` names a date one + // week back, which lands on a weekend more often than not, and there is no + // snapshot for a Saturday. Without snapping, `.snapshot_add` finds no file, + // degrades to `commitAtOrBeforeDate`, and the whole point is lost - which is + // exactly what `--since 1W` did while `compare 1W` was already correct. + const before_snapped = snapSpec(io, arena, repo, rel_paths, before, verbosity, "since"); + const after_snapped = snapSpec(io, arena, repo, rel_paths, after, verbosity, "until"); + + const range = git.resolveCommitRangeSpec(io, arena, env, repo, rel_paths, before_snapped, after_snapped, dirty) catch |err| { if (verbosity == .verbose) { switch (err) { error.NoCommitAtOrBefore => { @@ -711,12 +744,12 @@ fn resolveEndpoints( // Label the endpoints based on the resolution mode. Matches the // legacy phrasing where possible so existing test assertions still // pass. - const label = try buildLabelFromSpecs(arena, range, before, after, dirty); + const label = try buildLabelFromSpecs(arena, range, before_snapped, after_snapped, dirty); // Same-commit warning for the two-date window case. Legit confusion // trigger - the user asked for a diff between two dates that both // snap to the same commit (e.g., no activity in the window). - if (before != null and after != null and verbosity == .verbose) { + if (before_snapped != null and after_snapped != null and verbosity == .verbose) { if (range.after_rev) |after_rev| { if (std.mem.eql(u8, range.before_rev, after_rev)) { cli.stderrPrint(io, "Warning: before and after resolve to the same commit; no changes to report.\n"); @@ -731,7 +764,10 @@ fn resolveEndpoints( // `docs/notes/commit-window-edge-case.md` (aka TODO.md) for the // motivating scenario. if (verbosity == .verbose) { - try maybeSnapNote(io, arena, env, repo, before, range.before_rev, "before"); + try maybeSnapNote(io, arena, env, repo, before_snapped, range.before_rev, "before", range.snapshot_anchor); + if (range.after_rev) |ar| { + try maybeSnapNote(io, arena, env, repo, after_snapped, ar, "after", range.snapshot_anchor_after); + } } // Deliberately NOT gated on `verbosity`. `.silent` exists because @@ -741,10 +777,49 @@ fn resolveEndpoints( // when a snapshot WAS resolved and disagreed with the copy on disk, so it can // never fire on the case `.silent` was introduced for. maybeSnapshotAnchorNote(io, range.snapshot_anchor); + maybeSnapshotAnchorNote(io, range.snapshot_anchor_after); return .{ .range = range, .label = label }; } +/// Replace a `.snapshot_add` date with the nearest snapshot at-or-before it. +/// +/// Returns the spec untouched for every other variant, and untouched when no +/// snapshot resolves at all - the degraded path is `git.resolveSpec`'s to handle, +/// and `maybeSnapNote` reports it from there. +/// +/// Announces a snap the way `compare` does, because silently moving the date a +/// user typed is how two commands come to disagree about the same week while both +/// look right. +fn snapSpec( + io: std.Io, + arena: std.mem.Allocator, + repo: git.RepoInfo, + rel_paths: []const []const u8, + spec: ?git.CommitSpec, + verbosity: Verbosity, + flag: []const u8, +) ?git.CommitSpec { + const s = spec orelse return spec; + const requested = switch (s) { + .snapshot_add => |d| d, + else => return spec, + }; + + const first = if (rel_paths.len > 0) rel_paths[0] else return spec; + const pf_path = std.fs.path.join(arena, &.{ repo.root, first }) catch return spec; + const hist_dir = history.deriveHistoryDir(arena, pf_path) catch return spec; + const resolved = history.resolveSnapshotDate(io, arena, hist_dir, requested) catch return spec; + if (resolved.exact) return spec; + + if (verbosity == .verbose) { + var buf: [200]u8 = undefined; + const msg = std.fmt.bufPrint(&buf, "(--{s} {f} has no snapshot; using {f}, the nearest at-or-before)\n", .{ flag, requested, resolved.actual }) catch ""; + if (msg.len > 0) cli.stderrPrint(io, msg); + } + return .{ .snapshot_add = resolved.actual }; +} + /// Say something when the snapshot anchor was not clean. /// /// Both cases mean the same thing operationally: the value side of a comparison @@ -761,14 +836,14 @@ fn resolveEndpoints( /// slip that will recur. fn maybeSnapshotAnchorNote(io: std.Io, anchor: ?git.SnapshotAnchor) void { const a = anchor orelse return; - var buf: [420]u8 = undefined; + var buf: [460]u8 = undefined; if (a.corrected_from) |from| { - const msg = std.fmt.bufPrint(&buf, "Note: the {f} snapshot was regenerated after it was first committed; anchoring attribution on {s} rather than {s}, which described different positions.\n", .{ a.date, shortSha(a.commit), shortSha(from) }) catch return; + const msg = std.fmt.bufPrint(&buf, "Note: the {f} snapshot was regenerated after it was first committed; anchoring the {s} side on {s} rather than {s}, which described different positions.\n", .{ a.date, a.side.label(), shortSha(a.commit), shortSha(from) }) catch return; cli.stderrPrint(io, msg); return; } if (a.unmatched) { - const msg = std.fmt.bufPrint(&buf, "Warning: no commit of the {f} snapshot matches the copy on disk, so the value and attribution sides describe different portfolios. Contributions and gains for this window are NOT reliable - re-run `zfin snapshot --force` for that date and commit it alongside portfolio.srf.\n", .{a.date}) catch return; + const msg = std.fmt.bufPrint(&buf, "Warning: no commit of the {f} snapshot matches the copy on disk, so the {s} side's values and attribution describe different portfolios. Contributions and gains for this window are NOT reliable - re-run `zfin snapshot --force` for that date and commit it alongside portfolio.srf.\n", .{ a.date, a.side.label() }) catch return; cli.stderrPrint(io, msg); } } @@ -797,12 +872,23 @@ fn maybeSnapNote( spec: ?git.CommitSpec, resolved_ref: []const u8, label: []const u8, + /// The resolved anchor for this side, when the spec was `.snapshot_add`. Null + /// means no snapshot for that date was committed and resolution FELL BACK to + /// `commitAtOrBeforeDate` - which is precisely the case this note is for. + anchor: ?git.SnapshotAnchor, ) !void { const s = spec orelse return; - const requested_date = switch (s) { - .date_at_or_before => |d| d, - else => return, - }; + const requested_date = specDate(s) orelse return; + + // A snapshot-anchored side that actually found its snapshot is not drifting - + // it is pinned to the commit that recorded that date, which is the point. Only + // the degraded path, where no snapshot existed and it fell back to a date, can + // land arbitrarily far from what was asked for. + // + // Written as "did the anchor resolve" rather than "is the spec a date", because + // the previous form (`.date_at_or_before => |d| d, else => return`) silenced + // this note outright the moment `--since` began producing `.snapshot_add`. + if (s == .snapshot_add and anchor != null) return; // Get the committer-date of the resolved commit. `%ct` gives a // Unix timestamp. @@ -816,8 +902,9 @@ fn maybeSnapNote( var msg_buf: [320]u8 = undefined; const msg = std.fmt.bufPrint( &msg_buf, - "(git {s} uses commit {s} from {f}, {d} day{s} before requested {f} - " ++ - "use --commit-{s} HEAD or a later date to pin to your latest reconciliation commit)\n", + "(git {s} uses commit {s} from {f}, {d} day{s} before requested {f}; no " ++ + "snapshot for that date is committed, so there was nothing to anchor on - " ++ + "use --commit-{s} to pin a revision explicitly)\n", .{ label, shortSha(resolved_ref), @@ -876,6 +963,18 @@ fn buildLabelFromSpecs( return buildLabel(arena, range, before_date, after_date, dirty); } +/// The date a spec carries, for the two variants that carry one. +/// +/// Exists so callers stop pattern-matching a single variant. Both `--since 1W` and +/// `compare`'s snapshot anchoring are "a date the user named"; a check written +/// against `.date_at_or_before` alone goes quietly blind when that mapping changes. +fn specDate(spec: git.CommitSpec) ?zfin.Date { + return switch (spec) { + .date_at_or_before, .snapshot_add => |d| d, + .git_ref, .working_copy => null, + }; +} + fn specLabel(arena: std.mem.Allocator, spec: ?git.CommitSpec, resolved_ref: []const u8) ![]const u8 { const s = spec orelse return arena.dupe(u8, resolved_ref); return switch (s) { @@ -3882,16 +3981,50 @@ test "parseArgs: empty args produces both null" { try testing.expect(parsed.after == null); } -test "parseArgs: --since populates before as date_at_or_before" { +test "parseArgs: --since populates before as snapshot_add" { + // `.snapshot_add`, not `.date_at_or_before`. This is what makes a date resolve + // the same way `compare` resolves one, so the two commands report the same + // money for the same requested week. It degrades to the old date behaviour + // inside `git.resolveSpec` when no snapshot exists, so nothing is lost. const today = zfin.Date.fromYmd(2026, 5, 9); const args = [_][]const u8{ "--since", "2026-04-01" }; const parsed = try parseArgsForTest(today, &args); switch (parsed.before.?) { - .date_at_or_before => |d| try testing.expect(d.eql(zfin.Date.fromYmd(2026, 4, 1))), + .snapshot_add => |d| try testing.expect(d.eql(zfin.Date.fromYmd(2026, 4, 1))), else => try testing.expect(false), } } +test "parseArgs: --until populates after as snapshot_add too" { + // Both endpoints, because both can be wrong and they fail in opposite + // directions - a before anchor that is too early re-counts a window, an after + // anchor that is too early drops it. + const today = zfin.Date.fromYmd(2026, 5, 9); + const args = [_][]const u8{ "--since", "2026-04-01", "--until", "2026-04-15" }; + const parsed = try parseArgsForTest(today, &args); + switch (parsed.after.?) { + .snapshot_add => |d| try testing.expect(d.eql(zfin.Date.fromYmd(2026, 4, 15))), + else => try testing.expect(false), + } +} + +test "specDate: reads the date out of either date-carrying variant" { + // The ordering check used to test for `.date_at_or_before` by hand and went + // blind the moment a date started producing `.snapshot_add`. Routing both + // through one accessor is what stops that recurring. + const d = zfin.Date.fromYmd(2026, 4, 1); + try testing.expect(specDate(.{ .date_at_or_before = d }).?.eql(d)); + try testing.expect(specDate(.{ .snapshot_add = d }).?.eql(d)); + try testing.expect(specDate(.{ .git_ref = "HEAD" }) == null); + try testing.expect(specDate(.working_copy) == null); +} + +test "parseArgs: an inverted window is still rejected after the spec change" { + const today = zfin.Date.fromYmd(2026, 5, 9); + const args = [_][]const u8{ "--since", "2026-04-15", "--until", "2026-04-01" }; + try testing.expectError(error.InvalidArg, parseArgsForTest(today, &args)); +} + test "parseArgs: --since + --until populates both" { const today = zfin.Date.fromYmd(2026, 5, 9); const args = [_][]const u8{ "--since", "2026-04-01", "--until", "2026-05-01" }; @@ -6152,6 +6285,53 @@ test "computeReport: per-account totals separate drip_confirmed from rollup" { // which requires a real repo and is covered by `src/git.zig` tests // plus manual smoke-testing. +test "maybeSnapNote fires for a degraded snapshot_add, and stays quiet for a resolved one" { + // The regression this guards was nearly shipped. `maybeSnapNote` read + // `.date_at_or_before => |d| d, else => return`, so the moment `--since` began + // producing `.snapshot_add` the note went silent - including for a date far + // outside the snapshot series, which is the one case it exists to catch. + // + // Snapshot-anchored-and-resolved is not drift: the anchor is deliberately the + // commit that recorded that date. Snapshot-anchored-and-degraded is, because + // resolution fell back to whatever commit happened to precede the date. + const allocator = std.testing.allocator; + if (!test_git.available(allocator)) return; + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const dir_len = try tmp.dir.realPathFile(std.testing.io, ".", &path_buf); + const dir = path_buf[0..dir_len]; + + try tmp.dir.writeFile(std.testing.io, .{ .sub_path = "portfolio.srf", .data = "security_type::cash,shares:num:1.00,account::A\n" }); + try test_git.run(allocator, dir, null, &.{ "init", "-q" }); + try test_git.run(allocator, dir, null, &.{ "config", "user.email", "t@e.com" }); + try test_git.run(allocator, dir, null, &.{ "config", "user.name", "T" }); + try test_git.run(allocator, dir, null, &.{ "config", "commit.gpgsign", "false" }); + try test_git.run(allocator, dir, null, &.{ "add", "." }); + try test_git.run(allocator, dir, "2026-01-05T12:00:00", &.{ "commit", "-q", "-m", "one" }); + + var arena_state = std.heap.ArenaAllocator.init(allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + var env = try std.testing.environ.createMap(allocator); + defer env.deinit(); + + const repo = git.findRepo(std.testing.io, arena, &env, dir) catch return; + const far_future = zfin.Date.fromYmd(2026, 6, 1); + + // Degraded: no `history/` at all, so `.snapshot_add` fell back to a commit five + // months earlier. `anchor == null` is the signal, and the note must fire. + try maybeSnapNote(std.testing.io, arena, &env, repo, .{ .snapshot_add = far_future }, "HEAD", "before", null); + + // Resolved: an anchor came back, so the gap is intentional and there is + // nothing to report. Reaching the timestamp lookup at all would be the bug. + try maybeSnapNote(std.testing.io, arena, &env, repo, .{ .snapshot_add = far_future }, "HEAD", "before", .{ + .commit = "HEAD", + .date = far_future, + }); +} + test "resolveEndpoints: legacy dirty -> HEAD vs working copy" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); diff --git a/src/git.zig b/src/git.zig index e8852e3..c31fc3a 100644 --- a/src/git.zig +++ b/src/git.zig @@ -116,18 +116,42 @@ pub const CommitRange = struct { before_rev: []const u8, /// null = working copy; non-null = a concrete git revision. after_rev: ?[]const u8, - /// How the before-side anchor was chosen, when it came from a `.snapshot_add` + /// How each anchor was chosen, when that side came from a `.snapshot_add` /// spec. Non-null only in that case, and only worth reading to WARN - the /// resolution has already happened. See `snapshotAnchor`. + /// + /// BOTH sides, because both can be wrong and they fail in opposite + /// directions: a before anchor that is too early re-counts a window's + /// contributions, an after anchor that is too early drops them. Only the + /// before side was checked at first, which left `compare ` - the + /// two-snapshot form, where `after` is a real revision rather than the working + /// copy - unverified on the half that silently under-reports. snapshot_anchor: ?SnapshotAnchor = null, + snapshot_anchor_after: ?SnapshotAnchor = null, }; /// The outcome of resolving a `.snapshot_add` anchor, so the caller can say /// something when it was not clean. +/// Which endpoint a `SnapshotAnchor` describes. +pub const AnchorSide = enum { + before, + after, + + pub fn label(self: AnchorSide) []const u8 { + return switch (self) { + .before => "before", + .after => "after", + }; + } +}; + pub const SnapshotAnchor = struct { commit: []const u8, /// The snapshot's own date, for the message. date: Date, + /// Which endpoint this describes, so a message can name it. A reader who is + /// told an anchor moved needs to know which end of the window moved. + side: AnchorSide = .before, /// Set when the anchor moved FORWARD off `commitThatAdded` because that /// commit's snapshot described different positions than the snapshot on disk. /// Names the commit we moved off. @@ -829,10 +853,20 @@ pub fn resolveCommitRangeSpec( // same git plumbing, and paying for it keeps `resolveSpec` returning a plain // sha for every other spec rather than threading an optional through all six. var anchor: ?SnapshotAnchor = null; + var anchor_after: ?SnapshotAnchor = null; if (before) |b| switch (b) { .snapshot_add => |d| { const snap_rel = try snapshotRelPath(arena, rel_paths, d); anchor = try snapshotAnchor(io, arena, env, repo.root, snap_rel, d); + if (anchor) |*a| a.side = .before; + }, + else => {}, + }; + if (after) |a| switch (a) { + .snapshot_add => |d| { + const snap_rel = try snapshotRelPath(arena, rel_paths, d); + anchor_after = try snapshotAnchor(io, arena, env, repo.root, snap_rel, d); + if (anchor_after) |*x| x.side = .after; }, else => {}, }; @@ -854,7 +888,7 @@ pub fn resolveCommitRangeSpec( else "HEAD"; - return .{ .before_rev = before_rev, .after_rev = after_rev, .snapshot_anchor = anchor }; + return .{ .before_rev = before_rev, .after_rev = after_rev, .snapshot_anchor = anchor, .snapshot_anchor_after = anchor_after }; } /// Resolve one non-working `CommitSpec` to a string git can consume.