Handle cloning of objects larger than 4GB on Windows#2102
Open
dscho wants to merge 11 commits intogitgitgadget:masterfrom
Open
Handle cloning of objects larger than 4GB on Windows#2102dscho wants to merge 11 commits intogitgitgadget:masterfrom
dscho wants to merge 11 commits intogitgitgadget:masterfrom
Conversation
When unpacking objects from a packfile, the object size is decoded from a variable-length encoding. On platforms where unsigned long is 32-bit (such as Windows, even in 64-bit builds), the shift operation overflows when decoding sizes larger than 4GB. The result is a truncated size value, causing the unpacked object to be corrupted or rejected. Fix this by changing the size variable to size_t, which is 64-bit on 64-bit platforms, and ensuring the shift arithmetic occurs in 64-bit space. This was originally authored by LordKiRon <https://github.com/LordKiRon>, who preferred not to reveal their real name and therefore agreed that I take over authorship. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
On Windows, zlib's `uLong` type is 32-bit even on 64-bit systems. When processing data streams larger than 4GB, the `total_in` and `total_out` fields in zlib's `z_stream` structure wrap around, which caused the sanity checks in `zlib_post_call()` to trigger `BUG()` assertions. The git_zstream wrapper now tracks its own 64-bit totals rather than copying them from zlib. The sanity checks compare only the low bits, using `maximum_unsigned_value_of_type(uLong)` to mask appropriately for the platform's `uLong` size. This is based on work by LordKiRon in git-for-windows#6076. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The odb_read_stream structure uses unsigned long for the size field, which is 32-bit on Windows even in 64-bit builds. When streaming objects larger than 4GB, the size would be truncated to zero or an incorrect value, resulting in empty files being written to disk. Change the size field in odb_read_stream to size_t and introduce unpack_object_header_sz() to return sizes via size_t pointer. Since object_info.sizep remains unsigned long for API compatibility, use temporary variables where the types differ, with comments noting the truncation limitation for code paths that still use unsigned long. This was originally authored by LordKiRon <https://github.com/LordKiRon>, who preferred not to reveal their real name and therefore agreed that I take over authorship. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The delta header decoding functions return unsigned long, which truncates on Windows for objects larger than 4GB. Introduce size_t variants get_delta_hdr_size_sz() and get_size_from_delta_sz() that preserve the full 64-bit size, and use them in packed_object_info() where the size is needed for streaming decisions. This was originally authored by LordKiRon <https://github.com/LordKiRon>, who preferred not to reveal their real name and therefore agreed that I take over authorship. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To test Git's behavior with very large pack files, we need a way to generate such files quickly. A naive approach using only readily-available Git commands would take over 10 hours for a 4GB pack file, which is prohibitive. Side-stepping Git's machinery and actual zlib compression by writing uncompressed content with the appropriate zlib header makes things much faster. The fastest method using this approach generates many small, unreachable blob objects and takes about 1.5 minutes for 4GB. However, this cannot be used because we need to test git clone, which requires a reachable commit history. Generating many reachable commits with small, uncompressed blobs takes about 4 minutes for 4GB. But this approach 1) does not reproduce the issues we want to fix (which require individual objects larger than 4GB) and 2) is comparatively slow because of the many SHA-1 calculations. The approach taken here generates a single large blob (filled with NUL bytes), along with the trees and commits needed to make it reachable. This takes about 2.5 minutes for 4.5GB, which is the fastest option that produces a valid, clonable repository with an object large enough to trigger the bugs we want to test. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The shift overflow bug in index-pack and unpack-objects caused incorrect object size calculation when the encoded size required more than 32 bits of shift. This would result in corrupted or failed unpacking of objects larger than 4GB. Add a test that creates a pack file containing a 4GB+ blob using the new 'test-tool synthesize pack --reachable-large' command, then clones the repository to verify the fix works correctly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
Author
|
/submit |
|
Submitted as pull.2102.git.1777393580.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -86,8 +86,11 @@ void *patch_delta(const void *src_buf, unsigned long src_size, | |||
| * This must be called twice on the delta data buffer, first to get the | |||
There was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 4/28/26 12:26 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > The delta header decoding functions return unsigned long, which
> truncates on Windows for objects larger than 4GB. Introduce size_t
> variants get_delta_hdr_size_sz() and get_size_from_delta_sz() that
> preserve the full 64-bit size, and use them in packed_object_info()
> where the size is needed for streaming decisions.
> + * Size_t variant that doesn't truncate - use for >4GB objects on Windows.
> + */
> +static inline size_t get_delta_hdr_size_sz(const unsigned char **datap,
> + const unsigned char *top)
...
> +static inline unsigned long get_delta_hdr_size(const unsigned char **datap,
> + const unsigned char *top)
> +{
> + size_t size = get_delta_hdr_size_sz(datap, top);
> return cast_size_t_to_ulong(size);
> }
I like this trick to use the 64-bit implementation and only to
down-cast for API compatibility. This allows a more gradual
transition than if we replaced ulongs with size_ts everywhere
at once.
Thanks,
-StoleeThere was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Stolee,
On Sun, 3 May 2026, Derrick Stolee wrote:
> On 4/28/26 12:26 PM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The delta header decoding functions return unsigned long, which
> > truncates on Windows for objects larger than 4GB. Introduce size_t
> > variants get_delta_hdr_size_sz() and get_size_from_delta_sz() that
> > preserve the full 64-bit size, and use them in packed_object_info()
> > where the size is needed for streaming decisions.
>
> > + * Size_t variant that doesn't truncate - use for >4GB objects on Windows.
> > + */
> > +static inline size_t get_delta_hdr_size_sz(const unsigned char **datap,
> > + const unsigned char *top)
> ...
> > +static inline unsigned long get_delta_hdr_size(const unsigned char **datap,
> > + const unsigned char *top)
> > +{
> > + size_t size = get_delta_hdr_size_sz(datap, top);
> > return cast_size_t_to_ulong(size);
> > }
>
> I like this trick to use the 64-bit implementation and only to
> down-cast for API compatibility. This allows a more gradual
> transition than if we replaced ulongs with size_ts everywhere
> at once.
Thank you! That was indeed the exact thing I wanted to achieve: To allow
for incremental, easy-to-review patch series.
Ciao,
Johannes
|
User |
| @@ -49,4 +49,41 @@ test_expect_success 'clone - with worktree, file:// protocol' ' | |||
|
|
|||
There was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 4/28/26 12:26 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > The shift overflow bug in index-pack and unpack-objects caused incorrect
> object size calculation when the encoded size required more than 32 bits
> of shift. This would result in corrupted or failed unpacking of objects
> larger than 4GB.
> > Add a test that creates a pack file containing a 4GB+ blob using the
> new 'test-tool synthesize pack --reachable-large' command, then clones
> the repository to verify the fix works correctly.
As mentioned in the previous patch, constructing this large packfile
takes ~4 minutes in CI pipelines. That's quite a lot to handle for
every CI run.
> +test_expect_success SIZE_T_IS_64BIT 'set up repo with >4GB object' '
Your prereq here prevents it from running on 32-bit builds, which is
good. However, I wonder if it would be worth also specifying these
tests as expensive. It's less likely that these layers will be touched
often, so it should be enough to run these on major occasions, such as
testing a release candidate.
(I also think it's appropriate to have these tests _not_ be marked
expensive in their original contribution to git-for-windows/git,
because the pull request build should prove that the tests work. And
maybe git-for-windows/git should keep them for every PR since that's
where the tests matter the most.)
I suppose this also is a question for Junio and our process for
validating releases. Do we have a certain cadence where we run the
expensive tests? What has been our threshold for hiding a test case
behind the expensive label?
Thanks,
-StoleeThere was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Wed, Apr 29, 2026 at 09:34:21AM -0400, Derrick Stolee wrote:
> As mentioned in the previous patch, constructing this large packfile
> takes ~4 minutes in CI pipelines. That's quite a lot to handle for
> every CI run.
And for local runs, too. ;) The test suite takes less than 90 seconds to
run on my laptop, but t5608 by itself 160 seconds. Even if running it in
parallel didn't slow down the rest of the suite (which is not true,
because it's obviously hogging a whole processor the whole time), that's
still almost doubling the run-time.
I'd also worry about assuming that the trash directory can hold 4+GB
(maybe 8GB+ since we clone it?), especially since many of us use ram
disks.
That said...
> > +test_expect_success SIZE_T_IS_64BIT 'set up repo with >4GB object' '
>
> Your prereq here prevents it from running on 32-bit builds, which is
> good. However, I wonder if it would be worth also specifying these
> tests as expensive. It's less likely that these layers will be touched
> often, so it should be enough to run these on major occasions, such as
> testing a release candidate.
I think it is already skipped in most cases, because t5608 requires the
GIT_TEST_CLONE_2GB environment variable be set. Arguably it should just
be using EXPENSIVE, too, as I do not think there is much value in having
individual flags for all of the expensive tests. I think that test just
predates the modern prereq system entirely.
> I suppose this also is a question for Junio and our process for
> validating releases. Do we have a certain cadence where we run the
> expensive tests? What has been our threshold for hiding a test case
> behind the expensive label?
AFAIK the labeling of expensive things is mostly ad-hoc, and nobody is
systematically running them. Likewise for the t/perf tests, which are
super expensive but do (very occasionally) turn up interesting
regressions.
-PeffThere was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 5/1/2026 2:38 AM, Jeff King wrote:
> On Wed, Apr 29, 2026 at 09:34:21AM -0400, Derrick Stolee wrote:
>>> +test_expect_success SIZE_T_IS_64BIT 'set up repo with >4GB object' '
>>
>> Your prereq here prevents it from running on 32-bit builds, which is
>> good. However, I wonder if it would be worth also specifying these
>> tests as expensive. It's less likely that these layers will be touched
>> often, so it should be enough to run these on major occasions, such as
>> testing a release candidate.
>
> I think it is already skipped in most cases, because t5608 requires the
> GIT_TEST_CLONE_2GB environment variable be set. Arguably it should just
> be using EXPENSIVE, too, as I do not think there is much value in having
> individual flags for all of the expensive tests. I think that test just
> predates the modern prereq system entirely.
Thanks for the extra details here! That helps avoid the issues that I
was thinking about, but maybe doubling-down and adding EXPENSIVE is
still worth it.
>> I suppose this also is a question for Junio and our process for
>> validating releases. Do we have a certain cadence where we run the
>> expensive tests? What has been our threshold for hiding a test case
>> behind the expensive label?
>
> AFAIK the labeling of expensive things is mostly ad-hoc, and nobody is
> systematically running them. Likewise for the t/perf tests, which are
> super expensive but do (very occasionally) turn up interesting
> regressions.
I used to be more diligent about running the performance tests myself
around release windows. The EXPENSIVE tests would also be good to do
on rc0. I will contemplate how to put this into my routine.
Thanks,
-Stolee
There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Stolee & Jeff,
On Sun, 3 May 2026, Derrick Stolee wrote:
> On 5/1/2026 2:38 AM, Jeff King wrote:
> > On Wed, Apr 29, 2026 at 09:34:21AM -0400, Derrick Stolee wrote:
>
> >>> +test_expect_success SIZE_T_IS_64BIT 'set up repo with >4GB object' '
> >>
> >> Your prereq here prevents it from running on 32-bit builds, which is
> >> good. However, I wonder if it would be worth also specifying these
> >> tests as expensive. It's less likely that these layers will be touched
> >> often, so it should be enough to run these on major occasions, such as
> >> testing a release candidate.
> >
> > I think it is already skipped in most cases, because t5608 requires the
> > GIT_TEST_CLONE_2GB environment variable be set. Arguably it should just
> > be using EXPENSIVE, too, as I do not think there is much value in having
> > individual flags for all of the expensive tests. I think that test just
> > predates the modern prereq system entirely.
>
> Thanks for the extra details here! That helps avoid the issues that I
> was thinking about, but maybe doubling-down and adding EXPENSIVE is
> still worth it.
Indeed. The `GIT_TEST_CLONE_2GB` flag is set for all CI jobs:
https://gitlab.com/git-scm/git/-/blob/v2.54.0/ci/lib.sh#L314
So I've tried to accelerate it.
First, by using the "unsafe" SHA-1 (because we're in no danger here, we
generate the data ourselves). That helped some, but unfortunately the most
efficient implementation (OpenSSL) is out of bounds for us because we
cannot enable its use in the default configuration for licensing reasons:
OpenSSL's license and Git's GPLv2 are fundamentally incompatible with each
other, and even though Linus' intention with
https://gitlab.com/git-scm/git/-/blob/e83c5163316f89bfbde7d9ab23ca2e25604af290/Makefile#L11
was clearly to allow linking to OpenSSL (actually, requiring it), there is
one Git contributor I spoke to who flatly stated that they'd block every
attempt to add an exception to Git's license retroactively to allow
linking to OpenSSL (at least for distribution, which is when the GPLv2
would kick in). So that's that.
Second, I added shortcuts for the 4GiB+1 size exercised in the added test
cases. That did help! But only the generation time of those packfiles is
helpd by that. The `git clone` that is tested is still awfully slow, and
that _cannot_ be worked around in the same ways.
Therefore, I ended up marking the test cases as `EXPENSIVE`.
To allow them to be run regularly anyway, I added a final patch to the
series that lets the CI runs on the integration branches (other than
`seen`) run all `EXPENSIVE` test cases. One could argue that this patch
should be split out, and I'm open to it, but in the interest of keeping
the time I am working on this patch series _somewhat_ closer to what could
be called reasonable, I'll just keep it in the patch series for now,
hedging for the possibility that maybe nobody objects?
Ciao,
Johannes
> >> I suppose this also is a question for Junio and our process for
> >> validating releases. Do we have a certain cadence where we run the
> >> expensive tests? What has been our threshold for hiding a test case
> >> behind the expensive label?
> >
> > AFAIK the labeling of expensive things is mostly ad-hoc, and nobody is
> > systematically running them. Likewise for the t/perf tests, which are
> > super expensive but do (very occasionally) turn up interesting
> > regressions.
> I used to be more diligent about running the performance tests myself
> around release windows. The EXPENSIVE tests would also be good to do
> on rc0. I will contemplate how to put this into my routine.
>
> Thanks,
> -Stolee
>
>
>
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/28/26 12:26 PM, Johannes Schindelin via GitGitGadget wrote:
> On Windows, unsigned long is 32-bit even on 64-bit systems. This causes
> multiple problems when Git handles objects larger than 4GB. This patch
> series is a very targeted fix for a very early part of the problem: it
> addresses the most fundamental truncation points that prevent a >4GB object
> from surviving a clone at all.
> > Specifically, this fixes:
> > * zlib's uLong wrapping and triggering BUG() assertions in the git_zstream
> wrapper
> * Object sizes being truncated in pack streaming, delta headers, and
> index-pack/unpack-objects
> * pack-objects re-encoding reused pack entries with a truncated size,
> producing corrupt packs on the wire
I'm glad to see this progress in this direction. It's a big step!
> Many other code paths still use unsigned long for object sizes (e.g.,
> cat-file -s, object_info.sizep, the delta machinery) and will need their own
> conversions. This series does not attempt to fix those.
I appreciate the mechanisms used to keep the scope of change minimal. This
differs from the typical "replace all 'unsigned long's with 'size_t'"
proposals in some clever ways.
> Based on work by @LordKiRon in git-for-windows/git#6076.
> > The last two commits add a test helper that synthesizes a pack with a >4GB
> blob and regression tests that clone it via both the unpack-objects and
> index-pack code paths using file:// transport.
My biggest concern here is about how expensive this test is. I wonder if
we should mark it as expensive for the core project and leave it enabled by
default in git-for-windows/git.
Thanks,
-Stolee |
| @@ -37,7 +37,7 @@ static const char index_pack_usage[] = | |||
|
|
|||
There was a problem hiding this comment.
Torsten Bögershausen wrote on the Git mailing list (how to reply to this email):
On Tue, Apr 28, 2026 at 04:26:15PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When unpacking objects from a packfile, the object size is decoded
> from a variable-length encoding. On platforms where unsigned long is
> 32-bit (such as Windows, even in 64-bit builds), the shift operation
> overflows when decoding sizes larger than 4GB. The result is a
> truncated size value, causing the unpacked object to be corrupted or
> rejected.
>
> Fix this by changing the size variable to size_t, which is 64-bit on
> 64-bit platforms, and ensuring the shift arithmetic occurs in 64-bit
> space.
>
> This was originally authored by LordKiRon <https://github.com/LordKiRon>,
> who preferred not to reveal their real name and therefore agreed that I
> take over authorship.
Good to see things moving forward.
See even
https://github.com/git-for-windows/git/pull/2179
which is probably obsolete soon.There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Torsten,
On Sun, 3 May 2026, Torsten Bögershausen wrote:
> On Tue, Apr 28, 2026 at 04:26:15PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When unpacking objects from a packfile, the object size is decoded
> > from a variable-length encoding. On platforms where unsigned long is
> > 32-bit (such as Windows, even in 64-bit builds), the shift operation
> > overflows when decoding sizes larger than 4GB. The result is a
> > truncated size value, causing the unpacked object to be corrupted or
> > rejected.
> >
> > Fix this by changing the size variable to size_t, which is 64-bit on
> > 64-bit platforms, and ensuring the shift arithmetic occurs in 64-bit
> > space.
> >
> > This was originally authored by LordKiRon
> > <https://github.com/LordKiRon>, who preferred not to reveal their real
> > name and therefore agreed that I take over authorship.
>
> Good to see things moving forward.
>
> See even
> https://github.com/git-for-windows/git/pull/2179
> which is probably obsolete soon.
The word "probably" is maybe a bit overwhelmed in this sentence by the
sheer extent of what still needs to be done.
There have been multiple contributors who despaired over the task [*1*] to
split this PR apart in ways that would stand a chance to be accepted (or
for that matter: reviewed) on the Git mailing list...
But yes, it is my hope that I can wittle down that PR into more
contributions along the lines of f9ba6acaa934 (Merge branch
'mc/clean-smudge-with-llp64', 2021-11-29). I still have to upstream
https://github.com/git-for-windows/git/pull/3533, which is another teeny
tiny step toward completing what the PR you mentioned set out to
accomplish.
And yes, the pattern that Stolee already recognized, to duplicate function
signatures into the ones that the 20th century kindly asked to be returned
and the ones using `size_t` instead, this is the pattern I specifically
wanted to use so that incremental patch series have a chance of getting
reviews and to trickle into git/git.
Ciao,
Johannes
Footnote *1*: I specifically asked for this kind of splitting-out:
https://github.com/git-for-windows/git/pull/2179#issuecomment-525926366
|
User |
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@bfcd492. |
|
There was a status update in the "New Topics" section about the branch Update code paths that assumed "unsigned long" was long enough for "size_t". Expecting a reroll to mark EXPENSIVE tests as such. source: <pull.2102.git.1777393580.gitgitgadget@gmail.com> |
Jeff King pointed out on the mailing list [1] that t5608's new >4GB test cases dominate the entire test suite runtime: 160 seconds on his laptop when the rest of the suite finishes in under 90 seconds, and 305-850 seconds across CI jobs. The bottleneck is that the synthesize helper hashes roughly 8 GB of data through SHA-1 (4 GB for the pack checksum plus 4 GB for the blob OID) for a 4 GB+1 blob. Since the helper generates known test data, collision detection is unnecessary. Switch from repo->hash_algo to unsafe_hash_algo(), which uses hardware-accelerated SHA-1 (via OpenSSL or Apple CommonCrypto) when available. Benchmarks on an x86_64 machine generating a 4 GB+1 pack (2 runs each, interleaved): SHA-1 backend Run 1 Run 2 SHA1DC (safe) 75s 80s OpenSSL (unsafe) 21s 19s The effect scales linearly. At 64 MB with 10 randomized interleaved runs, the OpenSSL unsafe backend shows a 5.4x improvement (median 0.202s vs 1.088s) with tight variance (stdev 0.028s vs 0.095s). The speedup is only realized when the build has a fast unsafe backend compiled in. The CI's linux-TEST-vars job already sets OPENSSL_SHA1_UNSAFE=YesPlease; macOS benefits from Apple CommonCrypto when configured. On builds without a separate unsafe backend (such as the default Windows builds), unsafe_hash_algo() returns the regular collision-detecting implementation and the change is a no-op. [1] https://lore.kernel.org/git/20260501063805.GA2038915@coredump.intra.peff.net/ Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The synthesize helper hashes roughly 8 GiB of data through SHA-1 to produce a 4 GiB + 1 pack (4 GiB for the pack checksum, 4 GiB for the blob OID). Since the blob content is all NUL bytes, every byte in the resulting pack file is deterministic for a given blob size and hash algorithm. Add a fast path that writes the pack from precomputed constants: a 25-byte prefix (pack header, object header, zlib header, first block header), the zero-filled bulk with periodic 5-byte deflate block headers, and a 513-byte suffix (tree, two commits, empty tree, pack SHA-1 checksum). This eliminates all SHA-1 and adler32 computation, making the helper purely I/O-bound. The precomputed constants are stored in a struct fast_pack array keyed by hash algorithm format_id, so that adding SHA-256 support later requires only adding another array entry with its suffix. The constants were generated by running the generic path and extracting the non-zero bytes from the resulting pack file. Benchmarks generating a 4 GiB + 1 pack (3 runs each, SHA1DC on x86_64): generic path: 88s / 81s / 140s fast path: 14s / 13s / 15s On CI, where t5608 currently takes 200-850 seconds depending on the job, the fast path cuts the pack-generation phase from minutes to seconds, leaving only the clone operations themselves. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Add a SHA-256 entry to the fast_packs[] table. The pack prefix and deflate block structure are identical to SHA-1 (the pack format does not encode the hash algorithm in its header). Only the suffix differs: SHA-256 OIDs are 32 bytes instead of 20, giving a 609-byte suffix compared to 513 for SHA-1, and a different pack checksum. The constants were generated by running the generic path inside a repository initialized with --object-format=sha256. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Even with precomputed pack constants that reduced the helper's runtime from minutes to seconds, the >4GB clone tests still take 200-850 seconds across CI jobs. The bottleneck is no longer the pack generation but the clone operations themselves: transporting, unpacking, and indexing 4 GiB of data through unpack-objects and index-pack is inherently expensive. As Jeff King pointed out [1], t5608 alone takes 160 seconds on his laptop while the rest of the entire test suite finishes in under 90 seconds, and the test's disk footprint (4+ GiB source repo, then two clones) is problematic for developers who use RAM disks for their trash directories. Gate the >4GB tests on the EXPENSIVE prereq (which requires GIT_TEST_LONG to be set) in addition to SIZE_T_IS_64BIT, keeping them out of normal local test runs. [1] https://lore.kernel.org/git/20260501063805.GA2038915@coredump.intra.peff.net/ Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Derrick Stolee suggested [1] that expensive tests should be run at a regular cadence rather than on every PR iteration. Gate GIT_TEST_LONG on push builds to the integration branches (next, master, main, maint) so that the EXPENSIVE prereq is satisfied there but not during PR validation, where the extra minutes of wall-clock time do not justify themselves. [1] https://lore.kernel.org/git/e1e8837f-7374-4079-ba87-ab95dd156e33@gmail.com/ Helped-by: Derrick Stolee <derrickstolee@github.com> Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
Author
|
/submit |
|
Submitted as pull.2102.v2.git.1777914508.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| algo->update_fn(&ctx, zeros, BLOCK_SIZE); | ||
| algo->update_fn(&ctx, zeros, len % BLOCK_SIZE); | ||
| } | ||
| algo->final_oid_fn(oid, &ctx); |
There was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 5/4/2026 1:08 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Benchmarks generating a 4 GiB + 1 pack (3 runs each, SHA1DC on
> x86_64):
>
> generic path: 88s / 81s / 140s
> fast path: 14s / 13s / 15s
>
> On CI, where t5608 currently takes 200-850 seconds depending on the
> job, the fast path cuts the pack-generation phase from minutes to
> seconds, leaving only the clone operations themselves.
Are these numbers accurate for the patch position in the series?
The previous change replaced SHA1DC with the unsafe version, which
gained similar performance improvements. I'd be interested to see
the numbers for both enabled at the same time.
Thanks,
-Stolee
| @@ -314,6 +314,15 @@ export DEFAULT_TEST_TARGET=prove | |||
| export GIT_TEST_CLONE_2GB=true | |||
There was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 5/4/2026 1:08 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Derrick Stolee suggested [1] that expensive tests should be run at a
> regular cadence rather than on every PR iteration. Gate GIT_TEST_LONG
> on push builds to the integration branches (next, master, main, maint)
> so that the EXPENSIVE prereq is satisfied there but not during PR
> validation, where the extra minutes of wall-clock time do not justify
> themselves.
I like that this will be run as part of regular updates to the
important branches. The important bit after that is whether or
not a human pays attention to the signal of these builds.
Junio: Do you pay attention to CI breaks when you push to
'master'?
One way to help this procedure could be to have GitHub CI
failures trigger new issues, which could then be more easily
viewed and noticed by the community watching the repo. This
is of course out-of-scope for this patch series, but could be
considered in the future.
Thanks,
-Stolee
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.
On Windows,
unsigned longis 32-bit even on 64-bit systems. This causes multiple problems when Git handles objects larger than 4GB. This patch series is a very targeted fix for a very early part of the problem: it addresses the most fundamental truncation points that prevent a >4GB object from surviving a clone at all.Specifically, this fixes:
uLongwrapping and triggeringBUG()assertions in thegit_zstreamwrapperpack-objectsre-encoding reused pack entries with a truncated size, producing corrupt packs on the wireMany other code paths still use
unsigned longfor object sizes (e.g.,cat-file -s,object_info.sizep, the delta machinery) and will need their own conversions. This series does not attempt to fix those.Based on work by @LordKiRon in git-for-windows#6076.
The last two commits add a test helper that synthesizes a pack with a >4GB blob and regression tests that clone it via both the
unpack-objectsandindex-packcode paths usingfile://transport.Changes since v1:
git cloneis still slow (of course, because it cannot use any of those shortcuts), therefore the tests are marked asEXPENSIVE.EXPENSIVEtest cases be run for the integration branches other thanseen.cc: Derrick Stolee stolee@gmail.com
cc: Torsten Bögershausen tboegi@web.de
cc: Jeff King peff@peff.net