nix icon indicating copy to clipboard operation
nix copied to clipboard

Git fetcher: Don't compute revCount/lastModified if they're already specified

Open edolstra opened this issue 1 month ago • 5 comments

Motivation

This avoids a potentially expensive computation of revCount (and lastModified, but that's cheap) in the common case where it's already known (namely because it's in a lock file).

This means that we no longer check the correctness of those attributes, but we don't care if the user (or more likely the lock file) specifies an incorrect value for these attributes, since it doesn't matter for security (unlikely content hashes like narHash).

Context


Add :+1: to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

edolstra avatar Nov 19 '25 19:11 edolstra

I thought this is what final = true; was for, or did I misunderstand?

roberth avatar Nov 20 '25 12:11 roberth

No, final = true doesn't actually stop the fetcher from calculating lastModified and revCount.

edolstra avatar Nov 20 '25 16:11 edolstra

I think we should make nix flake lock still check and/or update the entries.

Would this be an accurate release note?


---
synopsis: Git fetcher accepts `revCount` and `lastModified` without validation
prs: [14596]
---

The git fetcher now trusts revCount and lastModified values without recomputing or validating them. This improves performance by skipping expensive git rev-list --count operations when these attributes are already provided.

When this applies:

  • Using existing lock files (common case) - Performance improves when evaluating flakes with locked git inputs
  • Manual fetchGit/fetchTree calls - These attributes are also accepted without validation if explicitly specified

When this doesn't apply:

  • nix flake update / nix flake lock compute fresh values for updated direct inputs
  • New lock file creation computes fresh values for direct inputs

Impact:

Incorrect values don't affect source integrity (correct files are still fetched based on rev and narHash), but they are exposed to evaluation. If your flake.nix uses these attributes (e.g., "version-${toString src.revCount}"), incorrect values will affect version strings, derivation names, and any logic depending on them.

Risks:

  • Merge conflicts in lock files: When resolving flake.lock conflicts, you can accidentally mix attributes from different lock states (e.g., rev from one branch, revCount from another). Previously Nix would detect this; now it won't.

  • Propagation through dependencies: Incorrect values in a dependency's lock file will propagate to your lock file when that dependency is copied during locking (i.e., when the dependency hasn't changed). Previously, Nix would recompute these values.

Recommendations:

  • Never hand-edit lock files
  • After resolving lock file conflicts, run nix flake update on affected inputs

ALTERNATIVELY IF IMPLEMENTED

  • After resolving lock file conflicts, run nix flake lock

roberth avatar Nov 20 '25 20:11 roberth

Yes that seems accurate. It's worth noting that the tarball fetcher already allows returning these attributes without checking them, so in that sense it makes behavior more consistent.

edolstra avatar Nov 24 '25 16:11 edolstra

I guess we could warn if there is a mismatch. That way the user will get a warning in most cases (namely when Nix does an actual fetch, rather than a substitution, which would be the case unless you don't change narHash).

edolstra avatar Nov 24 '25 17:11 edolstra