Skip to content

archive: preserve 4GiB+ entry sizes on LLP64 - #6427

Closed
i80487 wants to merge 3 commits into
git-for-windows:mainfrom
i80487:copilot/fix-zip-entry-size-truncation
Closed

i80487 wants to merge 3 commits into
git-for-windows:mainfrom
i80487:copilot/fix-zip-entry-size-truncation

Conversation

@i80487

@i80487 i80487 commented Sep 21, 2026 •

Copy link
Copy Markdown

Note: This PR was created as part of a test of GitHub Copilot agents. I believe the proposed changes are relevant and consistent, but I don’t have enough experience with the git-for-windows source code to evaluate them properly.

I was unable to verify that these changes work correctly using the test script ‎t/t5005-archive-large-files.sh, because it appears that the GIT_TEST_LONG=true option and the SIZE_T_IS_64BIT prerequisite must be enabled, and I don’t know how to configure this in GitHub CI.

Feel free to send me your comments, modify, defer, or reject this PR.

Details:

git archive still carried unsigned long size plumbing in both zip and tar paths, which truncates 4GiB+ entry sizes on LLP64 Windows before Zip64/PAX logic can trigger. As a result, large entries could be archived with silently wrapped sizes even though the surrounding format support already handled 64-bit lengths.

  • Common archive callback plumbing

    • Widen write_archive_entry_fn_t and the write_archive_entries() call chain from unsigned long to size_t.
    • Update indirect callers so large blob sizes are forwarded without narrowing before they reach the backend writers.
  • Zip backend: make Zip64 decisions from real sizes

    • Convert ZIP entry size and compressed-size state to size_t.
    • Keep CRC as uint32_t; do not conflate checksum width with payload size width.
    • Clamp 32-bit header fields only at the final non-Zip64 write sites, after the Zip64 decision is made.
    • Add a chunked CRC helper so in-memory CRC computation remains correct for size_t-sized buffers.
  • Tar backend: keep tar/PAX consistent with widened sizes

    • Update tar entry and extended-header size handling to size_t.
    • Preserve large regular-file sizes through PAX extended-header generation instead of narrowing them in the callback layer.
    • Adjust buffered tar write helpers to use size_t consistently.
  • Regression coverage

    • Add a new expensive SIZE_T_IS_64BIT archive test that creates a real 4GiB file, commits it, archives it, and verifies the reported entry size in both zip and tar output.
    • Register the test in t/meson.build.

Copilot AI and others added 3 commits September 19, 2026 06:37
Co-authored-by: i80487 <222595226+i80487@users.noreply.github.com>
Co-authored-by: i80487 <222595226+i80487@users.noreply.github.com>
@dscho

dscho commented Sep 22, 2026

Copy link
Copy Markdown
Member

I polished these changes rather dramatically, with the result being available here: main...dscho:git:copilot/fix-zip-entry-size-truncation. Please review, in particular the commit messages.

@i80487

i80487 commented Sep 23, 2026 •

Copy link
Copy Markdown
Author

@dscho: Yes, looking back at your work in copilot/fix-zip-entry-size-truncation branch, it's starting to make sense to me: my PR was a single oversized commit that mixed too many unrelated changes.

I've reviewed the changes you made, and everything looks correct to me.

There are still a few harmless implicit conversions between size_t and unsigned long here and there, but I think that's just a habit from my day job, and I wouldn't want to come across as nitpicking. If we ever want to clean them up, I can do it in a separate PR.

Do you want me to also review the commit messages?

@dscho

dscho commented Sep 23, 2026

Copy link
Copy Markdown
Member

There are still a few harmless implicit conversions between size_t and unsigned long here and there

You mean implicit casts from size_t to unsigned long? Those I care about, it's quite likely that I missed some.

Do you want me to also review the commit messages?

@i80487 That would be lovely!

@i80487

i80487 commented Sep 23, 2026 •

Copy link
Copy Markdown
Author

You mean implicit casts from size_t to unsigned long? Those I care about, it's quite likely that I missed some.

Yes, you’re right; that’s why I used the adjective "harmless" in my previous response. The remaining occurrences are harmless and can be ignored.

That would be lovely!

To be honest, I’m probably not the best person to review and comment on your commit messages. Here are a few thoughts on the commit messages, IMHO:

  • zlib: clamp total_in/total_out bit-wise to uLong

I'm not sure I understand the commit title correctly. I would rather write: zlib: truncate total_in/total_out modulo 2^32 again

  • t3706: cover large-file archive regressions

From my point of view, it doesn't seem to be a regression. I would just write: t3706: cover large-file archives on LLP64 (Windows)

  • t3706: reduce disk usage

I would just say explicitly : t3706: delete large files as soon as they are no longer needed

@i80487 i80487 closed this by deleting the head repository Sep 25, 2026
@i80487

i80487 commented Sep 25, 2026 •

Copy link
Copy Markdown
Author

@dscho: For your information, I deleted my clone of the Git for Windows repository, and it seems that this automatically closed this PR. However, the expected changes are still present in the copilot/fix-zip-entry-size-truncation branch of your fork. I’ll leave it to you to create a new PR and handle the merge. I’d be happy to help with anything else if needed!

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.

3 participants