Raised while reviewing the #381 change and deliberately kept out of it: that PR moves one damage
site onto a distinct error code, and the remaining sites are the same defect at the same
enforcement point, but each needs its own reasoning about what a caller should be told.
The state after #381
src/index/reader.cpp's open_index_in_window rejects a record for four reasons. #381 moved one
of them onto ErrorCode::IndexDamaged, which src/cli/context.cpp's load_old_index announces:
| Rejection |
Code |
Announced? |
file.size() < sizeof(RawHeader) + sizeof(RawFooter) — "Index file too small" |
InvalidFormat |
no |
| magic mismatch — "Invalid index file magic" |
InvalidFormat |
no |
| version outside the readable window — "Unsupported index version" |
InvalidFormat |
no, correctly |
declared_layout_fits fails — "Index sections do not fit the file" |
IndexDamaged |
yes |
| checksum mismatch |
IndexChecksumMismatch |
yes |
The first two are damage by any reading — a file too short to hold a header, or one that is not a
putup index at all, sitting at the path where a putup index belongs. They are announced exactly as
loudly as the benign case: not at all. So a build over a truncated or overwritten record still
reads as a first build, which is the failure IndexDamaged was introduced to end.
The third row is the reason the announcement filter was ever narrow: "unsupported version" is the
expected, benign outcome on every putup upgrade, and warning on it would fire for every user on
every upgrade. That conflation — one code covering both a benign outcome and real damage — is what
made the filter choose silence for the whole class.
Why this is one issue and not three
The principle is already recorded at ErrorCode::IndexDamaged's declaration in
include/pup/core/result.hpp: a code that conflates damage with an expected-benign outcome forces
every caller to choose between noise and silence. #381 applied it to one row. Applying it to the
rest is the same change: give each rejection a code that says which of the two happened, and let
the announced set follow from the code rather than from a filter that has to guess.
The natural split leaves InvalidFormat meaning only "this is a well-formed record whose version I
do not read" — or retires it here entirely in favour of a version-specific code, which reads better
at the call site and is the question this issue should settle first.
Dead code that belongs to the same decision
ErrorCode::IndexTruncated has no producers anywhere in the tree — grep -rn IndexTruncated src include test returns only its own declaration in include/pup/core/result.hpp. It is either the
code the "Index file too small" site should have been using all along, or it should go. Deciding
that is part of this issue, not a separate cleanup.
Reproducing the gap
Build any project once, then truncate .pup/index to a few bytes, or overwrite its first four bytes
so the magic no longer matches. The next build rebuilds everything and says nothing about why. The
E2E scenario "A damaged record says so instead of rebuilding in silence" (added by #381) is the
shape a test for this would take — it damages the record through the header's declared layout; the
sibling cases damage it through length and magic.
Not in scope
The per-record rejections #381 added, and the read-side disposition rule itself. Those are settled;
this is only about which error code each open-time rejection carries and which of them the build
announces.
Raised while reviewing the #381 change and deliberately kept out of it: that PR moves one damage
site onto a distinct error code, and the remaining sites are the same defect at the same
enforcement point, but each needs its own reasoning about what a caller should be told.
The state after #381
src/index/reader.cpp'sopen_index_in_windowrejects a record for four reasons. #381 moved oneof them onto
ErrorCode::IndexDamaged, whichsrc/cli/context.cpp'sload_old_indexannounces:file.size() < sizeof(RawHeader) + sizeof(RawFooter)— "Index file too small"InvalidFormatInvalidFormatInvalidFormatdeclared_layout_fitsfails — "Index sections do not fit the file"IndexDamagedIndexChecksumMismatchThe first two are damage by any reading — a file too short to hold a header, or one that is not a
putup index at all, sitting at the path where a putup index belongs. They are announced exactly as
loudly as the benign case: not at all. So a build over a truncated or overwritten record still
reads as a first build, which is the failure
IndexDamagedwas introduced to end.The third row is the reason the announcement filter was ever narrow: "unsupported version" is the
expected, benign outcome on every putup upgrade, and warning on it would fire for every user on
every upgrade. That conflation — one code covering both a benign outcome and real damage — is what
made the filter choose silence for the whole class.
Why this is one issue and not three
The principle is already recorded at
ErrorCode::IndexDamaged's declaration ininclude/pup/core/result.hpp: a code that conflates damage with an expected-benign outcome forcesevery caller to choose between noise and silence. #381 applied it to one row. Applying it to the
rest is the same change: give each rejection a code that says which of the two happened, and let
the announced set follow from the code rather than from a filter that has to guess.
The natural split leaves
InvalidFormatmeaning only "this is a well-formed record whose version Ido not read" — or retires it here entirely in favour of a version-specific code, which reads better
at the call site and is the question this issue should settle first.
Dead code that belongs to the same decision
ErrorCode::IndexTruncatedhas no producers anywhere in the tree —grep -rn IndexTruncated src include testreturns only its own declaration ininclude/pup/core/result.hpp. It is either thecode the "Index file too small" site should have been using all along, or it should go. Deciding
that is part of this issue, not a separate cleanup.
Reproducing the gap
Build any project once, then truncate
.pup/indexto a few bytes, or overwrite its first four bytesso the magic no longer matches. The next build rebuilds everything and says nothing about why. The
E2E scenario "A damaged record says so instead of rebuilding in silence" (added by #381) is the
shape a test for this would take — it damages the record through the header's declared layout; the
sibling cases damage it through length and magic.
Not in scope
The per-record rejections #381 added, and the read-side disposition rule itself. Those are settled;
this is only about which error code each open-time rejection carries and which of them the build
announces.