Next size_t stop: pack-objects/delta#2175
Open
dscho wants to merge 12 commits into
Open
Conversation
Preparation for widening the delta-encoding API to `size_t` in subsequent commits, which is what lets pack-objects drop the `cast_size_t_to_ulong()` shims that 606c192 (odb, packfile: use size_t for streaming object sizes, 2026-05-08) had to leave behind in `get_delta()` and `try_delta()` because their downstream consumers were still narrow. The struct is private to diff-delta.c, so widening its fields in isolation is a no-op at runtime: the values stored continue to fit in 32 bits on Windows because the public API around it still truncates. Splitting it out keeps the API-change commit focused on caller updates. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The sole caller (`try_delta()` in builtin/pack-objects.c) passes an `unsigned long`, which promotes safely, so no caller fixups are needed. Splitting it out keeps the `diff_delta()`/`create_delta()` widening, which does ripple to several callers, in its own commit. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
These three are a single accounting tuple (the globals tracking cumulative cached-delta bytes, plus the helper that compares them against an incoming delta size) and are latently 32-bit on Windows where `unsigned long` != `size_t`: a pack with many large cached deltas could wrap silently. The widening is internally consistent on its own: the additions and subtractions against delta_cache_size already come from `size_t` sources (`DELTA_SIZE()` returns `size_t`), and `delta_cacheable()`'s sole caller in `try_delta()` still passes `unsigned long`, which promotes. Prerequisite for dropping `try_delta()`'s `cast_size_t_to_ulong()` shims, which becomes possible once 1create_delta()` and `diff_delta()` are widened in a later commit. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
`free_unpacked()` sums two byte counts: `sizeof_delta_index()` and `SIZE(n->entry)`. The latter has been `size_t` since the prior topic "More work supporting objects larger than 4GB on Windows" widened `SIZE()`/`oe_size()` to `size_t`, so accumulating it into an `unsigned long` return was a silent Windows-only truncation on a packing run with many large objects. The sole caller, `find_deltas()`, still holds its own `mem_usage` in an `unsigned long` for now, and therefore still truncates silently. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…e_t` The pair must move together because `find_deltas()` passes `&mem_usage` to `try_delta()`: widening either alone breaks the type match. `mem_usage` accumulates per-object byte counts already computed in `size_t` (`SIZE()` and `sizeof_delta_index()` reach here through `free_unpacked()`, now `size_t`), and was the last 32-bit-on-Windows narrowing point in the delta-window memory accounting chain. With this commit, that chain uses `size_t` consistently except for `sizeof_delta_index()`'s still-narrow return, whose value is bounded by `create_delta_index()`'s entries cap. `window_memory_limit` (config-driven via `git_config_ulong()`) stays `unsigned long`: it is only compared against `mem_usage` and promotes. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Last stop in the delta-encoding API widening for >4 GiB blobs on
Windows: with `create_delta_index()` done in the prior commit and
`create_delta()`/`diff_delta()` finished here, every byte count that
crosses delta.h is now `size_t`. The struct fields they store into have
been `size_t` since the diff-delta struct widening.
The API change must move with all callers in the same commit (the build
only passes when every `&delta_size` matches the new `size_t*`). Caller
updates are kept minimal:
* builtin/pack-objects.c `get_delta()` and `try_delta()`: widen only
the local `delta_size` variable; the surrounding unsigned-long
locals and their `cast_size_t_to_ulong()` shims are out of scope
here and will be cleaned up in their own commits.
* builtin/fast-import.c, diff.c, t/helper/test-pack-deltas.c:
keep the local unsigned-long delta size (each feeds a still-
unsigned-long downstream consumer: zlib's `avail_in`,
`deflate_it()`, the test helper's own `do_compress()`), and bridge
via a temporary `size_t` plus `cast_size_t_to_ulong()`. The new
casts are paid back in later topics that widen those consumers.
* t/helper/test-delta.c: widen the local outright (no downstream
consumer beyond the test's own `out_size`, which is already
`size_t`).
Note that GCC struggles a bit to figure out that `deltalen` is always
initialized before it is used; To help it along, we initialize it to 0.
This work-around will go away in a later patch series when `deltalen`
can be widened to `size_t`.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…ize_t` Bundling the two widenings: four call sites pass `&stream.avail_in` directly to `use_pack()`, and widening either type fencepost alone would force a bridge variable at each. Doing both together is the simpler end state and is the prerequisite for the `do_compress()` widening in the next commit, which is what lets `write_no_reuse_object()` lose its last `cast_size_t_to_ulong()` shim. The unsigned-long locals widened at the other `use_pack()` callers (avail / remaining / left) hold pack-window sizes bounded by `core.packedGitWindowSize`, so the change is type consistency rather than a new >4GB capability. `git_zstream.avail_in`/`avail_out` likewise reach zlib's `uInt` fields only after `zlib_buf_cap()`'s 1 GiB cap, so the wrapper already accepted `size_t`-shaped inputs in practice. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Prep for the upcoming `git_deflate_bound()` widening to `size_t`: the local that catches its return needs to be `size_t` too, otherwise the widening would introduce a silent Windows narrowing here. No semantic effect with the current unsigned-long-returning `git_deflate_bound()` (`size_t == unsigned long` on this caller's platforms today). Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Fixes a pre-existing silent narrowing from `git_deflate_bound()`'s `unsigned long` return into an `int` local: anything past 2 GiB has always wrapped negative here and then been re-extended to `size_t` inside `xmalloc()`. Also prep for the upcoming `git_deflate_bound()` widening to `size_t`, which would extend the narrowing further if `bound` stayed `int`. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The local is initialised from `git_deflate_bound()` (an unsigned upper bound on the deflated output, never negative) and used in exactly three places: the initialising assignment, `strbuf_grow(buf, size)` whose parameter is already `size_t`, and `stream.avail_out` which became `size_t` in the prior commit. There is no comparison against zero or a negative value, no subtraction, no arithmetic that depends on signedness, and no path that would assign a signed quantity to it. The original `ssize_t` was the wrong type to begin with: a `git_deflate_bound()` result above `SSIZE_MAX` would have wrapped negative on assignment and then implicitly re-extended to a huge `size_t` at `strbuf_grow()`/`stream.avail_out`, requesting an absurd allocation. That is not a real-world concern for the object sizes http-push pushes today, but it is also the reason the type needs to move to `size_t` before `git_deflate_bound()` itself is widened. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…size_t` Prep for the upcoming `git_deflate_bound()` widening to `size_t`. The local is only ever the return value of `git_deflate_bound()` and the `xmalloc()`/`stream.avail_out` sizes derived from it; widening it has no semantic effect today. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
All four `unsigned long`/`int`/`ssize_t` receivers across archive-zip, diff, http-push and t/helper/test-pack-deltas were widened to `size_t` in the prior commits, and remote-curl and fast-import were already there. With every caller prepared, both the parameter and the return type can now move without introducing any silent narrowing. For inputs above zlib's `uLong` range (i.e. >4 GiB on platforms where `uLong` is 32-bit, notably 64-bit Windows), defer to zlib's stored-block formula (the same fallback it would itself use for an unknown stream state) plus the worst-case wrapper overhead. The existing path through `deflateBound()` is unchanged for inputs that fit. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
Author
|
/submit |
|
Submitted as pull.2175.git.1783615780.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@d9b734b. |
|
This branch is now known as |
|
There was a status update in the "New Topics" section about the branch The pack-objects and delta-encoding code paths have been updated to use 'size_t' instead of 'unsigned long' for object sizes and offset limits, avoiding potential truncation issues on 64-bit Windows. Needs review. source: <pull.2175.git.1783615780.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The pack-objects and delta-encoding code paths have been updated to use 'size_t' instead of 'unsigned long' for object sizes and offset limits, avoiding potential truncation issues on 64-bit Windows. Needs review. source: <pull.2175.git.1783615780.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The 'pack-objects' and delta-encoding code paths have been updated to use 'size_t' instead of 'unsigned long' for object sizes and offset limits, avoiding potential truncation issues on 64-bit Windows. Needs review. source: <pull.2175.git.1783615780.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The 'pack-objects' and delta-encoding code paths have been updated to use 'size_t' instead of 'unsigned long' for object sizes and offset limits, avoiding potential truncation issues on 64-bit Windows. Needs review. source: <pull.2175.git.1783615780.gitgitgadget@gmail.com> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch series continues the effort to stop using
unsigned longwheresize_tshould have been used in the first place. This makes a difference on 64-bit Windows, whereunsigned longis 32-bit.With these fixes, the pack-objects machinery works as intended on 64-bit Windows (and any other 64-bit platform where
unsigned longisn't 64-bit).