Skip to content

fix(vcs/cache): mailmap is read twice, so an edit mid-walk can mis-stamp a cache entry #1409

Description

@dekobon

Summary

build_cached and the walk it drives read the repository mailmap
twice, from two independent gix::Repository::open_mailmap() calls:

A .mailmap edit landing between the two therefore persists an entry
stamped fingerprint(pre-edit mailmap) over events walked under the
post-edit one.

Why it is narrow

It is self-healing in the common case: the next run sees the edited
mailmap, fingerprints differently, misses, and overwrites the entry with a
correct one. The wrong hit is only reachable if the mailmap is reverted
to its pre-edit bytes before any run under the edited version
, at the
same HEAD. Then fingerprint matches and the mis-stamped entry is
served — the exact failure mode #1262 closed, through a much smaller door.

Found in review of #1262 rather than in the field; no reproducer beyond a
deliberately-timed edit.

Fix

Open the mailmap once in build_cached and thread the
gix::mailmap::Snapshot into history::collect_events, so the digest and
the resolver are provably the same snapshot. That also removes the second
parse on every miss / splice.

It was deliberately not done in #1262 because it widens past that fix's
scope: collect_events gains a parameter (5 → 6), its third caller
(git::build, src/vcs/git/mod.rs) has to open the snapshot itself,
incremental_events reaches 10 arguments, and build_cached — already
carrying a halstead.effort baseline entry — grows again. Worth doing as
its own change, where that restructuring can be judged on its merits.

Acceptance

  • One open_mailmap() per bca vcs invocation, shared by the digest and
    the walk.
  • The digest a persisted entry carries is, by construction, the digest of
    the mailmap its events were resolved under.
  • build_cached's --no-cache / no-cache-directory path still does not
    pay for a digest it will not use.

Correction (from the #1424 review): the "Why it is narrow" section
understates the window. open_mailmap() swallows read errors, so a
transient partial read during a non-atomic save gives a wrong hit with no
edit and no revert, and it never self-heals. The single-snapshot fix below
closes that path too. Details are in the comments.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions