-
Notifications
You must be signed in to change notification settings - Fork 192
Next size_t stop: pack-objects/delta
#2175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
69c2c21
d92a5d4
4ef2886
07d0120
7dca160
e1ae83b
8353bc0
acffd23
b89d28c
2d4d19c
617960d
ab911cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -260,8 +260,8 @@ static int exclude_promisor_objects_best_effort; | |
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| 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; | ||
| static unsigned long cache_max_small_delta_size = 1000; | ||
|
|
||
| static unsigned long window_memory_limit = 0; | ||
|
|
@@ -353,7 +353,8 @@ static void index_commit_for_bitmap(struct commit *commit) | |
|
|
||
| static void *get_delta(struct object_entry *entry) | ||
| { | ||
| unsigned long size, base_size, delta_size; | ||
| unsigned long size, base_size; | ||
| size_t delta_size; | ||
| void *buf, *base_buf, *delta_buf; | ||
| enum object_type type; | ||
| size_t size_st = 0, base_size_st = 0; | ||
|
|
@@ -487,7 +488,7 @@ static void copy_pack_data(struct hashfile *f, | |
| off_t len) | ||
| { | ||
| unsigned char *in; | ||
| unsigned long avail; | ||
| size_t avail; | ||
|
|
||
| while (len) { | ||
| in = use_pack(p, w_curs, offset, &avail); | ||
|
|
@@ -2260,7 +2261,7 @@ static void check_object(struct object_entry *entry, uint32_t object_index) | |
| struct object_id base_ref; | ||
| struct object_entry *base_entry; | ||
| unsigned long used, used_0; | ||
| unsigned long avail; | ||
| size_t avail; | ||
| off_t ofs; | ||
| unsigned char *buf, c; | ||
| enum object_type type; | ||
|
|
@@ -2688,8 +2689,8 @@ struct unpacked { | |
| unsigned depth; | ||
| }; | ||
|
|
||
| static int delta_cacheable(unsigned long src_size, unsigned long trg_size, | ||
| unsigned long delta_size) | ||
| static int delta_cacheable(size_t src_size, size_t trg_size, | ||
| size_t delta_size) | ||
| { | ||
| if (max_delta_cache_size && delta_cache_size + delta_size > max_delta_cache_size) | ||
| return 0; | ||
|
|
@@ -2772,8 +2773,8 @@ size_t oe_get_size_slow(struct packing_data *pack, | |
| struct pack_window *w_curs; | ||
| unsigned char *buf; | ||
| enum object_type type; | ||
| unsigned long used, avail; | ||
| size_t size; | ||
| unsigned long used; | ||
| size_t avail, size; | ||
|
|
||
| if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) { | ||
| size_t sz; | ||
|
|
@@ -2804,11 +2805,12 @@ size_t oe_get_size_slow(struct packing_data *pack, | |
| } | ||
|
|
||
| static int try_delta(struct unpacked *trg, struct unpacked *src, | ||
| unsigned max_depth, unsigned long *mem_usage) | ||
| unsigned max_depth, size_t *mem_usage) | ||
| { | ||
| struct object_entry *trg_entry = trg->entry; | ||
| struct object_entry *src_entry = src->entry; | ||
| unsigned long trg_size, src_size, delta_size, sizediff, max_size, sz; | ||
| unsigned long trg_size, src_size, sizediff, max_size, sz; | ||
| size_t delta_size; | ||
| unsigned ref_depth; | ||
| enum object_type type; | ||
| void *delta_buf; | ||
|
|
@@ -2972,9 +2974,9 @@ static unsigned int check_delta_limit(struct object_entry *me, unsigned int n) | |
| return m; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
|
|
||
| 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); | ||
| free_delta_index(n->index); | ||
| n->index = NULL; | ||
| if (n->data) { | ||
|
|
@@ -2991,7 +2993,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size, | |
| { | ||
| uint32_t i, idx = 0, count = 0; | ||
| struct unpacked *array; | ||
| unsigned long mem_usage = 0; | ||
| size_t mem_usage = 0; | ||
|
|
||
| CALLOC_ARRAY(array, window); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,22 +125,22 @@ struct unpacked_index_entry { | |
| }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| 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]; | ||
| }; | ||
|
|
||
| struct delta_index * create_delta_index(const void *buf, unsigned long bufsize) | ||
| struct delta_index * create_delta_index(const void *buf, size_t bufsize) | ||
| { | ||
| unsigned int i, hsize, hmask, entries, prev_val, *hash_count; | ||
| const unsigned char *data, *buffer = buf; | ||
| struct delta_index *index; | ||
| 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; | ||
|
|
@@ -318,8 +318,8 @@ unsigned long sizeof_delta_index(struct delta_index *index) | |
|
|
||
| void * | ||
| create_delta(const struct delta_index *index, | ||
| const void *trg_buf, unsigned long trg_size, | ||
| unsigned long *delta_size, unsigned long max_size) | ||
| const void *trg_buf, size_t trg_size, | ||
| size_t *delta_size, size_t max_size) | ||
| { | ||
| unsigned int i, val; | ||
| off_t outpos, moff; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,9 +167,21 @@ int git_inflate(git_zstream *strm, int flush) | |
| return status; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
|
|
||
| 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 | ||
| return deflateBound(&strm->z, (uLong)size); | ||
| } | ||
|
|
||
| void git_deflate_init(git_zstream *strm, int level) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):