Skip to content

fix: preserve existing stream positions when appending - #20

Open
jovial-liu wants to merge 1 commit into
Rogdham:masterfrom
jovial-liu:fix/padded-stream-append
Open

jovial-liu wants to merge 1 commit into
Rogdham:masterfrom
jovial-liu:fix/padded-stream-append

Conversation

@jovial-liu

Copy link
Copy Markdown

Adding a new stream to an existing nonempty archive opened in r+ can overwrite an existing stream when the input contains padding or empty streams. For example, an archive with 64 bytes of internal padding and 8 trailing padding bytes originally decodes both payloads, but adding a third stream can leave only the first and third payloads even though native XZ reports success. Padding between or after streams is permitted by the XZ file format.

Place the new stream after the last retained stream's actual file region, rather than summing compressed stream lengths. Regression tests cover internal and trailing padding, empty streams, repeated stream changes, file and BytesIO storage, and appending after truncating the last stream.

Validation on macOS with Python 3.12.14:

  • All 15 new cases pass; 13 fail on the original source.
  • tox run -e py,build,generate-integration-files,lint,type passes: 627 tests pass, 16 skip, coverage remains 100%; both distributions build; all 13 native fixture-generation cases, Ruff, and both mypy checks pass.
  • Independent native XZ Utils 5.8.3 checks preserve every payload for the no-padding control, inputs with 4 or 12 internal padding bytes, and an input with 64 internal plus 8 trailing padding bytes.

AI-assisted with OpenAI Codex; independently reviewed before submission.

@Rogdham

Rogdham commented Oct 9, 2026

Copy link
Copy Markdown
Owner

Hello and thank you for the PR!

python-xz supports stream padding on read, but has no API to manage stream padding on write.

Although it has been almost 5 years since I implemented this, I think I did it on purpose, because I did not see any reason why keeping the existing padding.

I am not against it, but would like some justification of why this change is needed or better than before.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants