Re: [PATCH v2 1/4] odb: use size_t for the length of objects to write

[Elia Pinto <gitter.spiros@gmail.com>]

On Fri, Oct 30, 2026 at 12:44:34PM +0100, Patrick Steinhardt wrote:
> > 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.

>
> > +	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