more reviews! · Entire

*1. (High) Decimal numbers in commit/tag messages get silently rewritten into SHA256 hashes*
sha256convert.go:1369 — hashPattern = (?i)\b[0-9a-f]{7,40}\b

Digits are a subset of hex, so the pattern matches pure-decimal runs. Verified:
"Fixes #1234567"        -> ["1234567"]
"build 1700000000 done" -> ["1700000000"]
"ver 1234567.8901234"   -> ["1234567" "8901234"]   (split on the dot)

If a 7+ digit issue number, PR number, or unix timestamp happens to be a unique hex prefix of one reachable commit/tag SHA1, rewriteHashesInMessage replaces it with a 64-char SHA256 and bumps messageRewrites — *with no warning* (only the ambiguous case is surfaced). On a large active repo the per-number probability is low but the corpus of issue refs/timestamps is large, so the expected number of corrupted messages is non-trivial, and the corruption is silent and hard to detect after the fact. The doc/comment claim that false positives are "essentially impossible" only holds for long prefixes. Worth requiring more hex-ish context (e.g. a leading g-less SHA context, or only rewriting full 40-char hashes plus prefixes that contain at least one a-f).

Confirmed and fixed. The finding was valid.

What was wrong

hashPattern = (?i)\b[0-9a-f]{7,40}\b — since 0-9 is a subset of hex, it matches pure-decimal runs. Verified: "Fixes #1234567" → [1234567], "build 1700000000 done" → [1700000000]. If such a number (issue/PR ref, Unix timestamp) was a unique hex prefix of a reachable commit/tag SHA1, rewriteHashesInMessage would silently replace it with a 64-char SHA256 and bump messageRewrites — only the ambiguous case warned, so this corruption was undetectable.

Fix

I took the reviewer's recommended approach — require an abbreviated prefix to contain at least one a-f letter, keeping full 40-char hashes exempt:

• cmd/git-sync/internal/sha256convert/sha256convert.go

  • New containsHexLetter helper.
  • Guard in resolveMessageRefUncached: an abbreviated prefix (len < 40) with no a-f letter returns matchNone. Placed in the shared resolve path so both rewriteHashesInMessage and extractMessageReferences stay consistent, and it gets memoized. A pure-decimal run is treated as "no match" (not flagged ambiguous — we don't want to nag about issue numbers).
  • Full 40-char hashes stay exempt: a 40-digit decimal colliding with a real SHA1 is not a practical concern, and 40-digit numbers don't appear in messages.
  • Replaced the misleading "essentially impossible" comment.

• Tests: added TestTranslator_DecimalRunNotRewritten (forces the 1234567-prefix collision, asserts it's neither rewritten nor flagged ambiguous, while a sibling 1234abc prefix in the same message still rewrites). Made TestTranslator_RewritesCrossBranchReferences robust — it now picks a letter-bearing short prefix instead of cAHex[:8], which happened to be all-decimal and surfaced the guard.

• docs/convert-sha256.md: updated the false-positives section to describe the letter requirement and the safe-degradation trade-off.

The trade-off (intended): a genuine all-decimal abbreviated hash (~3.7% of random 7-char prefixes) is no longer rewritten — it's left as-is, the same safe outcome as an ambiguous prefix, rather than corrupting a number. Full hashes are unaffected.

go build, go vet, gofmt, and the full package tests all pass.

Documentation (stale vs. code)

4. docs/convert-sha256.md:234 — claims refs/notes/sha1-origin "differs [across runs] because its wrapper commit carries time.Now()." The code uses notesCommitTime() (:1517), which pins to SOURCE_DATE_EPOCH or Unix(0,0) — so the notes ref is deterministic. The doc states the opposite.

5. sha256convert.go:6 (package doc) — "no hash mapping is persisted." Contradicted by the default-on refs/notes/sha1-origin and --write-mapping, and by the Request field comments at :85-97. An operator could mistakenly treat the old SHA1s as unrecoverable.