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> |
|
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> |
|
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. cf. <pull.2175.git.1783615780.gitgitgadget@gmail.com> 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> |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -125,9 +125,9 @@ struct unpacked_index_entry { | |||
| }; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Jul 09, 2026 at 04:49:28PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/diff-delta.c b/diff-delta.c
> index 43c339f010..b6b65d7607 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -125,9 +125,9 @@ struct unpacked_index_entry {
> };
>
> struct delta_index {
> - unsigned long memsize;
> + size_t memsize;
> const void *src_buf;
> - unsigned long src_size;
> + size_t src_size;
> unsigned int hash_mask;
> struct index_entry *hash[FLEX_ARRAY];
> };
`sizeof_delta_index` returns `index->memsize`, so we'll also have to
adapt that function's return value and its callers.
> @@ -140,7 +140,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
I was about to complain that the input parameter here uses `unsigned
long`, too. But the next patch addresses that.
> struct unpacked_index_entry *entry, **hash;
> struct index_entry *packed_entry, **packed_hash;
> void *mem;
> - unsigned long memsize;
> + size_t memsize;
>
> if (!buf || !bufsize)
> return NULL;
Patrick
|
User |
| @@ -260,8 +260,8 @@ static int exclude_promisor_objects_best_effort; | |||
|
|
|||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Jul 09, 2026 at 04:49:30PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index e3760b3492..f89628a760 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -260,8 +260,8 @@ static int exclude_promisor_objects_best_effort;
>
> static int use_delta_islands;
>
> -static unsigned long delta_cache_size = 0;
> -static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
> +static size_t delta_cache_size = 0;
> +static size_t max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
The only other site that assigns `max_delta_cache_size` does so via
`git_config_int()`, so we happily accept negative values for
"pack.deltacachesize". This will cause a change in behaviour here, even
though arguably the behaviour both before and after this patch is broken
in the same way.
Ideally we'd have something like `git_config_size_t()`, or at least use
`git_config_uint()` here. But that could potentially break the case
where somebody mistakenly configured a negative value and took it as
"infinite", which was mostly true before.
In any case, our docs only mention positive values. So maybe this is
something we could fix while at it.
Patrick
| @@ -2972,9 +2972,9 @@ static unsigned int check_delta_limit(struct object_entry *me, unsigned int n) | |||
| return m; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Jul 09, 2026 at 04:49:31PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index f89628a760..4737a6a32c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2972,9 +2972,9 @@ static unsigned int check_delta_limit(struct object_entry *me, unsigned int n)
> return m;
> }
>
> -static unsigned long free_unpacked(struct unpacked *n)
> +static size_t free_unpacked(struct unpacked *n)
> {
> - unsigned long freed_mem = sizeof_delta_index(n->index);
> + size_t freed_mem = sizeof_delta_index(n->index);
Okay. As mentioned on a preceding patch, the function itself still
returns `unsigned long`, which should probably also be corrected in this
patch series.
Patrick
| @@ -962,7 +962,7 @@ static int store_object( | |||
| struct object_entry *e; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Jul 09, 2026 at 04:49:33PM +0000, Johannes Schindelin via GitGitGadget wrote:
[snip]
> 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`.
Thanks for putting this note here, I was wondering about that part.
Patrick
| @@ -167,9 +167,21 @@ int git_inflate(git_zstream *strm, int flush) | |||
| return status; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Jul 09, 2026 at 04:49:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: 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.
Nit, feel free to ignore: I feel like all of these patches could've been
squashed into a single one, as they're trivial enough.
> 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.
A link or something like that to the formula would've helped here, as
I'm not familiar with this mechanism.
> diff --git a/git-zlib.c b/git-zlib.c
> index d21adb3bf5..ebbbcc6d1a 100644
> --- a/git-zlib.c
> +++ b/git-zlib.c
> @@ -167,9 +167,21 @@ int git_inflate(git_zstream *strm, int flush)
> return status;
> }
>
> -unsigned long git_deflate_bound(git_zstream *strm, unsigned long size)
> +size_t git_deflate_bound(git_zstream *strm, size_t size)
> {
> - return deflateBound(&strm->z, size);
> +#if SIZE_MAX > ULONG_MAX
> + if (size > maximum_unsigned_value_of_type(uLong))
> + /*
> + * deflateBound() takes uLong, which is 32-bit on
> + * Windows. For inputs above that range, return zlib's
> + * stored-block formula (the conservative path it would
> + * itself use for an unknown stream state) plus the
> + * worst-case wrapper overhead.
> + */
> + return size + (size >> 5) + (size >> 7) + (size >> 11)
> + + 7 + 18;
> +#endif
So is the idea here that we estimate the highest number of bytes that
the deflated size could end up with?
Patrick
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).cc: Patrick Steinhardt ps@pks.im