From 9c06d25da33f1defeb1efcaecdd810f866ca9da8 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 24 Jun 2026 11:02:10 -0700 Subject: [PATCH] scrub git env variables from all tests --- src/commands/audit.zig | 13 +- src/commands/compare.zig | 4 +- src/commands/contributions.zig | 42 +++--- src/commands/snapshot.zig | 7 +- src/git.zig | 225 +++++++++++++++++++++++---------- src/portfolio_loader.zig | 43 ++++--- 6 files changed, 220 insertions(+), 114 deletions(-) diff --git a/src/commands/audit.zig b/src/commands/audit.zig index 5981074..d6c9dfb 100644 --- a/src/commands/audit.zig +++ b/src/commands/audit.zig @@ -1493,6 +1493,7 @@ fn printLargeLotWarning( fn runHygieneCheck( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, svc: *zfin.DataService, portfolio_path: []const u8, stale_days: u32, @@ -1574,7 +1575,7 @@ fn runHygieneCheck( { // Try to get committed version via git const git = @import("../git.zig"); - const repo_info: ?git.RepoInfo = git.findRepo(io, allocator, portfolio_path) catch null; + const repo_info: ?git.RepoInfo = git.findRepo(io, allocator, env, portfolio_path) catch null; defer if (repo_info) |ri| { allocator.free(ri.root); allocator.free(ri.rel_path); @@ -1588,7 +1589,7 @@ fn runHygieneCheck( defer if (committed_data) |d| allocator.free(d); if (repo_info) |ri| { - committed_data = git.show(io, allocator, ri.root, "HEAD", ri.rel_path) catch null; + committed_data = git.show(io, allocator, env, ri.root, "HEAD", ri.rel_path) catch null; if (committed_data) |cd| { committed_portfolio = zfin.cache.deserializePortfolio(allocator, cd) catch null; } @@ -1667,7 +1668,7 @@ fn runHygieneCheck( var since_buf: [32]u8 = undefined; const since = std.fmt.bufPrint(&since_buf, "{d} days ago", .{max_threshold}) catch "30 days ago"; - const commits = git.listCommitsTouching(io, allocator, ri.root, ri.rel_path, since) catch &.{}; + const commits = git.listCommitsTouching(io, allocator, env, ri.root, ri.rel_path, since) catch &.{}; defer git.freeCommitTouches(allocator, commits); var prev_data: ?[]const u8 = null; @@ -1677,7 +1678,7 @@ fn runHygieneCheck( // Stop early if every account already has a timestamp if (last_update_ts.count() >= all_accounts.count()) break; - const rev_data = git.show(io, allocator, ri.root, ct.commit, ri.rel_path) catch continue; + const rev_data = git.show(io, allocator, env, ri.root, ct.commit, ri.rel_path) catch continue; if (ci > 0) { if (prev_data) |pd| { @@ -1933,7 +1934,7 @@ fn runHygieneCheck( // there are no new lots at all, or when the pipeline can't run // (not in a git repo). Threshold is a judgment call; see // `audit_large_lot_threshold`. - if (contributions.findUnmatchedLargeLots(io, allocator, svc, portfolio_path, audit_large_lot_threshold, as_of, color, refresh)) |found| { + if (contributions.findUnmatchedLargeLots(io, allocator, env, svc, portfolio_path, audit_large_lot_threshold, as_of, color, refresh)) |found| { var found_mut = found; defer found_mut.deinit(); @@ -2063,7 +2064,7 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { if (fidelity_csv == null and schwab_csv == null and !schwab_summary) { const pf = ctx.resolvePortfolioPath(); defer pf.deinit(allocator); - return runHygieneCheck(io, allocator, svc, pf.path, stale_days, verbose, as_of, now_s, color, ctx.globals.refresh_policy, out); + return runHygieneCheck(io, allocator, ctx.environ_map, svc, pf.path, stale_days, verbose, as_of, now_s, color, ctx.globals.refresh_policy, out); } // Reconciliation modes (--fidelity / --schwab / --schwab-summary): diff --git a/src/commands/compare.zig b/src/commands/compare.zig index 9043d05..c9af66c 100644 --- a/src/commands/compare.zig +++ b/src/commands/compare.zig @@ -464,7 +464,7 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { // Attribution uses the resolved CommitSpecs so --commit-* // overrides + date fallbacks share one classifier. The caller // adapts dates to `CommitSpec.date_at_or_before` upstream. - const attribution = contributions.computeAttributionSpec(io, allocator, svc, portfolio_path, attr_before, attr_after_opt, as_of, color, ctx.globals.refresh_policy); + const attribution = contributions.computeAttributionSpec(io, allocator, ctx.environ_map, svc, portfolio_path, attr_before, attr_after_opt, as_of, color, ctx.globals.refresh_policy); try renderFromParts(out, color, allocator, .{ .then_date = then_date, @@ -481,7 +481,7 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { var now_side = try compare_core.loadSnapshotSide(io, allocator, hist_dir, now_date); defer now_side.deinit(allocator); - const attribution = contributions.computeAttributionSpec(io, allocator, svc, portfolio_path, attr_before, attr_after_opt, as_of, color, ctx.globals.refresh_policy); + const attribution = contributions.computeAttributionSpec(io, allocator, ctx.environ_map, svc, portfolio_path, attr_before, attr_after_opt, as_of, color, ctx.globals.refresh_policy); try renderFromParts(out, color, allocator, .{ .then_date = then_date, diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index e932c39..f7f61c7 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -333,12 +333,13 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { defer pf.deinit(allocator); const portfolio_path = pf.path; - return runImpl(io, allocator, svc, portfolio_path, before, after, as_of, color, ctx.globals.refresh_policy, out); + return runImpl(io, allocator, ctx.environ_map, svc, portfolio_path, before, after, as_of, color, ctx.globals.refresh_policy, out); } fn runImpl( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, svc: *zfin.DataService, portfolio_path: []const u8, before: ?git.CommitSpec, @@ -365,7 +366,7 @@ fn runImpl( return; } - var ctx = prepareReport(io, allocator, arena, svc, portfolio_path, before, after, as_of, color, refresh, .verbose) catch return; + var ctx = prepareReport(io, allocator, arena, env, svc, portfolio_path, before, after, as_of, color, refresh, .verbose) catch return; defer ctx.deinit(); try printReport(out, &ctx.report, ctx.endpoints.label, color); @@ -410,6 +411,7 @@ fn prepareReport( io: std.Io, allocator: std.mem.Allocator, arena: std.mem.Allocator, + env: *const std.process.Environ.Map, svc: *zfin.DataService, portfolio_path: []const u8, before_spec: ?git.CommitSpec, @@ -419,7 +421,7 @@ fn prepareReport( refresh: framework.RefreshPolicy, verbosity: Verbosity, ) PrepareError!ReportContext { - const repo = git.findRepo(io, arena, portfolio_path) catch |err| { + const repo = git.findRepo(io, arena, env, portfolio_path) catch |err| { if (verbosity == .verbose) { switch (err) { error.NotInGitRepo => cli.stderrPrint(io, "Error: contributions requires portfolio.srf to be in a git repo.\n"), @@ -430,7 +432,7 @@ fn prepareReport( return error.PrepareFailed; }; - const status = git.pathStatus(io, arena, repo.root, repo.rel_path) catch { + const status = git.pathStatus(io, arena, env, repo.root, repo.rel_path) catch { if (verbosity == .verbose) cli.stderrPrint(io, "Error: could not determine git status of portfolio.srf.\n"); return error.PrepareFailed; }; @@ -440,11 +442,11 @@ fn prepareReport( } const dirty = status == .modified; - const endpoints = resolveEndpoints(io, arena, repo, before_spec, after_spec, dirty, verbosity) catch return error.PrepareFailed; + const endpoints = resolveEndpoints(io, arena, env, repo, before_spec, after_spec, dirty, verbosity) catch return error.PrepareFailed; // Pull both sides: before is always from git; after is either // from git (at some revision) or from the working copy. - const before = git.show(io, arena, repo.root, endpoints.range.before_rev, repo.rel_path) catch |err| { + const before = git.show(io, arena, env, repo.root, endpoints.range.before_rev, repo.rel_path) catch |err| { if (verbosity == .verbose) { var buf: [256]u8 = undefined; const msg = std.fmt.bufPrint(&buf, "Error reading {s}:portfolio.srf from git: {s}\n", .{ endpoints.range.before_rev, @errorName(err) }) catch "Error reading before-side portfolio.\n"; @@ -454,7 +456,7 @@ fn prepareReport( }; const after = if (endpoints.range.after_rev) |rev| - git.show(io, arena, repo.root, rev, repo.rel_path) catch |err| { + git.show(io, arena, env, repo.root, rev, repo.rel_path) catch |err| { if (verbosity == .verbose) { var buf: [256]u8 = undefined; const msg = std.fmt.bufPrint(&buf, "Error reading {s}:portfolio.srf from git: {s}\n", .{ rev, @errorName(err) }) catch "Error reading after-side portfolio.\n"; @@ -530,14 +532,14 @@ fn prepareReport( }; var before_tlog_opt: ?transaction_log.TransactionLog = blk: { - const data = git.show(io, arena, repo.root, endpoints.range.before_rev, tlog_rel_path) catch break :blk null; + const data = git.show(io, arena, env, repo.root, endpoints.range.before_rev, tlog_rel_path) catch break :blk null; break :blk transaction_log.parseTransactionLogFile(arena, data) catch null; }; defer if (before_tlog_opt) |*tl| tl.deinit(); var after_tlog_opt: ?transaction_log.TransactionLog = blk: { if (endpoints.range.after_rev) |rev| { - const data = git.show(io, arena, repo.root, rev, tlog_rel_path) catch break :blk null; + const data = git.show(io, arena, env, repo.root, rev, tlog_rel_path) catch break :blk null; break :blk transaction_log.parseTransactionLogFile(arena, data) catch null; } else { break :blk svc.loadTransferLog(portfolio_path); @@ -598,13 +600,14 @@ const Verbosity = enum { verbose, silent }; fn resolveEndpoints( io: std.Io, arena: std.mem.Allocator, + env: *const std.process.Environ.Map, repo: git.RepoInfo, before: ?git.CommitSpec, after: ?git.CommitSpec, dirty: bool, verbosity: Verbosity, ) !Endpoints { - const range = git.resolveCommitRangeSpec(io, arena, repo, before, after, dirty) catch |err| { + const range = git.resolveCommitRangeSpec(io, arena, env, repo, before, after, dirty) catch |err| { if (verbosity == .verbose) { switch (err) { error.NoCommitAtOrBefore => { @@ -653,7 +656,7 @@ fn resolveEndpoints( // `docs/notes/commit-window-edge-case.md` (aka TODO.md) for the // motivating scenario. if (verbosity == .verbose) { - try maybeSnapNote(io, arena, repo, before, range.before_rev, "before"); + try maybeSnapNote(io, arena, env, repo, before, range.before_rev, "before"); } return .{ .range = range, .label = label }; @@ -678,6 +681,7 @@ fn specDisplayString(spec: ?git.CommitSpec, date_buf: *[10]u8) []const u8 { fn maybeSnapNote( io: std.Io, arena: std.mem.Allocator, + env: *const std.process.Environ.Map, repo: git.RepoInfo, spec: ?git.CommitSpec, resolved_ref: []const u8, @@ -691,7 +695,7 @@ fn maybeSnapNote( // Get the committer-date of the resolved commit. `%ct` gives a // Unix timestamp. - const ts = git.commitTimestamp(io, arena, repo.root, resolved_ref) catch return; + const ts = git.commitTimestamp(io, arena, env, repo.root, resolved_ref) catch return; const commit_date = zfin.Date.fromEpoch(ts); if (!commit_date.lessThan(requested_date)) return; @@ -870,6 +874,7 @@ pub const AttributionSummary = struct { pub fn computeAttributionSpec( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, svc: *zfin.DataService, portfolio_path: []const u8, before: ?git.CommitSpec, @@ -884,7 +889,7 @@ pub fn computeAttributionSpec( defer arena_state.deinit(); const arena = arena_state.allocator(); - var ctx = prepareReport(io, allocator, arena, svc, portfolio_path, before, after, as_of, color, refresh, .silent) catch return null; + var ctx = prepareReport(io, allocator, arena, env, svc, portfolio_path, before, after, as_of, color, refresh, .silent) catch return null; defer ctx.deinit(); return summarizeAttribution(ctx); @@ -940,6 +945,7 @@ pub const UnmatchedLargeLotSet = struct { pub fn findUnmatchedLargeLots( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, svc: *zfin.DataService, portfolio_path: []const u8, threshold: f64, @@ -959,7 +965,7 @@ pub fn findUnmatchedLargeLots( // // Separate allocator here so we can tear the whole thing down // via `arena_state.deinit` once we've copied out the descriptors. - var ctx = prepareReport(io, allocator, arena, svc, portfolio_path, null, null, as_of, color, refresh, .silent) catch { + var ctx = prepareReport(io, allocator, arena, env, svc, portfolio_path, null, null, as_of, color, refresh, .silent) catch { arena_state.deinit(); return null; }; @@ -3977,9 +3983,11 @@ test "computeReport: per-account totals separate drip_confirmed from rollup" { test "resolveEndpoints: legacy dirty → HEAD vs working copy" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); + var env = try std.testing.environ.createMap(std.testing.allocator); + defer env.deinit(); const repo: git.RepoInfo = .{ .root = "/tmp", .rel_path = "portfolio.srf" }; - const eps = try resolveEndpoints(std.testing.io, arena_state.allocator(), repo, null, null, true, .verbose); + const eps = try resolveEndpoints(std.testing.io, arena_state.allocator(), &env, repo, null, null, true, .verbose); try std.testing.expectEqualStrings("HEAD", eps.range.before_rev); try std.testing.expect(eps.range.after_rev == null); try std.testing.expect(std.mem.indexOf(u8, eps.label, "working copy against HEAD") != null); @@ -3988,9 +3996,11 @@ test "resolveEndpoints: legacy dirty → HEAD vs working copy" { test "resolveEndpoints: legacy clean → HEAD~1 vs HEAD" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); + var env = try std.testing.environ.createMap(std.testing.allocator); + defer env.deinit(); const repo: git.RepoInfo = .{ .root = "/tmp", .rel_path = "portfolio.srf" }; - const eps = try resolveEndpoints(std.testing.io, arena_state.allocator(), repo, null, null, false, .verbose); + const eps = try resolveEndpoints(std.testing.io, arena_state.allocator(), &env, repo, null, null, false, .verbose); try std.testing.expectEqualStrings("HEAD~1", eps.range.before_rev); try std.testing.expectEqualStrings("HEAD", eps.range.after_rev.?); try std.testing.expect(std.mem.indexOf(u8, eps.label, "HEAD~1 against HEAD") != null); diff --git a/src/commands/snapshot.zig b/src/commands/snapshot.zig index 911439d..06e3b05 100644 --- a/src/commands/snapshot.zig +++ b/src/commands/snapshot.zig @@ -425,6 +425,7 @@ fn loadPortfolioForSnapshot( ) !portfolio_loader.LoadedPortfolio { const io = ctx.io; const allocator = ctx.allocator; + const env = ctx.environ_map; const target = as_of orelse { // Normal mode — load working-copy union via the shared @@ -440,7 +441,7 @@ fn loadPortfolioForSnapshot( defer pf.deinit(allocator); const portfolio_path = pf.path; - const info = git.findRepo(io, allocator, portfolio_path) catch |err| switch (err) { + const info = git.findRepo(io, allocator, env, portfolio_path) catch |err| switch (err) { error.NotInGitRepo, error.GitUnavailable => { warnGitFallback(io, target, "no git repo"); return cli.loadPortfolio(ctx, target) orelse return error.WriteFailed; @@ -452,7 +453,7 @@ fn loadPortfolioForSnapshot( var date_buf: [10]u8 = undefined; const date_str = std.fmt.bufPrint(&date_buf, "{f}", .{target}) catch "????-??-??"; - const sha_opt = git.shaAtOrBefore(io, allocator, info.root, date_str) catch |err| switch (err) { + const sha_opt = git.shaAtOrBefore(io, allocator, env, info.root, date_str) catch |err| switch (err) { error.GitUnavailable, error.GitLogFailed => { warnGitFallback(io, target, "git lookup failed"); return cli.loadPortfolio(ctx, target) orelse return error.WriteFailed; @@ -472,7 +473,7 @@ fn loadPortfolioForSnapshot( }; defer resolved.deinit(); - return portfolio_loader.loadPortfolioFromPathsAtRev(io, allocator, resolved.paths, sha, target) orelse { + return portfolio_loader.loadPortfolioFromPathsAtRev(io, allocator, env, resolved.paths, sha, target) orelse { warnGitFallback(io, target, "could not load portfolio at rev"); return cli.loadPortfolio(ctx, target) orelse return error.WriteFailed; }; diff --git a/src/git.zig b/src/git.zig index f2dec37..598f517 100644 --- a/src/git.zig +++ b/src/git.zig @@ -112,12 +112,75 @@ pub const CommitRange = struct { // ── Implementation ─────────────────────────────────────────── +/// Build a child-process environment from `base` with every `GIT_*` +/// variable removed. +/// +/// zfin always targets a specific repo explicitly via `git -C `, +/// so it must never honor an ambient `GIT_DIR` / `GIT_WORK_TREE` / +/// `GIT_INDEX_FILE`. Those are set whenever zfin (or its test suite) +/// runs inside a git hook (pre-commit, prek, ...), and `git -C` changes +/// the CWD but does NOT clear those env vars — git would silently +/// operate on the hook's repo instead of the portfolio's, reading the +/// wrong file (or none). See https://github.com/j178/prek/issues/1786 +/// and https://pre-commit.com/ for the upstream guidance: code shelled +/// out by hooks that runs git against a different repo must explicitly +/// opt out of the inherited env. +/// +/// Caller owns the returned map; free with `.deinit()`. +fn scrubbedEnv( + allocator: std.mem.Allocator, + base: *const std.process.Environ.Map, +) std.mem.Allocator.Error!std.process.Environ.Map { + var map = try base.clone(allocator); + errdefer map.deinit(); + + // Collect-then-remove: the underlying ArrayHashMap can't be mutated + // mid-iteration, and `swapRemove` frees the map's owned key buffer, + // so the keys we want to remove must be duped first (otherwise the + // pointers in `keys_to_remove` would dangle). + var keys_to_remove: std.ArrayList([]u8) = .empty; + defer { + for (keys_to_remove.items) |k| allocator.free(k); + keys_to_remove.deinit(allocator); + } + + var it = map.iterator(); + while (it.next()) |entry| { + if (std.mem.startsWith(u8, entry.key_ptr.*, "GIT_")) { + try keys_to_remove.append(allocator, try allocator.dupe(u8, entry.key_ptr.*)); + } + } + for (keys_to_remove.items) |key| _ = map.swapRemove(key); + + return map; +} + +/// Run `git` with the ambient `GIT_*` environment variables stripped +/// (see `scrubbedEnv`). Thin wrapper over `std.process.run` — callers +/// keep their own result/term handling. `env` is the process +/// environment to derive the child env from (e.g. `ctx.environ_map`). +fn runGit( + io: std.Io, + allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, + argv: []const []const u8, + stdout_limit: std.Io.Limit, +) !std.process.RunResult { + var scrubbed = try scrubbedEnv(allocator, env); + defer scrubbed.deinit(); + return std.process.run(allocator, io, .{ + .argv = argv, + .environ_map = &scrubbed, + .stdout_limit = stdout_limit, + }); +} + /// Locate the git repository containing `path` and the path's position /// relative to the repo root. /// /// Allocator is used for the returned `root` and `rel_path` strings /// (caller-owned). -pub fn findRepo(io: std.Io, allocator: std.mem.Allocator, path: []const u8) Error!RepoInfo { +pub fn findRepo(io: std.Io, allocator: std.mem.Allocator, env: *const std.process.Environ.Map, path: []const u8) Error!RepoInfo { // Resolve the file's directory. realpath requires the file to exist. const abs_path = std.Io.Dir.cwd().realPathFileAlloc(io, path, allocator) catch { return error.NotInGitRepo; @@ -126,10 +189,7 @@ pub fn findRepo(io: std.Io, allocator: std.mem.Allocator, path: []const u8) Erro const dir = std.fs.path.dirname(abs_path) orelse "/"; // `git -C rev-parse --show-toplevel` prints the repo root. - const result = std.process.run(allocator, io, .{ - .argv = &.{ "git", "-C", dir, "rev-parse", "--show-toplevel" }, - .stdout_limit = .limited(64 * 1024), - }) catch { + const result = runGit(io, allocator, env, &.{ "git", "-C", dir, "rev-parse", "--show-toplevel" }, .limited(64 * 1024)) catch { return error.GitUnavailable; }; defer allocator.free(result.stdout); @@ -162,13 +222,11 @@ pub fn findRepo(io: std.Io, allocator: std.mem.Allocator, path: []const u8) Erro pub fn pathStatus( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, rel_path: []const u8, ) Error!PathStatus { - const result = std.process.run(allocator, io, .{ - .argv = &.{ "git", "-C", root, "status", "--porcelain", "--", rel_path }, - .stdout_limit = .limited(64 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, &.{ "git", "-C", root, "status", "--porcelain", "--", rel_path }, .limited(64 * 1024)) catch return error.GitUnavailable; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -194,6 +252,7 @@ pub fn pathStatus( pub fn show( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, rev: []const u8, rel_path: []const u8, @@ -201,10 +260,7 @@ pub fn show( const spec = try std.fmt.allocPrint(allocator, "{s}:{s}", .{ rev, rel_path }); defer allocator.free(spec); - const result = std.process.run(allocator, io, .{ - .argv = &.{ "git", "-C", root, "show", spec }, - .stdout_limit = .limited(32 * 1024 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, &.{ "git", "-C", root, "show", spec }, .limited(32 * 1024 * 1024)) catch return error.GitUnavailable; errdefer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -249,6 +305,7 @@ pub fn show( pub fn listCommitsTouching( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, rel_path: []const u8, since_iso: ?[]const u8, @@ -274,10 +331,7 @@ pub fn listCommitsTouching( } try argv.appendSlice(allocator, &.{ "--", rel_path }); - const result = std.process.run(allocator, io, .{ - .argv = argv.items, - .stdout_limit = .limited(16 * 1024 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, argv.items, .limited(16 * 1024 * 1024)) catch return error.GitUnavailable; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -321,13 +375,11 @@ pub fn freeCommitTouches(allocator: std.mem.Allocator, items: []const CommitTouc pub fn lastCommitTimestampForPath( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, rel_path: []const u8, ) Error!?i64 { - const result = std.process.run(allocator, io, .{ - .argv = &.{ "git", "-C", root, "log", "-1", "--format=%ct", "--", rel_path }, - .stdout_limit = .limited(64 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, &.{ "git", "-C", root, "log", "-1", "--format=%ct", "--", rel_path }, .limited(64 * 1024)) catch return error.GitUnavailable; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -354,6 +406,7 @@ pub fn lastCommitTimestampForPath( pub fn commitAtOrBeforeDate( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, rel_path: []const u8, date_iso: []const u8, @@ -370,14 +423,11 @@ pub fn commitAtOrBeforeDate( const until_arg = try std.fmt.allocPrint(allocator, "--until={s} 23:59:59", .{date_iso}); defer allocator.free(until_arg); - const result = std.process.run(allocator, io, .{ - .argv = &.{ - "git", "-C", root, - "log", "-1", "--format=%H", - until_arg, "--", rel_path, - }, - .stdout_limit = .limited(64 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, &.{ + "git", "-C", root, + "log", "-1", "--format=%H", + until_arg, "--", rel_path, + }, .limited(64 * 1024)) catch return error.GitUnavailable; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -413,6 +463,7 @@ pub fn commitAtOrBeforeDate( pub fn shaAtOrBefore( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, date_iso: []const u8, ) Error!?[]const u8 { @@ -422,14 +473,11 @@ pub fn shaAtOrBefore( const until_arg = try std.fmt.allocPrint(allocator, "--until={s} 23:59:59", .{date_iso}); defer allocator.free(until_arg); - const result = std.process.run(allocator, io, .{ - .argv = &.{ - "git", "-C", root, - "log", "-1", "--format=%H", - "HEAD", until_arg, - }, - .stdout_limit = .limited(64 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, &.{ + "git", "-C", root, + "log", "-1", "--format=%H", + "HEAD", until_arg, + }, .limited(64 * 1024)) catch return error.GitUnavailable; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -453,17 +501,15 @@ pub fn shaAtOrBefore( pub fn commitTimestamp( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, root: []const u8, ref: []const u8, ) Error!i64 { - const result = std.process.run(allocator, io, .{ - .argv = &.{ - "git", "-C", root, - "log", "-1", "--format=%ct", - ref, - }, - .stdout_limit = .limited(4 * 1024), - }) catch return error.GitUnavailable; + const result = runGit(io, allocator, env, &.{ + "git", "-C", root, + "log", "-1", "--format=%ct", + ref, + }, .limited(4 * 1024)) catch return error.GitUnavailable; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -528,6 +574,7 @@ pub fn commitTimestamp( pub fn resolveCommitRangeSpec( io: std.Io, arena: std.mem.Allocator, + env: *const std.process.Environ.Map, repo: RepoInfo, before: ?CommitSpec, after: ?CommitSpec, @@ -540,7 +587,7 @@ pub fn resolveCommitRangeSpec( // Resolve each endpoint independently. const before_rev: []const u8 = if (before) |b| - try resolveSpec(io, arena, repo, b) + try resolveSpec(io, arena, env, repo, b) else if (dirty) "HEAD" else @@ -549,7 +596,7 @@ pub fn resolveCommitRangeSpec( const after_rev: ?[]const u8 = if (after) |a| (switch (a) { .working_copy => null, - else => try resolveSpec(io, arena, repo, a), + else => try resolveSpec(io, arena, env, repo, a), }) else if (dirty) null @@ -562,14 +609,14 @@ pub fn resolveCommitRangeSpec( /// Resolve one non-working `CommitSpec` to a string git can consume. /// Caller handles the `.working_copy` case separately (it's not a /// git ref). -fn resolveSpec(io: std.Io, arena: std.mem.Allocator, repo: RepoInfo, spec: CommitSpec) Error![]const u8 { +fn resolveSpec(io: std.Io, arena: std.mem.Allocator, env: *const std.process.Environ.Map, repo: RepoInfo, spec: CommitSpec) Error![]const u8 { return switch (spec) { .git_ref => |r| r, .date_at_or_before => |d| blk: { var buf: [10]u8 = undefined; // SAFETY: 10-byte buffer is exactly the size of "YYYY-MM-DD". const date_str = std.fmt.bufPrint(&buf, "{f}", .{d}) catch buf[0..]; - const sha = (try commitAtOrBeforeDate(io, arena, repo.root, repo.rel_path, date_str)) orelse + const sha = (try commitAtOrBeforeDate(io, arena, env, repo.root, repo.rel_path, date_str)) orelse return error.NoCommitAtOrBefore; break :blk sha; }, @@ -587,6 +634,7 @@ fn resolveSpec(io: std.Io, arena: std.mem.Allocator, repo: RepoInfo, spec: Commi pub fn resolveCommitRange( io: std.Io, arena: std.mem.Allocator, + env: *const std.process.Environ.Map, repo: RepoInfo, since: ?Date, until: ?Date, @@ -595,7 +643,7 @@ pub fn resolveCommitRange( std.debug.assert(!(since == null and until != null)); const before: ?CommitSpec = if (since) |d| .{ .date_at_or_before = d } else null; const after: ?CommitSpec = if (until) |d| .{ .date_at_or_before = d } else null; - return resolveCommitRangeSpec(io, arena, repo, before, after, dirty); + return resolveCommitRangeSpec(io, arena, env, repo, before, after, dirty); } // ── Tests ──────────────────────────────────────────────────── @@ -604,14 +652,29 @@ pub fn resolveCommitRange( // require `git` to be on PATH, which every developer setup already has // (otherwise the `contributions` command would be unusable anyway). +/// Build a process-environment map for tests from the test runner's +/// captured environment. +/// +/// The map is deliberately NOT pre-scrubbed: the git helpers strip +/// `GIT_*` internally (see `scrubbedEnv`), so passing the raw map here +/// exercises that scrubbing — which is exactly what lets these tests +/// pass when run under a git hook (pre-commit/prek), where `GIT_DIR` +/// etc. point at the outer repo. Caller owns the map; free with +/// `.deinit()`. +fn gitTestEnv(allocator: std.mem.Allocator) std.process.Environ.Map { + return std.testing.environ.createMap(allocator) catch @panic("OOM building test env map"); +} + test "findRepo locates the ambient zfin checkout" { // The test binary runs with cwd set to the project root, so this // should always succeed in CI and local dev. If git isn't available // we get NotInGitRepo or GitUnavailable — tolerate both (the test // environment is responsible for providing git). const allocator = std.testing.allocator; + var env = gitTestEnv(allocator); + defer env.deinit(); // Pick any file that exists in the repo — build.zig is stable. - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + const info = findRepo(std.testing.io, allocator, &env, "build.zig") catch return; defer allocator.free(info.root); defer allocator.free(info.rel_path); try std.testing.expect(info.root.len > 0); @@ -620,10 +683,12 @@ test "findRepo locates the ambient zfin checkout" { test "listCommitsTouching returns at least one commit for build.zig" { const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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 commits = listCommitsTouching(std.testing.io, allocator, info.root, info.rel_path, null) catch return; + const commits = listCommitsTouching(std.testing.io, allocator, &env, info.root, info.rel_path, null) catch return; defer freeCommitTouches(allocator, commits); try std.testing.expect(commits.len >= 1); // Timestamps are plausible (after 2020). @@ -635,25 +700,29 @@ test "listCommitsTouching with non-null since_iso does not segfault" { // string literal when since_iso was non-null, segfaulting on the // debug allocator's memset-to-undefined. const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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); // The test is primarily about not segfaulting; we don't assert on // commits.len since git's --since parsing may decline values that // are too far back (e.g. "100 years ago" can hit pre-epoch dates). - const commits = listCommitsTouching(std.testing.io, allocator, info.root, info.rel_path, "30 years ago") catch return; + const commits = listCommitsTouching(std.testing.io, allocator, &env, info.root, info.rel_path, "30 years ago") catch return; defer freeCommitTouches(allocator, commits); } test "commitAtOrBeforeDate returns a SHA for a past date" { const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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); // Any date well after the repo's creation — commitAtOrBeforeDate // should find the most recent commit touching build.zig. - const sha_opt = commitAtOrBeforeDate(std.testing.io, allocator, info.root, info.rel_path, "2099-01-01") catch return; + const sha_opt = commitAtOrBeforeDate(std.testing.io, allocator, &env, info.root, info.rel_path, "2099-01-01") catch return; try std.testing.expect(sha_opt != null); const sha = sha_opt.?; defer allocator.free(sha); @@ -665,12 +734,14 @@ test "commitAtOrBeforeDate returns a SHA for a past date" { test "commitAtOrBeforeDate returns null for date before repo existed" { const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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); // Pre-git — before any sensible project history. - const sha_opt = commitAtOrBeforeDate(std.testing.io, allocator, info.root, info.rel_path, "1970-01-02") catch return; + const sha_opt = commitAtOrBeforeDate(std.testing.io, allocator, &env, info.root, info.rel_path, "1970-01-02") catch return; try std.testing.expect(sha_opt == null); } @@ -691,13 +762,15 @@ test "commitAtOrBeforeDate: --until=DATE covers end of day, not current time-of- // agreeing on `--since 1W` totals (see src/commands/contributions.zig // tests). const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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); // Future-dated cutoff — should always return the tip of history // regardless of current wall-clock time. - const sha_opt = commitAtOrBeforeDate(std.testing.io, allocator, info.root, info.rel_path, "2099-01-01") catch return; + const sha_opt = commitAtOrBeforeDate(std.testing.io, allocator, &env, info.root, info.rel_path, "2099-01-01") catch return; try std.testing.expect(sha_opt != null); if (sha_opt) |s| allocator.free(s); } @@ -708,11 +781,13 @@ test "shaAtOrBefore returns a SHA for a past date in the ambient repo" { // Same future-date trick as the path-scoped variant: a date well // beyond now should always return the tip of history. const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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 sha_opt = shaAtOrBefore(std.testing.io, allocator, info.root, "2099-01-01") catch return; + const sha_opt = shaAtOrBefore(std.testing.io, allocator, &env, info.root, "2099-01-01") catch return; try std.testing.expect(sha_opt != null); const sha = sha_opt.?; defer allocator.free(sha); @@ -722,20 +797,24 @@ test "shaAtOrBefore returns a SHA for a past date in the ambient repo" { test "shaAtOrBefore returns null for date before repo existed" { const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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 sha_opt = shaAtOrBefore(std.testing.io, allocator, info.root, "1970-01-02") catch return; + const sha_opt = shaAtOrBefore(std.testing.io, allocator, &env, info.root, "1970-01-02") catch return; try std.testing.expect(sha_opt == null); } test "resolveCommitRange: legacy clean → HEAD~1..HEAD" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); + var env = gitTestEnv(std.testing.allocator); + defer env.deinit(); const repo: RepoInfo = .{ .root = "/tmp", .rel_path = "portfolio.srf" }; - const range = try resolveCommitRange(std.testing.io, arena_state.allocator(), repo, null, null, false); + const range = try resolveCommitRange(std.testing.io, arena_state.allocator(), &env, repo, null, null, false); try std.testing.expectEqualStrings("HEAD~1", range.before_rev); try std.testing.expectEqualStrings("HEAD", range.after_rev.?); } @@ -743,16 +822,20 @@ test "resolveCommitRange: legacy clean → HEAD~1..HEAD" { test "resolveCommitRange: legacy dirty → HEAD..working-copy" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); + var env = gitTestEnv(std.testing.allocator); + defer env.deinit(); const repo: RepoInfo = .{ .root = "/tmp", .rel_path = "portfolio.srf" }; - const range = try resolveCommitRange(std.testing.io, arena_state.allocator(), repo, null, null, true); + const range = try resolveCommitRange(std.testing.io, arena_state.allocator(), &env, repo, null, null, true); try std.testing.expectEqualStrings("HEAD", range.before_rev); try std.testing.expect(range.after_rev == null); } test "resolveCommitRange: --since resolves to SHA..HEAD for clean tree" { const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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); @@ -763,6 +846,7 @@ test "resolveCommitRange: --since resolves to SHA..HEAD for clean tree" { const range = resolveCommitRange( std.testing.io, arena_state.allocator(), + &env, info, Date.fromYmd(2099, 1, 1), null, @@ -774,7 +858,9 @@ test "resolveCommitRange: --since resolves to SHA..HEAD for clean tree" { test "resolveCommitRange: --since with no earlier commit → NoCommitAtOrBefore" { const allocator = std.testing.allocator; - const info = findRepo(std.testing.io, allocator, "build.zig") catch return; + 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); @@ -785,6 +871,7 @@ test "resolveCommitRange: --since with no earlier commit → NoCommitAtOrBefore" const result = resolveCommitRange( std.testing.io, arena_state.allocator(), + &env, info, Date.fromYmd(1970, 1, 2), null, diff --git a/src/portfolio_loader.zig b/src/portfolio_loader.zig index 2e66f18..ad71476 100644 --- a/src/portfolio_loader.zig +++ b/src/portfolio_loader.zig @@ -220,6 +220,7 @@ pub fn loadPortfolioFromPaths(io: std.Io, allocator: std.mem.Allocator, paths: [ pub fn loadPortfolioFromPathsAtRev( io: std.Io, allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, paths: []const []const u8, rev: []const u8, as_of: zfin.Date, @@ -233,7 +234,7 @@ pub fn loadPortfolioFromPathsAtRev( // assumed to live in the same repo (typical for sibling // portfolio files in the same directory); we derive their // rel-paths below. - const info = git.findRepo(io, allocator, paths[0]) catch { + const info = git.findRepo(io, allocator, env, paths[0]) catch { var msg_buf: [512]u8 = undefined; const msg = std.fmt.bufPrint(&msg_buf, "Error: portfolio path not in a git repo: {s}\n", .{paths[0]}) catch "Error: portfolio path not in a git repo\n"; stderr.print(io, msg); @@ -272,7 +273,7 @@ pub fn loadPortfolioFromPathsAtRev( else std.fs.path.basename(real); - const data = git.show(io, allocator, info.root, rev, rel) catch |err| switch (err) { + const data = git.show(io, allocator, env, info.root, rev, rel) catch |err| switch (err) { error.PathMissingInRev => empty_blk: { // File didn't exist at this rev. Substitute empty // bytes; loadFromBytes skips empty entries. @@ -795,7 +796,14 @@ test "loadPortfolioFromPathsAtRev: union of two committed files at HEAD" { defer allocator.free(p2); const paths = [_][]const u8{ p1, p2 }; - var loaded = loadPortfolioFromPathsAtRev(testing.io, allocator, &paths, "HEAD", zfin.Date.fromYmd(2026, 5, 23)) orelse { + // Pass the RAW process env (GIT_* included): `git.zig` strips them + // internally, so this exercises that scrubbing — which is what + // lets this test pass under a git hook where GIT_DIR points at the + // outer repo (see `git.scrubbedEnv`). + var env = try testing.environ.createMap(allocator); + defer env.deinit(); + + var loaded = loadPortfolioFromPathsAtRev(testing.io, allocator, &env, &paths, "HEAD", zfin.Date.fromYmd(2026, 5, 23)) orelse { try testing.expect(false); return; }; @@ -838,18 +846,12 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli try gitInTestRepo(allocator, dir, &.{ "add", "portfolio.srf" }); try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "initial" }); - // Capture commit 1 SHA before adding file_b. - const result = try std.process.run(allocator, testing.io, .{ - .argv = &.{ "git", "-C", dir, "rev-parse", "HEAD" }, - .stdout_limit = .limited(1024), - }); - defer allocator.free(result.stdout); - defer allocator.free(result.stderr); - const sha = std.mem.trim(u8, result.stdout, " \t\r\n"); - const sha_owned = try allocator.dupe(u8, sha); - defer allocator.free(sha_owned); - - // Add and commit a second portfolio file. + // Add and commit a second portfolio file. After this commit, + // commit 1 (where portfolio_other.srf doesn't exist yet) is + // reachable as HEAD~1 — no need to capture its SHA explicitly, + // which also avoids a direct `git` call that would inherit the + // hook's GIT_* env. `git show HEAD~1:` and `git show + // :` exercise the identical code path in `git.show`. const file_b = \\#!srfv1 \\symbol::MSFT,shares:num:5,open_date::2024-02-01,open_price:num:300,account::Sample Roth @@ -865,9 +867,14 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli defer allocator.free(p2); const paths = [_][]const u8{ p1, p2 }; - // Load at commit 1 — second file didn't exist yet. Expect - // only AAPL from portfolio.srf, no error. - var loaded = loadPortfolioFromPathsAtRev(testing.io, allocator, &paths, sha_owned, zfin.Date.fromYmd(2026, 5, 23)) orelse { + // Load at commit 1 (HEAD~1) — second file didn't exist yet. + // Expect only AAPL from portfolio.srf, no error. Pass the RAW + // process env; `git.zig` strips GIT_* internally so this works + // under a git hook (see `git.scrubbedEnv`). + var env = try testing.environ.createMap(allocator); + defer env.deinit(); + + var loaded = loadPortfolioFromPathsAtRev(testing.io, allocator, &env, &paths, "HEAD~1", zfin.Date.fromYmd(2026, 5, 23)) orelse { try testing.expect(false); return; };