Re: [PATCH v2 1/4] odb: use size_t for the length of objects to write
- From
Elia Pinto <gitter.spiros@gmail.com>
- Date
- Oct 31, 2026, 10:16 UTC
- Message-ID
- <20261031101619.172770-1-gitter.spiros@gmail.com>
- In-Reply-To
- <auSDFRHpLy65PMF_@pks.im>
On Fri, Oct 30, 2026 at 12:44:34PM +0100, Patrick Steinhardt wrote:
Show 19 quoted lines
> > diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
> > index 463b38f990..0a89bbb455 100755
> > --- a/t/t1007-hash-object.sh
> > +++ b/t/t1007-hash-object.sh
> > @@ -297,4 +297,17 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT \
> > test_cmp expect actual
> > '
> >
> > +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT \
> > + 'write filtered files over 4GB correctly' '
> > + { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } &&
> > + test_oid large5GB >expect &&
> > + test_config filter.null-filter.clean "cat" &&
> > + echo "big filter=null-filter" >.gitattributes &&
> > + git hash-object -w -- big >actual &&
> > + test_cmp expect actual &&
> > + echo $((5*1024*1024*1024)) >expect &&
>
> Isn't this the exact same as `big`? So...Ah, there was a slight misunderstanding here: `expect` didn't contain the content of `big`, but its expected size in bytes (`5368709120`), which was then compared against the output of `cat-file -s`.
That said, your point stands: checking only the size was a weak and redundant check. The OID printed by `hash-object -w` is computed directly from the object content that was written. If the length had been truncated (like on Windows), the written object would be different and its OID wouldn't match `large5GB`. I verified this by simulating the truncation: forcing the length to 32 bits in `odb_write_object_ext()` causes the test to fail right there on the OID comparison.
Because of this, `test_cmp big actual` isn't needed, and running it would unnecessarily cost another expensive read of 5 GiB from the object database.
Show 5 quoted lines
> > > + git cat-file -s $(cat actual) >actual && > > We shouldn't read from and write to the same file. Maybe write it to > "actual.size" instead?
Agreed, reading from and writing to the same file was fragile, even though it worked because shell redirection happens after expansion. In v3 this goes away completely since we dropped the size line.
> > Also, it might make sense to use `test_cmp_bin ()` instead.
Since the size check is gone, we only compare the OID, which is text, so `test_cmp` remains the correct tool. `test_cmp_bin` would only make sense for comparing binary content, which we no longer do for the reasons mentioned above.
Instead, v3 checks that the object actually exists in the database by running `git cat-file -e "$(cat actual)"`. This is a cheap check that confirms `-w` wrote it successfully without needing to read back the 5 GiB content.
Thanks for the review! v3 will follow shortly in the same thread.
Elia