Fix reading multi-sector files whose sectors are stored verbatim

[dmang-dev]

The problem

MPQ_FILE_COMPRESS on a block entry means the file has a sector offset table, not that every sector is compressed. Storm deflates each sector independently and keeps the compressed form only when it is actually smaller, so an incompressible file carries MPQ_FILE_COMPRESS while every one of its sectors is stored as-is.

Deciding whether a sector is compressed therefore has to compare its stored size against the plain size of that sector, which is a full sector for every sector but the last. read_file compared it against sector_bytes_left, the total the file still owes:

if (block_entry.flags & MPQ_FILE_COMPRESS and
    (force_decompress or sector_bytes_left > len(sector))):

That is larger than the sector for every sector except the last one, so verbatim sectors in any multi-sector file get handed to decompress(), which reads the first data byte as a compression mask and raises Unsupported compression type: <whatever that byte was>.

Relationship to #26

This generalises #26 rather than reverting it. That change fixed the same class of problem for the final sector, and its description already cited the correct rule from StormLib — "they cut down the expected sector size if the last sector isn't big enough" — but the comparison it shipped only reduces to that rule when the sector happens to be the last one.

min(sector_size, sector_bytes_left) is that description implemented for every sector. For the last sector the two expressions are identical, so test/last_sector_compression.s2ma reads back unchanged.

For reference, in StormLib's src/SFileReadFile.cpp, dwBytesInThisSector is clamped to the bytes remaining (lines 108–117) and a sector is decompressed only when dwRawBytesInThisSector < dwBytesInThisSector (line 165).

Also fixed

The sector count used size // sector_size + 1, which over-counts by one when the file size is an exact multiple of the sector size. That makes positions one entry too long, reading 4 bytes of sector data as a table entry. StormLib uses ((size - 1) / sector_size) + 1.

Tests

test/test_multi_sector.py builds small archives in a temp file rather than adding another binary fixture, so the layouts under test are visible in the diff and reviewable. test/mpqbuilder.py is a minimal writer for that purpose (~150 lines: crypt table, hash/block tables, sector offset table; contributed under the project's BSD licence).

Covered: verbatim, deflated and mixed multi-sector files, a file that is an exact multiple of the sector size, a single short sector, and an empty file. One test also asserts the incompressible fixture really did end up stored verbatim, so the test cannot silently stop exercising the path it targets.

Without the fix, test_sectors_roundtrip and test_exact_multiple_of_sector_size fail with Unsupported compression type: 229. With it, the full suite passes (8 passed), including the two pre-existing fixtures.

How this turned up

Extracting the scenarios from a StarCraft 64 cartridge and repackaging them as PC StarCraft maps. Their scenario data is incompressible enough that every sector ends up stored verbatim, so all 96 maps were rejected by mpyq while StormLib read them fine. The same would happen for any archive holding a multi-sector file that does not compress.

🤖 Generated with Claude Code